From 50900231f0b220b0ebb489b7c2a59b4a7982cf3e Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Mon, 13 Sep 2021 09:20:51 +1000 Subject: [PATCH] feat: thread safe configuration overrides (#500) * chore: allow runtime configuration to be overridden for the scope of a request * chore: pass in retry_schedule to webhook job * chore: pass in http success codes to webhook job * style: rubocop * chore: pass in user agent to webhook job * chore: remove unused code * chore: remove configuration middleware from PB app as it will be added to PF --- .../api/middleware/configuration.rb | 33 ++++++++++ lib/pact_broker/app.rb | 2 + lib/pact_broker/configuration.rb | 60 ++++++++++--------- lib/pact_broker/domain/webhook.rb | 32 ++++++---- lib/pact_broker/domain/webhook_request.rb | 4 +- lib/pact_broker/test/test_data_builder.rb | 2 +- .../webhooks/execution_configuration.rb | 16 +++++ .../execution_configuration_creator.rb | 3 + lib/pact_broker/webhooks/job.rb | 2 +- lib/pact_broker/webhooks/trigger_service.rb | 4 +- .../webhooks/webhook_execution_result.rb | 10 +--- .../webhooks/webhook_request_logger.rb | 16 ++--- .../webhooks/webhook_request_template.rb | 14 ++--- pact_broker.gemspec | 1 + script/data/webhook.rb | 22 +++++++ ...webhook_execution_result_decorator_spec.rb | 2 +- .../api/middleware/configuration_spec.rb | 43 +++++++++++++ .../webhook_execution_result_spec.rb | 56 ----------------- .../api/resources/webhook_execution_spec.rb | 2 +- .../domain/webhook_request_spec.rb | 3 +- spec/lib/pact_broker/domain/webhook_spec.rb | 20 +++++-- spec/lib/pact_broker/webhooks/job_spec.rb | 8 +-- .../webhooks/trigger_service_spec.rb | 14 +++-- .../webhooks/webhook_request_logger_spec.rb | 18 ++---- .../webhooks/webhook_request_template_spec.rb | 5 +- 25 files changed, 234 insertions(+), 158 deletions(-) create mode 100644 lib/pact_broker/api/middleware/configuration.rb create mode 100755 script/data/webhook.rb create mode 100644 spec/lib/pact_broker/api/middleware/configuration_spec.rb delete mode 100644 spec/lib/pact_broker/api/resources/webhook_execution_result_spec.rb diff --git a/lib/pact_broker/api/middleware/configuration.rb b/lib/pact_broker/api/middleware/configuration.rb new file mode 100644 index 000000000..5d6846605 --- /dev/null +++ b/lib/pact_broker/api/middleware/configuration.rb @@ -0,0 +1,33 @@ +require "pact_broker/logging" + +module PactBroker + module Api + module Middleware + class Configuration + include PactBroker::Logging + + def initialize(app, configuration) + @app = app + @configuration = configuration + end + + def call(env) + if (overrides = env["pactbroker.configuration_overrides"])&.any? + dupped_configuration = configuration.dup + dupped_configuration.override_runtime_configuration!(overrides) + dupped_configuration.freeze + PactBroker.set_configuration(dupped_configuration) + app.call(env) + else + PactBroker.set_configuration(configuration) + app.call(env) + end + end + + private + + attr_reader :app, :configuration + end + end + end +end diff --git a/lib/pact_broker/app.rb b/lib/pact_broker/app.rb index dcf1698ed..43a3f8da2 100644 --- a/lib/pact_broker/app.rb +++ b/lib/pact_broker/app.rb @@ -25,6 +25,7 @@ require "rack/pact_broker/add_vary_header" require "rack/pact_broker/use_when" require "sucker_punch" +require "pact_broker/api/middleware/configuration" require "pact_broker/api/middleware/basic_auth" require "pact_broker/config/basic_auth_configuration" require "pact_broker/api/authorization/resource_access_policy" @@ -51,6 +52,7 @@ def initialize load_configuration_from_database seed_example_data print_startup_message + @configuration.freeze end # Allows middleware to be inserted at the bottom of the shared middlware stack diff --git a/lib/pact_broker/configuration.rb b/lib/pact_broker/configuration.rb index 713e09701..b04612b52 100644 --- a/lib/pact_broker/configuration.rb +++ b/lib/pact_broker/configuration.rb @@ -4,10 +4,15 @@ require "forwardable" require "pact_broker/config/runtime_configuration" require "anyway/auto_cast" +require "request_store" module PactBroker def self.configuration - @@configuration ||= Configuration.default_configuration + RequestStore.store[:pact_broker_configuration] ||= Configuration.default_configuration + end + + def self.set_configuration(configuration) + RequestStore.store[:pact_broker_configuration] = configuration end def self.with_runtime_configuration_overrides(overrides, &block) @@ -16,7 +21,7 @@ def self.with_runtime_configuration_overrides(overrides, &block) # @private, for testing only def self.reset_configuration - @@configuration = Configuration.default_configuration + RequestStore.store[:pact_broker_configuration] = Configuration.default_configuration end class Configuration @@ -83,12 +88,36 @@ def self.default_configuration def with_runtime_configuration_overrides(overrides) original_runtime_configuration = runtime_configuration - self.runtime_configuration = override_runtime_configuration(overrides, original_runtime_configuration.dup) + self.runtime_configuration = override_runtime_configuration!(overrides) yield ensure self.runtime_configuration = original_runtime_configuration end + def override_runtime_configuration!(overrides) + new_runtime_configuration = runtime_configuration.dup + valid_overrides = {} + invalid_overrides = {} + + overrides.each do | key, value | + if new_runtime_configuration.respond_to?("#{key}=") + valid_overrides[key] = Anyway::AutoCast.call(value) + else + invalid_overrides[key] = Anyway::AutoCast.call(value) + end + end + + if logger.debug? + logger.debug("Overridding runtime configuration", payload: { overrides: valid_overrides, ignoring: invalid_overrides }) + end + + valid_overrides.each do | key, value | + new_runtime_configuration.public_send("#{key}=", value) + end + + self.runtime_configuration = new_runtime_configuration + end + def logger_from_runtime_configuration @logger_from_runtime_configuration ||= begin runtime_configuration.validate_logging_attributes! @@ -206,30 +235,5 @@ def load_from_database! require "pact_broker/config/load" PactBroker::Config::Load.call(runtime_configuration) end - - private - - def override_runtime_configuration(overrides, new_runtime_configuration) - valid_overrides = {} - invalid_overrides = {} - - overrides.each do | key, value | - if new_runtime_configuration.respond_to?("#{key}=") - valid_overrides[key] = Anyway::AutoCast.call(value) - else - invalid_overrides[key] = Anyway::AutoCast.call(value) - end - end - - if logger.debug? - logger.debug("Overridding runtime configuration", payload: { overrides: valid_overrides, ignoring: invalid_overrides }) - end - - valid_overrides.each do | key, value | - new_runtime_configuration.public_send("#{key}=", value) - end - - new_runtime_configuration - end end end diff --git a/lib/pact_broker/domain/webhook.rb b/lib/pact_broker/domain/webhook.rb index cf63af1e1..0bc1f29e5 100644 --- a/lib/pact_broker/domain/webhook.rb +++ b/lib/pact_broker/domain/webhook.rb @@ -50,17 +50,12 @@ def request_description def execute pact, verification, event_context, options logger.info "Executing #{self} event_context=#{event_context}" template_params = template_parameters(pact, verification, event_context, options) - webhook_request = request.build(template_params) + webhook_request = request.build(template_params, options.fetch(:user_agent)) http_response, error = execute_request(webhook_request) - - logs = generate_logs(webhook_request, http_response, error, event_context, options.fetch(:logging_options)) + success = success?(http_response, options) http_request = webhook_request.http_request - PactBroker::Webhooks::WebhookExecutionResult.new( - http_request, - http_response, - logs, - error - ) + logs = generate_logs(webhook_request, http_response, success, error, event_context, options.fetch(:logging_options)) + result(http_request, http_response, success, logs, error) end def to_s @@ -116,16 +111,33 @@ def template_parameters(pact, verification, event_context, _options) PactBroker::Webhooks::PactAndVerificationParameters.new(pact, verification, event_context).to_hash end - def generate_logs(webhook_request, http_response, error, event_context, logging_options) + def success?(http_response, options) + !http_response.nil? && options.fetch(:http_success_codes).include?(http_response.code.to_i) + end + + # rubocop: disable Metrics/ParameterLists + def generate_logs(webhook_request, http_response, success, error, event_context, logging_options) webhook_request_logger = PactBroker::Webhooks::WebhookRequestLogger.new(logging_options) webhook_request_logger.log( uuid, webhook_request, http_response, + success, error, event_context ) end + # robocop: enable Metrics/ParameterLists + + def result(http_request, http_response, success, logs, error) + PactBroker::Webhooks::WebhookExecutionResult.new( + http_request, + http_response, + success, + logs, + error + ) + end end end end diff --git a/lib/pact_broker/domain/webhook_request.rb b/lib/pact_broker/domain/webhook_request.rb index 7be00b271..a55259964 100644 --- a/lib/pact_broker/domain/webhook_request.rb +++ b/lib/pact_broker/domain/webhook_request.rb @@ -18,7 +18,7 @@ class WebhookRequest HEADERS_TO_REDACT = [/authorization/i, /token/i] - attr_accessor :method, :url, :headers, :body, :username, :password, :uuid + attr_accessor :method, :url, :headers, :body, :username, :password, :uuid, :user_agent # Reform gets confused by the :method method, as :method is a standard # Ruby method. @@ -58,7 +58,7 @@ def http_request @http_request ||= begin req = Net::HTTP.const_get(method.capitalize).new(uri.request_uri) req.delete("accept-encoding") - req["user-agent"] = PactBroker.configuration.user_agent + req["user-agent"] = user_agent headers.each_pair { | name, value | req[name] = value } req.basic_auth(username, password) if username && username.size > 0 req.body = body unless body.nil? diff --git a/lib/pact_broker/test/test_data_builder.rb b/lib/pact_broker/test/test_data_builder.rb index 5439e2586..f3205aeb7 100644 --- a/lib/pact_broker/test/test_data_builder.rb +++ b/lib/pact_broker/test/test_data_builder.rb @@ -340,7 +340,7 @@ def create_triggered_webhook params = {} def create_webhook_execution params = {} params.delete(:comment) logs = params[:logs] || "logs" - webhook_execution_result = PactBroker::Webhooks::WebhookExecutionResult.new(nil, OpenStruct.new(code: "200"), logs, nil) + webhook_execution_result = PactBroker::Webhooks::WebhookExecutionResult.new(nil, OpenStruct.new(code: "200"), true, logs, nil) @webhook_execution = PactBroker::Webhooks::Repository.new.create_execution @triggered_webhook, webhook_execution_result created_at = params[:created_at] || @pact.created_at + Rational(1, 86400) set_created_at_if_set created_at, :webhook_executions, {id: @webhook_execution.id} diff --git a/lib/pact_broker/webhooks/execution_configuration.rb b/lib/pact_broker/webhooks/execution_configuration.rb index f0791e26a..2f7519a87 100644 --- a/lib/pact_broker/webhooks/execution_configuration.rb +++ b/lib/pact_broker/webhooks/execution_configuration.rb @@ -25,14 +25,30 @@ def with_failure_log_message(value) with_updated_attribute(logging_options: { failure_log_message: value }) end + def with_retry_schedule(value) + with_updated_attribute(retry_schedule: value) + end + + def with_http_success_codes(value) + with_updated_attribute(http_success_codes: value) + end + def with_webhook_context(value) with_updated_attribute(webhook_context: value) end + def with_user_agent(value) + with_updated_attribute(user_agent: value) + end + def webhook_context self[:webhook_context] end + def retry_schedule + self[:retry_schedule] + end + def [](key) params[key] end diff --git a/lib/pact_broker/webhooks/execution_configuration_creator.rb b/lib/pact_broker/webhooks/execution_configuration_creator.rb index 78e20a302..b51ceeffb 100644 --- a/lib/pact_broker/webhooks/execution_configuration_creator.rb +++ b/lib/pact_broker/webhooks/execution_configuration_creator.rb @@ -7,6 +7,9 @@ class ExecutionConfigurationCreator def self.call(resource) PactBroker::Webhooks::ExecutionConfiguration.new .with_show_response(PactBroker.configuration.show_webhook_response?) + .with_retry_schedule(PactBroker.configuration.webhook_retry_schedule) + .with_http_success_codes(PactBroker.configuration.webhook_http_code_success) + .with_user_agent(PactBroker.configuration.user_agent) .with_webhook_context(base_url: resource.base_url) end end diff --git a/lib/pact_broker/webhooks/job.rb b/lib/pact_broker/webhooks/job.rb index c10ed279d..4bd0201b0 100644 --- a/lib/pact_broker/webhooks/job.rb +++ b/lib/pact_broker/webhooks/job.rb @@ -104,7 +104,7 @@ def backoff_time end def retry_schedule - PactBroker.configuration.webhook_retry_schedule + data[:webhook_execution_configuration].retry_schedule end end end diff --git a/lib/pact_broker/webhooks/trigger_service.rb b/lib/pact_broker/webhooks/trigger_service.rb index 11bfc1d1d..63341c552 100644 --- a/lib/pact_broker/webhooks/trigger_service.rb +++ b/lib/pact_broker/webhooks/trigger_service.rb @@ -31,8 +31,8 @@ def test_execution webhook, event_context, execution_configuration end def execute_triggered_webhook_now triggered_webhook, webhook_execution_configuration_hash - webhook_execution_result = triggered_webhook.execute webhook_execution_configuration_hash - webhook_repository.create_execution triggered_webhook, webhook_execution_result + webhook_execution_result = triggered_webhook.execute(webhook_execution_configuration_hash) + webhook_repository.create_execution(triggered_webhook, webhook_execution_result) webhook_execution_result end diff --git a/lib/pact_broker/webhooks/webhook_execution_result.rb b/lib/pact_broker/webhooks/webhook_execution_result.rb index 4a92e1a8a..0bcf60f81 100644 --- a/lib/pact_broker/webhooks/webhook_execution_result.rb +++ b/lib/pact_broker/webhooks/webhook_execution_result.rb @@ -6,20 +6,16 @@ module Webhooks class WebhookExecutionResult attr_reader :request, :response, :logs, :error - def initialize(request, response, logs, error = nil) + def initialize(request, response, success, logs, error = nil) @request = PactBroker::Webhooks::HttpRequestWithRedactedHeaders.new(request) @response = response ? PactBroker::Webhooks::HttpResponseWithUtf8SafeBody.new(response) : nil + @success = success @logs = logs @error = error end def success? - if response - # Response HTTP Code must be in success list otherwise it is false - PactBroker.configuration.webhook_http_code_success.include? response.code.to_i - else - false - end + @success end end end diff --git a/lib/pact_broker/webhooks/webhook_request_logger.rb b/lib/pact_broker/webhooks/webhook_request_logger.rb index 33dbff05a..3928128bf 100644 --- a/lib/pact_broker/webhooks/webhook_request_logger.rb +++ b/lib/pact_broker/webhooks/webhook_request_logger.rb @@ -29,15 +29,17 @@ def initialize(options) @options = options end - def log(uuid, webhook_request, http_response, error, webhook_context) + # rubocop: disable Metrics/ParameterLists + def log(uuid, webhook_request, http_response, success, error, webhook_context) safe_response = http_response ? HttpResponseWithUtf8SafeBody.new(http_response) : nil log_webhook_context(webhook_context) log_request(webhook_request) log_response(uuid, safe_response, webhook_context[:base_url]) if safe_response log_error(uuid, error, webhook_context[:base_url]) if error - log_completion_message(success?(safe_response)) + log_completion_message(success) log_stream.string end + # rubocop: enable Metrics/ParameterLists private @@ -115,16 +117,6 @@ def log_error uuid, e, base_url execution_logger.error "Error executing webhook #{uuid}. #{response_body_hidden_message(base_url)}" end end - - def success?(response) - if response - # Response HTTP Code must be in success list otherwise it is false - PactBroker.configuration.webhook_http_code_success.include? response.code.to_i - else - false - end - end - end end end diff --git a/lib/pact_broker/webhooks/webhook_request_template.rb b/lib/pact_broker/webhooks/webhook_request_template.rb index 50a1d0fc9..0e34bb207 100644 --- a/lib/pact_broker/webhooks/webhook_request_template.rb +++ b/lib/pact_broker/webhooks/webhook_request_template.rb @@ -21,16 +21,13 @@ class WebhookRequestTemplate alias_method :http_method, :method def initialize attributes = {} - @method = attributes[:method] - @url = attributes[:url] - @username = attributes[:username] - @password = attributes[:password] + attributes.each do | (name, value) | + instance_variable_set("@#{name}", value) if respond_to?(name) + end @headers = Rack::Utils::HeaderHash.new(attributes[:headers] || {}) - @body = attributes[:body] - @uuid = attributes[:uuid] end - def build(template_params) + def build(template_params, user_agent) attributes = { method: http_method, url: build_url(template_params), @@ -38,7 +35,8 @@ def build(template_params) username: build_string(username, template_params), password: build_string(password, template_params), uuid: uuid, - body: build_body(template_params) + body: build_body(template_params), + user_agent: user_agent } PactBroker::Domain::WebhookRequest.new(attributes) end diff --git a/pact_broker.gemspec b/pact_broker.gemspec index 6c9248f5d..0d0838fd6 100644 --- a/pact_broker.gemspec +++ b/pact_broker.gemspec @@ -66,4 +66,5 @@ Gem::Specification.new do |gem| gem.add_runtime_dependency "sanitize", ">= 5.2.1", "~> 5.2" gem.add_runtime_dependency "wisper", "~> 2.0" gem.add_runtime_dependency "anyway_config", "~> 2.1" + gem.add_runtime_dependency "request_store", "~> 1.5" end diff --git a/script/data/webhook.rb b/script/data/webhook.rb new file mode 100755 index 000000000..80e439051 --- /dev/null +++ b/script/data/webhook.rb @@ -0,0 +1,22 @@ +#!/usr/bin/env ruby +begin + + $LOAD_PATH << "#{Dir.pwd}/lib" + require "pact_broker/test/http_test_data_builder" + base_url = ENV["PACT_BROKER_BASE_URL"] || "http://localhost:9292" + + td = PactBroker::Test::HttpTestDataBuilder.new(base_url) + td.delete_webhook(uuid: "7a5da39c-8e50-4cc9-ae16-dfa5be043e8c") + .create_global_webhook_for_contract_changed(uuid: "7a5da39c-8e50-4cc9-ae16-dfa5be043e8c") + .delete_pacticipant("foo-consumer") + .delete_pacticipant("bar-provider") + .create_pacticipant("foo-consumer") + .create_pacticipant("bar-provider") + .publish_pact(consumer: "foo-consumer", consumer_version: "1", provider: "bar-provider", content_id: "111", tag: "main") + + +rescue StandardError => e + puts "#{e.class} #{e.message}" + puts e.backtrace + exit 1 +end diff --git a/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb b/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb index 32f079d86..d2461629c 100644 --- a/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb +++ b/spec/lib/pact_broker/api/decorators/webhook_execution_result_decorator_spec.rb @@ -6,7 +6,7 @@ module Api module Decorators describe WebhookExecutionResultDecorator do describe "to_json" do - let(:webhook_execution_result) { PactBroker::Webhooks::WebhookExecutionResult.new(request, response, logs, error) } + let(:webhook_execution_result) { PactBroker::Webhooks::WebhookExecutionResult.new(request, response, true, logs, error) } let(:logs) { "logs" } let(:headers) { { "Something" => ["blah", "thing"]} } let(:request) do diff --git a/spec/lib/pact_broker/api/middleware/configuration_spec.rb b/spec/lib/pact_broker/api/middleware/configuration_spec.rb new file mode 100644 index 000000000..9bd1181d7 --- /dev/null +++ b/spec/lib/pact_broker/api/middleware/configuration_spec.rb @@ -0,0 +1,43 @@ +require "pact_broker/api/middleware/configuration" + +module PactBroker + module Api + module Middleware + class TestApp + def call(_) + [200, {}, [PactBroker.configuration.allow_dangerous_contract_modification.to_s]] + end + end + + describe Configuration do + describe "#call" do + let(:configuration) do + conf = PactBroker::Configuration.default_configuration + conf.allow_dangerous_contract_modification = false + conf + end + let(:app) { Configuration.new(TestApp.new, configuration) } + let(:rack_env) { {} } + + subject { get("/", nil, rack_env) } + + context "with no overrides" do + it "uses the default configuration" do + expect(subject.body).to eq "false" + end + end + + context "with overrides" do + let(:rack_env) do + { "pactbroker.configuration_overrides" => { allow_dangerous_contract_modification: true }} + end + + it "overrides the configuration for the duration of the request" do + expect(subject.body).to eq "true" + end + end + end + end + end + end +end diff --git a/spec/lib/pact_broker/api/resources/webhook_execution_result_spec.rb b/spec/lib/pact_broker/api/resources/webhook_execution_result_spec.rb deleted file mode 100644 index 0144b495a..000000000 --- a/spec/lib/pact_broker/api/resources/webhook_execution_result_spec.rb +++ /dev/null @@ -1,56 +0,0 @@ -module PactBroker - module Webhooks - describe WebhookExecutionResult do - subject { WebhookExecutionResult::new(request, response, nil) } - let(:request) do - Net::HTTP::Get.new("http://example.org?foo=bar") - end - - context "When 'webhook_http_code_success' has [200, 201]" do - before do - allow(PactBroker.configuration).to receive(:webhook_http_code_success).and_return([200, 201]) - end - - context "and response is '200'" do - let(:response) { double(code: "200") } - - it "then it should be success" do - expect(subject.success?).to be_truthy - end - end - - context "and response is '400'" do - let(:response) { double(code: "400") } - - it "then it should fail" do - expect(subject.success?).to be_falsey - end - end - end - - - context "When 'webhook_http_code_success' has [400, 401]" do - before do - allow(PactBroker.configuration).to receive(:webhook_http_code_success).and_return([400, 401]) - end - - context "and response is '200'" do - let(:response) { double(code: "200") } - - it "then it should fail" do - expect(subject.success?).to be_falsey - end - end - - context "and response is '400'" do - let(:response) { double(code: "400") } - - it "then it should be success" do - expect(subject.success?).to be_truthy - end - end - end - - end - end -end diff --git a/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb b/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb index cb47d3681..93d9797cd 100644 --- a/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb +++ b/spec/lib/pact_broker/api/resources/webhook_execution_spec.rb @@ -30,7 +30,7 @@ module Resources let(:pact) { instance_double("PactBroker::Domain::Pact") } let(:consumer_name) { "foo" } let(:provider_name) { "bar" } - let(:webhook_execution_configuration) { instance_double(PactBroker::Webhooks::ExecutionConfiguration, webhook_context: event_context) } + let(:webhook_execution_configuration) { instance_double(PactBroker::Webhooks::ExecutionConfiguration, retry_schedule: [], webhook_context: event_context) } let(:event_context) { { some: "data" } } before do diff --git a/spec/lib/pact_broker/domain/webhook_request_spec.rb b/spec/lib/pact_broker/domain/webhook_request_spec.rb index 078fe762d..c7314a546 100644 --- a/spec/lib/pact_broker/domain/webhook_request_spec.rb +++ b/spec/lib/pact_broker/domain/webhook_request_spec.rb @@ -19,7 +19,8 @@ module Domain headers: headers, username: username, password: password, - body: body) + body: body, + user_agent: "Pact Broker") end let(:execute) { subject.execute } diff --git a/spec/lib/pact_broker/domain/webhook_spec.rb b/spec/lib/pact_broker/domain/webhook_spec.rb index 46b4b385d..5f2e608fc 100644 --- a/spec/lib/pact_broker/domain/webhook_spec.rb +++ b/spec/lib/pact_broker/domain/webhook_spec.rb @@ -11,10 +11,11 @@ module Domain let(:webhook_template_parameters) { instance_double(PactBroker::Webhooks::PactAndVerificationParameters, to_hash: webhook_template_parameters_hash) } let(:webhook_template_parameters_hash) { { "foo" => "bar" } } let(:http_request) { double("http request") } - let(:http_response) { double("http response", code: "200") } + let(:http_response) { double("http response", code: response_code) } + let(:response_code) { "200" } let(:event_context) { { some: "things" } } let(:logging_options) { { other: "options" } } - let(:options) { { logging_options: logging_options } } + let(:options) { { logging_options: logging_options, http_success_codes: [200], user_agent: "user agent" } } let(:pact) { double("pact") } let(:verification) { double("verification") } let(:logger) { double("logger").as_null_object } @@ -71,7 +72,7 @@ module Domain end it "builds the request" do - expect(request_template).to receive(:build).with(webhook_template_parameters_hash) + expect(request_template).to receive(:build).with(webhook_template_parameters_hash, "user agent") execute end @@ -81,7 +82,7 @@ module Domain end it "generates the execution logs" do - expect(webhook_request_logger).to receive(:log).with(uuid, webhook_request, http_response, nil, event_context) + expect(webhook_request_logger).to receive(:log).with(uuid, webhook_request, http_response, true, nil, event_context) execute end @@ -90,6 +91,7 @@ module Domain expect(execute.response).to_not be nil expect(execute.logs).to eq "logs" expect(execute.error).to be nil + expect(execute.success?).to be true end it "logs before and after" do @@ -98,6 +100,14 @@ module Domain execute end + context "when a response status is returned that is not in the http_success_codes" do + let(:response_code) { "301" } + + it "returns a result with success? false" do + expect(execute.success?).to be false + end + end + context "when an error is thrown" do let(:error_class) { Class.new(StandardError) } @@ -106,7 +116,7 @@ module Domain end it "generates the execution logs" do - expect(webhook_request_logger).to receive(:log).with(uuid, webhook_request, nil, instance_of(error_class), event_context) + expect(webhook_request_logger).to receive(:log).with(uuid, webhook_request, nil, false, instance_of(error_class), event_context) execute end diff --git a/spec/lib/pact_broker/webhooks/job_spec.rb b/spec/lib/pact_broker/webhooks/job_spec.rb index 3f9d138b4..b3a1a9f68 100644 --- a/spec/lib/pact_broker/webhooks/job_spec.rb +++ b/spec/lib/pact_broker/webhooks/job_spec.rb @@ -5,7 +5,6 @@ module PactBroker module Webhooks describe Job do before do - PactBroker.configuration.webhook_retry_schedule = [10, 60, 120, 300, 600, 1200] allow(PactBroker::Webhooks::TriggerService).to receive(:execute_triggered_webhook_now).and_return(result) allow(PactBroker::Webhooks::Service).to receive(:update_triggered_webhook_status) allow(PactBroker::Webhooks::TriggeredWebhook).to receive(:find).and_return(triggered_webhook) @@ -17,12 +16,15 @@ module Webhooks let(:base_url) { "http://broker" } let(:triggered_webhook) { instance_double("PactBroker::Webhooks::TriggeredWebhook", webhook_uuid: "1234", id: 1) } let(:result) { instance_double("PactBroker::Domain::WebhookExecutionResult", success?: success) } - let(:webhook_execution_configuration) { instance_double(PactBroker::Webhooks::ExecutionConfiguration, to_hash: webhook_execution_configuration_hash) } + let(:webhook_execution_configuration) do + instance_double(PactBroker::Webhooks::ExecutionConfiguration, retry_schedule: retry_schedule, to_hash: webhook_execution_configuration_hash) + end let(:webhook_execution_configuration_hash) { { the: "options" } } let(:success) { true } let(:logger) { double("logger").as_null_object } let(:database_connector) { ->(&block) { block.call } } let(:webhook_context) { { the: "context" } } + let(:retry_schedule) { [10, 60, 120, 300, 600, 1200] } let(:job_params) do { triggered_webhook: triggered_webhook, @@ -125,8 +127,6 @@ module Webhooks job_params[:error_count] = 1 end - # subject { Job.new.perform(triggered_webhook: triggered_webhook, error_count: 1, database_connector: database_connector, base_url: base_url) } - it "reschedules the job in 60 seconds" do expect(Job).to receive(:perform_in).with(60, hash_including(error_count: 2)) subject diff --git a/spec/lib/pact_broker/webhooks/trigger_service_spec.rb b/spec/lib/pact_broker/webhooks/trigger_service_spec.rb index 90a0b714a..4c0a58931 100644 --- a/spec/lib/pact_broker/webhooks/trigger_service_spec.rb +++ b/spec/lib/pact_broker/webhooks/trigger_service_spec.rb @@ -340,6 +340,9 @@ def find_result_with_message_including(message) let(:webhook_execution_configuration) do PactBroker::Webhooks::ExecutionConfiguration.new .with_webhook_context(base_url: "http://example.org") + .with_retry_schedule([10, 60, 120, 300, 600, 1200]) + .with_http_success_codes([200]) + .with_user_agent("Pact Broker") .with_show_response(true) end let(:event_context) { { some: "data", base_url: "http://example.org" }} @@ -361,7 +364,7 @@ def find_result_with_message_including(message) .and_return(:pact) end - let(:triggered_webhooks) { PactBroker::Webhooks::TriggerService.create_triggered_webhooks_for_event(pact, td.verification, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED, event_context) } + let!(:triggered_webhooks) { PactBroker::Webhooks::TriggerService.create_triggered_webhooks_for_event(pact, td.verification, PactBroker::Webhooks::WebhookEvent::CONTRACT_CONTENT_CHANGED, event_context) } subject { PactBroker::Webhooks::TriggerService.schedule_webhooks(triggered_webhooks, options) } @@ -375,10 +378,6 @@ def find_result_with_message_including(message) subject end - it "saves the triggered webhook" do - expect { subject }.to change { PactBroker::Webhooks::TriggeredWebhook.count }.by(1) - end - it "saves the execution" do expect { subject }.to change { PactBroker::Webhooks::Execution.count }.by(1) end @@ -387,6 +386,11 @@ def find_result_with_message_including(message) subject expect(TriggeredWebhook.first.status).to eq TriggeredWebhook::STATUS_SUCCESS end + + it "does not call the PactBroker.configuration as it will have been reset after the end of the request" do + expect(PactBroker).to_not receive(:configuration) + subject + end end end end diff --git a/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb b/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb index 31ab94fce..0f9d4696a 100644 --- a/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb +++ b/spec/lib/pact_broker/webhooks/webhook_request_logger_spec.rb @@ -16,7 +16,7 @@ module Webhooks let(:logger) { double("logger").as_null_object } let(:uuid) { "uuid" } - let(:options) { { failure_log_message: "oops", show_response: show_response } } + let(:options) { { failure_log_message: "oops", show_response: show_response, http_code_success: [200] } } let(:show_response) { true } let(:username) { nil } let(:password) { nil } @@ -54,8 +54,9 @@ module Webhooks let(:webhook_context) { { consumer_version_number: "123", base_url: base_url } } let(:webhook_request_logger) { WebhookRequestLogger.new(options) } + let(:success) { true } - subject(:logs) { webhook_request_logger.log(uuid, webhook_request, response, error, webhook_context) } + subject(:logs) { webhook_request_logger.log(uuid, webhook_request, response, success, error, webhook_context) } describe "application logs" do it "logs the request" do @@ -129,16 +130,8 @@ module Webhooks end end - context "when the response code '100' is not in 'webhook_http_code_success'" do - let(:status) { 100 } - - it "not successful, code '100' not in 'webhook_http_code_success'" do - expect(logs).to include "oops" - end - end - - context "when the response code is not successful" do - let(:status) { 400 } + context "when the status is not successful" do + let(:success) { false } it "logs the failure_log_message" do expect(logs).to include "oops" @@ -174,6 +167,7 @@ class WebhookTestError < StandardError; end end let(:response) { nil } + let(:success) { false } let(:error) do err = WebhookTestError.new("blah") allow(err).to receive(:backtrace).and_return([]) diff --git a/spec/lib/pact_broker/webhooks/webhook_request_template_spec.rb b/spec/lib/pact_broker/webhooks/webhook_request_template_spec.rb index 19787b1ae..61605612e 100644 --- a/spec/lib/pact_broker/webhooks/webhook_request_template_spec.rb +++ b/spec/lib/pact_broker/webhooks/webhook_request_template_spec.rb @@ -23,7 +23,8 @@ module Webhooks password: "passwordBUILT", uuid: "1234", body: built_body, - headers: {"headername" => "headervalueBUILT"} + headers: {"headername" => "headervalueBUILT"}, + user_agent: "Pact Broker" } end @@ -45,7 +46,7 @@ module Webhooks let(:params_hash) { double("params hash") } - subject { WebhookRequestTemplate.new(attributes).build(params_hash) } + subject { WebhookRequestTemplate.new(attributes).build(params_hash, "Pact Broker") } it "renders the url template" do expect(PactBroker::Webhooks::Render).to receive(:call).with(url, params_hash) do | content, pact, verification, &block |