Skip to content
Browse files

Merge pull request #10681 from jholton/3-2-stable-fix_association_aut…

…o_save

autosave_association issue that occurs when table has unique index (3.2.x backport)
  • Loading branch information...
2 parents b0f96d4 + c236246 commit f8c4805a8247a62594d29c5f3ef634d57d2abbf1 @rafaelfranca rafaelfranca committed May 22, 2013
View
9 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".
View
48 activerecord/lib/active_record/autosave_association.rb
@@ -331,41 +331,33 @@ def save_collection_association(reflection)
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
View
22 activerecord/test/cases/autosave_association_test.rb
@@ -583,7 +583,7 @@ def test_autosave_new_record_on_has_many_can_be_disabled_per_relationship
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 @@ def test_when_new_record_a_child_marked_for_destruction_should_not_affect_other_
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 @@ def test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_savi
@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

0 comments on commit f8c4805

Please sign in to comment.
Something went wrong with that request. Please try again.