From 48f98538c3284e38d61f382664acbb1b56c42952 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Fri, 6 Oct 2017 08:40:25 +1100 Subject: [PATCH] fix: delete related triggered webhooks when webhook is deleted Trying to keep the triggered webhooks to keep the status on the index page after a webhook had been deleted just made things too complicated. Also, it was likely that the webhook was deleted because it was failing, so it just made the failure message hang around. --- ..._add_triggered_webhooks_fk_to_execution.rb | 2 +- .../42_delete_orphan_webhook_data.rb | 20 ++++ lib/pact_broker/webhooks/execution.rb | 2 + lib/pact_broker/webhooks/repository.rb | 8 +- lib/pact_broker/webhooks/service.rb | 2 +- lib/pact_broker/webhooks/webhook.rb | 4 + .../pact_broker/webhooks/repository_spec.rb | 36 +++++-- .../42_delete_ophan_webhook_data_spec.rb | 98 +++++++++++++++++++ 8 files changed, 157 insertions(+), 15 deletions(-) create mode 100644 db/migrations/42_delete_orphan_webhook_data.rb create mode 100644 spec/migrations/42_delete_ophan_webhook_data_spec.rb diff --git a/db/migrations/39_add_triggered_webhooks_fk_to_execution.rb b/db/migrations/39_add_triggered_webhooks_fk_to_execution.rb index d67ad5810..5e60e44e3 100644 --- a/db/migrations/39_add_triggered_webhooks_fk_to_execution.rb +++ b/db/migrations/39_add_triggered_webhooks_fk_to_execution.rb @@ -17,7 +17,7 @@ # TODO drop_column(:webhook_executions, :provider_id) # TODO - # alter_table(:triggered_webhooks) do + # alter_table(:webhook_executions) do # set_column_not_null(:triggered_webhook_id) # end end diff --git a/db/migrations/42_delete_orphan_webhook_data.rb b/db/migrations/42_delete_orphan_webhook_data.rb new file mode 100644 index 000000000..417a94f8c --- /dev/null +++ b/db/migrations/42_delete_orphan_webhook_data.rb @@ -0,0 +1,20 @@ +require 'securerandom' + +Sequel.migration do + up do + from(:triggered_webhooks).where(webhook_id: nil).each do | triggered_webhook | + from(:webhook_executions).where(triggered_webhook_id: triggered_webhook[:id]).delete + from(:triggered_webhooks).where(id: triggered_webhook[:id]).delete + end + + from(:webhook_executions).where(webhook_id: nil, triggered_webhook_id: nil).delete + end + + # TODO + # alter_table(:triggered_webhooks) do + # set_column_not_null(:webhook_id) + # end + + down do + end +end diff --git a/lib/pact_broker/webhooks/execution.rb b/lib/pact_broker/webhooks/execution.rb index 66cd2f472..e0d35bba4 100644 --- a/lib/pact_broker/webhooks/execution.rb +++ b/lib/pact_broker/webhooks/execution.rb @@ -27,6 +27,8 @@ def <=> other end end + # For a brief time, the code was released with a direct relationship between + # webhook and execution. Need to make sure any existing data is handled properly. class DeprecatedExecution < Sequel::Model(:webhook_executions) associate(:many_to_one, :provider, :class => "PactBroker::Domain::Pacticipant", :key => :provider_id, :primary_key => :id) associate(:many_to_one, :consumer, :class => "PactBroker::Domain::Pacticipant", :key => :consumer_id, :primary_key => :id) diff --git a/lib/pact_broker/webhooks/repository.rb b/lib/pact_broker/webhooks/repository.rb index 573496dc8..efaffa309 100644 --- a/lib/pact_broker/webhooks/repository.rb +++ b/lib/pact_broker/webhooks/repository.rb @@ -104,9 +104,11 @@ def delete_executions_by_pacticipant pacticipants Execution.where(id: execution_ids).delete end - def unlink_triggered_webhooks_by_webhook_uuid uuid - TriggeredWebhook.where(webhook: Webhook.where(uuid: uuid)).update(webhook_id: nil) - DeprecatedExecution.where(webhook_id: Webhook.where(uuid: uuid).select(:id)).update(webhook_id: nil) + def delete_triggered_webhooks_by_webhook_uuid uuid + triggered_webhook_ids = TriggeredWebhook.where(webhook: Webhook.where(uuid: uuid)).select_for_subquery(:id) + Execution.where(triggered_webhook_id: triggered_webhook_ids).delete + DeprecatedExecution.where(webhook_id: Webhook.where(uuid: uuid).select_for_subquery(:id)).delete + TriggeredWebhook.where(id: triggered_webhook_ids).delete end def delete_triggered_webhooks_by_pact_publication_ids pact_publication_ids diff --git a/lib/pact_broker/webhooks/service.rb b/lib/pact_broker/webhooks/service.rb index 3e881acf2..6bfd7a251 100644 --- a/lib/pact_broker/webhooks/service.rb +++ b/lib/pact_broker/webhooks/service.rb @@ -40,7 +40,7 @@ def self.update_by_uuid uuid, webhook end def self.delete_by_uuid uuid - webhook_repository.unlink_triggered_webhooks_by_webhook_uuid uuid + webhook_repository.delete_triggered_webhooks_by_webhook_uuid uuid webhook_repository.delete_by_uuid uuid end diff --git a/lib/pact_broker/webhooks/webhook.rb b/lib/pact_broker/webhooks/webhook.rb index 0584cc4c9..e1c17769d 100644 --- a/lib/pact_broker/webhooks/webhook.rb +++ b/lib/pact_broker/webhooks/webhook.rb @@ -11,6 +11,10 @@ class Webhook < Sequel::Model associate(:many_to_one, :consumer, :class => "PactBroker::Domain::Pacticipant", :key => :consumer_id, :primary_key => :id) one_to_many :headers, :class => "PactBroker::Webhooks::WebhookHeader", :reciprocal => :webhook + dataset_module do + include PactBroker::Repositories::Helpers + end + def before_destroy WebhookHeader.where(webhook_id: id).destroy end diff --git a/spec/lib/pact_broker/webhooks/repository_spec.rb b/spec/lib/pact_broker/webhooks/repository_spec.rb index 32b4ff6f9..bd3e2f05d 100644 --- a/spec/lib/pact_broker/webhooks/repository_spec.rb +++ b/spec/lib/pact_broker/webhooks/repository_spec.rb @@ -354,7 +354,7 @@ module Webhooks end end - describe "unlink_triggered_webhooks_by_webhook_uuid" do + describe "delete_triggered_webhooks_by_webhook_uuid" do let(:td) { TestDataBuilder.new } before do @@ -365,22 +365,38 @@ module Webhooks .create_webhook .create_triggered_webhook .create_deprecated_webhook_execution + .create_webhook_execution + .create_webhook + .create_triggered_webhook + .create_deprecated_webhook_execution + .create_webhook_execution + end + + let(:webhook_id) { Webhook.find(uuid: td.webhook.uuid).id } + subject { Repository.new.delete_triggered_webhooks_by_webhook_uuid td.webhook.uuid } + + it "deletes the related triggered webhooks" do + expect { subject }.to change { + TriggeredWebhook.where(id: td.triggered_webhook.id).count + }.from(1).to(0) end - subject { Repository.new.unlink_triggered_webhooks_by_webhook_uuid td.webhook.uuid } + it "does not delete the unrelated triggered webhooks" do + expect { subject }.to_not change { + TriggeredWebhook.exclude(id: td.triggered_webhook.id).count + } + end - it "sets the webhook id to nil" do - webhook_id = Webhook.find(uuid: td.webhook.uuid).id + it "deletes the related deprecated webhook executions" do expect { subject }.to change { - TriggeredWebhook.find(id: td.triggered_webhook.id).webhook_id - }.from(webhook_id).to(nil) + DeprecatedExecution.count + }.by(-2) end - it "sets the webhook id to nil for the deprecated webhook execution field" do - webhook_id = Webhook.find(uuid: td.webhook.uuid).id + it "deletes the related webhook executions" do expect { subject }.to change { - DeprecatedExecution.find(id: td.webhook_execution.id).webhook_id - }.from(webhook_id).to(nil) + Execution.count + }.by(-2) end end diff --git a/spec/migrations/42_delete_ophan_webhook_data_spec.rb b/spec/migrations/42_delete_ophan_webhook_data_spec.rb new file mode 100644 index 000000000..808163b61 --- /dev/null +++ b/spec/migrations/42_delete_ophan_webhook_data_spec.rb @@ -0,0 +1,98 @@ +describe 'creating triggered webhooks from webhook executions (migrate 36-41)', migration: true do + before do + PactBroker::Database.migrate(41) + end + + let(:before_now) { DateTime.new(2016, 1, 1) } + let(:now) { DateTime.new(2018, 2, 2) } + let!(:consumer) { create(:pacticipants, {name: 'Consumer', created_at: now, updated_at: now}) } + let!(:provider) { create(:pacticipants, {name: 'Provider', created_at: now, updated_at: now}) } + let!(:consumer_version) { create(:versions, {number: '1.2.3', order: 1, pacticipant_id: consumer[:id], created_at: now, updated_at: now}) } + let!(:pact_version) { create(:pact_versions, {content: {some: 'json'}.to_json, sha: '1234', consumer_id: consumer[:id], provider_id: provider[:id], created_at: now}) } + let!(:pact_publication) do + create(:pact_publications, { + consumer_version_id: consumer_version[:id], + provider_id: provider[:id], + revision_number: 1, + pact_version_id: pact_version[:id], + created_at: (now - 1) + }) + end + let!(:webhook) do + create(:webhooks, { + uuid: '1234', + method: 'GET', + url: 'http://www.example.org', + consumer_id: consumer[:id], + provider_id: provider[:id], + is_json_request_body: false, + created_at: now + }) + end + let!(:triggered_webhook) do + create(:triggered_webhooks, { + trigger_uuid: '12345', + trigger_type: 'publication', + pact_publication_id: pact_publication[:id], + webhook_id: webhook[:id], + webhook_uuid: webhook[:uuid], + consumer_id: consumer[:id], + provider_id: provider[:id], + status: 'success', + created_at: now, + updated_at: now + }) + end + let!(:webhook_execution) do + create(:webhook_executions, { + triggered_webhook_id: triggered_webhook[:id], + success: true, + logs: 'logs', + created_at: now + }) + end + + let!(:orphan_triggered_webhook) do + create(:triggered_webhooks, { + trigger_uuid: '12345', + trigger_type: 'publication', + pact_publication_id: pact_publication[:id], + webhook_id: nil, + webhook_uuid: webhook[:uuid], + consumer_id: consumer[:id], + provider_id: provider[:id], + status: 'success', + created_at: now, + updated_at: now + }) + end + + let!(:orphan_webhook_execution) do + create(:webhook_executions, { + triggered_webhook_id: orphan_triggered_webhook[:id], + success: true, + logs: 'logs', + created_at: now + }) + end + + let!(:deprecated_orphan_webhook_execution) do + create(:webhook_executions, { + triggered_webhook_id: nil, + webhook_id: nil, + success: true, + logs: 'logs', + created_at: now + }) + end + + subject { PactBroker::Database.migrate(42) } + + it "deletes the orphan triggered webhooks" do + expect { subject }.to change { database[:triggered_webhooks].count }.by(-1) + end + + it "deletes the orphan webhook executions" do + expect { subject }.to change { database[:webhook_executions].count }.by(-2) + end +end