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