From e366af496ae375de8f56cfdb81d1681190606cb4 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 25 May 2018 20:50:10 +1000 Subject: [PATCH] feat: include more columns in latest_verifications_for_consumer_version_tags to avoid having to do extra queries for pact_versions and provider_versions --- ...verifications_for_consumer_version_tags.rb | 37 +++++++++++++++---- lib/pact_broker/index/service.rb | 5 ++- lib/pact_broker/matrix/aggregated_row.rb | 27 +++++++++++--- lib/pact_broker/matrix/head_row.rb | 1 + lib/pact_broker/matrix/row.rb | 29 ++++----------- ...t_verification_for_consumer_version_tag.rb | 16 +++++++- 6 files changed, 79 insertions(+), 36 deletions(-) diff --git a/db/migrations/20180523_create_latest_verifications_for_consumer_version_tags.rb b/db/migrations/20180523_create_latest_verifications_for_consumer_version_tags.rb index 0c41e371d..21c04e9ae 100644 --- a/db/migrations/20180523_create_latest_verifications_for_consumer_version_tags.rb +++ b/db/migrations/20180523_create_latest_verifications_for_consumer_version_tags.rb @@ -2,7 +2,11 @@ up do # The latest verification id for each consumer version tag create_view(:latest_verifications_ids_for_consumer_version_tags, - "select pv.pacticipant_id as provider_id, lpp.consumer_id, t.name as consumer_version_tag_name, max(v.id) as latest_verification_id + "select + pv.pacticipant_id as provider_id, + lpp.consumer_id, + t.name as consumer_version_tag_name, + max(v.id) as latest_verification_id from verifications v join latest_pact_publications_by_consumer_versions lpp on v.pact_version_id = lpp.pact_version_id @@ -12,12 +16,31 @@ on v.provider_version_id = pv.id group by pv.pacticipant_id, lpp.consumer_id, t.name") - # The latest verification for each consumer version tag - create_view(:latest_verifications_for_consumer_version_tags, - "select v.*, lv.provider_id, lv.consumer_id, lv.consumer_version_tag_name - from verifications v - join latest_verifications_ids_for_consumer_version_tags lv - on lv.latest_verification_id = v.id") + # The most recent verification for each consumer/consumer version tag/provider + latest_verifications = from(:verifications) + .select( + Sequel[:lv][:consumer_id], + Sequel[:lv][:provider_id], + Sequel[:lv][:consumer_version_tag_name], + Sequel[:pv][:sha].as(:pact_version_sha), + Sequel[:prv][:number].as(:provider_version_number), + Sequel[:prv][:order].as(:provider_version_order), + ) + .select_append{ verifications.* } + .join(:latest_verifications_ids_for_consumer_version_tags, + { + Sequel[:verifications][:id] => Sequel[:lv][:latest_verification_id], + }, { table_alias: :lv }) + .join(:versions, + { + Sequel[:verifications][:provider_version_id] => Sequel[:prv][:id] + }, { table_alias: :prv }) + .join(:pact_versions, + { + Sequel[:verifications][:pact_version_id] => Sequel[:pv][:id] + }, { table_alias: :pv }) + + create_or_replace_view(:latest_verifications_for_consumer_version_tags, latest_verifications) end down do diff --git a/lib/pact_broker/index/service.rb b/lib/pact_broker/index/service.rb index 30ff4b460..db02bad8c 100644 --- a/lib/pact_broker/index/service.rb +++ b/lib/pact_broker/index/service.rb @@ -19,7 +19,7 @@ def self.find_index_items options = {} .eager(:latest_triggered_webhooks) .eager(:webhooks) .order(:consumer_name, :provider_name) - .eager(:verification) + #.eager(verification: [:provider_version, :pact_version]) if !options[:tags] rows = rows.where(consumer_version_tag_name: nil) @@ -34,12 +34,13 @@ def self.find_index_items options = {} rows = rows.all.group_by(&:pact_publication_id).values.collect{ | rows| Matrix::AggregatedRow.new(rows) } rows.sort.collect do | row | + # TODO simplify PactBroker::Domain::IndexItem.create( row.consumer, row.provider, row.pact, row.overall_latest?, - options[:tags] ? row.latest_verification : row.verification, + row.latest_verification, row.webhooks, row.latest_triggered_webhooks, options[:tags] ? row.consumer_head_tag_names : [], diff --git a/lib/pact_broker/matrix/aggregated_row.rb b/lib/pact_broker/matrix/aggregated_row.rb index c6c45f16c..0bc5b78e2 100644 --- a/lib/pact_broker/matrix/aggregated_row.rb +++ b/lib/pact_broker/matrix/aggregated_row.rb @@ -2,7 +2,7 @@ # A collection of matrix rows with the same pact publication id # It's basically a normalised view of a denormalised view :( - +# A pact publication may be the overall latest, and/or the latest for a tag module PactBroker module Matrix class AggregatedRow @@ -13,7 +13,6 @@ class AggregatedRow delegate [:pact, :pact_version, :pact_revision_number, :webhooks, :latest_triggered_webhooks, :'<=>'] => :first_row delegate [:verification_id, :verification] => :first_row - def initialize matrix_rows @matrix_rows = matrix_rows @first_row = matrix_rows.first @@ -23,20 +22,26 @@ def overall_latest? !!matrix_rows.find{ | row| row.consumer_version_tag_name.nil? } end + # If this comes back nil, it won't be "cached", but it's a reasonably + # quick query, so it's probably ok. def latest_verification @latest_verification ||= begin verification = matrix_rows.collect do | row| - row.verification || row.latest_verification_for_consumer_version_tag - end.compact.sort{ |v1, v2| v1.id <=> v2.id}.last + row.verification || latest_verification_for_consumer_version_tag(row) + end.compact.sort{ |v1, v2| v1.id <=> v2.id }.last if !verification && overall_latest? - PactBroker::Verifications::Repository.new.find_latest_verification_for(consumer_name, provider_name) + overall_latest_verification else verification end end end + # The list of tag names for which this pact publication is the most recent with that tag + # There could, however, be a later consumer version that does't have a pact (perhaps because it was deleted) + # that has the same tag. + # TODO show a warning when the data is "corrupted" as above. def consumer_head_tag_names matrix_rows.collect(&:consumer_version_tag_name).compact end @@ -45,6 +50,18 @@ def consumer_head_tag_names attr_reader :matrix_rows + def latest_verification_for_consumer_version_tag row + row.latest_verification_for_consumer_version_tag if row.consumer_version_tag_name + end + + def overall_latest_verification + # This causes the + # SELECT "latest_verifications".* FROM "latest_verifications" + # query in the logs for the tagged index. + # Given it only happens rarely, it's ok to not lazy load it. + PactBroker::Verifications::Repository.new.find_latest_verification_for(consumer_name, provider_name) + end + def first_row @first_row end diff --git a/lib/pact_broker/matrix/head_row.rb b/lib/pact_broker/matrix/head_row.rb index 3c65d006e..c2e8e8001 100644 --- a/lib/pact_broker/matrix/head_row.rb +++ b/lib/pact_broker/matrix/head_row.rb @@ -21,6 +21,7 @@ class HeadRow < Row tag_to_row = eo_opts[:rows].each_with_object({}) { | row, map | map[[row.provider_id, row.consumer_id, row.consumer_version_tag_name]] = row } eo_opts[:rows].each{|row| row.associations[:latest_verification_for_consumer_version_tag] = nil} + # Need the all then the each to ensure the eager loading works PactBroker::Verifications::LatestVerificationForConsumerVersionTag.each do | verification | key = [verification.provider_id, verification.consumer_id, verification.consumer_version_tag_name] if tag_to_row[key] diff --git a/lib/pact_broker/matrix/row.rb b/lib/pact_broker/matrix/row.rb index d401c8c8a..045f52f70 100644 --- a/lib/pact_broker/matrix/row.rb +++ b/lib/pact_broker/matrix/row.rb @@ -16,7 +16,6 @@ class Row < Sequel::Model(:materialized_matrix) associate(:one_to_many, :webhooks, :class => "PactBroker::Webhooks::Webhook", primary_key: [:consumer_id, :provider_id], key: [:consumer_id, :provider_id]) associate(:one_to_many, :consumer_version_tags, :class => "PactBroker::Tags::TagWithLatestFlag", primary_key: :consumer_version_id, key: :version_id) associate(:one_to_many, :provider_version_tags, :class => "PactBroker::Tags::TagWithLatestFlag", primary_key: :provider_version_id, key: :version_id) - associate(:many_to_one, :verification, :class => "PactBroker::Domain::Verification", primary_key: :id, key: :verification_id) dataset_module do include PactBroker::Repositories::Helpers @@ -114,20 +113,6 @@ def [] key end end - # tags for which this pact publication is the latest of that tag - # this is set in the code rather than the database - def consumer_head_tag_names - @consumer_head_tag_names ||= [] - end - - def consumer_head_tag_names= consumer_head_tag_names - @consumer_head_tag_names = consumer_head_tag_names - end - - # def latest_triggered_webhooks - # @latest_triggered_webhooks ||= [] - # end - def summary "#{consumer_name}#{consumer_version_number} #{provider_name}#{provider_version_number || '?'} (r#{pact_revision_number}n#{verification_number || '?'})" end @@ -141,17 +126,18 @@ def provider end def consumer_version - @consumer_version ||= OpenStruct.new(number: consumer_version_number, id: consumer_version_id, pacticipant: consumer) + @consumer_version ||= OpenStruct.new(number: consumer_version_number, order: consumer_version_order, id: consumer_version_id, pacticipant: consumer) end def provider_version if provider_version_number - @provider_version ||= OpenStruct.new(number: provider_version_number, id: provider_version_id, pacticipant: provider) + @provider_version ||= OpenStruct.new(number: provider_version_number, order: provider_version_order, id: provider_version_id, pacticipant: provider) end end def pact - @pact ||= OpenStruct.new(consumer: consumer, + @pact ||= OpenStruct.new( + consumer: consumer, provider: provider, consumer_version: consumer_version, consumer_version_number: consumer_version_number, @@ -161,7 +147,7 @@ def pact ) end - def latest_verification + def verification if verification_executed_at @latest_verification ||= OpenStruct.new( id: verification_id, @@ -170,10 +156,12 @@ def latest_verification execution_date: verification_executed_at, created_at: verification_executed_at, provider_version_number: provider_version_number, + provider_version_order: provider_version_order, build_url: verification_build_url, provider_version: provider_version, consumer_name: consumer_name, - provider_name: provider_name + provider_name: provider_name, + pact_version_sha: pact_version_sha ) end end @@ -190,7 +178,6 @@ def <=> other ] comparisons.find{|c| c != 0 } || 0 - end def compare_name_asc name1, name2 diff --git a/lib/pact_broker/verifications/latest_verification_for_consumer_version_tag.rb b/lib/pact_broker/verifications/latest_verification_for_consumer_version_tag.rb index 2c07b4541..b4398d329 100644 --- a/lib/pact_broker/verifications/latest_verification_for_consumer_version_tag.rb +++ b/lib/pact_broker/verifications/latest_verification_for_consumer_version_tag.rb @@ -4,6 +4,20 @@ module PactBroker module Verifications class LatestVerificationForConsumerVersionTag < PactBroker::Domain::Verification set_dataset(:latest_verifications_for_consumer_version_tags) + + def pact_version_sha + # Don't need to load the pact_version as we do in the superclass, + # as pact_version_sha is included in the view for convenience + values[:pact_version_sha] + end + + def provider_version_number + values[:provider_version_number] + end + + def provider_version_order + values[:provider_version_order] + end end end -end \ No newline at end of file +end