From 757f0301f09a2e5d95f00d5b22e78ec9ec44e5ed Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Thu, 6 Oct 2022 11:38:25 +1100 Subject: [PATCH] fix(wip pacts): fix performance issue encountered when removing explicitly specified pacts from the list of potential WIP pacts (#573) --- .../pacts_for_verification_repository.rb | 117 +++++++++++------- lib/pact_broker/pacts/repository.rb | 4 +- lib/pact_broker/pacts/service.rb | 3 +- 3 files changed, 75 insertions(+), 49 deletions(-) diff --git a/lib/pact_broker/pacts/pacts_for_verification_repository.rb b/lib/pact_broker/pacts/pacts_for_verification_repository.rb index 039a21e99..94759dd36 100644 --- a/lib/pact_broker/pacts/pacts_for_verification_repository.rb +++ b/lib/pact_broker/pacts/pacts_for_verification_repository.rb @@ -12,6 +12,7 @@ module PactBroker module Pacts + # rubocop: disable Metrics/ClassLength class PactsForVerificationRepository include PactBroker::Logging include PactBroker::Repositories @@ -26,6 +27,7 @@ class PactsForVerificationRepository :pact_version ] + # @return [VerifiablePact] an array of VerifiablePact objects def find(provider_name, consumer_version_selectors) selected_pacts = find_pacts_by_selector(provider_name, consumer_version_selectors) selected_pacts = selected_pacts + find_pacts_for_fallback_tags(selected_pacts, provider_name, consumer_version_selectors) @@ -43,7 +45,7 @@ def find(provider_name, consumer_version_selectors) # provider tags they are pending for. # Don't include pact publications that were created before the provider tag was first used # (that is, before the provider's git branch was created). - def find_wip provider_name, provider_version_branch, provider_tags_names, specified_pact_version_shas, options = {} + def find_wip(provider_name, provider_version_branch, provider_tags_names, explicitly_specified_verifiable_pacts, options = {}) # TODO not sure about this if provider_tags_names.empty? && provider_version_branch == nil log_debug_for_wip do @@ -53,7 +55,7 @@ def find_wip provider_name, provider_version_branch, provider_tags_names, specif end if provider_version_branch - return find_wip_pact_versions_for_provider_by_provider_branch(provider_name, provider_version_branch, specified_pact_version_shas, options) + return find_wip_pact_versions_for_provider_by_provider_branch(provider_name, provider_version_branch, explicitly_specified_verifiable_pacts, options) end provider = pacticipant_repository.find_by_name(provider_name) @@ -63,7 +65,7 @@ def find_wip provider_name, provider_version_branch, provider_tags_names, specif provider, provider_tags_names, wip_start_date, - specified_pact_version_shas, + explicitly_specified_verifiable_pacts, :latest_by_consumer_tag ) @@ -71,7 +73,7 @@ def find_wip provider_name, provider_version_branch, provider_tags_names, specif provider, provider_tags_names, wip_start_date, - specified_pact_version_shas, + explicitly_specified_verifiable_pacts, :latest_by_consumer_branch ) @@ -193,11 +195,11 @@ def merge_selected_pacts(selected_pacts) end # TODO ? find the WIP pacts by consumer branch - def find_wip_pact_versions_for_provider_by_provider_tags(provider, provider_tags_names, wip_start_date, specified_pact_version_shas, pact_publication_scope) + def find_wip_pact_versions_for_provider_by_provider_tags(provider, provider_tags_names, wip_start_date, explicitly_specified_verifiable_pacts, pact_publication_scope) potential_wip_pacts_by_consumer_tag_query = PactPublication.for_provider(provider).created_after(wip_start_date).send(pact_publication_scope) log_debug_for_wip do - log_pact_publications("Potential WIP pacts for provider tag(s) #{provider_tags_names.join(", ")} created after #{wip_start_date} by #{pact_publication_scope}", potential_wip_pacts_by_consumer_tag_query) + log_pact_publications_from_query("Potential WIP pacts for provider tag(s) #{provider_tags_names.join(", ")} created after #{wip_start_date} by #{pact_publication_scope}", potential_wip_pacts_by_consumer_tag_query) end tag_to_pact_publications = provider_tags_names.each_with_object({}) do | provider_tag_name, tag_to_pact_publication | @@ -205,7 +207,7 @@ def find_wip_pact_versions_for_provider_by_provider_tags(provider, provider_tags potential_wip_pacts_by_consumer_tag_query, provider, provider_tag_name, - specified_pact_version_shas + explicitly_specified_verifiable_pacts ) end @@ -225,7 +227,7 @@ def create_selectors_for_wip_pact(pact_publication) end end - def find_wip_pact_versions_for_provider_by_provider_branch(provider_name, provider_version_branch, specified_pact_version_shas, options) + def find_wip_pact_versions_for_provider_by_provider_branch(provider_name, provider_version_branch, explicitly_specified_verifiable_pacts, options) provider = pacticipant_repository.find_by_name(provider_name) wip_start_date = options.fetch(:include_wip_pacts_since) @@ -233,22 +235,22 @@ def find_wip_pact_versions_for_provider_by_provider_branch(provider_name, provid potential_wip_by_consumer_tag = PactPublication.for_provider(provider).created_after(wip_start_date).latest_by_consumer_tag log_debug_for_wip do - log_pact_publications("Potential WIP pacts for provider branch #{provider_version_branch} created after #{wip_start_date} by consumer branch", potential_wip_by_consumer_branch) - log_pact_publications("Potential WIP pacts for provider branch #{provider_version_branch} created after #{wip_start_date} by consumer tag", potential_wip_by_consumer_tag) + log_pact_publications_from_query("Potential WIP pacts for provider branch #{provider_version_branch} created after #{wip_start_date} by consumer branch", potential_wip_by_consumer_branch) + log_pact_publications_from_query("Potential WIP pacts for provider branch #{provider_version_branch} created after #{wip_start_date} by consumer tag", potential_wip_by_consumer_tag) end wip_pact_publications_by_branch = remove_non_wip_for_branch( potential_wip_by_consumer_branch, provider, provider_version_branch, - specified_pact_version_shas + explicitly_specified_verifiable_pacts ) wip_pact_publications_by_tag = remove_non_wip_for_branch( potential_wip_by_consumer_tag, provider, provider_version_branch, - specified_pact_version_shas + explicitly_specified_verifiable_pacts ) verifiable_pacts = (wip_pact_publications_by_branch + wip_pact_publications_by_tag).collect do | pact_publication | @@ -259,55 +261,61 @@ def find_wip_pact_versions_for_provider_by_provider_branch(provider_name, provid deduplicate_verifiable_pacts(verifiable_pacts).sort end - def remove_non_wip_for_branch(pact_publications_query, provider, provider_version_branch, specified_pact_version_shas) - specified_explicitly = pact_publications_query.for_pact_version_sha(specified_pact_version_shas) + def remove_non_wip_for_branch(pact_publications_query, provider, provider_version_branch, explicitly_specified_verifiable_pacts) verified_by_this_branch = pact_publications_query.successfully_verified_by_provider_branch_when_not_wip(provider.id, provider_version_branch) verified_by_other_branch = pact_publications_query.successfully_verified_by_provider_another_branch_before_this_branch_first_created(provider.id, provider_version_branch) log_debug_for_wip do - log_pact_publications("Ignoring pacts explicitly specified in the selectors", specified_explicitly) - log_pact_publications("Ignoring pacts successfully verified by this provider branch when not WIP", verified_by_this_branch) - log_pact_publications("Ignoring pacts successfully verified by another provider branch when not WIP", verified_by_other_branch) + log_pact_publications("Ignoring pacts explicitly specified in the selectors", explicitly_specified_verifiable_pacts) + log_pact_publications_from_query("Ignoring pacts successfully verified by this provider branch when not WIP", verified_by_this_branch) + log_pact_publications_from_query("Ignoring pacts successfully verified by another provider branch when not WIP", verified_by_other_branch) end - PactPublication.subtract( - pact_publications_query.eager(*PUBLICATION_ASSOCIATIONS_FOR_EAGER_LOAD).all, - specified_explicitly.all, - verified_by_this_branch.all, - verified_by_other_branch.all - ) + remove_explicitly_specified_verifiable_pacts(PactPublication.subtract( + pact_publications_query.eager(*PUBLICATION_ASSOCIATIONS_FOR_EAGER_LOAD).all, + verified_by_this_branch.all, + verified_by_other_branch.all), + explicitly_specified_verifiable_pacts) end - def remove_non_wip_for_tag(pact_publications_query, provider, tag, specified_pact_version_shas) - specified_explicitly = pact_publications_query.for_pact_version_sha(specified_pact_version_shas) + def remove_non_wip_for_tag(pact_publications_query, provider, tag, explicitly_specified_verifiable_pacts) verified_by_this_tag = pact_publications_query.successfully_verified_by_provider_tag_when_not_wip(tag) verified_by_another_tag = pact_publications_query.successfully_verified_by_provider_another_tag_before_this_tag_first_created(provider.id, tag) log_debug_for_wip do - log_pact_publications("Ignoring pacts explicitly specified in the selectors", specified_explicitly) - log_pact_publications("Ignoring pacts successfully verified by this provider tag when not WIP", verified_by_this_tag) - log_pact_publications("Ignoring pacts successfully verified by another provider tag when not WIP", verified_by_another_tag) + log_pact_publications("Ignoring pacts explicitly specified in the selectors", explicitly_specified_verifiable_pacts) + log_pact_publications_from_query("Ignoring pacts successfully verified by this provider tag when not WIP", verified_by_this_tag) + log_pact_publications_from_query("Ignoring pacts successfully verified by another provider tag when not WIP", verified_by_another_tag) end - PactPublication.subtract( - pact_publications_query.eager(*PUBLICATION_ASSOCIATIONS_FOR_EAGER_LOAD).all, - specified_explicitly.all, - verified_by_this_tag.all, - verified_by_another_tag.all) + remove_explicitly_specified_verifiable_pacts( + PactPublication.subtract( + pact_publications_query.eager(*PUBLICATION_ASSOCIATIONS_FOR_EAGER_LOAD).all, + verified_by_this_tag.all, + verified_by_another_tag.all), + explicitly_specified_verifiable_pacts) end - def collect_consumer_name_and_version_number(pact_publications_query) - pact_publications = pact_publications_query - .eager(:consumer, :consumer_version) - .order(:consumer_version_order) - .all_forbidding_lazy_load - .sort + def remove_explicitly_specified_verifiable_pacts(pact_publications, explicitly_specified_verifiable_pacts) + pact_publications.reject do | pact_publication | + explicitly_specified_verifiable_pacts.find{ | explict_pact | + explict_pact.consumer.id == pact_publication.consumer_id && + explict_pact.provider.id == pact_publication.provider_id && + explict_pact.pact_version_sha == pact_publication.pact_version_sha + } + end + end + def collect_consumer_name_and_version_number(pact_publications) pact_publications.collect do |p| - suffix = if p.values[:tag_name] - " (tag #{p.values[:tag_name]})" - elsif p.values[:branch_name] - " (branch #{p.values[:branch_name]})" + suffix = if p.respond_to?(:values) + if p.values[:tag_name] + " (tag #{p.values[:tag_name]})" + elsif p.values[:branch_name] + " (branch #{p.values[:branch_name]})" + else + "" + end else "" end @@ -316,8 +324,26 @@ def collect_consumer_name_and_version_number(pact_publications_query) end end - def log_pact_publications(message, pact_publications_query) - pact_publication_descriptions = collect_consumer_name_and_version_number(pact_publications_query) + def with_sorted_eager_fields(pact_publications_query) + pact_publications_query + .eager(:provider) + .eager(:consumer, :consumer_version) + .order(:consumer_version_order) + .all_forbidding_lazy_load + .sort + end + + def log_pact_publications_from_query(message, pact_publications_query) + pact_publication_descriptions = collect_consumer_name_and_version_number(with_sorted_eager_fields(pact_publications_query)) + if pact_publication_descriptions.any? + logger.debug("#{message}", pact_publication_descriptions) + else + logger.debug("#{message} (none)") + end + end + + def log_pact_publications(message, pact_publications) + pact_publication_descriptions = collect_consumer_name_and_version_number(pact_publications) if pact_publication_descriptions.any? logger.debug("#{message}", pact_publication_descriptions) else @@ -333,5 +359,6 @@ def log_debug_for_wip end end end + # rubocop: enable Metrics/ClassLength end end diff --git a/lib/pact_broker/pacts/repository.rb b/lib/pact_broker/pacts/repository.rb index 45367d64c..8f0d039f0 100644 --- a/lib/pact_broker/pacts/repository.rb +++ b/lib/pact_broker/pacts/repository.rb @@ -158,8 +158,8 @@ def find_for_verification(provider_name, consumer_version_selectors) PactsForVerificationRepository.new.find(provider_name, consumer_version_selectors) end - def find_wip_pact_versions_for_provider provider_name, provider_version_branch, provider_tags_names, specified_pact_version_shas, options = {} - PactsForVerificationRepository.new.find_wip(provider_name, provider_version_branch, provider_tags_names, specified_pact_version_shas, options) + def find_wip_pact_versions_for_provider provider_name, provider_version_branch, provider_tags_names, explicitly_specified_verifiable_pacts, options = {} + PactsForVerificationRepository.new.find_wip(provider_name, provider_version_branch, provider_tags_names, explicitly_specified_verifiable_pacts, options) end def find_pact_versions_for_provider provider_name, tag = nil diff --git a/lib/pact_broker/pacts/service.rb b/lib/pact_broker/pacts/service.rb index 52ff65299..0d8ebdbb5 100644 --- a/lib/pact_broker/pacts/service.rb +++ b/lib/pact_broker/pacts/service.rb @@ -126,8 +126,7 @@ def find_for_verification(provider_name, provider_version_branch, provider_versi end verifiable_wip_pacts = if options[:include_wip_pacts_since] - specified_pact_version_shas = explicitly_specified_verifiable_pacts.collect(&:pact_version_sha) - pact_repository.find_wip_pact_versions_for_provider(provider_name, provider_version_branch, provider_version_tags, specified_pact_version_shas, options) + pact_repository.find_wip_pact_versions_for_provider(provider_name, provider_version_branch, provider_version_tags, explicitly_specified_verifiable_pacts, options) else [] end