From ea7d7e462e92adeba6a6c244b3dfaaabe72eb11c Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Fri, 5 Aug 2022 13:40:37 +0200 Subject: [PATCH 1/3] Add delete on cascade to fk_svc_inst_op_svc_instance_id We can see double entries for the same service_instance_id in the service_instance_operations table. This leads to: PG::ForeignKeyViolation: ERROR: update or delete on table "service_instances" violates foreign key constraint "fk_svc_inst_op_svc_instance_id" on table "service_instance_operations" For service bindings, service keys and route bindings the foreign key constraint from operation to resource contains ON DELETE CASCADE; for service instances this is missing. Adding ON DELETE CASCADE to FK fk_svc_inst_op_svc_instance_id in service_instance_operations --- ...scade_to_fk_svc_inst_op_svc_instance_id.rb | 18 +++++++++++++ .../models/services/service_instance_spec.rb | 25 +++++++++++++++++++ 2 files changed, 43 insertions(+) create mode 100644 db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb diff --git a/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb b/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb new file mode 100644 index 00000000000..40333467634 --- /dev/null +++ b/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb @@ -0,0 +1,18 @@ +Sequel.migration do + # Add delete cascade to fk_svc_inst_op_svc_instance_id, without there won't be cleaned up all ocurrencies in + # the service_instance_operations table + + up do + alter_table :service_instance_operations do + drop_constraint :fk_svc_inst_op_svc_instance_id, type: :foreign_key + add_foreign_key [:service_instance_id], :service_instances, key: :id, name: :fk_svc_inst_op_svc_instance_id, on_delete: :cascade + end + end + + down do + alter_table :service_instance_operations do + drop_constraint :fk_svc_inst_op_svc_instance_id, type: :foreign_key + add_foreign_key [:service_instance_id], :service_instances, key: :id, name: :fk_svc_inst_op_svc_instance_id + end + end +end diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 9a4ac055557..9dc267e186c 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -183,6 +183,31 @@ module VCAP::CloudController expect(ServiceInstanceAnnotationModel.where(id: annotation.id)).to be_empty end + it 'cascade deletes all ServiceInstanceOperations for this instance' do + last_operation = ServiceInstanceOperation.make + service_instance.service_instance_operation = last_operation + + service_instance.destroy + + expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil + expect(ServiceInstanceOperation.find(guid: last_operation.guid)).to be_nil + end + + it 'cascades deletion of related dependencies' do + binding = ServiceInstance.make + ServiceInstanceLabelModel.make(key_name: 'foo', value: 'bar', service_instance: binding) + ServiceInstanceAnnotationModel.make(key_name: 'baz', value: 'wow', service_instance: binding) + last_operation = ServiceInstanceOperation.make + binding.service_instance_operation = last_operation + + binding.destroy + + expect(ServiceInstance.find(guid: binding.guid)).to be_nil + expect(ServiceInstanceOperation.find(id: last_operation.id)).to be_nil + expect(ServiceInstanceLabelModel.find(resource_guid: binding.guid)).to be_nil + expect(ServiceInstanceAnnotationModel.find(resource_guid: binding.guid)).to be_nil + end + it 'creates a DELETED service usage event' do expect { service_instance.destroy From 87799ea2a13ed16fdf7c27b9dab239c6c51d1a56 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 12 Sep 2022 13:07:15 +0200 Subject: [PATCH 2/3] Improve description text of migration and test, remove unrelated test, find by PK; as suggested --- ...scade_to_fk_svc_inst_op_svc_instance_id.rb | 4 ++-- .../models/services/service_instance_spec.rb | 19 ++----------------- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb b/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb index 40333467634..c4409cb0646 100644 --- a/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb +++ b/db/migrations/20220805145100_add_delete_on_cascade_to_fk_svc_inst_op_svc_instance_id.rb @@ -1,6 +1,6 @@ Sequel.migration do - # Add delete cascade to fk_svc_inst_op_svc_instance_id, without there won't be cleaned up all ocurrencies in - # the service_instance_operations table + # Add DELETE CASCADE to foreign key fk_svc_inst_op_svc_instance_id to ensure that 'service instance operations' + # are deleted together with the referenced 'service instance'. up do alter_table :service_instance_operations do diff --git a/spec/unit/models/services/service_instance_spec.rb b/spec/unit/models/services/service_instance_spec.rb index 9dc267e186c..96b01a6d2c1 100644 --- a/spec/unit/models/services/service_instance_spec.rb +++ b/spec/unit/models/services/service_instance_spec.rb @@ -183,29 +183,14 @@ module VCAP::CloudController expect(ServiceInstanceAnnotationModel.where(id: annotation.id)).to be_empty end - it 'cascade deletes all ServiceInstanceOperations for this instance' do + it 'cascade deletes the related ServiceInstanceOperation for this instance' do last_operation = ServiceInstanceOperation.make service_instance.service_instance_operation = last_operation service_instance.destroy - expect(ServiceInstance.find(guid: service_instance.guid)).to be_nil - expect(ServiceInstanceOperation.find(guid: last_operation.guid)).to be_nil - end - - it 'cascades deletion of related dependencies' do - binding = ServiceInstance.make - ServiceInstanceLabelModel.make(key_name: 'foo', value: 'bar', service_instance: binding) - ServiceInstanceAnnotationModel.make(key_name: 'baz', value: 'wow', service_instance: binding) - last_operation = ServiceInstanceOperation.make - binding.service_instance_operation = last_operation - - binding.destroy - - expect(ServiceInstance.find(guid: binding.guid)).to be_nil + expect(ServiceInstance.find(id: service_instance.id)).to be_nil expect(ServiceInstanceOperation.find(id: last_operation.id)).to be_nil - expect(ServiceInstanceLabelModel.find(resource_guid: binding.guid)).to be_nil - expect(ServiceInstanceAnnotationModel.find(resource_guid: binding.guid)).to be_nil end it 'creates a DELETED service usage event' do From dad5875e8f32035f4a3acdc763b7aa5df8535230 Mon Sep 17 00:00:00 2001 From: Katharina Przybill <30441792+kathap@users.noreply.github.com> Date: Mon, 12 Sep 2022 13:18:27 +0200 Subject: [PATCH 3/3] Remove association dependency to be consistent with service binding operations, route binding operations and service key operations; as suggested --- app/models/services/service_instance.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/models/services/service_instance.rb b/app/models/services/service_instance.rb index 612b2e9bac8..6e5355b6a1e 100644 --- a/app/models/services/service_instance.rb +++ b/app/models/services/service_instance.rb @@ -24,7 +24,6 @@ class InvalidServiceBinding < StandardError; end serialize_attributes :json, :tags one_to_one :service_instance_operation - add_association_dependencies service_instance_operation: :destroy one_to_many :service_bindings, before_add: :validate_service_binding, key: :service_instance_guid, primary_key: :guid one_to_many :service_keys