From cfe5b652afe65e0e0c4009ecb89d62c9e85d4928 Mon Sep 17 00:00:00 2001 From: johha Date: Thu, 16 Oct 2025 13:38:02 +0200 Subject: [PATCH] Make many-to-many additions idempotent on unique constraint violation 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. --- app/models/runtime/security_group.rb | 3 +- lib/sequel_plugins/vcap_relations.rb | 15 +++-- .../lib/sequel_plugins/vcap_relations_spec.rb | 57 +++++++++++++++++++ 3 files changed, 70 insertions(+), 5 deletions(-) diff --git a/app/models/runtime/security_group.rb b/app/models/runtime/security_group.rb index e473f9d8eaa..281255b875a 100644 --- a/app/models/runtime/security_group.rb +++ b/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 diff --git a/lib/sequel_plugins/vcap_relations.rb b/lib/sequel_plugins/vcap_relations.rb index d2e0f2ac513..2bf04bf4543 100644 --- a/lib/sequel_plugins/vcap_relations.rb +++ b/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 diff --git a/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb b/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb index 29976689cd3..df0494f98b1 100644 --- a/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb +++ b/spec/unit/lib/sequel_plugins/vcap_relations_spec.rb @@ -297,6 +297,63 @@ def define_model(name) it 'raises an error using the remove__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