Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Lighthouse 3353 #132

Closed
wants to merge 2 commits into from

4 participants

@chielwester

There was an issue that belongs_to associations with autosave enabled calls save and all callbacks every time the parent record was saved. This results in unexpected behaviour. I've changed this to only call save if the associated record is changed.

@chielwester

I believe this can be pushed to the 3-0-stable branch as well

@joshk

This looks good, do you test to make sure assocs are not saved if they have not changed?

@chielwester

I added an extra test to check if save is indeed not called when the associated record has not changed.
This pull request is indeed already in master for some time. What should i do to get this in a stable release soon?

@josevalim
Owner

@chielwester Rails 3.1 is getting released soon, so I am closing this issue. When 3.1 is out and you feel you need this on 3-0-stable, please reopen the issue. Thanks for your pull request.

@josevalim josevalim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 13, 2010
  1. @chielwester

    Only call save on belongs_to associations if the record has changed o…

    chielwester authored
    …r any nested associations have changed (resolves #3353)
Commits on Apr 27, 2011
  1. @chielwester
This page is out of date. Refresh to see the latest.
View
2  activerecord/lib/active_record/autosave_association.rb
@@ -363,7 +363,7 @@ def save_belongs_to_association(reflection)
if autosave && association.marked_for_destruction?
association.destroy
elsif autosave != false
- saved = association.save(:validate => !autosave) if association.new_record? || autosave
+ saved = association.save(:validate => !autosave) if association.new_record? || (autosave && association.changed_for_autosave?)
if association.updated?
association_id = association.send(reflection.options[:primary_key] || :id)
View
24 activerecord/test/cases/autosave_association_test.rb
@@ -667,10 +667,34 @@ def save(*args)
end
end
+ @ship.pirate.catchphrase = "Changed Catchphrase"
+
assert_raise(RuntimeError) { assert !@ship.save }
assert_not_nil @ship.reload.pirate
end
+ def test_shoud_not_save_associated_record_if_not_changed
+ # Stub the save method of the @ship.pirate instance to make sure save is not called
+ class << @ship.pirate
+ def save(*args)
+ super
+ raise "Oh noes!"
+ end
+ end
+
+ assert_nothing_raised(RuntimeError) { assert @ship.save }
+ assert_not_nil @ship.reload.pirate
+ end
+
+ def test_should_save_changed_child_objects_if_parent_is_saved
+ @pirate = @ship.create_pirate(:catchphrase => "Don' botharrr talkin' like one, savvy?")
+ @parrot = @pirate.parrots.create!(:name => 'Posideons Killer')
+ @parrot.name = "NewName"
+ @ship.save
+
+ assert_equal 'NewName', @parrot.reload.name
+ end
+
# has_many & has_and_belongs_to
%w{ parrots birds }.each do |association_name|
define_method("test_should_destroy_#{association_name}_as_part_of_the_save_transaction_if_they_were_marked_for_destroyal") do
Something went wrong with that request. Please try again.