From 524e08d0b68b74021d0260e5f82cdc1c99af74e9 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 25 May 2018 14:13:46 +1000 Subject: [PATCH] feat: optimise queries for index page with tags --- ...verifications_for_consumer_version_tags.rb | 17 ++-- lib/pact_broker/domain/index_item.rb | 2 +- lib/pact_broker/index/service.rb | 96 +++---------------- lib/pact_broker/matrix/aggregated_row.rb | 57 +++++++++++ lib/pact_broker/matrix/head_row.rb | 21 ++++ lib/pact_broker/matrix/row.rb | 12 +-- .../pact_broker/matrix/aggregated_row_spec.rb | 80 ++++++++++++++++ spec/lib/pact_broker/matrix/head_row_spec.rb | 55 +++++++++++ spec/lib/pact_broker/matrix/row_spec.rb | 41 -------- spec/support/rspec_match_hash.rb | 31 +++--- 10 files changed, 253 insertions(+), 159 deletions(-) create mode 100644 lib/pact_broker/matrix/aggregated_row.rb create mode 100644 spec/lib/pact_broker/matrix/aggregated_row_spec.rb 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 ade9fcef1..0c41e371d 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,19 +2,22 @@ up do # The latest verification id for each consumer version tag create_view(:latest_verifications_ids_for_consumer_version_tags, - "select t.name as consumer_version_tag_name, max(lv.id) as latest_verification_id - from verifications lv + "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 lv.pact_version_id = lpp.pact_version_id - join tags t on lpp.consumer_version_id = t.version_id - group by t.name") + on v.pact_version_id = lpp.pact_version_id + join tags t + on lpp.consumer_version_id = t.version_id + join versions pv + 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.consumer_version_tag_name + "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") + on lv.latest_verification_id = v.id") end down do diff --git a/lib/pact_broker/domain/index_item.rb b/lib/pact_broker/domain/index_item.rb index f1eb5a7d1..3a010fab6 100644 --- a/lib/pact_broker/domain/index_item.rb +++ b/lib/pact_broker/domain/index_item.rb @@ -102,7 +102,7 @@ def latest_verification_successful? end def pact_changed_since_last_verification? - latest_verification.pact_version_sha != latest_pact.pact_version_sha + latest_verification.pact_version_id != latest_pact.pact_version_id end def latest_verification_provider_version_number diff --git a/lib/pact_broker/index/service.rb b/lib/pact_broker/index/service.rb index 77f27faac..30ff4b460 100644 --- a/lib/pact_broker/index/service.rb +++ b/lib/pact_broker/index/service.rb @@ -2,6 +2,7 @@ require 'pact_broker/logging' require 'pact_broker/domain/index_item' require 'pact_broker/matrix/head_row' +require 'pact_broker/matrix/aggregated_row' module PactBroker @@ -13,107 +14,38 @@ class Service extend PactBroker::Logging def self.find_index_items options = {} - rows = [] - overall_latest_publication_ids = nil - rows = PactBroker::Matrix::HeadRow .select_all_qualified .eager(:latest_triggered_webhooks) .eager(:webhooks) .order(:consumer_name, :provider_name) - .eager(:consumer_version_tags) - .eager(:provider_version_tags) + .eager(:verification) if !options[:tags] - rows = rows.where(consumer_version_tag_name: nil).all - overall_latest_publication_ids = rows.collect(&:pact_publication_id) - end - - if options[:tags] + rows = rows.where(consumer_version_tag_name: nil) + else if options[:tags].is_a?(Array) rows = rows.where(consumer_version_tag_name: options[:tags]).or(consumer_version_tag_name: nil) end - - rows = rows.all - overall_latest_publication_ids = rows.select{|r| !r[:consumer_version_tag_name] }.collect(&:pact_publication_id).uniq - - # Smoosh all the rows with matching pact publications together - # and collect their consumer_head_tag_names - rows = rows - .group_by(&:pact_publication_id) - .values - .collect{|group| [group.last, group.collect{|r| r[:consumer_version_tag_name]}.compact] } - .collect{ |(row, tag_names)| row.consumer_head_tag_names = tag_names; row } + rows = rows.eager(:consumer_version_tags) + .eager(:provider_version_tags) + .eager(:latest_verification_for_consumer_version_tag) end + rows = rows.all.group_by(&:pact_publication_id).values.collect{ | rows| Matrix::AggregatedRow.new(rows) } - index_items = [] - rows.sort.each do | row | - tag_names = [] - if options[:tags] - tag_names = row.consumer_version_tags.collect(&:name) - end - - overall_latest = overall_latest_publication_ids.include?(row.pact_publication_id) - latest_verification = if overall_latest - verification_repository.find_latest_verification_for row.consumer_name, row.provider_name - else - tag_names.collect do | tag_name | - verification_repository.find_latest_verification_for row.consumer_name, row.provider_name, tag_name - end.compact.sort do | v1, v2 | - # Some provider versions have nil orders, not sure why - # Sort by execution_date instead of order - v1.execution_date <=> v2.execution_date - end.last - end - - index_items << PactBroker::Domain::IndexItem.create( + rows.sort.collect do | row | + PactBroker::Domain::IndexItem.create( row.consumer, row.provider, row.pact, - overall_latest, - latest_verification, + row.overall_latest?, + options[:tags] ? row.latest_verification : row.verification, row.webhooks, row.latest_triggered_webhooks, - row.consumer_head_tag_names, - row.provider_version_tags.select(&:latest?) + options[:tags] ? row.consumer_head_tag_names : [], + options[:tags] ? row.provider_version_tags.select(&:latest?) : [] ) end - - index_items - end - - def self.tags_for(pact, options) - if options[:tags] == true - tag_service.find_all_tag_names_for_pacticipant(pact.consumer_name) - elsif options[:tags].is_a?(Array) - options[:tags] - else - [] - end - end - - def self.build_index_item_rows(pact, tags) - index_items = [build_latest_pact_index_item(pact, tags)] - tags.each do | tag | - index_items << build_index_item_for_tagged_pact(pact, tag) - end - index_items.compact - end - - def self.build_latest_pact_index_item pact, tags - latest_verification = verification_service.find_latest_verification_for(pact.consumer, pact.provider) - webhooks = webhook_service.find_by_consumer_and_provider pact.consumer, pact.provider - triggered_webhooks = webhook_service.find_latest_triggered_webhooks pact.consumer, pact.provider - tag_names = pact.consumer_version_tag_names.select{ |name| tags.include?(name) } - PactBroker::Domain::IndexItem.create pact.consumer, pact.provider, pact, true, latest_verification, webhooks, triggered_webhooks, tag_names - end - - def self.build_index_item_for_tagged_pact latest_pact, tag - pact = pact_service.find_latest_pact consumer_name: latest_pact.consumer_name, provider_name: latest_pact.provider_name, tag: tag - return nil unless pact - return nil if pact.id == latest_pact.id - verification = verification_repository.find_latest_verification_for pact.consumer_name, pact.provider_name, tag - PactBroker::Domain::IndexItem.create pact.consumer, pact.provider, pact, false, verification, [], [], [tag] end end end diff --git a/lib/pact_broker/matrix/aggregated_row.rb b/lib/pact_broker/matrix/aggregated_row.rb new file mode 100644 index 000000000..c6c45f16c --- /dev/null +++ b/lib/pact_broker/matrix/aggregated_row.rb @@ -0,0 +1,57 @@ +require 'pact_broker/verifications/repository' + +# A collection of matrix rows with the same pact publication id +# It's basically a normalised view of a denormalised view :( + +module PactBroker + module Matrix + class AggregatedRow + extend Forwardable + + delegate [:consumer, :consumer_name, :consumer_version, :consumer_version_number, :consumer_version_order, :consumer_version_tags] => :first_row + delegate [:provider, :provider_name, :provider_version, :provider_version_number, :provider_version_order, :provider_version_tags] => :first_row + 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 + end + + def overall_latest? + !!matrix_rows.find{ | row| row.consumer_version_tag_name.nil? } + end + + 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 + + if !verification && overall_latest? + PactBroker::Verifications::Repository.new.find_latest_verification_for(consumer_name, provider_name) + else + verification + end + end + end + + def consumer_head_tag_names + matrix_rows.collect(&:consumer_version_tag_name).compact + end + + private + + attr_reader :matrix_rows + + def first_row + @first_row + end + + def consumer_version_tag_names + matrix_rows.collect(&:consumer_version_tag_name) + end + end + end +end diff --git a/lib/pact_broker/matrix/head_row.rb b/lib/pact_broker/matrix/head_row.rb index 816d6447e..cbf6462b3 100644 --- a/lib/pact_broker/matrix/head_row.rb +++ b/lib/pact_broker/matrix/head_row.rb @@ -7,6 +7,27 @@ module Matrix class HeadRow < Row set_dataset(:materialized_head_matrix) + # one_to_one :latest_verification_for_consumer_version_tag, + # :class => "PactBroker::Verifications::LatestVerificationForConsumerVersionTag", + # primary_key: [:provider_id, :consumer_id, :consumer_version_tag_name], key: [:provider_id, :consumer_id, :consumer_version_tag_name] + + # Loading the latest_verification_for_consumer_version_tag objects this way is quicker than + # doing it using an inbult relation with primary_key/key, if we are loading the relation for + # the entire HeadRow table + # Using the inbuilt relation puts constraints on the columns that are not necessary and slow + # the query down. + one_to_one :latest_verification_for_consumer_version_tag, :class => "PactBroker::Verifications::LatestVerificationForConsumerVersionTag", primary_keys: [], key: [], :eager_loader=>(proc do |eo_opts| + 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} + + PactBroker::Verifications::LatestVerificationForConsumerVersionTag.each do | verification | + key = [verification.provider_id, verification.consumer_id, verification.consumer_version_tag_name] + if tag_to_row[key] + tag_to_row[key].associations[:latest_verification_for_consumer_version_tag] = verification + end + end + end) + dataset_module do include PactBroker::Repositories::Helpers include PactBroker::Logging diff --git a/lib/pact_broker/matrix/row.rb b/lib/pact_broker/matrix/row.rb index a19f9e702..bdf3a286a 100644 --- a/lib/pact_broker/matrix/row.rb +++ b/lib/pact_broker/matrix/row.rb @@ -17,17 +17,7 @@ class Row < Sequel::Model(:materialized_matrix) 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) - - one_to_one :latest_verification_for_consumer_version_tag, primary_keys: [], key: [], :eager_loader=>(proc do |eo_opts| - tag_to_row = eo_opts[:rows].each_with_object({}) { | row, map | row.consumer_version_tags.each{ | tag | map[tag.name] = row } } - eo_opts[:rows].each{|row| row.associations[:latest_verification_for_consumer_version_tag] = nil} - - PactBroker::Verifications::LatestVerificationForConsumerVersionTag.each do | verification | - if tag_to_row[verification.consumer_version_tag_name] - tag_to_row[verification.consumer_version_tag_name].associations[:latest_verification_for_consumer_version_tag] = verification - end - end - end) + # associate(:many_to_one, :pact_version, :class => "PactBroker::Pacts::PactVersion", primary_key: :id, key: :pact_version_id) dataset_module do include PactBroker::Repositories::Helpers diff --git a/spec/lib/pact_broker/matrix/aggregated_row_spec.rb b/spec/lib/pact_broker/matrix/aggregated_row_spec.rb new file mode 100644 index 000000000..43dd2dd20 --- /dev/null +++ b/spec/lib/pact_broker/matrix/aggregated_row_spec.rb @@ -0,0 +1,80 @@ +require 'pact_broker/matrix/aggregated_row' + +module PactBroker + module Matrix + describe AggregatedRow do + describe "latest_verification" do + let(:row_1) do + instance_double('PactBroker::Matrix::HeadRow', + consumer_name: "Foo", + provider_name: "Bar", + verification: verification_1, + latest_verification_for_consumer_version_tag: tag_verification_1, + consumer_version_tag_name: consumer_version_tag_name_1) + end + let(:row_2) do + instance_double('PactBroker::Matrix::HeadRow', + verification: verification_2, + latest_verification_for_consumer_version_tag: tag_verification_2, + consumer_version_tag_name: consumer_version_tag_name_2) + end + let(:verification_1) { instance_double('PactBroker::Domain::Verification', id: 1) } + let(:verification_2) { instance_double('PactBroker::Domain::Verification', id: 2) } + let(:tag_verification_1) { instance_double('PactBroker::Domain::Verification', id: 3) } + let(:tag_verification_2) { instance_double('PactBroker::Domain::Verification', id: 4) } + let(:consumer_version_tag_name_1) { 'master' } + let(:consumer_version_tag_name_2) { 'prod' } + let(:rows) { [row_1, row_2] } + let(:aggregated_row) { AggregatedRow.new(rows) } + + subject { aggregated_row.latest_verification } + + context "when the rows have verifications" do + it "returns the verification with the largest id" do + expect(subject).to be verification_2 + end + end + + context "when the rows do not have verifications, but there are a previous verifications for a pacts with the same tag" do + let(:verification_1) { nil } + let(:verification_2) { nil } + + it "returns the verification for the previous pact that has the largest id" do + expect(subject).to be tag_verification_2 + end + end + + context "when there is no verification for any of the rows or any of the pacts with the same tag" do + before do + allow_any_instance_of(PactBroker::Verifications::Repository).to receive(:find_latest_verification_for).and_return(overall_latest_verification) + end + + let(:overall_latest_verification) { instance_double('PactBroker::Domain::Verification', id: 5) } + let(:verification_1) { nil } + let(:verification_2) { nil } + let(:tag_verification_1) { nil } + let(:tag_verification_2) { nil } + + context "when one of the rows is the overall latest" do + let(:consumer_version_tag_name_1) { nil } + + it "looks up the overall latest verification" do + expect_any_instance_of(PactBroker::Verifications::Repository).to receive(:find_latest_verification_for).with("Foo", "Bar") + subject + end + + it "returns the overall latest verification" do + expect(subject).to be overall_latest_verification + end + end + + context "when none of the rows is not the overall latest (they are all the latest with a tag)" do + it "returns nil" do + expect(subject).to be nil + end + end + end + end + end + end +end diff --git a/spec/lib/pact_broker/matrix/head_row_spec.rb b/spec/lib/pact_broker/matrix/head_row_spec.rb index 5f16aa9fa..b8f47ef9c 100644 --- a/spec/lib/pact_broker/matrix/head_row_spec.rb +++ b/spec/lib/pact_broker/matrix/head_row_spec.rb @@ -3,6 +3,61 @@ module PactBroker module Matrix describe HeadRow do + let(:td) { TestDataBuilder.new } + + describe "latest_verification_for_consumer_version_tag" do + context "when the pact with a given tag has been verified" do + before do + td.create_pact_with_hierarchy("Foo", "1", "Bar") + .create_consumer_version_tag("prod") + .create_verification(provider_version: "10") + .create_consumer_version("2", comment: "latest prod version for Foo") + .create_consumer_version_tag("prod") + .create_pact + .create_verification(provider_version: "11") + .create_verification(provider_version: "12", number: 2) + .create_consumer("Wiffle") + .create_consumer_version("30", comment: "latest prod version for Wiffle") + .create_consumer_version_tag("prod") + .create_pact + .create_verification(provider_version: "12") + .create_provider("Meep") + .create_pact + .create_verification(provider_version: "40") + end + + subject { HeadRow.eager(:consumer_version_tags).eager(:latest_verification_for_consumer_version_tag).order(:pact_publication_id, :verification_id).exclude(consumer_version_tag_name: nil).all } + + it "returns its most recent verification" do + cols = [:consumer_name, :consumer_version_number, :consumer_version_tag_name, :provider_name, :provider_version_number] + rows = subject.collect{ | row | cols.collect{ | col | row[col]} } + + expect(subject.size).to eq 3 + expect(rows).to include ["Foo", "2", "prod", "Bar", "12"] + expect(rows).to include ["Wiffle", "30", "prod", "Bar", "12"] + expect(rows).to include ["Wiffle", "30", "prod", "Meep", "40"] + end + end + + context "when the most recent pact with a given tag has not been verified, but a previous version with the same tag has" do + before do + td.create_pact_with_hierarchy("Foo", "1", "Bar") + .create_consumer_version_tag("prod") + .create_verification(provider_version: "10") + .create_verification(provider_version: "11", number: 2, comment: "this is the latest verification for a pact with cv tag prod") + .create_consumer_version("2") + .create_consumer_version_tag("prod") + .create_pact + end + + subject { HeadRow.eager(:consumer_version_tags).eager(:latest_verification_for_consumer_version_tag).order(:pact_publication_id).all } + + it "returns the most recent verification for the previous version with the same tag" do + expect(subject.last.verification_id).to be nil # this pact version has not been verified directly + expect(subject.last.latest_verification_for_consumer_version_tag.provider_version.number).to eq "11" + end + end + end describe "refresh", migration: true do before do PactBroker::Database.migrate diff --git a/spec/lib/pact_broker/matrix/row_spec.rb b/spec/lib/pact_broker/matrix/row_spec.rb index d59d45ac8..b2df06730 100644 --- a/spec/lib/pact_broker/matrix/row_spec.rb +++ b/spec/lib/pact_broker/matrix/row_spec.rb @@ -5,47 +5,6 @@ module Matrix describe Row do let(:td) { TestDataBuilder.new } - describe "latest_verification_for_consumer_version_tag" do - context "when the pact with a given tag has been verified" do - before do - td.create_pact_with_hierarchy("Foo", "1", "Bar") - .create_consumer_version_tag("prod") - .create_verification(provider_version: "10") - .create_consumer_version("2") - .create_consumer_version_tag("prod") - .create_pact - .create_verification(provider_version: "11", number: 1) - .create_verification(provider_version: "12", number: 2) - end - - subject { Row.eager(:consumer_version_tags).eager(:latest_verification_for_consumer_version_tag).order(:pact_publication_id, :verification_id).all } - - it "returns its most recent verification" do - expect(subject.last.provider_version_number).to eq "12" - expect(subject.last.latest_verification_for_consumer_version_tag.provider_version.number).to eq "12" - end - end - - context "when the most recent pact with a given tag has not been verified, but a previous version with the same tag has" do - before do - td.create_pact_with_hierarchy("Foo", "1", "Bar") - .create_consumer_version_tag("prod") - .create_verification(provider_version: "10") - .create_verification(provider_version: "11", number: 2, comment: "this is the latest verification for a pact with cv tag prod") - .create_consumer_version("2") - .create_consumer_version_tag("prod") - .create_pact - end - - subject { Row.eager(:consumer_version_tags).eager(:latest_verification_for_consumer_version_tag).order(:pact_publication_id).all } - - it "returns the most recent verification for the previous version with the same tag" do - expect(subject.last.verification_id).to be nil # this pact version has not been verified directly - expect(subject.last.latest_verification_for_consumer_version_tag.provider_version.number).to eq "11" - end - end - end - describe "refresh", migration: true do let(:td) { TestDataBuilder.new(auto_refresh_matrix: false) } diff --git a/spec/support/rspec_match_hash.rb b/spec/support/rspec_match_hash.rb index c5a7046d8..0acad199a 100644 --- a/spec/support/rspec_match_hash.rb +++ b/spec/support/rspec_match_hash.rb @@ -4,26 +4,23 @@ match do |actual| contains_hash?(expected, actual) end -end + failure_message do |actual| + "expected #{actual.class} to include #{expected.class}\n" + formatted_diffs + end -def contains_hash?(expected, actual) - if actual.is_a?(Array) - actual.any? && actual.any?{|actual_item| contains_hash?(expected, actual_item)} - else - expected.all? do |key, value| - unordered_match(actual[key], value) - end + def formatted_diffs + @diffs.collect{ | diff| Pact::Matchers::UnixDiffFormatter.call(diff) }.join("\n") end -end -def unordered_match(expected, actual) - case - when [expected, actual].all?{|val| val.is_a? Array } - expected.all?{|el| actual.include? el } - when [expected, actual].all?{|val| val.is_a? Hash } - contains_hash?(expected, actual) - else - expected == actual + def contains_hash?(expected, actual) + if actual.is_a?(Array) + actual.any? && actual.any?{|actual_item| contains_hash?(expected, actual_item)} + else + @diffs ||= [] + diff = Pact::Matchers.diff(expected, actual.to_hash) + @diffs << diff + diff.empty? + end end end