From 956227fe4b0a175f656355166410a656b85981f1 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Wed, 13 Oct 2021 11:52:40 +1100 Subject: [PATCH] feat: apply allow_dangerous_contract_modification setting when publishing using the /contracts/publish endpoint --- lib/pact_broker/api/resources/pact.rb | 2 +- .../api/resources/publish_contracts.rb | 31 +++++++-- .../contracts_publication_results.rb | 9 +-- lib/pact_broker/contracts/notice.rb | 8 +++ lib/pact_broker/contracts/service.rb | 25 ++++++++ .../views/index/publish-contracts.markdown | 34 +++++++++- lib/pact_broker/locale/en.yml | 2 +- lib/pact_broker/pacts/content.rb | 18 ++++++ .../pacts/create_formatted_diff.rb | 11 +++- lib/pact_broker/pacts/sort_content.rb | 1 + spec/features/publish_pact_all_in_one_spec.rb | 19 ++++++ .../lib/pact_broker/contracts/service_spec.rb | 64 +++++++++++++++++++ 12 files changed, 207 insertions(+), 17 deletions(-) diff --git a/lib/pact_broker/api/resources/pact.rb b/lib/pact_broker/api/resources/pact.rb index 05bee573e..dbd73267a 100644 --- a/lib/pact_broker/api/resources/pact.rb +++ b/lib/pact_broker/api/resources/pact.rb @@ -110,7 +110,7 @@ def pact def disallowed_modification? if request.really_put? && pact_service.disallowed_modification?(pact, pact_params.json_content) - message_params = { consumer_name: pact_params.consumer_name, consumer_version_number: pact_params.consumer_version_number } + 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 else diff --git a/lib/pact_broker/api/resources/publish_contracts.rb b/lib/pact_broker/api/resources/publish_contracts.rb index e19e4b399..64eb723ba 100644 --- a/lib/pact_broker/api/resources/publish_contracts.rb +++ b/lib/pact_broker/api/resources/publish_contracts.rb @@ -31,11 +31,13 @@ def malformed_request? end def process_post - handle_webhook_events(consumer_version_branch: parsed_contracts.branch, build_url: parsed_contracts.build_url) do - results = contract_service.publish(parsed_contracts, base_url: base_url) - response.body = decorator_class(:publish_contracts_results_decorator).new(results).to_json(decorator_options) + if conflict_notices.any? + set_conflict_response + 409 + else + publish_contracts + true end - true end def policy_name @@ -75,6 +77,27 @@ def decode_and_parse_content(contract) contract["decodedParsedContent"] = PactBroker::Pacts::Parse.call(contract["decodedContent"]) rescue nil end end + + def publish_contracts + handle_webhook_events(consumer_version_branch: parsed_contracts.branch, build_url: parsed_contracts.build_url) do + results = contract_service.publish(parsed_contracts, base_url: base_url) + response.body = decorator_class(:publish_contracts_results_decorator).new(results).to_json(decorator_options) + end + end + + def set_conflict_response + response.body = { + notices: conflict_notices.collect(&:to_h), + errors: { + contracts: conflict_notices.select(&:error?).collect(&:text) + } + }.to_json + response.headers["Content-Type"] = "application/json;charset=utf-8" + end + + def conflict_notices + @conflict_notices ||= contract_service.conflict_notices(parsed_contracts) + end end end end diff --git a/lib/pact_broker/contracts/contracts_publication_results.rb b/lib/pact_broker/contracts/contracts_publication_results.rb index 15862b668..9dd1234d5 100644 --- a/lib/pact_broker/contracts/contracts_publication_results.rb +++ b/lib/pact_broker/contracts/contracts_publication_results.rb @@ -1,13 +1,8 @@ module PactBroker module Contracts - ContractsPublicationResults = Struct.new(:pacticipant, :version, :tags, :contracts, :notices) do + ContractsPublicationResults = Struct.new(:pacticipant, :version, :tags, :contracts, :notices, keyword_init: true) do def self.from_hash(params) - new(params[:pacticipant], - params[:version], - params[:tags], - params[:contracts], - params[:notices] - ) + new(params) end end end diff --git a/lib/pact_broker/contracts/notice.rb b/lib/pact_broker/contracts/notice.rb index 98b85505e..94f1f28dd 100644 --- a/lib/pact_broker/contracts/notice.rb +++ b/lib/pact_broker/contracts/notice.rb @@ -20,6 +20,14 @@ def self.prompt(text) def self.success(text) Notice.new("success", text) end + + def self.error(text) + Notice.new("error", text) + end + + def error? + type == "error" + end end end end diff --git a/lib/pact_broker/contracts/service.rb b/lib/pact_broker/contracts/service.rb index 97369c326..5d4feda02 100644 --- a/lib/pact_broker/contracts/service.rb +++ b/lib/pact_broker/contracts/service.rb @@ -6,6 +6,7 @@ require "pact_broker/contracts/notice" require "pact_broker/events/subscriber" require "pact_broker/api/pact_broker_urls" +require "pact_broker/pacts/create_formatted_diff" module PactBroker module Contracts @@ -42,6 +43,30 @@ def publish(parsed_contracts, base_url: ) ) end + def conflict_notices(parsed_contracts) + notices = [] + parsed_contracts.contracts.collect do | contract_to_publish, i | + 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) + end + end + notices + end + + def add_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, + provider_name: contract_to_publish.provider_name + } + notices << Notice.error(message("errors.validation.pact_content_modification_not_allowed", message_params)) + notices << Notice.info(PactBroker::Pacts::CreateFormattedDiff.call(new_json_content, existing_json_content)) + end + + private :add_conflict_notice + def create_version(parsed_contracts) version_params = { build_url: parsed_contracts.build_url, diff --git a/lib/pact_broker/doc/views/index/publish-contracts.markdown b/lib/pact_broker/doc/views/index/publish-contracts.markdown index f54790971..2c2efe213 100644 --- a/lib/pact_broker/doc/views/index/publish-contracts.markdown +++ b/lib/pact_broker/doc/views/index/publish-contracts.markdown @@ -12,6 +12,8 @@ This is the preferred endpoint with which to publish contracts (previously, cont The previous tag and pact endpoints are still supported, however, future features that build on this endpoint may not be able to be backported into those endpoints. +This endpoint is designed to be used by a command line tool, and hence, the response notices are designed for output to the user in a terminal. + ## Parameters * `pacticipantName`: the name of the application. Required. @@ -31,7 +33,7 @@ The previous tag and pact endpoints are still supported, however, future feature ### Success * `notices` - * `level`: one of `debug`, `info`, `warning`,`prompt`,`success` + * `level`: one of `debug`, `info`, `warning`,`prompt`,`success`, `error`, `danger` * `text`: the text of the notice. This is designed to be displayed in the output of a CLI. The `_links` section will contain links to all the resources created by the publication. The relations are: @@ -43,7 +45,9 @@ The `_links` section will contain links to all the resources created by the publ ### Errors -Any validation errors will be returned in the standard Pact Broker format: +### Schema validation errors + +Any validation errors will be returned in the standard Pact Broker format with a 400 status: { "errors": { @@ -51,6 +55,32 @@ Any validation errors will be returned in the standard Pact Broker format: } } +### Contract conflict errors + +If there is a conflict with an existing published pact and `allow_dangerous_contract_modification` is set to false, a 409 will be returned with an array of notices, which will contain a diff between the existing pact content and the content that was attempted to be published. For consistency with the existing error responses, the errors hash will also contain the error messages, but there will be no diff included. For CLI usage, when there are notices and errors, just the notices should be displayed to the user. + + { + "notices": + [ + { + "text": "Cannot change the content of the pact for Foo version 183a77b0 and provider Bar, as race conditions will cause unreliable results for can-i-deploy. Each pact must be published with a unique consumer version number. For more information see https://docs.pact.io/go/versioning", + "type": "error" + }, + { + "text": "", + "type": "info" + } + ], + "errors": + { + "content": + [ + "Cannot change the content of the pact for Foo version 183a77b0 and provider Bar, as race conditions will cause unreliable results for can-i-deploy. Each pact must be published with a unique consumer version number. For more information see https://docs.pact.io/go/versioning" + ] + } + } + + ## Example POST http://broker/contracts/publish diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml index d1f5c8804..0c3ac25fb 100644 --- a/lib/pact_broker/locale/en.yml +++ b/lib/pact_broker/locale/en.yml @@ -74,7 +74,7 @@ en: invalid_http_method: "Invalid HTTP method '%{method}'" invalid_url: "Invalid URL '%{url}'. Expected format: http://example.org" pact_missing_pacticipant_name: "was not found at expected path $.%{pacticipant}.name in the submitted pact file." - pact_content_modification_not_allowed: "Cannot change the content of the pact for %{consumer_name} version %{consumer_version_number}, as race conditions will cause unreliable results for can-i-deploy. Each pact must be published with a unique consumer version number. For more information see https://docs.pact.io/go/versioning" + pact_content_modification_not_allowed: "Cannot change the content of the pact for %{consumer_name} version %{consumer_version_number} and provider %{provider_name}, as race conditions will cause unreliable results for can-i-deploy. Each pact must be published with a unique consumer version number. For more information see https://docs.pact.io/go/versioning" consumer_version_number_missing: "Please specify the consumer version number by setting the X-Pact-Consumer-Version header." consumer_version_number_header_invalid: "X-Pact-Consumer-Version '%{consumer_version_number}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" consumer_version_number_invalid: "Consumer version number '%{consumer_version_number}' cannot be parsed to a version number. The expected format (unless this configuration has been overridden) is a semantic version. eg. 1.3.0 or 2.0.4.rc1" diff --git a/lib/pact_broker/pacts/content.rb b/lib/pact_broker/pacts/content.rb index c7aecc503..6d6788dbe 100644 --- a/lib/pact_broker/pacts/content.rb +++ b/lib/pact_broker/pacts/content.rb @@ -1,11 +1,13 @@ require "pact_broker/pacts/parse" require "pact_broker/pacts/sort_content" require "pact_broker/pacts/generate_interaction_sha" +require "pact_broker/hash_refinements" module PactBroker module Pacts class Content include GenerateInteractionSha + using PactBroker::HashRefinements def initialize pact_hash @pact_hash = pact_hash @@ -75,6 +77,18 @@ def with_ids(overwrite_existing_id = true) Content.from_hash(new_pact_hash) end + def without_ids + new_pact_hash = pact_hash.dup + if interactions && interactions.is_a?(Array) + new_pact_hash["interactions"] = remove_ids(interactions) + end + + if messages && messages.is_a?(Array) + new_pact_hash["messages"] = remove_ids(messages) + end + Content.from_hash(new_pact_hash) + end + def interaction_ids messages_or_interaction_or_empty_array.collect do | interaction | interaction["_id"] @@ -138,6 +152,10 @@ def add_ids(interactions, overwrite_existing_id) end end + def remove_ids(interactions_or_messages) + interactions_or_messages.collect{ | h | h.without("_id") } + end + def merge_verification_results(interactions, tests) interactions.collect(&:dup).collect do | interaction | interaction["tests"] = tests.select do | test | diff --git a/lib/pact_broker/pacts/create_formatted_diff.rb b/lib/pact_broker/pacts/create_formatted_diff.rb index e6fd6cf22..684ec47b8 100644 --- a/lib/pact_broker/pacts/create_formatted_diff.rb +++ b/lib/pact_broker/pacts/create_formatted_diff.rb @@ -1,16 +1,23 @@ require "pact/matchers" require "pact_broker/json" require "pact/matchers/unix_diff_formatter" +require "pact_broker/pacts/sort_content" +require "pact_broker/pacts/content" module PactBroker module Pacts class CreateFormattedDiff - extend Pact::Matchers - def self.call pact_json_content, previous_pact_json_content + def self.call pact_json_content, previous_pact_json_content, raw: false pact_hash = JSON.load(pact_json_content, nil, PactBroker::PACT_PARSING_OPTIONS) previous_pact_hash = JSON.load(previous_pact_json_content, nil, PactBroker::PACT_PARSING_OPTIONS) + + if !raw + pact_hash = SortContent.call(PactBroker::Pacts::Content.from_hash(pact_hash).without_ids.to_hash) + previous_pact_hash = SortContent.call(PactBroker::Pacts::Content.from_hash(previous_pact_hash).without_ids.to_hash) + end + difference = diff(previous_pact_hash, pact_hash) Pact::Matchers::UnixDiffFormatter.call(difference, colour: false, include_explanation: false) end diff --git a/lib/pact_broker/pacts/sort_content.rb b/lib/pact_broker/pacts/sort_content.rb index 23a5cc08a..47a0b5418 100644 --- a/lib/pact_broker/pacts/sort_content.rb +++ b/lib/pact_broker/pacts/sort_content.rb @@ -6,6 +6,7 @@ module Pacts class SortContent extend OrderHashKeys + # TODO handle interactions and messages at the same time def self.call pact_hash key = verifiable_content_key_for(pact_hash) diff --git a/spec/features/publish_pact_all_in_one_spec.rb b/spec/features/publish_pact_all_in_one_spec.rb index b11a9f204..45f0245be 100644 --- a/spec/features/publish_pact_all_in_one_spec.rb +++ b/spec/features/publish_pact_all_in_one_spec.rb @@ -39,4 +39,23 @@ it { is_expected.to be_a_json_error_response("missing") } end + + context "with a conflicting pact" do + before do + allow(PactBroker.configuration).to receive(:allow_dangerous_contract_modification).and_return(false) + td.create_pact_with_hierarchy("Foo", "1", "Bar") + end + + its(:status) { is_expected.to eq 409 } + + it "returns notices" do + expect(JSON.parse(subject.body)["notices"]).to be_a(Array) + end + + it "returns errors" do + contract_errors = JSON.parse(subject.body)["errors"]["contracts"] + expect(contract_errors).to be_a(Array) + expect(contract_errors.size).to eq 1 + end + end end diff --git a/spec/lib/pact_broker/contracts/service_spec.rb b/spec/lib/pact_broker/contracts/service_spec.rb index 63b033bb8..77213d37f 100644 --- a/spec/lib/pact_broker/contracts/service_spec.rb +++ b/spec/lib/pact_broker/contracts/service_spec.rb @@ -124,6 +124,70 @@ module Contracts end end end + + describe "#conflict_errors" do + let(:contracts_to_publish) do + ContractsToPublish.from_hash( + pacticipant_name: "Foo", + pacticipant_version_number: "1", + tags: ["a", "b"], + branch: branch, + contracts: contracts + ) + end + + let(:on_conflict) { "overwrite" } + let(:branch) { "main" } + let(:contracts) { [contract_1] } + let(:contract_1) do + ContractToPublish.from_hash( + consumer_name: "Foo", + provider_name: "Bar", + decoded_content: decoded_contract, + specification: "pact", + on_conflict: on_conflict + ) + end + + 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) } + + context "when a pact already exists" do + before do + allow(PactBroker.configuration).to receive(:allow_dangerous_contract_modification).and_return(allow_dangerous_contract_modification) + td.create_pact_with_hierarchy("Foo", "1", "Bar", existing_json_content) + end + + let(:existing_json_content) { td.random_json_content("Foo", "Bar") } + + context "when allow_dangerous_contract_modification=false and the pact content is different" do + let(:allow_dangerous_contract_modification) { false } + + it "returns errors" do + expect(subject).to_not be_empty + end + end + + context "when allow_dangerous_contract_modification=false and the pact content is the same" do + let(:allow_dangerous_contract_modification) { false } + let(:existing_json_content) { decoded_contract } + + it { is_expected.to be_empty } + end + + context "when allow_dangerous_contract_modification=true and the pact content is different" do + let(:allow_dangerous_contract_modification) { true } + + it { is_expected.to be_empty } + end + end + + context "when no pacts exist" do + it { is_expected.to be_empty } + end + end end end end