Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Nested attributes with callbacks bugfix #5476

Closed
wants to merge 1 commit into from

8 participants

@joergschray

I discovered a bug in the nested attributes assignment: It occurs when
an association callback changes the "loaded-ness" of the association
during the assignment. After association.loaded? changes from false to
true, the destroy flag fails to function. The first commit illustrates
the issue with a unit test. The test fails in the master branch -
cherry-picked to 3-2-stable, 3-1-stable, 3-0-stable fails the same way.

The second commit is the suggested fix. It replaces a section that
previously addressed the same issue of recognizing if a record was
already in the proxy_target array. When it wasn't, it called
add_to_target, which in turn calls the before_add callback. IMO this is
not correct, because the record concerned already exists in the database
as part of the association. The documentation of the callback states
that an exception may be raised to prevent the record from being added
to the association which contradicts the fact. Therefore I just plainly
appended the record to the proxy_target array. (The unit test in the
first commit pins down this behaviour). The fix is tested on sqlite on
master, 3-2-stable, 3-1-stable. The fix to 3-0-stable is slightly
different, so there is another pull request off of branch
nested_attributes_with_callbacks_bug_3-0 (if I manage to create one...).

Considering the whole method
assign_nested_attributes_for_collection_association - it almost
exclusively calls methods on the association and its logic is deeply
entangled with the implmentation details of the CollectionAssociation
class. I've seen some effort to augment the responsibilites of the
association classes, which IMO points in the right direction. A next
step to clear up this spot would be to move the main part of the method
to something like "CollectionAssociation.attributes=" - any comments!?

activerecord/lib/active_record/nested_attributes.rb
@@ -415,20 +415,14 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
unless reject_new_record?(association_name, attributes)
association.build(attributes.except(*unassignable_keys(assignment_opts)), assignment_opts)
end
- elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
- unless association.loaded? || call_reject_if(association_name, attributes)
+ elsif existing_record = begin
@josevalim Owner

Please don't use elsif existing_record = begin. Could you break it apart?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@josevalim
Owner

This looks good but there are some style considerations to be done. From what I see, the code should be basically the same as before, except this line:

target_record = association.target.detect { |record| record == existing_record }

Should be rewritten to:

target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }

No?

@joergschray

No! Aren't the two lines are equivalent, since "==" means same 'id'. I actually prefer the second version because it highlights the problem. Although "target_record == existing_record" they may be different objects, which is the root of the problem (IdentityMap could have helped ...).

The problem with the previous code is the control structure. The lookup in the target cannot be constrained by "unless association.loaded?". So the alternative would be something like:

        elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
          if target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
            existing_record = target_record
          else
            association.target << existing_record # or association.add_to_target(existing_record)
          end
          if !call_reject_if(association_name, attributes)
            assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy], assignment_opts)
          end

Do you like this better?

@josevalim
Owner

Yes, that's what I had in mind. But please break this in two lines:

if target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }

And continue using add_to_target(existing_record) instead of << even though they are equivalent, let's keep the previous semantics. THanks!

@josevalim
Owner

I see you have closed this, could you send a new pull request please? Thanks!

@joergschray joergschray reopened this
@joergschray

I didn't mean to close - hit the wrong button.

I don't agree about the semantics. Shouldn't an association with the target loaded from a previous call like this

    owner.associaton.to_a
    owner.attributes = hash_with_nested_attributes

call the callbacks the same number of times as just the nested attribute assignment:

    # target empty here
    owner.attributes = hash_with_nested_attributes

In the current implementation the callback is called in the second version but not in the first. And by the way, in the application it may not be obvious which of the two situations apply. Since the loading can happen at some completely different place. This action at a distance has made my job in hunting this down a lot more difficult.

If we keep the current behavior, the implementation of the callback needs to check if the record is already in the association or if it is a really new record - which is short of yet another database lookup not quite so straightforward.

@joergschray

I don't quite understand one of your comments:

Yes, that's what I had in mind. But please break this in two lines:

if target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }

The line above does the same thing:

elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }

So would you like

target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
if target_record

or split the "if" across two lines!??

