From 30522c0d52a657c62744bcb0b3b95de64272b6a3 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Sat, 27 Apr 2019 19:35:25 +1000 Subject: [PATCH] feat: log the reason why a webhook has been triggered --- lib/pact_broker/pacts/repository.rb | 6 +- lib/pact_broker/pacts/service.rb | 27 +--- lib/pact_broker/services.rb | 5 + lib/pact_broker/webhooks/trigger_service.rb | 68 +++++++++ spec/lib/pact_broker/pacts/service_spec.rb | 98 +----------- .../webhooks/trigger_service_spec.rb | 140 ++++++++++++++++++ 6 files changed, 229 insertions(+), 115 deletions(-) create mode 100644 lib/pact_broker/webhooks/trigger_service.rb create mode 100644 spec/lib/pact_broker/webhooks/trigger_service_spec.rb diff --git a/lib/pact_broker/pacts/repository.rb b/lib/pact_broker/pacts/repository.rb index 891475065..7db682fc0 100644 --- a/lib/pact_broker/pacts/repository.rb +++ b/lib/pact_broker/pacts/repository.rb @@ -258,9 +258,11 @@ def find_previous_distinct_pact pact def find_previous_pacts pact if pact.consumer_version_tag_names.any? - pact.consumer_version_tag_names.map { |tag| find_previous_pact(pact, tag) } + pact.consumer_version_tag_names.each_with_object({}) do |tag, tags_to_pacts| + tags_to_pacts[tag] = find_previous_pact(pact, tag) + end else - [find_previous_pact(pact, :untagged)] + { :untagged => find_previous_pact(pact, :untagged) } end end diff --git a/lib/pact_broker/pacts/service.rb b/lib/pact_broker/pacts/service.rb index 5db35e3b2..9bbd2b09c 100644 --- a/lib/pact_broker/pacts/service.rb +++ b/lib/pact_broker/pacts/service.rb @@ -110,13 +110,9 @@ def find_distinct_pacts_between consumer, options distinct end - # TODO also take into account overridden revisions - def pact_is_new_or_pact_has_changed_since_previous_version? pact - pact_repository.find_previous_pacts(pact).any? { |previous_pact| previous_pact.nil? || pact.json_content != previous_pact.json_content} - end - private + # Overwriting an existing pact with the same consumer/provider/consumer version number def update_pact params, existing_pact logger.info "Updating existing pact publication with params #{params.reject{ |k, v| k == :json_content}}" logger.debug "Content #{params[:json_content]}" @@ -125,17 +121,12 @@ def update_pact params, existing_pact update_params = { pact_version_sha: pact_version_sha, json_content: json_content } updated_pact = pact_repository.update(existing_pact.id, update_params) - webhook_service.trigger_webhooks updated_pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED - # TODO this should use the sha! - if existing_pact.json_content != updated_pact.json_content - webhook_service.trigger_webhooks updated_pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED - else - logger.debug "Pact has not changed since previous version, not triggering webhooks for changed content" - end + webhook_trigger_service.trigger_webhooks_for_updated_pact(existing_pact, updated_pact) updated_pact end + # When no publication for the given consumer/provider/consumer version number exists def create_pact params, version, provider logger.info "Creating new pact publication with params #{params.reject{ |k, v| k == :json_content}}" logger.debug "Content #{params[:json_content]}" @@ -148,7 +139,7 @@ def create_pact params, version, provider pact_version_sha: pact_version_sha, json_content: json_content ) - trigger_webhooks pact + webhook_trigger_service.trigger_webhooks_for_new_pact pact pact end @@ -159,16 +150,6 @@ def generate_sha(json_content) def add_interaction_ids(json_content) Content.from_json(json_content).with_ids.to_json end - - def trigger_webhooks pact - # TODO add tests for this - webhook_service.trigger_webhooks pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED - if pact_is_new_or_pact_has_changed_since_previous_version?(pact) - webhook_service.trigger_webhooks pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED - else - logger.debug "Pact has not changed since previous version, not triggering webhooks for changed content" - end - end end end end diff --git a/lib/pact_broker/services.rb b/lib/pact_broker/services.rb index 214a63d29..8a08065b4 100644 --- a/lib/pact_broker/services.rb +++ b/lib/pact_broker/services.rb @@ -66,5 +66,10 @@ def integration_service require 'pact_broker/integrations/service' Integrations::Service end + + def webhook_trigger_service + require 'pact_broker/webhooks/trigger_service' + Webhooks::TriggerService + end end end diff --git a/lib/pact_broker/webhooks/trigger_service.rb b/lib/pact_broker/webhooks/trigger_service.rb new file mode 100644 index 000000000..4c1bd7d69 --- /dev/null +++ b/lib/pact_broker/webhooks/trigger_service.rb @@ -0,0 +1,68 @@ +require 'pact_broker/services' + +module PactBroker + module Webhooks + module TriggerService + + extend self + extend PactBroker::Repositories + extend PactBroker::Services + include PactBroker::Logging + + def trigger_webhooks_for_new_pact pact + webhook_service.trigger_webhooks pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED + if pact_is_new_or_newly_tagged_or_pact_has_changed_since_previous_version?(pact) + webhook_service.trigger_webhooks pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED + else + logger.debug "Pact content has not changed since previous version, not triggering webhooks for changed content" + end + end + + def trigger_webhooks_for_updated_pact(existing_pact, updated_pact) + webhook_service.trigger_webhooks updated_pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED + # TODO this should use the sha! + if existing_pact.pact_version_sha != updated_pact.pact_version_sha + logger.debug "Existing pact for version #{existing_pact.consumer_version_number} has been updated with new content, triggering webhooks for changed content" + webhook_service.trigger_webhooks updated_pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED + else + logger.debug "Pact content has not changed since previous revision, not triggering webhooks for changed content" + end + end + + private + + def pact_is_new_or_newly_tagged_or_pact_has_changed_since_previous_version? pact + changed_pacts = pact_repository + .find_previous_pacts(pact) + .reject { |_, previous_pact| !sha_changed_or_no_previous_version?(previous_pact, pact) } + print_debug_messages(changed_pacts) + changed_pacts.any? + end + + def sha_changed_or_no_previous_version?(previous_pact, new_pact) + previous_pact.nil? || new_pact.pact_version_sha != previous_pact.pact_version_sha + end + + def print_debug_messages(changed_pacts) + if changed_pacts.any? + messages = changed_pacts.collect do |tag, previous_pact| + if tag == :untagged + if previous_pact + "pact content has changed since previous untagged version" + else + "first time untagged pact published" + end + else + if previous_pact + "pact content has changed since the last consumer version tagged with #{tag}" + else + "first time pact published with consumer version tagged #{tag}" + end + end + end + logger.debug("Webhook triggered for the following reasons: #{messages.join(',')}" ) + end + end + end + end +end diff --git a/spec/lib/pact_broker/pacts/service_spec.rb b/spec/lib/pact_broker/pacts/service_spec.rb index 87e92e278..4e842ddff 100644 --- a/spec/lib/pact_broker/pacts/service_spec.rb +++ b/spec/lib/pact_broker/pacts/service_spec.rb @@ -14,6 +14,7 @@ module Pacts before do allow(described_class).to receive(:webhook_service).and_return(webhook_service) + allow(described_class).to receive(:webhook_trigger_service).and_return(webhook_trigger_service) allow(pacticipant_repository).to receive(:find_by_name_or_create).with(params[:consumer_name]).and_return(consumer) allow(pacticipant_repository).to receive(:find_by_name_or_create).with(params[:provider_name]).and_return(provider) allow(version_repository).to receive(:find_by_pacticipant_id_and_number_or_create).and_return(version) @@ -22,9 +23,12 @@ module Pacts allow(pact_repository).to receive(:update).and_return(new_pact) allow(pact_repository).to receive(:find_previous_pacts).and_return(previous_pacts) allow(webhook_service).to receive(:trigger_webhooks) + allow(webhook_trigger_service).to receive(:trigger_webhooks_for_new_pact) + allow(webhook_trigger_service).to receive(:trigger_webhooks_for_updated_pact) end let(:webhook_service) { class_double("PactBroker::Webhooks::Service").as_stubbed_const } + let(:webhook_trigger_service) { class_double("PactBroker::Webhooks::TriggerService").as_stubbed_const } let(:consumer) { double('consumer', id: 1) } let(:provider) { double('provider', id: 2) } let(:version) { double('version', id: 3, pacticipant_id: 1) } @@ -52,7 +56,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 @@ -65,8 +68,8 @@ module Pacts subject end - it "triggers webhooks for contract publications" do - expect(webhook_service).to receive(:trigger_webhooks).with(new_pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED) + it "triggers webhooks" do + expect(webhook_trigger_service).to receive(:trigger_webhooks_for_new_pact).with(new_pact) subject end end @@ -85,8 +88,8 @@ module Pacts subject end - it "triggers webhooks for contract publications" do - expect(webhook_service).to receive(:trigger_webhooks).with(new_pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED) + it "triggers webhooks" do + expect(webhook_trigger_service).to receive(:trigger_webhooks_for_updated_pact).with(existing_pact, new_pact) subject end end @@ -111,91 +114,6 @@ module Pacts end end - describe "#pact_is_new_or_pact_has_changed_since_previous_version?" do - let(:json_content) { { 'some' => 'json'}.to_json } - let(:pact) { instance_double(PactBroker::Domain::Pact, json_content: json_content)} - - subject { Service.pact_is_new_or_pact_has_changed_since_previous_version? pact } - - context "when consumer version is untagged" do - before do - allow(pact).to receive(:consumer_version_tag_names).and_return([]); - allow_any_instance_of(Pacts::Repository).to receive(:find_previous_pact).with(pact, :untagged).and_return(previous_pact) - end - - context "when a previous pact is found" do - let(:previous_pact) { instance_double(PactBroker::Domain::Pact, json_content: previous_json_content)} - let(:previous_json_content) { {'some' => 'json'}.to_json } - - context "when the json_content is the same" do - it "returns false" do - expect(subject).to be_falsey - end - end - - context "when the json_content is not the same" do - let(:previous_json_content) { {'some-other' => 'json'}.to_json } - it "returns truthy" do - expect(subject).to be_truthy - end - end - end - - context "when a previous pact is not found" do - let(:previous_pact) { nil } - - it "returns true" do - expect(subject).to be_truthy - end - end - end - - context "when consumer version has two tags" do - before do - allow(pact).to receive(:consumer_version_tag_names).and_return(['tag_1', 'tag_2']); - allow_any_instance_of(Pacts::Repository).to receive(:find_previous_pact).with(pact, 'tag_1').and_return(previous_pact_tag_1) - allow_any_instance_of(Pacts::Repository).to receive(:find_previous_pact).with(pact, 'tag_2').and_return(previous_pact_tag_2) - end - - context "when a previous pact is found for both tags" do - let(:previous_pact_tag_1) { instance_double(PactBroker::Domain::Pact, json_content: previous_json_content_tag_1)} - let(:previous_json_content_tag_1) { {'some' => 'json'}.to_json } - - let(:previous_pact_tag_2) { instance_double(PactBroker::Domain::Pact, json_content: previous_json_content_tag_2)} - let(:previous_json_content_tag_2) { {'some' => 'json'}.to_json } - - context "when the json_content of both previous pacts and new pact is the same" do - it "returns false" do - expect(subject).to be_falsey - end - end - - context "when the json_content of first previous pact is not the same" do - let(:previous_json_content_tag_1) { {'some-other' => 'json'}.to_json } - it "returns truthy" do - expect(subject).to be_truthy - end - end - - context "when the json_content of second previous pact not the same" do - let(:previous_json_content_tag_2) { {'some-other' => 'json'}.to_json } - it "returns truthy" do - expect(subject).to be_truthy - end - end - end - - context "when no previous pacts are found" do - let(:previous_pact_tag_1) { nil } - let(:previous_pact_tag_2) { nil } - - it "returns true" do - expect(subject).to be_truthy - end - end - end - end - describe "delete" do before do td.create_pact_with_hierarchy diff --git a/spec/lib/pact_broker/webhooks/trigger_service_spec.rb b/spec/lib/pact_broker/webhooks/trigger_service_spec.rb new file mode 100644 index 000000000..d4ca461b1 --- /dev/null +++ b/spec/lib/pact_broker/webhooks/trigger_service_spec.rb @@ -0,0 +1,140 @@ +require 'pact_broker/webhooks/trigger_service' + +module PactBroker + module Webhooks + describe TriggerService do + let(:pact) { double("pact", pact_version_sha: pact_version_sha) } + let(:pact_version_sha) { "111" } + let(:pact_repository) { double("pact_repository", find_previous_pacts: previous_pacts) } + let(:webhook_service) { double("webhook_service", trigger_webhooks: nil) } + let(:previous_pact) { double("previous_pact", pact_version_sha: previous_pact_version_sha) } + let(:previous_pact_version_sha) { "111" } + let(:previous_pacts) { { untagged: previous_pact } } + let(:logger) { double('logger').as_null_object } + + before do + allow(TriggerService).to receive(:pact_repository).and_return(pact_repository) + allow(TriggerService).to receive(:webhook_service).and_return(webhook_service) + allow(TriggerService).to receive(:logger).and_return(logger) + end + + shared_examples_for "triggering a contract_published event" do + it "triggers a contract_published webhook" do + expect(webhook_service).to receive(:trigger_webhooks).with(pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_PUBLISHED) + subject + end + end + + shared_examples_for "triggering a contract_content_changed event" do + it "triggers a contract_content_changed webhook" do + expect(webhook_service).to receive(:trigger_webhooks).with(pact, nil, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED) + subject + end + end + + shared_examples_for "not triggering a contract_content_changed event" do + it "does not trigger a contract_content_changed webhook" do + expect(webhook_service).to_not receive(:trigger_webhooks).with(anything, anything, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED) + subject + end + end + + describe "#trigger_webhooks_for_new_pact" do + subject { TriggerService.trigger_webhooks_for_new_pact(pact) } + + context "when no previous untagged pact exists" do + let(:previous_pact) { nil } + + include_examples "triggering a contract_published event" + include_examples "triggering a contract_content_changed event" + + it "logs the reason why it triggered the contract_content_changed event" do + expect(logger).to receive(:debug).with(/first time untagged pact published/) + subject + end + end + + context "when a previous untagged pact exists and the sha is different" do + let(:previous_pact_version_sha) { "222" } + + let(:previous_pacts) { { :untagged => previous_pact } } + + include_examples "triggering a contract_published event" + include_examples "triggering a contract_content_changed event" + + it "logs the reason why it triggered the contract_content_changed event" do + expect(logger).to receive(:debug).with(/pact content has changed since previous untagged version/) + subject + end + end + + context "when a previous untagged pact exists and the sha is the same" do + let(:previous_pact_version_sha) { pact_version_sha } + + let(:previous_pacts) { { :untagged => previous_pact } } + + include_examples "triggering a contract_published event" + include_examples "not triggering a contract_content_changed event" + end + + context "when no previous pact with a given tag exists" do + let(:previous_pact) { nil } + let(:previous_pacts) { { "dev" => previous_pact } } + + include_examples "triggering a contract_published event" + include_examples "triggering a contract_content_changed event" + + it "logs the reason why it triggered the contract_content_changed event" do + expect(logger).to receive(:debug).with(/first time pact published with consumer version tagged dev/) + subject + end + end + + context "when a previous pact with a given tag exists and the sha is different" do + let(:previous_pact_version_sha) { "222" } + let(:previous_pacts) { { "dev" => previous_pact } } + + include_examples "triggering a contract_published event" + include_examples "triggering a contract_content_changed event" + end + + context "when a previous pact with a given tag exists and the sha is the same" do + let(:previous_pact_version_sha) { pact_version_sha } + let(:previous_pacts) { { "dev" => previous_pact } } + + include_examples "triggering a contract_published event" + include_examples "not triggering a contract_content_changed event" + end + end + + describe "#trigger_webhooks_for_updated_pact" do + let(:existing_pact) do + double('existing_pact', + pact_version_sha: existing_pact_version_sha, + consumer_version_number: "1.2.3" + ) + end + let(:existing_pact_version_sha) { pact_version_sha } + + subject { TriggerService.trigger_webhooks_for_updated_pact(existing_pact, pact) } + + context "when the pact version sha of the previous revision is different" do + let(:existing_pact_version_sha) { "456" } + + include_examples "triggering a contract_published event" + include_examples "triggering a contract_content_changed event" + + it "logs the reason why it triggered the contract_content_changed event" do + expect(logger).to receive(:debug).with(/version 1.2.3 has been updated with new content/) + subject + end + end + + context "when the pact version sha of the previous revision is not different, not sure if we'll even get this far if it hasn't changed, but just in case..." do + include_examples "triggering a contract_published event" + include_examples "not triggering a contract_content_changed event" + end + end + end + end +end