#4642 Unsaved changes to nested attribute collections are lost when using collection - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4642 ✓resolved

Unsaved changes to nested attribute collections are lost when using collection

Reported by James Le Cuirot | May 18th, 2010 @ 05:05 PM | in 3.0.2

Commit 5efb1503dd88b59fe491dade92790c3f06293445 causes unsaved changes to records in nested attribute collections to be lost when the collection is used afterwards. This is because the collection is no longer loaded when the update occurs and any subsequent loading afterwards erases the changes.

I tried to find some reasonable way to work around this but even using the new association callbacks doesn't work. If I load the collection with the before_add callback, the record then appears in the collection twice, once from the load and once from the update. Having to load the collection explicitly before any update is just a pain.

I think this feature is fundamentally broken unless some extra code is written to append the records that haven't been updated instead. I therefore would like to see this commit reverted. Attached is a failing test case.

Assigning this to Pratik as it was his commit.

test_should_not_overwrite_unsaved_updates_when_loading_association.patch 946 Bytes

Comments and changes to this ticket

You flagged this item as spam.
James Le Cuirot May 18th, 2010 @ 05:06 PM
- Tag set to “nested attributes, active_record, testcase”
Rizwan Reza May 21st, 2010 @ 12:49 AM
- no changes were found...
You flagged this item as spam.
Raphael Emourgeon May 26th, 2010 @ 10:52 PM
Same problem here and no good workaround.
You flagged this item as spam.
Pete Deffendol June 3rd, 2010 @ 11:38 PM
I"m not sure if my problem is exactly the same as reported here, but I've narrowed the cause down to the same commit. My issue is that the _destroy attributes in my nested collection hashes aren't causing ActiveRecord to remove the referenced members from the collection. It doesn't happen for all of my models using nested attribute, though.

I'll try to squash it down to a simple example soon.
You flagged this item as spam.
James Le Cuirot June 3rd, 2010 @ 11:51 PM
I think that would happen if you're somehow loading (i.e. accessing) the collection before the parent record is finally saved.
You flagged this item as spam.
Pete Deffendol June 4th, 2010 @ 05:13 PM
I stripped down my model and controller to pinpoint the problem, and it turns out that I have a custom validation method that is accessing the collection before it is being saved. So, yes, I have the same problem as the other commenters to this ticket.
You flagged this item as spam.
James Le Cuirot June 6th, 2010 @ 01:05 PM
Here's a possible fix that doesn't involve removing the feature. Records that are already present in the collection are removed from the records that are loaded. I tried to make it not load these existings records in the first place but I couldn't get it to work.
- no_overwrite_on_load_target.patch 1.4 KB
You flagged this item as spam.
James Le Cuirot June 6th, 2010 @ 01:11 PM
Oh and I wasn't sure if it was worth preserving the order of the loaded records. With the above patch, updated records would appear on the end. Does this matter? Maybe it does for things like acts_as_list. If so, I'll adjust it.
You flagged this item as spam.
Jarl Friis June 7th, 2010 @ 08:48 PM
Is #4766 duplicate of this one?
You flagged this item as spam.
James Le Cuirot June 7th, 2010 @ 09:18 PM
Yes!
Pratik June 8th, 2010 @ 11:30 PM
Hey James,

Yeah, I think it'll be good to preserve the order. Could you also use git-format-patch for generating the patch so that you're credited when it gets committed. Might be a good idea to combine test and fix too.

Thanks !
You flagged this item as spam.
James Le Cuirot June 9th, 2010 @ 12:40 PM
- Tag changed from “nested attributes, active_record, testcase” to “nested attributes, active_record, patch, testcase”
Here you go. I thought it might be as simple as find_target.map { |r| @target.delete(r) || r } + @target but to my surprise, Array#delete returns the object you give it, not the object that was actually deleted.