@josevalim
Owner

Yeah, this:

target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
if target_record

Sorry for not making it clear before. :)

@joergschray

OK.

What about the comment above about "before_add callback" being called dependend on the association being loaded?

I added a couple of tests (see commit below) highlighting the inconsistent behavior: With the current implementation the :before_add callback is called when a nested attribute assignment modifies an existing record if the association is not loaded before. The callback is not called if the association is loaded. I.e. on current state of master the last test fails, the one before does not.

...d/test/cases/nested_attributes_with_callbacks_test.rb
((47 lines not shown))
+ end
+
+ test "Assignment to nested attributes with callbacks deletes last bird and calls before_add for new bird" do
+ setup_new_bird
+ assert_deletes_last_bird_and_calls_callback_before_add_for_new_birds(:birds_with_interfering_callback,0)
+ end
+
+ def assert_callback_not_called_when_assigning_to_existing_record
+ # Only change first bird
+ @pirate_with_more_than_one_bird.birds_with_non_interfering_callback_attributes = @birds_attributes[0..0]
+ assert_equal([],@add_callback_called)
+ end
+
+ test "Assignment to nested attributes for existing record does not trigger callback when association is loaded" do
+ @pirate_with_more_than_one_bird.birds_with_non_interfering_callback.load_target
+ assert_equal(true,@pirate_with_more_than_one_bird.birds_with_non_interfering_callback.loaded?)

This test passes with the current implementation in master, because the association is loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...d/test/cases/nested_attributes_with_callbacks_test.rb
((52 lines not shown))
+ end
+
+ def assert_callback_not_called_when_assigning_to_existing_record
+ # Only change first bird
+ @pirate_with_more_than_one_bird.birds_with_non_interfering_callback_attributes = @birds_attributes[0..0]
+ assert_equal([],@add_callback_called)
+ end
+
+ test "Assignment to nested attributes for existing record does not trigger callback when association is loaded" do
+ @pirate_with_more_than_one_bird.birds_with_non_interfering_callback.load_target
+ assert_equal(true,@pirate_with_more_than_one_bird.birds_with_non_interfering_callback.loaded?)
+ assert_callback_not_called_when_assigning_to_existing_record
+ end
+
+ test "Assignment to nested attributes for existing record does not trigger callback when association is not loaded" do
+ assert_equal(false,@pirate_with_more_than_one_bird.birds_with_non_interfering_callback.loaded?)

This test fails with the current implementation in master, because the association is not loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@joergschray

I just did a little research, when the second issue (about calling the before_add callback) was introduced into the code base. It was a side effect of the following commit:

"Ensure not to load the entire association when bulk updating existing records using nested attributes"
2ff5f38

It explicitly adds a call to "add_to_target_with_callback" for an existing record.

@isaacsanders

Is this still being worked on?

activerecord/lib/active_record/nested_attributes.rb
@@ -416,17 +416,15 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
association.build(attributes.except(*unassignable_keys(assignment_opts)), assignment_opts)
end
elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
- unless association.loaded? || call_reject_if(association_name, attributes)
@josevalim Owner

Should we also ignore call_reject_if? What if I am updating the record to a state that should be rejected?

call_reject_if is called later in the same branch to decide if assign_to_or_mark_for_destruction is called or not.

The lines following here deal with the existing record and its occurrence in the target array. This intention is obscured by checking call_reject_if.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva

@josevalim what's the status of this one? ^^

@steveklabnik
Collaborator

Ping! We will need a rebase.

@joergschray

Hi there! Seems like roughly another 4 months are over...

I am sorry - I don't have the capacity to work on this right now - swamped by RL.

Just in case - what's the procedure of a "rebase"?

From what I understood, the procedure to include changes in master in a pull request goes like this:

  • merge my branch locally with master
  • push to the same branch on github, which fast-forwards the github branch and also updates the pull request

If I instead rebase my local branch on master, I wouldn't be able to push to the same branch on github any more, since it's no longer a fast-forward!? So what do I do after the local rebase?

@strzalek
Collaborator

@tandem-softworks how is it going? Do you need any help?

