This project is archived and is in readonly mode.
Nested child only updates if parent changes.
Reported by chap | March 20th, 2010 @ 02:51 PM | in 3.0.2
Found a bug in rails 3.0.0.beta with accepts_nested_attributes_for
In this video (10 sec) the nested attribute is only updated if it's parent model is changed.
Using rails 3.0.0.beta and full project is on github.
Summary of models and form:
class Project < ActiveRecord::Base
has_many :tasks
accepts_nested_attributes_for :tasks
end
class Task < ActiveRecord::Base
belongs_to :project
has_many :assignments
accepts_nested_attributes_for :assignments
end
class Assignment < ActiveRecord::Base
belongs_to :task
end
form_for(@project) do |f|
Project: f.text_field :name
f.fields_for :tasks do |task_form|
Task: task_form.text_field :name
task_form.fields_for :assignments do |assignment_form|
Assignment: assignment_form.text_field :name
end
end
f.submit
end
Comments and changes to this ticket
-
chap March 20th, 2010 @ 03:15 PM
Added failing test to project:
http://github.com/chap/broken_nested_attributes/blob/master/test/un...projects(:one).update_attributes :name => projects(:one).name, :tasks_attributes => [{ :id => tasks(:one).id, :name => tasks(:one).name, :assignments_attributes => [{ :id => assignments(:one).id, :name => 'Paul' }] }] assignments(:one).reload assert_equal 'Paul', assignments(:one).name
-
Dirk Holzapfel April 2nd, 2010 @ 10:51 AM
I can confirm this bug on rails 3 beta2/edge.
If you pull chaps test project from github and have problems with rake: take a look at http://github.com/wycats/bundler/issues/issue/159/#comment_175945 -
Jeff Bigler April 5th, 2010 @ 04:06 AM
I can also confirm this bug on Rails 3 beta2/edge. My parent object only contained foreign keys, so the problem was not obvious at all, glad I found this ticket!
-
chap April 5th, 2010 @ 04:22 PM
Good to know I'm not alone.
I think the next step is for one of us to fork the active record repo and create a failing test in the nested attributes test cases:
http://github.com/rails/rails/blob/master/activerecord/test/cases/n...I got a big week ahead of me, but if nobody's done it by Friday I'll try to get it done.
-
Jeff Bigler April 5th, 2010 @ 09:25 PM
- Tag changed from accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for to accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, patch
Here is a patch with the failing test cases.
-
Jeff Bigler April 6th, 2010 @ 06:18 AM
I've been able to narrow the problem down to the associated_records_to_validate_or_save method in activerecord/lib/active_record/autosave_association.rb.
The line:
association.target.find_all { |record| record.new_record? || record.changed? || record.marked_for_destruction? }
doesn't take into consideration children of the selected record with changes. I'm still trying to figure out how to modify the code to work appropriately.
-
chap April 6th, 2010 @ 02:01 PM
Great find Jeff!
For posterity, here's how the method has been modified from 2.3.5 to 3.0.0.beta2.3.5 def associated_records_to_validate_or_save(association, new_record, autosave) if new_record association elsif association.loaded? autosave ? association : association.select { |record| record.new_record? } else autosave ? association.target : association.target.select { |record| record.new_record? } end end
3.0.0.beta def associated_records_to_validate_or_save(association, new_record, autosave) if new_record association elsif autosave association.target.find_all { |record| record.new_record? || record.changed? || record.marked_for_destruction? } else association.target.find_all { |record| record.new_record? } end end
-
Jeff Bigler April 7th, 2010 @ 11:03 PM
- Tag changed from accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, patch to accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, fix, patch
OK, I've worked up a patch that fixes the problem without breaking anything. This patch file includes the updated tests as well as the changes to ActiveRecord to support the nested associations. Please try it out and let me know if there are any problems.
-
Jeff Bigler April 7th, 2010 @ 11:05 PM
- Tag changed from accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, fix, patch to accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, patch
Oops, didn't mean to add a "fix" tag. D'oh!
-
Jeff Bigler April 18th, 2010 @ 10:11 PM
I've updated the patch to work with the latest revision of Rails 3.0 head. Since no one has responded, I'll try contacting someone via #rails-config.
-
eydaimon April 22nd, 2010 @ 12:27 AM
I was just logging on to file a bug for this. Thanks Jeff, for making a patch.
-
Eloy Duran April 27th, 2010 @ 09:46 AM
- Assigned user set to José Valim
Hmm, this feels a bit like duplication. I’d rather be able to call save on a parent, which will run it's before/after filters even if it's not dirty. This way the saving should cascade down to the children…
I hope that explanation is a bit clear :)
-
Jeff Bigler April 27th, 2010 @ 03:53 PM
I know what you mean Eloy. It seemed that the change which broke the original functionality was intended to reduce unnecessary database writes, so I wrote the patch with that goal in mind. But since it's my first time digging around in the Rails internals, I'm definitely in over my head. :)
-
José Valim April 29th, 2010 @ 01:03 PM
- Milestone cleared.
I think the sanest way to handle this patch is by marking the record as changed whenever association_attributes= are set. The implementation should be simple, the ticket will be solved but without saving the record all the time.
-
Cicero Oliveira May 1st, 2010 @ 02:57 PM
I am having the same problem when trying to destroy the record. The behaviour seems to be the same as for changing. If I make changes to the parent record, the child record, marked for deletion is successfully deleted, but if I don't change the parent the child is not deleted.
-
Dan Pickett May 9th, 2010 @ 06:19 PM
- Tag changed from accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, patch to accepts_nested_attributes_for fields_for, rails 3.0 beta, accepts_nested_attributes_for, bugmash, patch
-
Ian White May 15th, 2010 @ 03:13 PM
I've tidied up Jeff's patch, and have also added code to make creating and deleting records in nested associations work as well. Posting here as proof of concept, this is speced against my application code, but I've not checked how performant it is (although it should not 'open' any associations that are not already in memory)
It's for rails 2.3.5, you can use it by dropping the code into an initializer.
# this fixes the problem that deeply nested changed records do not trigger a save on parent objects module ActiveRecord module AutosaveAssociation def associated_records_to_validate_or_save(association, new_record, autosave) if new_record association elsif autosave association.target.select { |record| record.changed_for_autosave? } else association.target.select { |record| record.new_record? } end end def changed_for_autosave? new_record? || changed? || marked_for_destruction? || nested_records_changed_for_autosave? end def nested_records_changed_for_autosave? self.class.reflect_on_all_autosave_associations.each do |reflection| if association = association_instance_get(reflection.name) if [:belongs_to, :has_one].include?(reflection.macro) return true if association.changed_for_autosave? else association.target.each {|ele| return true if ele.changed_for_autosave? } end end end false end end end
If this looks ok, I'll work on a patch
-
José Valim May 15th, 2010 @ 03:21 PM
Ian, your code looks cleaner, but I'm wondering wether a patch that marks the current record as dirty when association_attributes= would perform better.
Isn't "association.target.each { |ele|" makes all autosave association be loaded and retrieved from the database? Or target brings only the objects that were already instantiated?
-
Ian White May 15th, 2010 @ 03:43 PM
(replied via email, but the reply doesn't seem to have showed up here, so I'll post here as well - apologies if there is duplication)
Hi José,
I believe that the conditional (association = association_instance_get...) should guard against instantiating an association that is not already in memory.
To answer your question about #target, my understanding from the code is that #target just returns the ivar in its current state from an assoc proxy. This would mean that no extra records are loaded up, just the records that have been loaded/created by the #_attributes= method.
The problems, as I see it, with marking a record as dirty when attributes are changed are (i) if the subsidiary records are not changed by the #attributes= , the record is saved anyway,and (ii) changing a record by traversal in memory (e.g. in console: @blog.posts.first.comments.first.name = "changed") would not trigger an autosave.
Cheers,
Ian -
Ian White May 18th, 2010 @ 10:36 AM
Ok, here's 2 patches, one against 2-3-stable, and one against master.
The patch tests cases where the grand-parent is has_many, and where the great-grandparent is has_one, and also tests that no extra queries are done in the collection of changed records phase.
the existing tests all pass with these patches applied.
Let me know if these need any more work.
Cheers,
Ian -
Repository May 18th, 2010 @ 03:12 PM
(from [a5696e36c6b49381e84184fcc2f164285a26a166]) Nested records (re: autosave) are now updated even when the intermediate parent record is unchanged [#4242]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/a5696e36c6b49381e84184fcc2f164... -
Repository May 18th, 2010 @ 03:13 PM
- State changed from new to resolved
(from [b439d85a19c02eefab1ee308fff334ac1049524d]) Nested records (re: autosave) are now updated even when the intermediate parent record is unchanged [#4242 state:resolved]
Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/b439d85a19c02eefab1ee308fff334... -
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to 3.0.2
- Importance changed from to Low
-
Ivan January 5th, 2011 @ 08:22 PM
I'm using Rails 3.0.3 and still having the same problem.
class Project < ActiveRecord::Base
has_many :tasks, :dependent => :destroy, :inverse_of => :project, :autosave => true accepts_nested_attributes_for :tasks endclass Task < ActiveRecord::Base
belongs_to :project, :inverse_of => :tasks has_many :assignments, :dependent => :destroy, :inverse_of => :task, :autosave => true accepts_nested_attributes_for :assignments endclass Assignment < ActiveRecord::Base
belongs_to :task, :inverse_of => assignments endassert_equal, "yyy", Assignment.find(1).nome # => true
project = projects(:one)
project.tasks_attributes = [{:id => 1, :assignments_attributes => [{:id => 1, :name = "xxx"}]} ]
assert project.valid? # => true
assert_equal true, project.save # => trueassert_equal "xxx", Assignment.find(1).nome # => false
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
<h2 style="font-size: 14px">Tickets have moved to Github</h2>
The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>
People watching this ticket
Attachments
Tags
Referenced by
- 4242 Nested child only updates if parent changes. (from [a5696e36c6b49381e84184fcc2f164285a26a166]) Nested ...
- 4242 Nested child only updates if parent changes. (from [b439d85a19c02eefab1ee308fff334ac1049524d]) Nested ...
- 4648 Autosave association class names inside tests with nested attributes are mispelled The attached patch fixes some typos in test class names f...