This project is archived and is in readonly mode.

#4242 ✓resolved
chap

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

    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

    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

    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

    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

    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

    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

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

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

    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

    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

    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

    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

    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

    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

    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

    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

    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

    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

    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

    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

  • José Valim

    José Valim May 15th, 2010 @ 03:45 PM

    Awesome! So please do provide a patch! :)

  • Ian White

    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

    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

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

  • Eloy Duran

    Eloy Duran May 18th, 2010 @ 03:32 PM

    Nice work Ian!

  • Jeff Bigler

    Jeff Bigler May 20th, 2010 @ 01:59 PM

    Thanks Ian!

  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to “3.0.2”
    • Importance changed from “” to “Low”
  • Ivan

    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 end

    class Task < ActiveRecord::Base
    belongs_to :project, :inverse_of => :tasks has_many :assignments, :dependent => :destroy, :inverse_of => :task, :autosave => true accepts_nested_attributes_for :assignments end

    class Assignment < ActiveRecord::Base
    belongs_to :task, :inverse_of => assignments end

    assert_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 # => true

    assert_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

Pages