diff --git a/lib/pact_broker/matrix/repository.rb b/lib/pact_broker/matrix/repository.rb index 11fb81581..c7b410433 100644 --- a/lib/pact_broker/matrix/repository.rb +++ b/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) diff --git a/lib/pact_broker/matrix/resolved_selector.rb b/lib/pact_broker/matrix/resolved_selector.rb index 72ef2efd4..a04b3f6b6 100644 --- a/lib/pact_broker/matrix/resolved_selector.rb +++ b/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], diff --git a/lib/pact_broker/matrix/row.rb b/lib/pact_broker/matrix/row.rb index 373ef344b..ee5327b6f 100644 --- a/lib/pact_broker/matrix/row.rb +++ b/lib/pact_broker/matrix/row.rb @@ -67,7 +67,9 @@ 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]) @@ -75,7 +77,9 @@ def self.consumer_and_maybe_consumer_version_match_selector(s) 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]) diff --git a/spec/lib/pact_broker/matrix/deployment_status_summary_spec.rb b/spec/lib/pact_broker/matrix/deployment_status_summary_spec.rb index eb67dc24e..c60fb97a9 100644 --- a/spec/lib/pact_broker/matrix/deployment_status_summary_spec.rb +++ b/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 diff --git a/spec/lib/pact_broker/matrix/repository_query_limit_spec.rb b/spec/lib/pact_broker/matrix/repository_query_limit_spec.rb new file mode 100644 index 000000000..aab3fa4fa --- /dev/null +++ b/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