@joergschray

@strzalek - thanks for asking. I just needed a little motiviation...

@strzalek
Collaborator

@tandem-softworks thanks for getting back so quickly.

Can you squash those two commits into one?

@joergschray

I updated the pull request a month ago. - What's next?

@neerajdotname
Collaborator

@tandem-softworks I want to review it but a lot of lines are causing horizontal scrolling . That is making it hard to review code. Can you split the lines so that the diff fits into the window without horizontal scrolling ? Thanks.

@joergschray

Better? - I'd rather not rename too many things.

@joergschray

@neerajdotname - do you still plan to review this pull request?

@neerajdotname
Collaborator

@tandem-softworks I tried to review it. I could not understand the tests very well. And a lot of lines are still causing horizontal scrolling for me.

@joergschray joergschray Issue #1: :before_add callback is called when nested attributes assig…
…nment assigns to existing record if the association is not yet loaded

Issue #2: Nested Attributes assignment does not affect the record in the association target when callback triggers loading of the association

Complete rewrite of tests
1eced51
@joergschray

@neerajdotname - You are right - the tests did too many things at the same time. Please, have another go at it now.

@neerajdotname
Collaborator

@tandem-softworks I think tests needs a bit more clarity. For example following method does not tell me what is happening in one glance.

def b1_update_new_b_b2_destroy_attributes
+    [{'id' => @birds[0].id.to_s, 'name' => 'New Name'},
+     {'name' => "New Bird"},
+     {'id' => @birds[1].id.to_s, "_destroy" => true}]
+  end
@joergschray

This is a utility method, which provides attributes to update bird1 (b1_update), create a new bird (new_b), and destroy bird2 (b2_destroy). I can't name it short enough to avoid horizontal scroliing and make the name more telling than it is at the same time. (Are you familiar with assignments to nested attributes? This method strips the needed content down to the bare minimum.) The combination of changes is necessary to exhibit the second issue - so yes, things are a little complicated. But there is nothing I can do about it.

Maybe you start by looking at the first set of tests about triggering the callback.

@joergschray

@neerajdotname - did you have a chance to look at the first set of tests?

# Characterizing when :before_add callback is called
":before_add called for new bird when not loaded"
":before_add called for new bird when loaded"
":before_add not called for identical assignment when not loaded"
":before_add not called for identical assignment when loaded"
":before_add not called for deletion assignment when not loaded"
":before_add not called for deletion assignment when loaded"
@joergschray

@neerajdotname - seems like this is not your cup of tea - fine ...

@joergschray

The rebase dates back over two months - the fix is still the same as in March 2012. - What's next?

@joergschray

@Empact - how about working together on this???

@Empact

Hey @tandem-softworks - sorry that you feel otherwise, but from my perspective we are working together. You identified the problem and solution, and I better integrated it into the codebase and clarified the tests. That's the beauty of git - it's an open system where we can all improve on the work of others.

Seems the last remaining bit is to add a changelog for your work. Would you be up for pulling from my branch, adding a changelog for your work, and updating this pull request? I would close my pull request if you did that. Otherwise I'm happy to write a changelog on your behalf.

@joergschray

@Empact I can see your perspective - a little note to me about your engagement would have helped. - It's been a mixed experience with the community culture...

Nevertheless, I am happy about you stepping in and about your improvements.

I'll have a go at the remaining steps today - as you described including the changlog entry.

@joergschray

@Empact - Could you have a look at nested_attributes_with_callbacks_bug_review - in particular the changelog entry? Are you happy with the commit message?

If everythings OK I'll update the PR.

What I did:

  • rebased my branch
  • cherry-picked your two commits
  • added the changelog entry for the fix
  • squashed all the changes into one commit.

I made two minor changes in the test flle:

  • renamed new_update_and_destroy_bird_attributes to update_new_and_destroy_bird_attributes, since it reflects the order of the attributes. (The order matters,when one tries alternative fixes.)
  • renamed the test assigning the destroy_bird_attributes to have destroy in the title rather than deletion

I get one regression failure running rake test_sqlite3, which is probably due to my sqlite3-version being 3.6.20:

