Make many-to-many additions idempotent on unique constraint violation (#4611)

Adds an `ignored_unique_constraint_violation_errors` option to the many_to_many association in the `VcapRelations` Sequel plugin.
When this option is provided with a list of index name patterns, any `Sequel::UniqueConstraintViolation` error that occurs during an `add_` operation on an association and matches one of the patterns will be caught.
The operation is wrapped in a transaction with a savepoint, which is rolled back upon catching the specific error, effectively making the addition idempotent. This prevents race conditions where concurrent requests attempt to add the same association.
This issue can be observed when a user makes two POST requests at the same time to create a new resource instance.

This new option is applied to the `staging_spaces relationship in the `SecurityGroup` model to handle potential concurrent updates. Further models will updated in separate commits.

Author: johha

app/models/runtime/security_group.rb

@@ -17,7 +17,8 @@ class SecurityGroup < Sequel::Model
                  class: 'VCAP::CloudController::Space',
                  join_table: 'staging_security_groups_spaces',
                  right_key: :staging_space_id,
-                 left_key: :staging_security_group_id
+                 left_key: :staging_security_group_id,
+                 ignored_unique_constraint_violation_errors: %w[staging_security_groups_spaces_ids]
 
     add_association_dependencies spaces: :nullify, staging_spaces: :nullify
 

lib/sequel_plugins/vcap_relations.rb

@@ -65,10 +65,17 @@ def many_to_many(name, opts={})
         # sequel is not capable of merging adds to a many_to_many association
         # like it is for a one_to_many and nds up throwing a db exception,
         # so lets squash the add
-        if other.is_a?(Integer)
-          super(other) unless send(ids_attr).include? other
-        else
-          super(other) unless send(name).include? other
+        db.transaction(savepoint: true) do
+          if other.is_a?(Integer)
+            super(other) unless send(ids_attr).include? other
+          else
+            super(other) unless send(name).include? other
+          end
+        rescue Sequel::UniqueConstraintViolation => e
+          # ignore the error and rollback the inner transaction
+          raise Sequel::Rollback if opts[:ignored_unique_constraint_violation_errors]&.any? { |pattern| e.message.include?(pattern) }
+
+          raise e
         end
       end
 

spec/unit/lib/sequel_plugins/vcap_relations_spec.rb

@@ -297,6 +297,63 @@ def define_model(name)
     it 'raises an error using the remove_<relation>_by_guid' do
       expect { @d1.remove_name_by_guid('bogus-guid') }.to raise_error(CloudController::Errors::ApiError, /Could not find/)
     end
+
+    context 'concurrent insert statements' do
+      let(:db_connection) { DbConfig.new.connection }
+
+      before do
+        allow(@d1).to receive(:add_associated_object).and_wrap_original do |original_add_associated_object, *args, &block|
+          # rubocop:disable Rails/SkipsModelValidations
+          db_connection[:dogs_names].insert(dog_id: @d1.id, name_id: @n1.id) # Simulate concurrent insert from a different thread/connection
+          # rubocop:enable Rails/SkipsModelValidations
+          expect(db_connection[:dogs_names].count).to eq(1)
+          original_add_associated_object.call(*args, &block) # this will raise the UniqueConstraintViolation error
+        end
+      end
+
+      it 'raises an UniqueConstraintViolation error' do
+        expect { @d1.add_name(@n1) }.to raise_error(Sequel::UniqueConstraintViolation)
+      end
+
+      it 'does not catch other errors accidentally' do
+        allow(@d1).to receive(:add_associated_object).and_raise(Sequel::DatabaseError.new('some other error'))
+        expect { @d1.add_name(@n1) }.to raise_error(Sequel::DatabaseError, /some other error/)
+      end
+
+      context 'with ignored_unique_constraint_violation_errors option' do
+        before { dog_klass.many_to_many :names, ignored_unique_constraint_violation_errors: %w[dog_id_name_id_idx] }
+
+        it('catches the error and makes the insert idempotent when called with an object') do
+          expect { @d1.add_name(@n1) }.not_to raise_error
+        end
+
+        it 'catches the error and makes the insert idempotent when called with an id' do
+          expect { @d1.add_name(@n1.id) }.not_to raise_error
+        end
+
+        it 'does not rollback or modify other entries in the join table' do
+          db_connection[:dogs_names].db.transaction do
+            expect { @d1.add_name(@n2) }.not_to raise_error
+            expect(db_connection[:dogs_names].count).to eq(2)
+            expect(@d1.names).to include(@n2)
+          end
+        end
+      end
+
+      context 'when the join table does not have a unique constraint' do
+        # This test proves that without a unique constraint or combined primary key duplicate entries can be created
+        # Join tables should always have a unique constraint or combined primary key
+        before do
+          skip unless db_connection.database_type == :postgres # mysql does not allow dropping foreign key connected indexes easily
+          db_connection.run('DROP INDEX IF EXISTS dog_id_name_id_idx')
+        end
+
+        it 'creates duplicate entries' do
+          expect { @d1.add_name(@n1) }.not_to raise_error
+          expect(db_connection[:dogs_names].count).to eq(2)
+        end
+      end
+    end
   end
 
   describe '#has_one_to_many?' do