From a947e40926acc9cb29ddef04ff1e28169e695f78 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Wed, 17 Jan 2024 11:23:55 +1100 Subject: [PATCH] feat: improve performance of publication for very large pacts by calculating the content SHA only once per request --- .../decorators/publish_contract_decorator.rb | 7 +++++- lib/pact_broker/api/resources/pact.rb | 10 +++++--- .../api/resources/publish_contracts.rb | 2 +- .../contracts/contract_to_publish.rb | 9 ++++---- lib/pact_broker/contracts/service.rb | 4 ++-- lib/pact_broker/pacts/pact_params.rb | 2 +- lib/pact_broker/pacts/service.rb | 23 +++++++++---------- lib/pact_broker/test/test_data_builder.rb | 8 ++++++- .../lib/pact_broker/contracts/service_spec.rb | 9 +++++--- spec/lib/pact_broker/pacts/service_spec.rb | 15 ++---------- 10 files changed, 47 insertions(+), 42 deletions(-) diff --git a/lib/pact_broker/api/decorators/publish_contract_decorator.rb b/lib/pact_broker/api/decorators/publish_contract_decorator.rb index 48e3c40f7..ec336372f 100644 --- a/lib/pact_broker/api/decorators/publish_contract_decorator.rb +++ b/lib/pact_broker/api/decorators/publish_contract_decorator.rb @@ -11,8 +11,13 @@ class PublishContractDecorator < BaseDecorator property :provider_name property :specification property :content_type - property :decoded_content + property :decoded_content, setter: -> (fragment:, represented:, user_options:, **) { + represented.decoded_content = fragment + # Set the pact version sha when we set the content + represented.pact_version_sha = user_options.fetch(:sha_generator).call(fragment) + } property :on_conflict, default: "overwrite" + end end end diff --git a/lib/pact_broker/api/resources/pact.rb b/lib/pact_broker/api/resources/pact.rb index 226ec827d..a27c9a4d6 100644 --- a/lib/pact_broker/api/resources/pact.rb +++ b/lib/pact_broker/api/resources/pact.rb @@ -71,9 +71,9 @@ def from_json subscribe(PactBroker::Integrations::EventListener.new) do handle_webhook_events do if request.patch? && resource_exists? - @pact = pact_service.merge_pact(pact_params) + @pact = pact_service.merge_pact(pact_params.merge(pact_version_sha: pact_version_sha)) else - @pact = pact_service.create_or_update_pact(pact_params) + @pact = pact_service.create_or_update_pact(pact_params.merge(pact_version_sha: pact_version_sha)) end end end @@ -114,7 +114,7 @@ def pact end def disallowed_modification? - if request.really_put? && pact_service.disallowed_modification?(pact, pact_params.json_content) + if request.really_put? && pact_service.disallowed_modification?(pact, pact_version_sha) message_params = { consumer_name: pact_params.consumer_name, consumer_version_number: pact_params.consumer_version_number, provider_name: pact_params.provider_name } set_json_error_message(message("errors.validation.pact_content_modification_not_allowed", message_params)) true @@ -126,6 +126,10 @@ def disallowed_modification? def schema api_contract_class(:put_pact_params_contract) end + + def pact_version_sha + @pact_version_sha ||= pact_service.generate_sha(pact_params.json_content) + end end end end diff --git a/lib/pact_broker/api/resources/publish_contracts.rb b/lib/pact_broker/api/resources/publish_contracts.rb index 45b921735..c7e598c6e 100644 --- a/lib/pact_broker/api/resources/publish_contracts.rb +++ b/lib/pact_broker/api/resources/publish_contracts.rb @@ -53,7 +53,7 @@ def policy_record_context private def parsed_contracts - @parsed_contracts ||= decorator_class(:publish_contracts_decorator).new(PactBroker::Contracts::ContractsToPublish.new).from_hash(params) + @parsed_contracts ||= decorator_class(:publish_contracts_decorator).new(PactBroker::Contracts::ContractsToPublish.new).from_hash(params, { user_options: { sha_generator: PactBroker.configuration.sha_generator } } ) end def params diff --git a/lib/pact_broker/contracts/contract_to_publish.rb b/lib/pact_broker/contracts/contract_to_publish.rb index 96a7ad5a5..a710925cf 100644 --- a/lib/pact_broker/contracts/contract_to_publish.rb +++ b/lib/pact_broker/contracts/contract_to_publish.rb @@ -1,11 +1,10 @@ module PactBroker module Contracts - ContractToPublish = Struct.new(:consumer_name, :provider_name, :decoded_content, :content_type, :specification, :on_conflict) do - # rubocop: disable Metrics/ParameterLists - def self.from_hash(consumer_name: nil, provider_name: nil, decoded_content: nil, content_type: nil, specification: nil, on_conflict: nil) - new(consumer_name, provider_name, decoded_content, content_type, specification, on_conflict) + ContractToPublish = Struct.new(:consumer_name, :provider_name, :decoded_content, :content_type, :specification, :on_conflict, :pact_version_sha, keyword_init: true) do + + def self.from_hash(hash) + new(**hash) end - # rubocop: enable Metrics/ParameterLists def pact? specification == "pact" diff --git a/lib/pact_broker/contracts/service.rb b/lib/pact_broker/contracts/service.rb index 6847419aa..7776bcecb 100644 --- a/lib/pact_broker/contracts/service.rb +++ b/lib/pact_broker/contracts/service.rb @@ -57,7 +57,7 @@ def add_pact_conflict_notices(notices, parsed_contracts) parsed_contracts.contracts.collect do | contract_to_publish | pact_params = create_pact_params(parsed_contracts, contract_to_publish) existing_pact = pact_service.find_pact(pact_params) - if existing_pact && pact_service.disallowed_modification?(existing_pact, contract_to_publish.decoded_content) + if existing_pact && pact_service.disallowed_modification?(existing_pact, contract_to_publish.pact_version_sha) add_pact_conflict_notice(notices, parsed_contracts, contract_to_publish, existing_pact.json_content, contract_to_publish.decoded_content) end end @@ -134,7 +134,7 @@ def create_pacts(parsed_contracts, base_url) pact_params = create_pact_params(parsed_contracts, contract_to_publish) existing_pact = pact_service.find_pact(pact_params) listener = TriggeredWebhooksCreatedListener.new - created_pact = create_or_merge_pact(contract_to_publish.merge?, existing_pact, pact_params, listener) + created_pact = create_or_merge_pact(contract_to_publish.merge?, existing_pact, pact_params.merge(pact_version_sha: contract_to_publish.pact_version_sha), listener) notices.concat(notices_for_pact(parsed_contracts, contract_to_publish, existing_pact, created_pact, listener, base_url)) created_pact end diff --git a/lib/pact_broker/pacts/pact_params.rb b/lib/pact_broker/pacts/pact_params.rb index e7f75723c..c6d9cb733 100644 --- a/lib/pact_broker/pacts/pact_params.rb +++ b/lib/pact_broker/pacts/pact_params.rb @@ -20,7 +20,7 @@ def self.from_path_info path_info ) end - def self.from_request request, path_info + def self.from_request(request, path_info) json_content = request.body.to_s parsed_content = begin parsed = JSON.parse(json_content, PACT_PARSING_OPTIONS) diff --git a/lib/pact_broker/pacts/service.rb b/lib/pact_broker/pacts/service.rb index fb59bd7ad..6ec1adacb 100644 --- a/lib/pact_broker/pacts/service.rb +++ b/lib/pact_broker/pacts/service.rb @@ -22,6 +22,10 @@ module Service extend PactBroker::Messages extend SquashPactsForVerification + def generate_sha(json_content) + PactBroker.configuration.sha_generator.call(json_content) + end + def find_latest_pact params pact_repository.find_latest_pact(params[:consumer_name], params[:provider_name], params[:tag], params[:branch_name]) end @@ -154,10 +158,12 @@ def find_for_verification_publication(pact_params, consumer_version_selector_has end # Overwriting an existing pact with the same consumer/provider/consumer version number + # by creating a new revision (that is, a new PactPublication with an incremented revision number) + # Modifing pacts is strongly discouraged now, and support for it will be dropped in the next major version of the Pact Broker def create_pact_revision params, existing_pact logger.info("Updating existing pact publication", params.without(:json_content)) logger.debug("Content #{params[:json_content]}") - pact_version_sha = generate_sha(params[:json_content]) + pact_version_sha = params.fetch(:pact_version_sha) json_content = add_interaction_ids(params[:json_content]) update_params = { pact_version_sha: pact_version_sha, json_content: json_content } updated_pact = pact_repository.update(existing_pact.id, update_params) @@ -178,21 +184,20 @@ def create_pact_revision params, existing_pact private :create_pact_revision - def disallowed_modification?(existing_pact, new_json_content) - !PactBroker.configuration.allow_dangerous_contract_modification && existing_pact && existing_pact.pact_version_sha != generate_sha(new_json_content) + def disallowed_modification?(existing_pact, new_pact_version_sha) + !PactBroker.configuration.allow_dangerous_contract_modification && existing_pact && existing_pact.pact_version_sha != new_pact_version_sha end # When no publication for the given consumer/provider/consumer version number exists - def create_pact params, version, provider + def create_pact(params, version, provider) logger.info("Creating new pact publication", params.without(:json_content)) logger.debug("Content #{params[:json_content]}") - pact_version_sha = generate_sha(params[:json_content]) json_content = add_interaction_ids(params[:json_content]) pact = pact_repository.create( version_id: version.id, provider_id: provider.id, consumer_id: version.pacticipant_id, - pact_version_sha: pact_version_sha, + pact_version_sha: params.fetch(:pact_version_sha), json_content: json_content, version: version ) @@ -212,12 +217,6 @@ def create_pact params, version, provider private :create_pact - def generate_sha(json_content) - PactBroker.configuration.sha_generator.call(json_content) - end - - private :generate_sha - def add_interaction_ids(json_content) Content.from_json(json_content).with_ids.to_json end diff --git a/lib/pact_broker/test/test_data_builder.rb b/lib/pact_broker/test/test_data_builder.rb index 2cdd4d2b2..92d6bbcbc 100644 --- a/lib/pact_broker/test/test_data_builder.rb +++ b/lib/pact_broker/test/test_data_builder.rb @@ -256,7 +256,13 @@ def create_label label_name def publish_pact(consumer_name:, provider_name:, consumer_version_number: , tags: nil, branch: nil, build_url: nil, json_content: nil) json_content = json_content || random_json_content(consumer_name, provider_name) contracts = [ - PactBroker::Contracts::ContractToPublish.from_hash(consumer_name: consumer_name, provider_name: provider_name, decoded_content: json_content, content_type: "application/json", specification: "pact") + PactBroker::Contracts::ContractToPublish.from_hash( + consumer_name: consumer_name, + provider_name: provider_name, + decoded_content: json_content, + content_type: "application/json", + specification: "pact", + pact_version_sha: PactBroker::Pacts::GenerateSha.call(json_content)) ] contracts_to_publish = PactBroker::Contracts::ContractsToPublish.from_hash( pacticipant_name: consumer_name, diff --git a/spec/lib/pact_broker/contracts/service_spec.rb b/spec/lib/pact_broker/contracts/service_spec.rb index fafa7891f..349fa38f7 100644 --- a/spec/lib/pact_broker/contracts/service_spec.rb +++ b/spec/lib/pact_broker/contracts/service_spec.rb @@ -18,10 +18,11 @@ module Contracts let(:branch) { "main" } let(:contracts) { [contract_1] } let(:contract_1) do - ContractToPublish.from_hash( + ContractToPublish.new( consumer_name: "Foo", provider_name: "Bar", decoded_content: decoded_contract, + pact_version_sha: PactBroker::Pacts::GenerateSha.call(decoded_contract), specification: "pact", on_conflict: on_conflict ) @@ -149,17 +150,19 @@ module Contracts let(:branch) { "main" } let(:contracts) { [contract_1] } let(:contract_1) do - ContractToPublish.from_hash( + ContractToPublish.new( consumer_name: "Foo", provider_name: "Bar", decoded_content: decoded_contract, specification: "pact", - on_conflict: on_conflict + on_conflict: on_conflict, + pact_version_sha: new_pact_version_sha ) end let(:contract_hash) { { consumer: { name: "Foo" }, provider: { name: "Bar" }, interactions: [{a: "b"}] } } let(:decoded_contract) { contract_hash.to_json } + let(:new_pact_version_sha) { PactBroker::Pacts::GenerateSha.call(decoded_contract) } subject { Service.conflict_notices(contracts_to_publish, base_url: "base_url") } diff --git a/spec/lib/pact_broker/pacts/service_spec.rb b/spec/lib/pact_broker/pacts/service_spec.rb index a656a710c..3aa0a156c 100644 --- a/spec/lib/pact_broker/pacts/service_spec.rb +++ b/spec/lib/pact_broker/pacts/service_spec.rb @@ -31,7 +31,8 @@ module Pacts consumer_name: "Foo", provider_name: "Bar", consumer_version_number: "1", - json_content: json_content + json_content: json_content, + pact_version_sha: PactBroker::Pacts::GenerateSha.call(json_content) } end let(:content) { double("content") } @@ -48,12 +49,6 @@ module Pacts subject { Service.create_or_update_pact(params) } context "when no pact exists with the same params" do - it "creates the sha before adding the interaction ids" do - expect(PactBroker::Pacts::GenerateSha).to receive(:call).ordered - expect(content).to receive(:with_ids).ordered - subject - end - it "saves the pact interactions/messages with ids added to them" do expect(pact_repository).to receive(:create).with hash_including(json_content: json_content_with_ids) subject @@ -135,12 +130,6 @@ module Pacts let(:expected_event_context) { { consumer_version_tags: ["dev"] } } - it "creates the sha before adding the interaction ids" do - expect(PactBroker::Pacts::GenerateSha).to receive(:call).ordered - expect(content).to receive(:with_ids).ordered - subject - end - it "saves the pact interactions/messages with ids added to them" do expect(pact_repository).to receive(:update).with(anything, hash_including(json_content: json_content_with_ids)) subject