From 88ba2ac782dce856a70a39dc4c5a3a5271d4783f Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Tue, 5 Sep 2017 21:23:09 +1000 Subject: [PATCH] feat(webhook status): ensure triggered webhook and webhook execution objects are saved to database even when webhook fails and response code is 500 --- .../pact_webhooks_status_decorator.rb | 33 +++++++-- .../api/decorators/webhook_decorator.rb | 14 ++-- .../api/resources/webhook_execution.rb | 8 +- lib/pact_broker/constants.rb | 2 +- lib/pact_broker/webhooks/service.rb | 1 + lib/rack/pact_broker/database_transaction.rb | 10 ++- spec/features/execute_webhook_spec.rb | 74 +++++++++++++++++++ .../pact_webhooks_status_decorator_spec.rb | 11 ++- .../pact_broker/database_transaction_spec.rb | 13 +++- tasks/database.rb | 7 +- 10 files changed, 152 insertions(+), 21 deletions(-) create mode 100644 spec/features/execute_webhook_spec.rb diff --git a/lib/pact_broker/api/decorators/pact_webhooks_status_decorator.rb b/lib/pact_broker/api/decorators/pact_webhooks_status_decorator.rb index 9e23461bb..00294c424 100644 --- a/lib/pact_broker/api/decorators/pact_webhooks_status_decorator.rb +++ b/lib/pact_broker/api/decorators/pact_webhooks_status_decorator.rb @@ -60,16 +60,20 @@ class PactWebhooksStatusDecorator < BaseDecorator end link :'pb:pact-version' do | context | - { - href: pact_url(context[:base_url], pact), - title: "Pact", - name: pact.name - } + if pact + { + href: pact_url(context[:base_url], pact), + title: "Pact", + name: pact.name + } + else + nil + end end link :'pb:consumer' do | context | { - href: pacticipant_url(context[:base_url], OpenStruct.new(name: context[:consumer_name])), + href: pacticipant_url(context[:base_url], fake_consumer(context)), title: "Consumer", name: context[:consumer_name] } @@ -77,12 +81,19 @@ class PactWebhooksStatusDecorator < BaseDecorator link :'pb:provider' do | context | { - href: pacticipant_url(context[:base_url], OpenStruct.new(name: context[:provider_name])), + href: pacticipant_url(context[:base_url], fake_provider(context)), title: "Provider", name: context[:provider_name] } end + link :'pb:pact-webhooks' do | context | + { + title: "Webhooks for the pact between #{context[:consumer_name]} and #{context[:provider_name]}", + href: webhooks_for_pact_url(fake_consumer(context), fake_provider(context), context.fetch(:base_url)) + } + end + def summary counts = represented.group_by(&:status).each_with_object({}) do | (status, triggered_webhooks), counts | counts[status] = triggered_webhooks.count @@ -97,6 +108,14 @@ def triggered_webhooks_with_error_logs def pact represented.any? ? represented.first.pact_publication : nil end + + def fake_consumer context + OpenStruct.new(name: context[:consumer_name]) + end + + def fake_provider context + OpenStruct.new(name: context[:provider_name]) + end end end end diff --git a/lib/pact_broker/api/decorators/webhook_decorator.rb b/lib/pact_broker/api/decorators/webhook_decorator.rb index 00f1d5ea2..62816e218 100644 --- a/lib/pact_broker/api/decorators/webhook_decorator.rb +++ b/lib/pact_broker/api/decorators/webhook_decorator.rb @@ -24,6 +24,13 @@ class WebhookDecorator < BaseDecorator end + link :'pb:execute' do | options | + { + title: "Test the execution of the webhook by sending a POST request to this URL", + href: webhook_execution_url(represented, options[:base_url]) + } + end + link :'pb:pact-webhooks' do | options | { title: "All webhooks for the pact between #{represented.consumer.name} and #{represented.provider.name}", @@ -38,13 +45,6 @@ class WebhookDecorator < BaseDecorator } end - link :'pb:execute' do | options | - { - title: "Test the execution of the webhook by sending a POST request to this URL", - href: webhook_execution_url(represented, options[:base_url]) - } - - end end end diff --git a/lib/pact_broker/api/resources/webhook_execution.rb b/lib/pact_broker/api/resources/webhook_execution.rb index f83ebd82c..11632412d 100644 --- a/lib/pact_broker/api/resources/webhook_execution.rb +++ b/lib/pact_broker/api/resources/webhook_execution.rb @@ -1,5 +1,6 @@ require 'pact_broker/services' require 'pact_broker/api/decorators/webhook_execution_result_decorator' +require 'pact_broker/constants' module PactBroker module Api @@ -15,7 +16,12 @@ def process_post webhook_execution_result = webhook_service.execute_webhook_now webhook, pact response.headers['Content-Type'] = 'application/hal+json;charset=utf-8' response.body = post_response_body webhook_execution_result - webhook_execution_result.success? ? true : 500 + if webhook_execution_result.success? + true + else + response.headers[PactBroker::DO_NOT_ROLLBACK] = 'true' + 500 + end end def resource_exists? diff --git a/lib/pact_broker/constants.rb b/lib/pact_broker/constants.rb index 1ce71188a..9c72dd228 100644 --- a/lib/pact_broker/constants.rb +++ b/lib/pact_broker/constants.rb @@ -1,5 +1,5 @@ module PactBroker CONSUMER_VERSION_HEADER = 'X-Pact-Consumer-Version'.freeze - + DO_NOT_ROLLBACK = 'X-Pact-Broker-Do-Not-Rollback'.freeze end diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index 2e59e23bd..2c4fbe8cc 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -62,6 +62,7 @@ def self.execute_webhook_now webhook, pact else webhook_repository.update_triggered_webhook_status triggered_webhook, TriggeredWebhook::STATUS_FAILURE end + webhook_execution_result end def self.execute_triggered_webhook_now triggered_webhook diff --git a/lib/rack/pact_broker/database_transaction.rb b/lib/rack/pact_broker/database_transaction.rb index b8a974ebd..ea5f29315 100644 --- a/lib/rack/pact_broker/database_transaction.rb +++ b/lib/rack/pact_broker/database_transaction.rb @@ -1,4 +1,4 @@ -require 'pact_broker/version' +require 'pact_broker/constants' require 'sequel' module Rack @@ -33,10 +33,16 @@ def call_with_transaction env response = nil @database_connection.transaction do response = @app.call(env) - raise Sequel::Rollback if response.first == 500 + if response.first == 500 + raise Sequel::Rollback unless do_not_rollback?(response) + end end response end + + def do_not_rollback? response + response[1].delete(::PactBroker::DO_NOT_ROLLBACK) + end end end end diff --git a/spec/features/execute_webhook_spec.rb b/spec/features/execute_webhook_spec.rb new file mode 100644 index 000000000..5195feafa --- /dev/null +++ b/spec/features/execute_webhook_spec.rb @@ -0,0 +1,74 @@ +require 'support/test_data_builder' +require 'webmock/rspec' +require 'rack/pact_broker/database_transaction' + +describe "Execute a webhook" do + + let(:td) { TestDataBuilder.new } + + before do + td.create_pact_with_hierarchy("Some Consumer", "1", "Some Provider") + .create_webhook(method: 'POST') + end + + let(:path) { "/webhooks/#{td.webhook.uuid}/execute" } + let(:response_body) { JSON.parse(last_response.body, symbolize_names: true)} + + subject { post path; last_response } + + context "when the execution is successful" do + let!(:request) do + stub_request(:post, /http/).to_return(:status => 200) + end + + it "performs the HTTP request" do + subject + expect(request).to have_been_made + end + + it "returns a 200 response" do + puts subject.body + expect(subject.status).to be 200 + end + + it "saves a TriggeredWebhook" do + expect { subject }.to change { PactBroker::Webhooks::TriggeredWebhook.count }.by(1) + end + + it "saves a WebhookExecution" do + expect { subject }.to change { PactBroker::Webhooks::Execution.count }.by(1) + end + end + + context "when an error occurs", no_db_clean: true do + let(:app) { Rack::PactBroker::DatabaseTransaction.new(PactBroker::API, PactBroker::DB.connection) } + + let!(:request) do + stub_request(:post, /http/).to_raise(Errno::ECONNREFUSED) + end + + before do + PactBroker::Database.truncate + td.create_pact_with_hierarchy("Some Consumer", "1", "Some Provider") + .create_webhook(method: 'POST') + end + + after do + PactBroker::Database.truncate + end + + subject { post path; last_response } + + it "returns a 500 response" do + expect(subject.status).to be 500 + end + + it "saves a TriggeredWebhook" do + expect { subject }.to change { PactBroker::Webhooks::TriggeredWebhook.count }.by(1) + end + + it "saves a WebhookExecution" do + expect { subject }.to change { PactBroker::Webhooks::Execution.count }.by(1) + end + end +end diff --git a/spec/lib/pact_broker/api/decorators/pact_webhooks_status_decorator_spec.rb b/spec/lib/pact_broker/api/decorators/pact_webhooks_status_decorator_spec.rb index 7cbe0f1b9..7b23b1bea 100644 --- a/spec/lib/pact_broker/api/decorators/pact_webhooks_status_decorator_spec.rb +++ b/spec/lib/pact_broker/api/decorators/pact_webhooks_status_decorator_spec.rb @@ -36,9 +36,10 @@ module Decorators let(:retrying) { false } let(:status) { PactBroker::Webhooks::TriggeredWebhook::STATUS_SUCCESS } let(:logs_url) { "http://example.org/webhooks/4321/trigger/1234/logs" } + let(:triggered_webhooks) { [triggered_webhook] } let(:json) do - PactWebhooksStatusDecorator.new([triggered_webhook]).to_json(user_options: user_options) + PactWebhooksStatusDecorator.new(triggered_webhooks).to_json(user_options: user_options) end subject { JSON.parse json, symbolize_names: true } @@ -112,6 +113,14 @@ module Decorators expect(subject[:summary]).to eq({successful: 0, failed: 0, notRun: 1}) end end + + context "when there are no triggered webhooks" do + let(:triggered_webhooks) { [] } + + it "doesn't blow up" do + subject + end + end end end end diff --git a/spec/lib/rack/pact_broker/database_transaction_spec.rb b/spec/lib/rack/pact_broker/database_transaction_spec.rb index 85e09c0f0..91d1a6eed 100644 --- a/spec/lib/rack/pact_broker/database_transaction_spec.rb +++ b/spec/lib/rack/pact_broker/database_transaction_spec.rb @@ -13,8 +13,10 @@ module PactBroker ::PactBroker::Database.truncate end + let(:headers) { {} } + let(:api) do - ->(env) { ::PactBroker::Domain::Pacticipant.create(name: 'Foo'); [500, {}, []] } + ->(env) { ::PactBroker::Domain::Pacticipant.create(name: 'Foo'); [500, headers, []] } end let(:app) do @@ -39,6 +41,15 @@ module PactBroker end end end + + context "when there is an error but the resource sets the no rollback header" do + let(:headers) { {::PactBroker::DO_NOT_ROLLBACK => 'true'} } + let(:http_method) { :post } + + it "does not roll back" do + expect { subject }.to change { ::PactBroker::Domain::Pacticipant.count }.by(1) + end + end end end end diff --git a/tasks/database.rb b/tasks/database.rb index 47ef49b70..096dfac8a 100644 --- a/tasks/database.rb +++ b/tasks/database.rb @@ -77,7 +77,12 @@ def recreate def truncate TABLES.each do | table_name | if database.table_exists?(table_name) - database[table_name].delete + begin + database[table_name].delete + rescue SQLite3::ConstraintException => e + puts "Could not delete the following records from #{table_name}: #{database[table_name].select_all}" + raise e + end end end end