From f9ba9ab5b19245bb8e30263c3ec7bc18bd278761 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Thu, 13 Feb 2020 11:06:11 +1100 Subject: [PATCH] feat(webhooks): support upsert of webhook via a PUT to /webhooks/{uuid} --- lib/pact_broker/api/resources/index.rb | 5 ++ lib/pact_broker/api/resources/webhook.rb | 10 +-- .../api/resources/webhook_resource_methods.rb | 4 +- lib/pact_broker/locale/en.yml | 1 + lib/pact_broker/webhooks/service.rb | 15 ++++- spec/features/create_webhook_spec.rb | 8 +++ .../pact_broker/api/resources/webhook_spec.rb | 67 +++++++++++++++---- spec/lib/pact_broker/webhooks/service_spec.rb | 31 ++++++++- 8 files changed, 120 insertions(+), 21 deletions(-) diff --git a/lib/pact_broker/api/resources/index.rb b/lib/pact_broker/api/resources/index.rb index 984bc463b..7d2a5c67b 100644 --- a/lib/pact_broker/api/resources/index.rb +++ b/lib/pact_broker/api/resources/index.rb @@ -94,6 +94,11 @@ def links title: 'Webhooks', templated: false }, + 'pb:webhook' => { + href: base_url + '/webhooks/{uuid}', + title: 'Webhook', + templated: true + }, 'pb:integrations' => { href: base_url + '/integrations', title: 'Integrations', diff --git a/lib/pact_broker/api/resources/webhook.rb b/lib/pact_broker/api/resources/webhook.rb index 0eb41c486..0864aa2ae 100644 --- a/lib/pact_broker/api/resources/webhook.rb +++ b/lib/pact_broker/api/resources/webhook.rb @@ -28,7 +28,7 @@ def resource_exists? def malformed_request? if request.put? - return invalid_json? || webhook_validation_errors?(new_webhook) + return invalid_json? || webhook_validation_errors?(parsed_webhook, uuid) end false end @@ -38,7 +38,9 @@ def from_json @webhook = webhook_service.update_by_uuid uuid, params_with_string_keys response.body = to_json else - 404 + @webhook = webhook_service.create(uuid, parsed_webhook, consumer, provider) + response.body = to_json + 201 end end @@ -57,8 +59,8 @@ def webhook @webhook ||= webhook_service.find_by_uuid uuid end - def new_webhook - @new_webhook ||= Decorators::WebhookDecorator.new(PactBroker::Domain::Webhook.new).from_json(request_body) + def parsed_webhook + @parsed_webhook ||= Decorators::WebhookDecorator.new(PactBroker::Domain::Webhook.new).from_json(request_body) end def uuid diff --git a/lib/pact_broker/api/resources/webhook_resource_methods.rb b/lib/pact_broker/api/resources/webhook_resource_methods.rb index 388ff9283..4cc1191cc 100644 --- a/lib/pact_broker/api/resources/webhook_resource_methods.rb +++ b/lib/pact_broker/api/resources/webhook_resource_methods.rb @@ -2,8 +2,8 @@ module PactBroker module Api module Resources module WebhookResourceMethods - def webhook_validation_errors? webhook - errors = webhook_service.errors(webhook) + def webhook_validation_errors?(webhook, uuid = nil) + errors = webhook_service.errors(webhook, uuid) if !errors.empty? set_json_validation_error_messages(errors.messages) true diff --git a/lib/pact_broker/locale/en.yml b/lib/pact_broker/locale/en.yml index 7e420a18d..8e9790bdd 100644 --- a/lib/pact_broker/locale/en.yml +++ b/lib/pact_broker/locale/en.yml @@ -43,6 +43,7 @@ en: 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" pacticipant_name_mismatch: "in pact ('%{name_in_pact}') does not match %{pacticipant} name in path ('%{name}')." connection_encoding_not_utf8: "The Sequel connection encoding (%{encoding}) is strongly recommended to be \"utf8\". If you need to set it to %{encoding} for some particular reason, then disable this check by setting config.validate_database_connection_config = false" + invalid_webhook_uuid: The UUID can only contain the characters A-Z, a-z, 0-9, _ and -, and must be 16 or more characters. 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: diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index 3baecefe3..7ee611108 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -14,6 +14,7 @@ require 'pact_broker/webhooks/execution_configuration' require 'pact_broker/messages' require 'pact_broker/webhooks/pact_and_verification_parameters' +require 'reform/contract/errors' module PactBroker module Webhooks @@ -28,14 +29,24 @@ class Service include Logging extend PactBroker::Messages + # Not actually a UUID. Ah well. + def self.valid_uuid_format?(uuid) + !!(uuid =~ /^[A-Za-z0-9_\-]{16,}$/) + end + def self.next_uuid SecureRandom.urlsafe_base64 end - def self.errors webhook + def self.errors webhook, uuid = nil contract = PactBroker::Api::Contracts::WebhookContract.new(webhook) contract.validate(webhook.attributes) - contract.errors + errors = contract.errors + + if uuid && !valid_uuid_format?(uuid) + errors.add("uuid", message("errors.validation.invalid_webhook_uuid")) + end + errors end def self.create uuid, webhook, consumer, provider diff --git a/spec/features/create_webhook_spec.rb b/spec/features/create_webhook_spec.rb index faa4fa819..f53a0a6a8 100644 --- a/spec/features/create_webhook_spec.rb +++ b/spec/features/create_webhook_spec.rb @@ -127,4 +127,12 @@ expect(PactBroker::Webhooks::Webhook.first.provider).to be nil end end + + context "with a UUID" do + let(:path) { "/webhooks/1234123412341234" } + + subject { put(path, webhook_json, headers) } + + its(:status) { is_expected.to be 201 } + end end diff --git a/spec/lib/pact_broker/api/resources/webhook_spec.rb b/spec/lib/pact_broker/api/resources/webhook_spec.rb index cffd3966c..31b770ac6 100644 --- a/spec/lib/pact_broker/api/resources/webhook_spec.rb +++ b/spec/lib/pact_broker/api/resources/webhook_spec.rb @@ -2,11 +2,8 @@ require 'pact_broker/api/resources/webhook' module PactBroker::Api - module Resources - describe Webhook do - before do allow(PactBroker::Webhooks::Service).to receive(:find_by_uuid).and_return(webhook) end @@ -23,15 +20,14 @@ module Resources end context "when the webhook exists" do + before do + allow(Decorators::WebhookDecorator).to receive(:new).and_return(decorator) + end let(:webhook) { double("webhook") } let(:decorator) { double(Decorators::WebhookDecorator, to_json: json)} let(:json) { {some: 'json'}.to_json } - before do - allow(Decorators::WebhookDecorator).to receive(:new).and_return(decorator) - end - it "finds the webhook by UUID" do expect(PactBroker::Webhooks::Service).to receive(:find_by_uuid).with('some-uuid') subject @@ -56,14 +52,61 @@ module Resources end describe "PUT" do + before do + allow(Decorators::WebhookDecorator).to receive(:new).and_return(decorator) + allow(PactBroker::Webhooks::Service).to receive(:create).and_return(created_webhook) + allow_any_instance_of(Webhook).to receive(:consumer).and_return(consumer) + allow_any_instance_of(Webhook).to receive(:provider).and_return(provider) + allow_any_instance_of(Webhook).to receive(:webhook_validation_errors?).and_return(false) + end + + let(:consumer) { double('consumer') } + let(:provider) { double('provider') } + let(:webhook) { double("webhook") } + let(:decorator) { double(Decorators::WebhookDecorator, from_json: parsed_webhook, to_json: json)} + let(:json) { {some: 'json'}.to_json } + + let(:parsed_webhook) { double('parsed_webhook') } + let(:created_webhook) { double('created_webhook') } + let(:webhook) { nil } + let(:webhook_json) { load_fixture('webhook_valid.json') } + let(:uuid) { 'some-uuid' } + + subject { put("/webhooks/#{uuid}", webhook_json, 'CONTENT_TYPE' => 'application/json') } + + it "validates the UUID" do + expect_any_instance_of(Webhook).to receive(:webhook_validation_errors?).with(parsed_webhook, uuid) + subject + end + context "when the webhook does not exist" do - let(:webhook) { nil } - let(:webhook_json) { load_fixture('webhook_valid.json') } + it "creates the webhook" do + expect(PactBroker::Webhooks::Service).to receive(:create).with(uuid, parsed_webhook, consumer, provider) + subject + end - subject { put '/webhooks/some-uuid', webhook_json, {'CONTENT_TYPE' => 'application/json'}; last_response } + its(:status) { is_expected.to eq 201 } - it "returns a 404" do - expect(subject).to be_a_404_response + it "returns the JSON respresentation of the webhook" do + expect(subject.body).to eq json + end + end + + context "when the webhook does exist" do + before do + allow(PactBroker::Webhooks::Service).to receive(:update_by_uuid).and_return(created_webhook) + end + let(:webhook) { double('existing webhook') } + + its(:status) { is_expected.to eq 200 } + + it "updates the webhook" do + expect(PactBroker::Webhooks::Service).to receive(:update_by_uuid).with(uuid, JSON.parse(webhook_json)) + subject + end + + it "returns the JSON respresentation of the webhook" do + expect(subject.body).to eq json end end end diff --git a/spec/lib/pact_broker/webhooks/service_spec.rb b/spec/lib/pact_broker/webhooks/service_spec.rb index 6160ad711..34e9f6f1e 100644 --- a/spec/lib/pact_broker/webhooks/service_spec.rb +++ b/spec/lib/pact_broker/webhooks/service_spec.rb @@ -14,9 +14,38 @@ module Webhooks allow(Service).to receive(:logger).and_return(logger) end - let(:td) { TestDataBuilder.new } let(:logger) { double('logger').as_null_object } + describe "validate - integration test" do + let(:invalid_webhook) { PactBroker::Domain::Webhook.new } + + context "with no uuid" do + subject { Service.errors(invalid_webhook) } + + it "does not contain an error for the uuid" do + expect(subject.messages).to_not have_key('uuid') + end + end + + context "with a uuid" do + subject { Service.errors(invalid_webhook, '') } + + it "merges the uuid errors with the webhook errors" do + expect(subject.messages['uuid'].first).to include "can only contain" + end + end + end + + describe ".valid_uuid_format?" do + it 'does something' do + expect(Service.valid_uuid_format?("_-bcdefghigHIJKLMNOP")).to be true + expect(Service.valid_uuid_format?("HIJKLMNOP")).to be false + expect(Service.valid_uuid_format?("abcdefghigHIJKLMNOP\\")).to be false + expect(Service.valid_uuid_format?("abcdefghigHIJKLMNOP#")).to be false + expect(Service.valid_uuid_format?("abcdefghigHIJKLMNOP$")).to be false + end + end + describe ".delete_by_uuid" do before do td.create_pact_with_hierarchy