diff options
author | Johnny Holton <johnny@holton.co> | 2013-05-18 18:16:38 -0400 |
---|---|---|
committer | Johnny Holton <johnny@holton.co> | 2013-05-18 18:20:52 -0400 |
commit | c2362461cdf9615d5704d6f2942921b84b854c3c (patch) | |
tree | 171e4129bc722d229f108ad86cde5678f6060025 /activerecord | |
parent | 0d8dbd131b551051ce35379093832fa12fb0cf10 (diff) | |
download | rails-c2362461cdf9615d5704d6f2942921b84b854c3c.tar.gz rails-c2362461cdf9615d5704d6f2942921b84b854c3c.tar.bz2 rails-c2362461cdf9615d5704d6f2942921b84b854c3c.zip |
destroys association records before saving/inserting new association records
This is a backport of #10417
fixes bug introduced by #3329
These are the conditions necessary to reproduce the bug:
- For an association, autosave => true.
- An association record is being destroyed
- A new association record is being created.
- There is a unique index one of the association's fields.
- The record being created has the same value as the record being
destroyed on the indexed field.
Before, the deletion of records was postponed until after all
insertions/saves. Therefore the new record with the identical value in
the indexed field caused a non-unique value error to be thrown at the
database
level.
With this fix, the deletions happen first, before the insertions/saves.
Therefore the record with the duplicate value is gone from the database
before the new record is created, thereby avoiding the non-uniuqe value
error.
Diffstat (limited to 'activerecord')
-rw-r--r-- | activerecord/CHANGELOG.md | 9 | ||||
-rw-r--r-- | activerecord/lib/active_record/autosave_association.rb | 48 | ||||
-rw-r--r-- | activerecord/test/cases/autosave_association_test.rb | 22 |
3 files changed, 48 insertions, 31 deletions
diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index f8c75c0fba..ebd1a3424a 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,5 +1,14 @@ ## unreleased ## +* fixes bug introduced by #3329. Now, when autosaving associations, + deletions happen before inserts and saves. This prevents a 'duplicate + unique value' database error that would occur if a record being created had + the same value on a unique indexed field as that of a record being destroyed. + + Backport of #10417 + + *Johnny Holton* + * Fix that under some conditions, Active Record could produce invalid SQL of the sort: "SELECT DISTINCT DISTINCT". diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index e1499fc3b0..7bc103043e 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -331,41 +331,33 @@ module ActiveRecord autosave = reflection.options[:autosave] if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) - begin - records_to_destroy = [] - - records.each do |record| - next if record.destroyed? - - saved = true - - if autosave && record.marked_for_destruction? - records_to_destroy << record - elsif autosave != false && (@new_record_before_save || record.new_record?) - if autosave - saved = association.insert_record(record, false) - else - association.insert_record(record) unless reflection.nested? - end - elsif autosave - saved = record.save(:validate => false) - end - raise ActiveRecord::Rollback unless saved - end + if autosave + records_to_destroy = records.select(&:marked_for_destruction?) + records_to_destroy.each { |record| association.proxy.destroy(record) } + records -= records_to_destroy + end + + records.each do |record| - records_to_destroy.each do |record| - association.proxy.destroy(record) + saved = true + + if autosave != false && (@new_record_before_save || record.new_record?) + if autosave + saved = association.insert_record(record, false) + else + association.insert_record(record) unless reflection.nested? + end + elsif autosave + saved = record.save(:validate => false) end - rescue - records.each {|x| IdentityMap.remove(x) } if IdentityMap.enabled? - raise - end + raise ActiveRecord::Rollback unless saved + end end # reconstruct the scope now that we know the owner's id - association.send(:reset_scope) if association.respond_to?(:reset_scope) + association.reset_scope if association.respond_to?(:reset_scope) end end diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index e6b881389b..bb07cf4185 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -583,7 +583,7 @@ class TestDefaultAutosaveAssociationOnNewRecord < ActiveRecord::TestCase end class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase - self.use_transactional_fixtures = false unless supports_savepoints? + self.use_transactional_fixtures = false def setup @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") @@ -780,6 +780,20 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase assert_equal 2, @pirate.birds.reload.length end + def test_should_save_new_record_that_has_same_value_as_existing_record_marked_for_destruction_on_field_that_has_unique_index + Bird.connection.add_index :birds, :name, unique: true + + 3.times { |i| @pirate.birds.create(name: "unique_birds_#{i}") } + + @pirate.birds[0].mark_for_destruction + @pirate.birds.build(name: @pirate.birds[0].name) + @pirate.save! + + assert_equal 3, @pirate.birds.reload.length + ensure + Bird.connection.remove_index :birds, column: :name + end + # Add and remove callbacks tests for association collections. %w{ method proc }.each do |callback_type| define_method("test_should_run_add_callback_#{callback_type}s_for_has_many") do @@ -862,8 +876,10 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase @pirate.parrots.each { |parrot| parrot.mark_for_destruction } assert @pirate.save - assert_queries(0) do - assert @pirate.save + Pirate.transaction do + assert_queries(0) do + assert @pirate.save + end end end |