Nested child only updates if parent changes.

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

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

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!

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.

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.

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

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.

I was just logging on to file a bug for this. Thanks Jeff, for making a patch.

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

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.

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

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?

(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

Awesome! So please do provide a patch! :)

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

(from [a5696e36c6b49381e84184fcc2f164285a26a166]) Nested records (re: autosave) are now updated even when the intermediate parent record is unchanged [#4242 Nested child only updates if parent changes.]

Signed-off-by: José Valim jose.valim@gmail.com
http://github.com/rails/rails/commit/a5696e36c6b49381e84184fcc2f164...

Nice work Ian!

Thanks Ian!

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