ActiveRecord::AdapterTest#test_uniqueness_violations_are_translated_to_specific_exception [rails/activerecord/test/cases/adapter_test.rb:141]:
[ActiveRecord::RecordNotUnique] exception expected, not
Class: ActiveRecord::StatementInvalid
Message: <"SQLite3::ConstraintException: constraint failed: INSERT INTO subscribers(nick) VALUES('me')">

@Empact

Yep the the test changes you mention seem good - and I agree the sqlite failure is due to being and older version, I'm not seeing it over here.

I would prefer you not squash my commits into yours, as keeping them separate preserves the line-by-line authorship of the changes and the motivation for them. I updated my PR #11525 to include your changelog and other changes (in your initial commit) while preserving authorship. Take a look!

@joergschray

I understand your point about not squashing the commits.

As I commented over at your PR - I close mine.

@joergschray joergschray deleted the tandem-softworks:nested_attributes_with_callbacks_bug branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 27, 2013
  1. @joergschray

    Issue #1: :before_add callback is called when nested attributes assig…

    joergschray authored
    …nment assigns to existing record if the association is not yet loaded
    
    Issue #2: Nested Attributes assignment does not affect the record in the association target when callback triggers loading of the association
    
    Complete rewrite of tests
This page is out of date. Refresh to see the latest.
View
19 activerecord/lib/active_record/nested_attributes.rb
@@ -412,16 +412,15 @@ def assign_nested_attributes_for_collection_association(association_name, attrib
association.build(attributes.except(*UNASSIGNABLE_KEYS))
end
elsif existing_record = existing_records.detect { |record| record.id.to_s == attributes['id'].to_s }
- unless association.loaded? || call_reject_if(association_name, attributes)
- # Make sure we are operating on the actual object which is in the association's
- # proxy_target array (either by finding it, or adding it if not found)
- target_record = association.target.detect { |record| record == existing_record }
-
- if target_record
- existing_record = target_record
- else
- association.add_to_target(existing_record)
- end
+ # Make sure we are operating on the actual object which is in the association's
+ # proxy_target array (either by finding it, or adding it if not found)
+ # Take into account that the proxy_target may have changed due to callbacks
+ target_record = association.target.detect { |record| record.id.to_s == attributes['id'].to_s }
+ if target_record
+ existing_record = target_record
+ else
+ #FIXME: there is no good way of adding the record without callback
+ association.target << existing_record
end
if !call_reject_if(association_name, attributes)
View
145 activerecord/test/cases/nested_attributes_with_callbacks_test.rb
@@ -0,0 +1,145 @@
+require "cases/helper"
+require "models/pirate"
+require "models/bird"
+
+class TestNestedAttributesWithCallbacksInterferingWithAssignment < ActiveRecord::TestCase
+ Pirate.has_many(:birds_with_interfering_callback,
+ :class_name => "Bird",
+ :before_add => proc { |p,b|
+ @@add_callback_called << b
+ p.birds_with_interfering_callback.to_a
+ })
+ Pirate.has_many(:birds_with_callback,
+ :class_name => "Bird",
+ :before_add => proc { |p,b| @@add_callback_called << b })
+
+ Pirate.accepts_nested_attributes_for(:birds_with_interfering_callback,
+ :birds_with_callback,
+ :allow_destroy => true)
+
+ def setup
+ @@add_callback_called = []
+ @expect_callbacks_for = []
+ @pirate_with_two_birds = Pirate.new.tap do |pirate|
+ pirate.catchphrase = "Don't call me!"
+ pirate.birds_attributes = [{:name => 'Bird1'},{:name => 'Bird2'}]
+ pirate.save!
+ end
+ @birds = @pirate_with_two_birds.birds.to_a
+ @birds_attributes = @birds.map do |bird|
+ bird.attributes.slice("id","name")
+ end
+ end
+
+ def new_birds
+ @pirate_with_two_birds.birds_with_callback.to_a - @birds
+ end
+
+ def new_bird_attributes
+ [{'name' => "New Bird"}]
+ end
+
+ def bird2_deletion_attributes
+ [{'id' => @birds[1].id.to_s, "_destroy" => true}]
+ end
+
+ def b1_update_new_b_b2_destroy_attributes
+ [{'id' => @birds[0].id.to_s, 'name' => 'New Name'},
+ {'name' => "New Bird"},
+ {'id' => @birds[1].id.to_s, "_destroy" => true}]
+ end
+
+ # Characterizing when :before_add callback is called
+ test ":before_add called for new bird when not loaded" do
+ assert_birds_with_callback_not_loaded
+ assert_callback_called_for_new_bird_assignment
+ end
+
+ test ":before_add called for new bird when loaded" do
+ @pirate_with_two_birds.birds_with_callback.load_target
+ assert_callback_called_for_new_bird_assignment
+ end
+
+ def assert_callback_called_for_new_bird_assignment
+ @pirate_with_two_birds.birds_with_callback_attributes = new_bird_attributes
+ assert_equal(1,new_birds.size)
+ assert_callback_called_for_new_birds
+ end
+
+ test ":before_add not called for identical assignment when not loaded" do
+ assert_birds_with_callback_not_loaded
+ assert_callback_not_called_for_identical_assignment
+ end
+
+ test ":before_add not called for identical assignment when loaded" do
+ @pirate_with_two_birds.birds_with_callback.load_target
+ assert_callback_not_called_for_identical_assignment
+ end
+
+ def assert_callback_not_called_for_identical_assignment
+ @pirate_with_two_birds.birds_with_callback_attributes = @birds_attributes
+ assert_equal([],new_birds)
+ assert_callback_called_for_new_birds
+ end
+
+ test ":before_add not called for deletion assignment when not loaded" do
+ assert_birds_with_callback_not_loaded
+ assert_callback_not_called_for_deletion_assignment
+ end
+
+ test ":before_add not called for deletion assignment when loaded" do
+ @pirate_with_two_birds.birds_with_callback.load_target
+ assert_callback_not_called_for_deletion_assignment
+ end
+
+ def assert_callback_not_called_for_deletion_assignment
+ @pirate_with_two_birds.birds_with_callback_attributes =
+ bird2_deletion_attributes
+ assert_callback_called_for_new_birds
+ end
+
+ def assert_birds_with_callback_not_loaded
+ assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?)
+ end
+
+ def assert_callback_called_for_new_birds
+ assert_equal(new_birds,@@add_callback_called)
+ end
+
+ # Ensuring that the records in the association target are updated,
+ # whether the association is loaded before or not
+ test "Assignment updates records in target when not loaded" do
+ assert_equal(false,@pirate_with_two_birds.birds_with_callback.loaded?)
+ assert_assignment_affects_records_in_target(:birds_with_callback)
+ end
+
+ test "Assignment updates records in target when loaded" do
+ @pirate_with_two_birds.birds_with_callback.load_target
+ assert_assignment_affects_records_in_target(:birds_with_callback)
+ end
+
+ test("Assignment updates records in target when not loaded" +
+ " and callback loads target") do
+ assert_equal(false,
+ @pirate_with_two_birds.birds_with_interfering_callback.loaded?)
+ assert_assignment_affects_records_in_target(
+ :birds_with_interfering_callback)
+ end
+
+ test("Assignment updates records in target when loaded" +
+ " and callback loads target") do
+ @pirate_with_two_birds.birds_with_interfering_callback.load_target
+ assert_assignment_affects_records_in_target(
+ :birds_with_interfering_callback)
+ end
+
+ def assert_assignment_affects_records_in_target(association_name)
+ @pirate_with_two_birds.send("#{association_name}_attributes=",
+ b1_update_new_b_b2_destroy_attributes)
+ association = @pirate_with_two_birds.send(association_name)
+ birds_in_target = @birds.map { |b| association.detect { |b_in_t| b_in_t == b }}
+ assert_equal(true,birds_in_target[0].name_changed?,'First record not changed')
+ assert_equal(true,birds_in_target[1].marked_for_destruction?,
+ 'Second record not marked for destruction')
+ end
+end
Something went wrong with that request. Please try again.