From cce461e7c3fb97161bf7501d6ab2d51953d95f89 Mon Sep 17 00:00:00 2001 From: kathap <30441792+kathap@users.noreply.github.com> Date: Mon, 10 Oct 2022 10:19:35 +0200 Subject: [PATCH] Add an unique index to service_instance_operations (#2929) * Add unique index on column service_instance_id to prevent double entries for the same service_instance * Remove duplicate service_instance_id entries based on max(updated_at) (=keep the newest entry) to prepare for adding a uniqueness constraint * Drop foreign_key constraint which references service_instance_id (needed for mysql) and old index before creating the unique index --- ...instance_operations_service_instance_id.rb | 37 +++++++++++ ...nce_operations_service_instance_id_spec.rb | 62 +++++++++++++++++++ .../service_instance_operation_spec.rb | 13 ++++ 3 files changed, 112 insertions(+) create mode 100644 db/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id.rb create mode 100644 spec/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id_spec.rb diff --git a/db/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id.rb b/db/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id.rb new file mode 100644 index 00000000000..48e480a54bd --- /dev/null +++ b/db/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id.rb @@ -0,0 +1,37 @@ +Sequel.migration do + up do + # Remove duplicate service_instance_operations.service_instance_id to prepare for adding a uniqueness constraint + + dup_groups = self[:service_instance_operations].exclude(service_instance_id: nil). + select(:service_instance_id). + group_by(:service_instance_id). + having { count.function.* > 1 } + + dup_groups.each do |group| + ids_to_remove = self[:service_instance_operations]. + where(service_instance_id: group[:service_instance_id]). + order(Sequel.desc(:updated_at)).order_append(Sequel.desc(:id)). + offset(1). + select_map(:id) + + self[:service_instance_operations].where(id: ids_to_remove).delete + end + + # for mysql the foreign_key constraint which references service_instance_id has to be removed before you can + # delete the old index from service_instance_id + alter_table :service_instance_operations do + drop_constraint :fk_svc_inst_op_svc_instance_id, type: :foreign_key + drop_index :service_instance_id, name: :svc_instance_id_index + add_index :service_instance_id, name: :svc_inst_op_svc_instance_id_unique_index, unique: true + 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 + drop_index :service_instance_id, name: :svc_inst_op_svc_instance_id_unique_index + add_index :service_instance_id, name: :svc_instance_id_index + add_foreign_key [:service_instance_id], :service_instances, key: :id, name: :fk_svc_inst_op_svc_instance_id, on_delete: :cascade + end + end +end diff --git a/spec/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id_spec.rb b/spec/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id_spec.rb new file mode 100644 index 00000000000..b54ffcf9b56 --- /dev/null +++ b/spec/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +RSpec.describe 'migration to add unique index on service_instance_id to service_instance_operations', isolation: :truncation do + let(:filename) { '20220818142407_add_unique_index_to_service_instance_operations_service_instance_id.rb' } + let(:tmp_migrations_dir) { Dir.mktmpdir } + let(:db) { Sequel::Model.db } + let(:service_instance) { VCAP::CloudController::ServiceInstance.make } + + before do + FileUtils.cp(File.join(DBMigrator::SEQUEL_MIGRATIONS, filename), tmp_migrations_dir) + + # Override the 'allow_manual_update' option of 'Sequel::Plugins::Timestamps' for 'ServiceInstanceOperation'. + VCAP::CloudController::ServiceInstanceOperation.instance_exec do + @allow_manual_timestamp_update = true + end + + # Revert the given migration, i.e. remove the uniqueness constraint. + Sequel::Migrator.run(db, tmp_migrations_dir, target: 0, allow_missing_migration_files: true) + end + + it 'removes duplicate service instance operations' do + # Two operations that do not reference a service instance (i.e. service_instance_id is nil); + # none of them should be removed. + VCAP::CloudController::ServiceInstanceOperation.make + VCAP::CloudController::ServiceInstanceOperation.make + + # Two operations that each reference a different service instance (the 'normal' situation); + # none of them should be removed. + si1 = VCAP::CloudController::ServiceInstance.make + VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si1.id) + si2 = VCAP::CloudController::ServiceInstance.make + VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si2.id) + + # Three operations that reference the same service instance and have the same 'updated_at' value; + # the one with the highest 'id' should be kept (o3). + si3 = VCAP::CloudController::ServiceInstance.make + o1 = VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si3.id) + o2 = VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si3.id) + o3 = VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si3.id) + o2.update({ updated_at: o1.updated_at }) + o3.update({ updated_at: o1.updated_at }) + + # Three operations that reference the same service instance and have different 'updated_at' values; + # the one with the newest 'updated_at' value should be kept (o5). + si4 = VCAP::CloudController::ServiceInstance.make + o4 = VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si4.id) + o5 = VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si4.id) + o6 = VCAP::CloudController::ServiceInstanceOperation.make(service_instance_id: si4.id) + o5.update({ updated_at: o4.updated_at + 1 }) + o6.update({ updated_at: o4.updated_at - 1 }) + + expect(VCAP::CloudController::ServiceInstanceOperation.count).to eq(10) + + Sequel::Migrator.run(db, tmp_migrations_dir, allow_missing_migration_files: true) + + expect(VCAP::CloudController::ServiceInstanceOperation.count).to eq(6) + expect { o1.reload }.to raise_error(Sequel::NoExistingObject) + expect { o2.reload }.to raise_error(Sequel::NoExistingObject) + expect { o4.reload }.to raise_error(Sequel::NoExistingObject) + expect { o6.reload }.to raise_error(Sequel::NoExistingObject) + end +end diff --git a/spec/unit/models/services/service_instance_operation_spec.rb b/spec/unit/models/services/service_instance_operation_spec.rb index 4be1a5de7a8..8638ac543ee 100644 --- a/spec/unit/models/services/service_instance_operation_spec.rb +++ b/spec/unit/models/services/service_instance_operation_spec.rb @@ -51,5 +51,18 @@ module VCAP::CloudController expect(operation.state).to eq 'finished' end end + + describe 'when two are created with the same id' do + describe 'when a ServiceInstanceOperation exists' do + let(:service_instance) { ServiceInstance.make } + before { ServiceInstanceOperation.make(service_instance_id: service_instance.id) } + + it 'raises an exception when creating another ServiceInstanceOperation' do + expect { + ServiceInstanceOperation.make(service_instance_id: service_instance.id) + }.to raise_error(Sequel::UniqueConstraintViolation) + end + end + end end end