Add an unique index to service_instance_operations

db/migrations/20220818142407_add_unique_index_to_service_instance_operations_service_instance_id.rb

@@ -0,0 +1,90 @@
+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|
+      sorted_ids = self[:service_instance_operations].
+                   select(:id).
+                   where(service_instance_id: group[:service_instance_id]).
+                   map(&:values).
+                   flatten.
+                   sort
+      ids_to_remove = sorted_ids
+
+      same_si_same_updated_at_take_max_id = self[:service_instance_operations].
+                                            select(Sequel.function(:max, :id)).
+                                            where(service_instance_id: group[:service_instance_id]).
+                                            group_by(:updated_at, :service_instance_id).
+                                            having { count.function.* > 1 }.
+                                            map(&:values).
+                                            flatten.
+                                            sort
+
+      same_si_same_updated_at_take_si_ids = self[:service_instance_operations].exclude(id: same_si_same_updated_at_take_max_id).
+                                            select(:service_instance_id).
+                                            where(service_instance_id: group[:service_instance_id]).
+                                            group_by(:updated_at, :service_instance_id).
+                                            having { count.function.* > 1 }.
+                                            map(&:values).
+                                            flatten.
+                                            sort
+
+      same_si_same_updated_at_take_date = self[:service_instance_operations].exclude(id: same_si_same_updated_at_take_max_id).
+                                          select(:updated_at).
+                                          where(service_instance_id: group[:service_instance_id]).
+                                          group_by(:updated_at, :service_instance_id).
+                                          having { count.function.* > 1 }.
+                                          map(&:values).
+                                          flatten.
+                                          sort
+
+      id_for_same_si_same_updated_at = self[:service_instance_operations].
+                                       select(:id).
+                                       where(updated_at: same_si_same_updated_at_take_date, service_instance_id: same_si_same_updated_at_take_si_ids).
+                                       map(&:values).
+                                       flatten.
+                                       sort
+
+      same_si_diff_updated_at_take_max_updated_at = self[:service_instance_operations].exclude(id: id_for_same_si_same_updated_at).
+                                                    select(Sequel.function(:max, :updated_at)).
+                                                    where(service_instance_id: group[:service_instance_id]).
+                                                    group_by(:service_instance_id).
+                                                    having { count.function.* > 1 }.
+                                                    map(&:values).
+                                                    flatten.
+                                                    sort
+
+      id_for_same_si_diff_updated_at = self[:service_instance_operations].
+                                       select(:id).
+                                       where(updated_at: same_si_diff_updated_at_take_max_updated_at).
+                                       map(&:values).
+                                       flatten.
+                                       sort
+
+      ids_to_keep = same_si_same_updated_at_take_max_id + id_for_same_si_diff_updated_at
+
+      self[:service_instance_operations].exclude(id: ids_to_keep).
+        where(id: ids_to_remove).delete
+    end
+
+    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

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

spec/unit/models/services/service_instance_operation_spec.rb

@@ -51,5 +51,37 @@ 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_attrs) do
+          {
+            name: 'bommel_instance',
+            space: VCAP::CloudController::Space.make
+          }
+        end
+
+        let(:service_instance) { ServiceInstance.create(service_instance_attrs) }
+        let(:operation_attributes2) do
+          {
+            service_instance_id: service_instance.id,
+            state: 'in progress',
+            description: '50% all the time',
+            type: 'create',
+            proposed_changes: {
+              name: 'pizza',
+              service_plan_guid: '1800-pizza',
+            },
+          }
+        end
+        before { ServiceInstanceOperation.create(operation_attributes2) }
+
+        it 'raises an exception when creating another ServiceInstanceOperation' do
+          expect {
+            ServiceInstanceOperation.create(operation_attributes2)
+          }.to raise_error(Sequel::UniqueConstraintViolation)
+        end
+      end
+    end
   end
 end