Nested attributes with callbacks bugfix #5476

joergschray · 2012-03-16T21:44:54Z

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!?

josevalim · 2012-03-17T07:25:06Z

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


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

josevalim · 2012-03-17T07:29:50Z

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 · 2012-03-17T10:11:42Z

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 · 2012-03-17T12:12:04Z

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 · 2012-03-17T12:12:49Z

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

joergschray · 2012-03-17T12:29:53Z

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 · 2012-03-17T13:17:18Z

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 · 2012-03-17T14:13:01Z

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 · 2012-03-17T17:19:05Z

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.

joergschray · 2012-03-19T07:25:16Z

activerecord/test/cases/nested_attributes_with_callbacks_test.rb

+
+  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.

joergschray · 2012-03-29T06:56:22Z

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 · 2012-07-28T03:12:36Z

Is this still being worked on?

josevalim · 2012-07-28T07:17:04Z

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)


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.

carlosantoniodasilva · 2012-08-21T05:09:58Z

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

steveklabnik · 2012-11-17T13:53:01Z

Ping! We will need a rebase.

joergschray · 2012-11-17T16:02:33Z

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?

steveklabnik · 2012-11-17T18:20:54Z

You do this:
http://blog.steveklabnik.com/posts/2012-11-08-how-to-squash-commits-in-a-github-pull-request

And yes, you force push.

lukaszx0 · 2013-03-14T16:25:19Z

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

joergschray · 2013-03-14T22:09:55Z

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

lukaszx0 · 2013-03-15T06:51:18Z

@tandem-softworks thanks for getting back so quickly.

Can you squash those two commits into one?

joergschray · 2013-04-15T19:18:14Z

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

neerajsingh0101 · 2013-04-15T19:58:08Z

@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 · 2013-04-15T21:38:11Z

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

joergschray · 2013-04-26T19:49:09Z

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

neerajsingh0101 · 2013-04-26T20:54:32Z

@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.

…assignment assigns to existing record if the association is not yet loaded Issue rails#2: Nested Attributes assignment does not affect the record in the association target when callback triggers loading of the association Complete rewrite of tests

joergschray · 2013-04-27T10:12:06Z

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

neerajsingh0101 · 2013-04-30T16:13:35Z

@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 · 2013-05-01T14:34:17Z

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 · 2013-05-12T18:56:18Z

@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 · 2013-05-23T20:59:54Z

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

joergschray · 2013-05-23T21:28:43Z

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

joergschray · 2013-07-27T10:59:25Z

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

Empact · 2013-07-27T12:02:26Z

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 · 2013-07-27T12:48:00Z

@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 · 2013-07-27T16:08:18Z

@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 · 2013-07-27T19:34:28Z

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 · 2013-07-27T20:35:43Z

I understand your point about not squashing the commits.

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

…s_bug Improve #5476 - "Nested attributes with callbacks bugfix" to use add_to_target and clearer tests

josevalim reviewed Mar 17, 2012
View reviewed changes

joergschray closed this Mar 17, 2012

joergschray reopened this Mar 17, 2012

joergschray reviewed Mar 19, 2012
View reviewed changes

josevalim reviewed Jul 28, 2012
View reviewed changes

Empact mentioned this pull request Jul 21, 2013

Improve #5476 - "Nested attributes with callbacks bugfix" to use add_to_target and clearer tests #11525

Merged

joergschray closed this Jul 27, 2013

joergschray deleted the nested_attributes_with_callbacks_bug branch July 27, 2013 22:55

drogus added a commit that referenced this pull request Aug 12, 2013

Merge pull request #11525 from Empact/nested_attributes_with_callback…

81acaaa

…s_bug Improve #5476 - "Nested attributes with callbacks bugfix" to use add_to_target and clearer tests

Nested attributes with callbacks bugfix #5476

Nested attributes with callbacks bugfix #5476

Conversation

joergschray commented Mar 16, 2012

josevalim Mar 17, 2012

Choose a reason for hiding this comment

josevalim commented Mar 17, 2012

joergschray commented Mar 17, 2012

josevalim commented Mar 17, 2012

josevalim commented Mar 17, 2012

joergschray commented Mar 17, 2012

joergschray commented Mar 17, 2012

josevalim commented Mar 17, 2012

joergschray commented Mar 17, 2012

joergschray Mar 19, 2012

Choose a reason for hiding this comment

joergschray commented Mar 29, 2012

isaacsanders commented Jul 28, 2012

josevalim Jul 28, 2012

Choose a reason for hiding this comment

joergschray Jul 29, 2012

Choose a reason for hiding this comment

carlosantoniodasilva commented Aug 21, 2012

steveklabnik commented Nov 17, 2012

joergschray commented Nov 17, 2012

steveklabnik commented Nov 17, 2012

lukaszx0 commented Mar 14, 2013

joergschray commented Mar 14, 2013

lukaszx0 commented Mar 15, 2013

joergschray commented Apr 15, 2013

neerajsingh0101 commented Apr 15, 2013

joergschray commented Apr 15, 2013

joergschray commented Apr 26, 2013

neerajsingh0101 commented Apr 26, 2013

joergschray commented Apr 27, 2013

neerajsingh0101 commented Apr 30, 2013

joergschray commented May 1, 2013

joergschray commented May 12, 2013

joergschray commented May 23, 2013

joergschray commented May 23, 2013

joergschray commented Jul 27, 2013

Empact commented Jul 27, 2013

joergschray commented Jul 27, 2013

joergschray commented Jul 27, 2013

Empact commented Jul 27, 2013

joergschray commented Jul 27, 2013