I haven't tried this with _destroy but it should work just the same.
- 0001-don-t-overwrite-unsaved-updates-when-loading-an-asso.patch 3.6 KB
Repository June 9th, 2010 @ 01:48 PM
- State changed from “new” to “resolved”
(from [0265c708b9696c3943518ad5f3dabdc22c5eba11]) Don't overwrite unsaved updates when loading an association but preserve the order of the loaded records. [#4642 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/0265c708b9696c3943518ad5f3dabd...
Repository June 9th, 2010 @ 01:50 PM
(from [b41c3ba154c2038ecc7b230693662257833869b8]) Don't overwrite unsaved updates when loading an association but preserve the order of the loaded records. [#4642 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/b41c3ba154c2038ecc7b2306936622...
Repository June 11th, 2010 @ 04:07 PM
- State changed from “resolved” to “open”
(from [85cc1fa657f441417f36998a32a6a158c2697aad]) Revert "Don't overwrite unsaved updates when loading an association but preserve the order of the loaded records. [#4642 state:open]"

This commit introduced a regression described in ticket [#4830].

This reverts commit 0265c708b9696c3943518ad5f3dabdc22c5eba11.
http://github.com/rails/rails/commit/85cc1fa657f441417f36998a32a6a1...
José Valim June 11th, 2010 @ 05:25 PM
- Milestone cleared.
Guys,

This commit was reverted because it make Sam's Ruby test suite (that runs the Depot web application for the Agile Web Development with Rails) fail.

Sam attached a failing scenario at ticket #4830. Could someone please work on it?

Thanks!
You flagged this item as spam.
Jeff Bigler June 17th, 2010 @ 03:47 PM
On a side note, rolling back this commit prevents nested attributes that are not mapped to database fields from being updated.
You flagged this item as spam.
James Le Cuirot June 17th, 2010 @ 04:00 PM
Don't worry, I have fixed the above failure in a way that satisfies both issues. See #4830.
Jeremy Kemper June 17th, 2010 @ 06:41 PM
- State changed from “open” to “duplicate”
You flagged this item as spam.
helg June 30th, 2010 @ 12:37 AM
- Importance changed from “” to “High”
at :allow_destroy -> true, I'm still loosing _destroy attribute
You flagged this item as spam.
James Le Cuirot June 30th, 2010 @ 11:40 PM
Damn. Sorry. I should have tested that explicitly. Now I have and here's a patch with a fix. Note that this will still load any outside updates to the record if there haven't been any locally. The key difference is that the destruction property is carried across.

Devs, please apply this to master and 2-3-stable ASAP.
- 0001-don-t-remove-scheduled-destroys-when-loading-an-asso.patch 2.2 KB
Repository July 1st, 2010 @ 12:07 AM
- State changed from “duplicate” to “resolved”
(from [f3fedd7f84c25d1d99a70af1e21e20abb48f100f]) Don't remove scheduled destroys when loading an association. [#4642 state:resolved]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/f3fedd7f84c25d1d99a70af1e21e20...
You flagged this item as spam.
Jarl Friis August 3rd, 2010 @ 11:43 AM
Why is Milestone not set to 2.3.9 on this ticket (as it is on ticket #4766) when it apparently is high-priority? Version 2.3.8 is seriously broken with this bug.

Further when milestone is set to 2.3.9 (please...), then the patch should also be applied to 2-3-stable. I am still stuck with 2.3.5 due to this bug... Hoping to see the fix in 2.3.9 before being forced to upgrade to 3.x one day.

Jarl
José Valim August 3rd, 2010 @ 11:45 AM
This was also applied on 2-3-stable. Please check the commits on Github.
You flagged this item as spam.
Jarl Friis August 3rd, 2010 @ 12:19 PM
Thanks for the info, I really tried to find the info myself (I really did), but lack of experience with github and git made me run a
```
git branch --contains f3fedd7f84c25d1d99a70af1e21e20abb48f100f
```
but that only revealed the master branch (because the commit on the 2-3-stable was a different commit). I would have expcted a commit on one branch and the a cherry-pick merge from one to the other...

Thanks and sorry for the inconvenience.
José Valim August 3rd, 2010 @ 12:22 PM
No problem! I usually cherry pick commits from one branch to another but that is not always possible. :(
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]