From ed714f0306c21946368e285a7f0ca3db835067db Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 20 May 2022 18:40:40 +1000 Subject: [PATCH] feat: check for potential duplicate pacticipants in publish contracts endpoint (#558) --- .../resources/pacticipant_resource_methods.rb | 1 - .../api/resources/publish_contracts.rb | 2 +- .../contracts/contract_to_publish.rb | 4 +++ .../contracts/contracts_to_publish.rb | 4 +++ lib/pact_broker/contracts/service.rb | 28 ++++++++++++++---- lib/pact_broker/locale/en.yml | 10 ++++--- lib/pact_broker/messages.rb | 15 ---------- lib/pact_broker/pacticipants/service.rb | 12 +++++++- .../lib/pact_broker/contracts/service_spec.rb | 24 ++++++++++++++- spec/lib/pact_broker/messages_spec.rb | 29 ------------------- .../pact_broker/pacticipants/service_spec.rb | 9 ++---- 11 files changed, 75 insertions(+), 63 deletions(-) delete mode 100644 spec/lib/pact_broker/messages_spec.rb diff --git a/lib/pact_broker/api/resources/pacticipant_resource_methods.rb b/lib/pact_broker/api/resources/pacticipant_resource_methods.rb index 43e9dfc83..68d45c7d9 100644 --- a/lib/pact_broker/api/resources/pacticipant_resource_methods.rb +++ b/lib/pact_broker/api/resources/pacticipant_resource_methods.rb @@ -1,7 +1,6 @@ module PactBroker module Api module Resources - module PacticipantResourceMethods def potential_duplicate_pacticipants? pacticipant_names diff --git a/lib/pact_broker/api/resources/publish_contracts.rb b/lib/pact_broker/api/resources/publish_contracts.rb index 27c73c0cd..8bc125a5d 100644 --- a/lib/pact_broker/api/resources/publish_contracts.rb +++ b/lib/pact_broker/api/resources/publish_contracts.rb @@ -100,7 +100,7 @@ def set_conflict_response end def conflict_notices - @conflict_notices ||= contract_service.conflict_notices(parsed_contracts) + @conflict_notices ||= contract_service.conflict_notices(parsed_contracts, base_url: base_url) end def base64_decode(content) diff --git a/lib/pact_broker/contracts/contract_to_publish.rb b/lib/pact_broker/contracts/contract_to_publish.rb index 99c0f7f65..96a7ad5a5 100644 --- a/lib/pact_broker/contracts/contract_to_publish.rb +++ b/lib/pact_broker/contracts/contract_to_publish.rb @@ -14,6 +14,10 @@ def pact? def merge? on_conflict == "merge" end + + def pacticipant_names + [consumer_name, provider_name] + end end end end diff --git a/lib/pact_broker/contracts/contracts_to_publish.rb b/lib/pact_broker/contracts/contracts_to_publish.rb index dd6df5a24..3440832f2 100644 --- a/lib/pact_broker/contracts/contracts_to_publish.rb +++ b/lib/pact_broker/contracts/contracts_to_publish.rb @@ -6,6 +6,10 @@ def self.from_hash(pacticipant_name: nil, pacticipant_version_number: nil, tags: new(pacticipant_name, pacticipant_version_number, tags, branch, build_url, contracts) end # rubocop: enable Metrics/ParameterLists + + def pacticipant_names + contracts.flat_map(&:pacticipant_names).uniq + end end end end diff --git a/lib/pact_broker/contracts/service.rb b/lib/pact_broker/contracts/service.rb index 53d359794..7b0c77f23 100644 --- a/lib/pact_broker/contracts/service.rb +++ b/lib/pact_broker/contracts/service.rb @@ -43,19 +43,26 @@ def publish(parsed_contracts, base_url: ) ) end - def conflict_notices(parsed_contracts) + def conflict_notices(parsed_contracts, base_url: ) notices = [] + add_pact_conflict_notices(notices, parsed_contracts) + add_pacticipant_conflict_notices(notices, parsed_contracts, base_url) + notices + end + + 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) - add_conflict_notice(notices, parsed_contracts, contract_to_publish, existing_pact.json_content, contract_to_publish.decoded_content) + add_pact_conflict_notice(notices, parsed_contracts, contract_to_publish, existing_pact.json_content, contract_to_publish.decoded_content) end end - notices end - def add_conflict_notice(notices, parsed_contracts, contract_to_publish, existing_json_content, new_json_content) + private :add_pact_conflict_notices + + def add_pact_conflict_notice(notices, parsed_contracts, contract_to_publish, existing_json_content, new_json_content) message_params = { consumer_name: contract_to_publish.provider_name, consumer_version_number: parsed_contracts.pacticipant_version_number, @@ -65,7 +72,18 @@ def add_conflict_notice(notices, parsed_contracts, contract_to_publish, existing notices << Notice.info(PactBroker::Pacts::CreateFormattedDiff.call(new_json_content, existing_json_content)) end - private :add_conflict_notice + private :add_pact_conflict_notice + + def add_pacticipant_conflict_notices(notices, parsed_contracts, base_url) + if PactBroker.configuration.check_for_potential_duplicate_pacticipant_names + duplicate_pacticipant_messages = pacticipant_service.messages_for_potential_duplicate_pacticipants(parsed_contracts.pacticipant_names, base_url) + duplicate_pacticipant_messages.each do | message_text | + notices << Notice.error(message_text) + end + end + end + + private :add_pacticipant_conflict_notices def create_version(parsed_contracts) version_params = { diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml index f0828122c..a44b852d1 100644 --- a/lib/pact_broker/locale/en.yml +++ b/lib/pact_broker/locale/en.yml @@ -96,12 +96,14 @@ en: invalid_limit: The limit must be 1 or greater. duplicate_pacticipant: | This is the first time a pact has been published for "%{new_name}". - The name "%{new_name}" is very similar to the following existing consumers/providers: + The name "%{new_name}" is very similar to the following existing pacticipants: %{existing_names} If you meant to specify one of the above names, please correct the pact configuration, and re-publish the pact. - If the pact is intended to be for a new consumer or provider, please manually create "%{new_name}" using the following command, and then re-publish the pact: - $ curl -v -XPOST -H "Content-Type: application/json" -d "{\"name\": \"%{new_name}\"}" %{create_pacticipant_url} - If the pact broker requires basic authentication, add '-u ' to the command. + If the pact is intended to be for a new consumer or provider, please manually create "%{new_name}" using the following command from the Pact Broker CLI, and then re-publish the pact: + + pact-broker create-or-update-pacticipant --name "%{new_name}" --broker-base-url %{base_url} + + For information on the Pact Broker CLI see https://docs.pact.io/go/pbc-cli To disable this check, set `check_for_potential_duplicate_pacticipant_names` to false in the configuration. new_line_in_url_path: URL path cannot contain a new line character. tab_in_url_path: URL path cannot contain a tab character. diff --git a/lib/pact_broker/messages.rb b/lib/pact_broker/messages.rb index fc90cc9e1..ebfae29d9 100644 --- a/lib/pact_broker/messages.rb +++ b/lib/pact_broker/messages.rb @@ -23,15 +23,6 @@ def validation_message key, options = {} message("errors.validation." + key, options) end - def potential_duplicate_pacticipant_message new_name, potential_duplicate_pacticipants, base_url - existing_names = potential_duplicate_pacticipants. - collect{ | p | "* #{p.name}" }.join("\n") - message("errors.duplicate_pacticipant", - new_name: new_name, - existing_names: existing_names, - create_pacticipant_url: pacticipants_url(base_url)) - end - def pluralize(word, count) if count == 1 word @@ -43,11 +34,5 @@ def pluralize(word, count) end end end - - private - - def pacticipants_url base_url - PactBroker::Api::PactBrokerUrls.pacticipants_url base_url - end end end diff --git a/lib/pact_broker/pacticipants/service.rb b/lib/pact_broker/pacticipants/service.rb index 84c79d2ee..13fa82578 100644 --- a/lib/pact_broker/pacticipants/service.rb +++ b/lib/pact_broker/pacticipants/service.rb @@ -10,13 +10,14 @@ class Service extend PactBroker::Repositories extend PactBroker::Services include PactBroker::Logging + extend PactBroker::Messages def self.messages_for_potential_duplicate_pacticipants(pacticipant_names, base_url) messages = [] pacticipant_names.each do | name | potential_duplicate_pacticipants = find_potential_duplicate_pacticipants(name) if potential_duplicate_pacticipants.any? - messages << Messages.potential_duplicate_pacticipant_message(name, potential_duplicate_pacticipants, base_url) + messages << potential_duplicate_pacticipant_message(name, potential_duplicate_pacticipants, base_url) end end messages @@ -98,6 +99,15 @@ def self.maybe_set_main_branch(pacticipant, potential_main_branch) pacticipant end end + + private_class_method def self.potential_duplicate_pacticipant_message(new_name, potential_duplicate_pacticipants, base_url) + existing_names = potential_duplicate_pacticipants. + collect{ | p | "* #{p.name}" }.join("\n") + message("errors.duplicate_pacticipant", + new_name: new_name, + existing_names: existing_names, + base_url: base_url) + end end end end diff --git a/spec/lib/pact_broker/contracts/service_spec.rb b/spec/lib/pact_broker/contracts/service_spec.rb index 77213d37f..fafa7891f 100644 --- a/spec/lib/pact_broker/contracts/service_spec.rb +++ b/spec/lib/pact_broker/contracts/service_spec.rb @@ -126,6 +126,15 @@ module Contracts end describe "#conflict_errors" do + before do + allow(Service).to receive(:pacticipant_service).and_return(pacticipant_service) + allow(pacticipant_service).to receive(:messages_for_potential_duplicate_pacticipants).and_return(duplicate_pacticipant_messages) + allow(PactBroker.configuration).to receive(:check_for_potential_duplicate_pacticipant_names).and_return(true) + end + + let(:pacticipant_service) { class_double("PactBroker::Pacticipants::Service").as_stubbed_const } + let(:duplicate_pacticipant_messages) { [] } + let(:contracts_to_publish) do ContractsToPublish.from_hash( pacticipant_name: "Foo", @@ -152,7 +161,7 @@ module Contracts let(:contract_hash) { { consumer: { name: "Foo" }, provider: { name: "Bar" }, interactions: [{a: "b"}] } } let(:decoded_contract) { contract_hash.to_json } - subject { Service.conflict_notices(contracts_to_publish) } + subject { Service.conflict_notices(contracts_to_publish, base_url: "base_url") } context "when a pact already exists" do before do @@ -187,6 +196,19 @@ module Contracts context "when no pacts exist" do it { is_expected.to be_empty } end + + it "checks if there are potential duplicate pacticipants" do + expect(pacticipant_service).to receive(:messages_for_potential_duplicate_pacticipants).with(["Foo", "Bar"], "base_url") + subject + end + + context "when there are potential duplicate pacticipants" do + let(:duplicate_pacticipant_messages) { ["some message" ] } + + it "returns the messages as error notices" do + expect(subject).to contain_exactly(have_attributes(type: "error", text: "some message")) + end + end end end end diff --git a/spec/lib/pact_broker/messages_spec.rb b/spec/lib/pact_broker/messages_spec.rb deleted file mode 100644 index 858bf5eb2..000000000 --- a/spec/lib/pact_broker/messages_spec.rb +++ /dev/null @@ -1,29 +0,0 @@ -require "spec_helper" -require "pact_broker/messages" - -module PactBroker - module Messages - describe "#potential_duplicate_pacticipant_message" do - let(:new_name) { "Contracts" } - let(:fred) { double("Contracts Service", name: "Contracts Service") } - let(:frederich) { double("Accepted Contracts", name: "Accepted Contracts") } - let(:potential_duplicate_pacticipants) { [fred, frederich]} - - let(:expected_message) { String.new <<-EOS -This is the first time a pact has been published for "Contracts". -The name "Contracts" is very similar to the following existing consumers/providers: -* Contracts Service -* Accepted Contracts -If you meant to specify one of the above names, please correct the pact configuration, and re-publish the pact. -If the pact is intended to be for a new consumer or provider, please manually create "Contracts" using the following command, and then re-publish the pact: -$ curl -v -XPOST -H "Content-Type: application/json" -d "{\\\"name\\\": \\\"Contracts\\\"}" http://example.org/pacticipants -EOS - } - subject { Messages.potential_duplicate_pacticipant_message new_name, potential_duplicate_pacticipants, "http://example.org" } - - it "returns a message" do - expect(subject).to start_with expected_message - end - end - end -end diff --git a/spec/lib/pact_broker/pacticipants/service_spec.rb b/spec/lib/pact_broker/pacticipants/service_spec.rb index acd148ed2..b0a563582 100644 --- a/spec/lib/pact_broker/pacticipants/service_spec.rb +++ b/spec/lib/pact_broker/pacticipants/service_spec.rb @@ -1,4 +1,3 @@ -require "spec_helper" require "pact_broker/pacticipants/service" require "pact_broker/domain/tag" require "pact_broker/domain/pact" @@ -15,7 +14,6 @@ module Pacticipants subject { Service } describe ".messages_for_potential_duplicate_pacticipants" do - let(:base_url) { "http://example.org" } let(:fred_duplicates) { [double("Frederich pacticipant")] } let(:mary_dulicates) { [double("Marta pacticipant")] } @@ -23,7 +21,7 @@ module Pacticipants before do allow(Service).to receive(:find_potential_duplicate_pacticipants).with("Fred").and_return(fred_duplicates) allow(Service).to receive(:find_potential_duplicate_pacticipants).with("Mary").and_return(mary_dulicates) - allow(Messages).to receive(:potential_duplicate_pacticipant_message).and_return("message1", "message2") + allow(Service).to receive(:potential_duplicate_pacticipant_message).and_return("message1", "message2") end subject { Service.messages_for_potential_duplicate_pacticipants ["Fred", "Mary"], base_url } @@ -35,10 +33,9 @@ module Pacticipants end context "when there are potential duplicates" do - it "creates a message for each dupliate" do - expect(Messages).to receive(:potential_duplicate_pacticipant_message).with("Fred", fred_duplicates, base_url) - expect(Messages).to receive(:potential_duplicate_pacticipant_message).with("Mary", mary_dulicates, base_url) + expect(Service).to receive(:potential_duplicate_pacticipant_message).with("Fred", fred_duplicates, base_url) + expect(Service).to receive(:potential_duplicate_pacticipant_message).with("Mary", mary_dulicates, base_url) subject end