feat: optimise queries for index page with tags

bethesque · bethesque · commit 524e08d0b68b · 2018-05-25T14:13:46.000+10:00
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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
diff --git a/spec/lib/pact_broker/matrix/row_spec.rb b/spec/lib/pact_broker/matrix/row_spec.rb
diff --git a/spec/support/rspec_match_hash.rb b/spec/support/rspec_match_hash.rb
