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>

4 files changed, 106 insertions, 2 deletions

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index fd1082a268..c553e95bad 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -214,6 +214,12 @@ module ActiveRecord
       @marked_for_destruction
     end
 
+    # Returns whether or not this record has been changed in any way (including whether
+    # any of its nested autosave associations are likewise changed)
+    def changed_for_autosave?
+      new_record? || changed? || marked_for_destruction? || nested_records_changed_for_autosave?
+    end
+    
     private
 
     # Returns the record for an association collection that should be validated
@@ -223,12 +229,27 @@ module ActiveRecord
       if new_record
         association
       elsif autosave
-        association.target.find_all { |record| record.new_record? || record.changed? || record.marked_for_destruction? }
+        association.target.find_all { |record| record.changed_for_autosave? }
       else
         association.target.find_all { |record| record.new_record? }
       end
     end
 
+    # go through nested autosave associations that are loaded in memory (without loading
+    # any new ones), and return true if is changed for autosave
+    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.target && association.target.changed_for_autosave?
+          else
+            association.target.each {|record| return true if record.changed_for_autosave? }
+          end
+        end
+      end
+      false
+    end
+    
     # Validate the association if <tt>:validate</tt> or <tt>:autosave</tt> is
     # turned on for the association specified by +reflection+.
     def validate_single_association(reflection)
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index fadd62b5a1..57b66fb312 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -1,6 +1,7 @@
 require "cases/helper"
 require "models/pirate"
 require "models/ship"
+require "models/ship_part"
 require "models/bird"
 require "models/parrot"
 require "models/treasure"
@@ -732,3 +733,82 @@ class TestNestedAttributesWithNonStandardPrimaryKeys < ActiveRecord::TestCase
     assert_equal ['Foo', 'Bar'], @owner.pets.map(&:name)
   end
 end
+
+class TestHasOneAutosaveAssoictaionWhichItselfHasAutosaveAssociations < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @pirate = Pirate.create!(:catchphrase => "My baby takes tha mornin' train!")
+    @ship = @pirate.create_ship(:name => "The good ship Dollypop")
+    @part = @ship.parts.create!(:name => "Mast")
+    @trinket = @part.trinkets.create!(:name => "Necklace")
+  end
+  
+  test "when great-grandchild changed in memory, saving parent should save great-grandchild" do
+    @trinket.name = "changed"
+    @pirate.save
+    assert_equal "changed", @trinket.reload.name
+  end
+  
+  test "when great-grandchild changed via attributes, saving parent should save great-grandchild" do
+    @pirate.attributes = {:ship_attributes => {:id => @ship.id, :parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :name => "changed"}]}]}}
+    @pirate.save
+    assert_equal "changed", @trinket.reload.name
+  end
+  
+  test "when great-grandchild marked_for_destruction via attributes, saving parent should destroy great-grandchild" do
+    @pirate.attributes = {:ship_attributes => {:id => @ship.id, :parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :_destroy => true}]}]}}
+    assert_difference('@part.trinkets.count', -1) { @pirate.save }
+  end
+  
+  test "when great-grandchild added via attributes, saving parent should create great-grandchild" do
+    @pirate.attributes = {:ship_attributes => {:id => @ship.id, :parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:name => "created"}]}]}}
+    assert_difference('@part.trinkets.count', 1) { @pirate.save }
+  end
+  
+  test "when extra records exist for associations, validate (which calls nested_records_changed_for_autosave?) should not load them up" do
+    @trinket.name = "changed"
+    Ship.create!(:pirate => @pirate, :name => "The Black Rock")
+    ShipPart.create!(:ship => @ship, :name => "Stern")
+    assert_no_queries { @pirate.valid? }
+  end
+end
+
+class TestHasManyAutosaveAssoictaionWhichItselfHasAutosaveAssociations < ActiveRecord::TestCase
+  self.use_transactional_fixtures = false
+
+  def setup
+    @ship = Ship.create!(:name => "The good ship Dollypop")
+    @part = @ship.parts.create!(:name => "Mast")
+    @trinket = @part.trinkets.create!(:name => "Necklace")
+  end
+  
+  test "when grandchild changed in memory, saving parent should save grandchild" do
+    @trinket.name = "changed"
+    @ship.save
+    assert_equal "changed", @trinket.reload.name
+  end
+  
+  test "when grandchild changed via attributes, saving parent should save grandchild" do
+    @ship.attributes = {:parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :name => "changed"}]}]}
+    @ship.save
+    assert_equal "changed", @trinket.reload.name
+  end
+  
+  test "when grandchild marked_for_destruction via attributes, saving parent should destroy grandchild" do
+    @ship.attributes = {:parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:id => @trinket.id, :_destroy => true}]}]}
+    assert_difference('@part.trinkets.count', -1) { @ship.save }
+  end
+  
+  test "when grandchild added via attributes, saving parent should create grandchild" do
+    @ship.attributes = {:parts_attributes => [{:id => @part.id, :trinkets_attributes => [{:name => "created"}]}]}
+    assert_difference('@part.trinkets.count', 1) { @ship.save }
+  end
+  
+  test "when extra records exist for associations, validate (which calls nested_records_changed_for_autosave?) should not load them up" do
+    @trinket.name = "changed"
+    Ship.create!(:name => "The Black Rock")
+    ShipPart.create!(:ship => @ship, :name => "Stern")
+    assert_no_queries { @ship.valid? }
+  end
+end
diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb
index 75c792d176..3da031946f 100644
--- a/activerecord/test/models/ship.rb
+++ b/activerecord/test/models/ship.rb
@@ -3,8 +3,9 @@ class Ship < ActiveRecord::Base
 
   belongs_to :pirate
   belongs_to :update_only_pirate, :class_name => 'Pirate'
-  has_many :parts, :class_name => 'ShipPart', :autosave => true
+  has_many :parts, :class_name => 'ShipPart'
 
+  accepts_nested_attributes_for :parts, :allow_destroy => true
   accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
   accepts_nested_attributes_for :update_only_pirate, :update_only => true
 
diff --git a/activerecord/test/models/ship_part.rb b/activerecord/test/models/ship_part.rb
index 0a606db239..b6a8a506b4 100644
--- a/activerecord/test/models/ship_part.rb
+++ b/activerecord/test/models/ship_part.rb
@@ -1,5 +1,7 @@
 class ShipPart < ActiveRecord::Base
   belongs_to :ship
+  has_many :trinkets, :class_name => "Treasure", :as => :looter
+  accepts_nested_attributes_for :trinkets, :allow_destroy => true
 
   validates_presence_of :name
 end
\ No newline at end of file