Permalink
Browse files

Merge pull request #10183 from jholton/fix_association_auto_save

autosave_association issue that occurs when table has unique index
  • Loading branch information...
2 parents d098e1c + 9de2841 commit e8727d37fc49d5bf9976c3cb5c46badb92cf4ced @jonleighton jonleighton committed Apr 19, 2013
@@ -1,5 +1,12 @@
## Rails 4.0.0 (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.
+
+ *Johnny Holton*
+
* Run `rake migrate:down` & `rake migrate:up` in transaction if database supports.
*Alexander Bondarev*
@@ -334,15 +334,18 @@ def save_collection_association(reflection)
autosave = reflection.options[:autosave]
if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave)
- records_to_destroy = []
+
+ if autosave
+ records_to_destroy = records.select(&:marked_for_destruction?)
+ records_to_destroy.each { |record| association.destroy(record) }
+ records -= records_to_destroy
+ end
+
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 != false && (@new_record_before_save || record.new_record?)
if autosave
saved = association.insert_record(record, false)
else
@@ -354,10 +357,6 @@ def save_collection_association(reflection)
raise ActiveRecord::Rollback unless saved
end
-
- records_to_destroy.each do |record|
- association.destroy(record)
- end
end
# reconstruct the scope now that we know the owner's id
@@ -764,6 +764,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: "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

4 comments on commit e8727d3

Member

jonleighton replied Apr 19, 2013

@rafaelfranca thanks for the alert

@jholton I have reverted the merge. Please take a look at the problems and resubmit when it is working on all databases. At a guess, I imagine the problem if that you're doing DDL statements in a transaction, which is unsupported by mysql. It would be better not to change the schema directly in the test.

Contributor

jholton replied Apr 23, 2013

@jonleighton Thanks for the info.
Any tips on how to proceed?

I posted the following to the Rails Core Google group, but it looks like my post was deleted.
Where should I ask questions like the following?

What is the preferred steps to take in regards to the Rails testing suite schema/models?
Here are some options:

  • Add unique index to existing table field (assuming it doesn't break existing tests)
  • Create a new field on an existing table and add an index for that
  • Create new model/table and wire up the associations
  • Something else?

Related, I've been playing around with a few of these and am having trouble adding a column to a table.
I changed the schema for 'Parrot' table to add a 'nick' column:

...
t.column :nick, :string
...
add_index :parrots, :nick, :unique => true

But then when I run my test:
ARCONN=mysql2 bundle exec ruby -I test test/cases/autosave_association_test.rb -n test_should_save_new_record_that_has_same_value_as_existing_record_marked_for_destruction_on_field_that_has_unique_index

I get ActiveRecord::UnknownAttributeError: unknown attribute: nick

What do I have to do to have these changes reflected in the db?

I tried:
bundle exec rake mysql:drop_databases
bundle exec rake mysql:build_databases

but got the same error.

Thanks!

Member

jonleighton replied May 3, 2013

@jholton sorry for my slow reply.

It is preferable to use the existing test schema to implement your test if there is already something in the schema that is sufficient to reproduce the problem. If there's not, it's preferable to change the schema as little as possible (i.e. adding a new field is better than adding a new model).

Regarding your issue, that's odd. The schema should get recreated on every test run. Just to confirm, you are editing test/schema/schema.rb, right?

Please sign in to comment.