fix: pact broker client issue 53 (pact-foundation#299)

* fix(matrix): ensure relevant rows are not removed from the result set by the default query limit

fixes: pact-foundation/pact_broker-client#53

Author: bethesque

lib/pact_broker/matrix/repository.rb

@@ -103,6 +103,7 @@ def is_a_row_for_this_integration_required?(specified_pacticipant_names, consume
         specified_pacticipant_names.include?(consumer_name)
       end
 
+      # It would be nicer to do this in the SQL, but it requires time that I don't have at the moment
       def apply_latestby options, selectors, lines
         return lines unless options[:latestby]
         group_by_columns = case options[:latestby]
@@ -119,6 +120,7 @@ def apply_latestby options, selectors, lines
           .flatten
       end
 
+      # It would be nicer to do this in the SQL, but it requires time that I don't have at the moment
       def remove_overwritten_revisions lines
         latest_revisions_keys = {}
         latest_revisions = []
@@ -143,7 +145,7 @@ def query_matrix selectors, options
       end
 
       def resolve_selectors(specified_selectors, options)
-        resolved_specified_selectors = resolve_versions_and_add_ids(specified_selectors, :specified)
+        resolved_specified_selectors = resolve_versions_and_add_ids(specified_selectors, :specified, options[:latestby])
         if options[:latest] || options[:tag]
           add_inferred_selectors(resolved_specified_selectors, options)
         else
@@ -152,19 +154,19 @@ def resolve_selectors(specified_selectors, options)
       end
 
       # Find the version number for selectors with the latest and/or tag specified
-      def resolve_versions_and_add_ids(selectors, selector_type)
+      def resolve_versions_and_add_ids(selectors, selector_type, latestby)
         selectors.collect do | selector |
           pacticipant = PactBroker::Domain::Pacticipant.find(name: selector[:pacticipant_name])
           versions = find_versions_for_selector(selector)
-          build_selectors_for_pacticipant_and_versions(pacticipant, versions, selector, selector_type)
+          build_selectors_for_pacticipant_and_versions(pacticipant, versions, selector, selector_type, latestby)
         end.flatten
       end
 
-      def build_selectors_for_pacticipant_and_versions(pacticipant, versions, original_selector, selector_type)
+      def build_selectors_for_pacticipant_and_versions(pacticipant, versions, original_selector, selector_type, latestby)
         if versions
           versions.collect do | version |
             if version
-              selector_for_version(pacticipant, version, original_selector, selector_type)
+              selector_for_version(pacticipant, version, original_selector, selector_type, latestby)
             else
               selector_for_non_existing_version(pacticipant, original_selector, selector_type)
             end
@@ -192,23 +194,6 @@ def find_versions_for_selector(selector)
         end
       end
 
-      def add_ids_to_selector(selector)
-        if selector[:pacticipant_name]
-          pacticipant = PactBroker::Domain::Pacticipant.find(name: selector[:pacticipant_name])
-          selector[:pacticipant_id] = pacticipant ? pacticipant.id : nil
-        end
-
-        if selector[:pacticipant_name] && selector[:pacticipant_version_number] && !selector[:pacticipant_version_id]
-          version = version_repository.find_by_pacticipant_name_and_number(selector[:pacticipant_name], selector[:pacticipant_version_number])
-          selector[:pacticipant_version_id] = version ? version.id : nil
-        end
-
-        if !selector.key?(:pacticipant_version_id)
-           selector[:pacticipant_version_id] = nil
-        end
-        selector
-      end
-
       # When only one selector is specified, (eg. checking to see if Foo version 2 can be deployed to prod),
       # need to look up all integrated pacticipants, and determine their relevant (eg. latest prod) versions
       def add_inferred_selectors(resolved_specified_selectors, options)
@@ -228,7 +213,7 @@ def build_inferred_selectors(inferred_pacticipant_names, options)
           selector[:latest] = options[:latest] if options[:latest]
           selector
         end
-        resolve_versions_and_add_ids(selectors, :inferred)
+        resolve_versions_and_add_ids(selectors, :inferred, options[:latestby])
       end
 
       def all_pacticipant_names_in_specified_matrix(selectors)
@@ -242,8 +227,27 @@ def selector_for_non_existing_version(pacticipant, original_selector, selector_t
         ResolvedSelector.for_pacticipant_and_non_existing_version(pacticipant, original_selector, selector_type)
       end
 
-      def selector_for_version(pacticipant, version, original_selector, selector_type)
-        ResolvedSelector.for_pacticipant_and_version(pacticipant, version, original_selector, selector_type)
+      def selector_for_version(pacticipant, version, original_selector, selector_type, latestby)
+        pact_publication_ids, verification_ids = nil, nil
+
+        # Querying for the "pre-filtered" pact publications and verifications directly, rather than just using the consumer
+        # and provider versions allows us to remove the "overwritten" pact revisions and verifications from the
+        # matrix result set, making the final matrix query more efficient and stopping the query limit from
+        # removing relevant data (eg. https://github.com/pact-foundation/pact_broker-client/issues/53)
+        if latestby
+          pact_publication_ids = PactBroker::Pacts::LatestPactPublicationsByConsumerVersion
+                                            .select(:id)
+                                            .where(consumer_version_id: version.id)
+                                            .collect{ |pact_publication| pact_publication[:id] }
+
+          verification_ids = PactBroker::Verifications::LatestVerificationIdForPactVersionAndProviderVersion
+                                .select(:verification_id)
+                                .distinct
+                                .where(provider_version_id: version.id)
+                                .collect{ |pact_publication| pact_publication[:verification_id] }
+        end
+
+        ResolvedSelector.for_pacticipant_and_version(pacticipant, version, pact_publication_ids, verification_ids, original_selector, selector_type)
       end
 
       def selector_without_version(pacticipant, selector_type)

lib/pact_broker/matrix/resolved_selector.rb

@@ -23,11 +23,13 @@ def self.for_pacticipant(pacticipant, type)
         )
       end
 
-      def self.for_pacticipant_and_version(pacticipant, version, original_selector, type)
+      def self.for_pacticipant_and_version(pacticipant, version, pact_publication_ids = [], verification_ids = [], original_selector, type)
         ResolvedSelector.new(
           pacticipant_id: pacticipant.id,
           pacticipant_name: pacticipant.name,
           pacticipant_version_id: version.id,
+          pact_publication_ids: pact_publication_ids,
+          verification_ids: verification_ids,
           pacticipant_version_number: version.number,
           latest: original_selector[:latest],
           tag: original_selector[:tag],

lib/pact_broker/matrix/row.rb

@@ -67,15 +67,19 @@ def self.consumer_and_maybe_consumer_version_match_any_selector(selectors)
           end
 
           def self.consumer_and_maybe_consumer_version_match_selector(s)
-            if s[:pacticipant_version_id]
+            if s[:pact_publication_ids]
+              Sequel.&(pact_publication_id: s[:pact_publication_ids])
+            elsif s[:pacticipant_version_id]
               Sequel.&(consumer_id: s[:pacticipant_id], consumer_version_id: s[:pacticipant_version_id])
             else
               Sequel.&(consumer_id: s[:pacticipant_id])
             end
           end
 
           def self.provider_and_maybe_provider_version_match_selector(s)
-            if s[:pacticipant_version_id]
+            if s[:verification_ids]
+              Sequel.&(verification_id: s[:verification_ids])
+            elsif s[:pacticipant_version_id]
               Sequel.&(provider_id: s[:pacticipant_id], provider_version_id: s[:pacticipant_version_id])
             else
               Sequel.&(provider_id: s[:pacticipant_id])

spec/lib/pact_broker/matrix/deployment_status_summary_spec.rb

@@ -190,6 +190,8 @@ module Matrix
               pacticipant_name: "Bar",
               pacticipant_version_id: 10,
               pacticipant_version_number: "14131c5da3abf323ccf410b1b619edac76231243",
+              pact_publication_ids: [],
+              verification_ids: [],
               latest: nil,
               tag: nil,
               type: :inferred

spec/lib/pact_broker/matrix/repository_query_limit_spec.rb

@@ -0,0 +1,44 @@
+require 'pact_broker/matrix/repository'
+
+module PactBroker
+  module Matrix
+    describe Repository do
+      # See https://github.com/pact-foundation/pact_broker-client/issues/53
+      # The problem occurs when a pact has so many verifications for the same provider
+      # version that relevant rows do get returned in the result set because the specified limit
+      # causes them to be truncated.
+      # The most elegant solution is to create views have the data already grouped by
+      # consumer version/provider version, consumer version/provider, and consumer/provider,
+      # however, I don't have the time to work out how to make that view query efficient - I suspect
+      # it will require lots of full table scans, as it will have to work out the latest pact revision
+      # and latest verification for each pact publication and I'm not sure if it will have to do it
+      # for the entire table, or whether it will apply the consumer/provider filters...
+      # The quick and dirty solution is to do a pre-query to get the latest pact revision and latest
+      # verifications for the pact versions before we do the matrix query.
+      describe "Querying for can-i-deploy when there are more matrix rows than the specified query limit" do
+        before do
+          td.create_consumer("Foo")
+            .create_provider("Bar")
+            .create_consumer_version("1")
+            .create_pact
+            .create_verification(number: 1, provider_version: "2", tag_names: ['staging'])
+            .create_verification(number: 2, provider_version: "2")
+            .create_verification(number: 3, provider_version: "2")
+            .create_verification(number: 4, provider_version: "2")
+            .create_verification(number: 5, provider_version: "3")
+            .create_provider("Wiffle")
+            .create_pact
+            .create_verification(number: 1, provider_version: "6", tag_names: ['staging'])
+        end
+
+        let(:path) { "/matrix?q[][pacticipant]=Foo&q[][version]=1&tag=staging&latestby=cvp&limit=2" }
+
+        subject { get(path) }
+
+        it "does not remove relevant rows from the query due to the specified limit" do
+          expect(JSON.parse(subject.body)['summary']['deployable']).to be true
+        end
+      end
+    end
+  end
+end