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