Sam/reenable automatic inference of inverse of for autosave associations

activerecord/lib/active_record/autosave_association.rb

@@ -270,6 +270,16 @@ def changed_for_autosave?
       new_record? || changed? || marked_for_destruction? || nested_records_changed_for_autosave?
     end
 
+    # Returns true if this record has triggered saving of other records through
+    # autosave associations, and will remain true for the length of the cycle
+    # until all autosaves have completed.
+    #
+    # This is necessary so that saves triggered on associated records know not
+    # to re-save the parent if the autosave is bidirectional.
+    def _triggering_record? # :nodoc:
+      defined?(@_triggering_record) ? @_triggering_record : false
+    end
+
     private
 
       # Returns the record for an association collection that should be validated
@@ -371,6 +381,15 @@ def before_save_collection_association
         true
       end
 
+      # Temporarily mark this record as a triggering_record for the duration of
+      # the block call
+      def with_self_as_triggering_record
+        @_triggering_record = true
+        yield
+      ensure
+        @_triggering_record = false
+      end
+
       # Saves any new associated records, or all loaded autosave associations if
       # <tt>:autosave</tt> is enabled on the association.
       #
@@ -391,18 +410,20 @@ def save_collection_association(reflection)
             end
 
             records.each do |record|
-              next if record.destroyed?
+              next if record.destroyed? || record._triggering_record?
 
               saved = true
 
-              if autosave != false && (@new_record_before_save || record.new_record?)
-                if autosave
-                  saved = association.insert_record(record, false)
-                else
-                  association.insert_record(record) unless reflection.nested?
+              with_self_as_triggering_record do
+                if autosave != false && (@new_record_before_save || record.new_record?)
+                  if autosave
+                    saved = association.insert_record(record, false)
+                  else
+                    association.insert_record(record) unless reflection.nested?
+                  end
+                elsif autosave
+                  saved = record.save(validate: false)
                 end
-              elsif autosave
-                saved = record.save(:validate => false)
               end
 
               raise ActiveRecord::Rollback unless saved
@@ -426,7 +447,7 @@ def save_has_one_association(reflection)
         association = association_instance_get(reflection.name)
         record      = association && association.load_target
 
-        if record && !record.destroyed?
+        if record && !record.destroyed? && !record._triggering_record?
           autosave = reflection.options[:autosave]
 
           if autosave && record.marked_for_destruction?
@@ -439,7 +460,10 @@ def save_has_one_association(reflection)
                 record[reflection.foreign_key] = key
               end
 
-              saved = record.save(:validate => !autosave)
+              saved = with_self_as_triggering_record do
+                record.save(validate: !autosave)
+              end
+
               raise ActiveRecord::Rollback if !saved && autosave
               saved
             end
@@ -462,14 +486,19 @@ def save_belongs_to_association(reflection)
         return unless association && association.loaded? && !association.stale_target?
 
         record = association.load_target
-        if record && !record.destroyed?
+        record      = association && association.load_target
+        if record && !record.destroyed? && !record._triggering_record?
           autosave = reflection.options[:autosave]
 
           if autosave && record.marked_for_destruction?
             self[reflection.foreign_key] = nil
             record.destroy
           elsif autosave != false
-            saved = record.save(:validate => !autosave) if record.new_record? || (autosave && record.changed_for_autosave?)
+            if record.new_record? || (autosave && record.changed_for_autosave?)
+              saved = with_self_as_triggering_record do
+                record.save(validate: !autosave)
+              end
+            end
 
             if association.updated?
               association_id = record.send(reflection.options[:primary_key] || :id)

activerecord/lib/active_record/reflection.rb

@@ -281,7 +281,6 @@ def initialize(name, scope, options, active_record)
       end
 
       def autosave=(autosave)
-        @automatic_inverse_of = false
         @options[:autosave] = autosave
         parent_reflection = self.parent_reflection
         if parent_reflection

activerecord/test/cases/associations/has_one_associations_test.rb

@@ -601,7 +601,8 @@ def test_has_one_assignment_triggers_save_on_change_on_replacing_object
 
     new_ship = Ship.create(name: 'new name')
     assert_queries(2) do
-      # One query for updating name and second query for updating pirate_id
+      # One query for nullifying pirate_id on the previous ship and second query
+      # for updating pirate_id on new_ship
       pirate.ship = new_ship
     end
 

activerecord/test/cases/autosave_association_test.rb

@@ -1369,15 +1369,15 @@ def test_should_automatically_save_the_associated_models
     @pirate.send(@association_name).each_with_index { |child, i| child.name = new_names[i] }
 
     @pirate.save
-    assert_equal new_names, @pirate.reload.send(@association_name).map(&:name)
+    assert_equal new_names.to_set, @pirate.reload.send(@association_name).map(&:name).to_set
   end
 
   def test_should_automatically_save_bang_the_associated_models
     new_names = ['Grace OMalley', 'Privateers Greed']
     @pirate.send(@association_name).each_with_index { |child, i| child.name = new_names[i] }
 
     @pirate.save!
-    assert_equal new_names, @pirate.reload.send(@association_name).map(&:name)
+    assert_equal new_names.to_set, @pirate.reload.send(@association_name).map(&:name).to_set
   end
 
   def test_should_update_children_when_autosave_is_true_and_parent_is_new_but_child_is_not
@@ -1698,3 +1698,45 @@ def test_autosave_with_touch_should_not_raise_system_stack_error
     assert_nothing_raised { invoice.line_items.create(:amount => 10) }
   end
 end
+
+class TestBidirectionalAutosave < ActiveRecord::TestCase
+  def test_should_not_create_and_update_parent_for_has_many
+    ship = Ship.new(name: "The Black Rock")
+    ship.parts.build(name: "Stern")
+
+    assert_queries 2 do
+      ship.save!
+    end
+  end
+
+  def test_should_not_update_parent_twice_for_has_many
+    ship = Ship.create!(name: "The Black Rock")
+    ship.parts.create!(name: "Stern")
+
+    assert_queries(1) { ship.name = 'The Clone Rock'; ship.save! }
+  end
+
+  def test_should_not_update_parent_twice_for_has_one
+    pirate = Pirate.create!(catchphrase: 'Yarrr!')
+    pirate.create_ship(name: 'The Black Rock')
+
+    assert_queries(1) { pirate.catchphrase = 'Arggggh?'; pirate.save! }
+  end
+
+  def test_should_not_update_parent_twice_for_belongs_to
+    ship = Ship.create!(name: "The Black Rock")
+    ship.create_pirate(catchphrase: 'Yarrr!')
+
+    assert_queries(1) { ship.name = 'Pearl'; ship.save! }
+  end
+
+  def test_should_not_update_parent_twice_when_using_nested_attributes
+    ship = Ship.new(name: 'The Black Rock')
+
+    assert_queries 2 do
+      # One for saving the ship
+      # One for saving the part
+      ship.update!(parts_attributes: [{ name: 'Stern' }])
+    end
+  end
+end

activerecord/test/cases/nested_attributes_test.rb

@@ -1096,3 +1096,25 @@ def setup
     assert_equal ["Ship name can't be blank"], part.errors.full_messages
   end
 end
+
+class TestModelsInstantiatedViaNestedAttributesHaveReflectionOnParent < ActiveRecord::TestCase
+  setup do
+    @pirate = Pirate.new(catchphrase: 'Yarrr!')
+    @ship = Ship.new(name: "The Black Rock")
+  end
+
+  def test_accepts_nested_attributes_and_has_one_work_with_validate_presence_of
+    assert @pirate.update(ship_attributes: { name: "Pearl", validate_pirate: true }),
+           "expected pirate to be saved successfully, but it wasn't"
+  end
+
+  def test_accepts_nested_attributes_and_has_many_work_with_validate_presence_of
+    assert @pirate.update(unscoped_birds_attributes: [{ name: "Polly", validate_pirate: true }]),
+           "expected pirate to be saved successfully, but it wasn't"
+  end
+
+  def test_accepts_nested_attributes_and_belongs_to_work_with_validate_presence_of
+    assert @ship.update(pirate_attributes: { catchphrase: 'Yarrr!', validate_ship: true }),
+           "expected ship to be saved successfully, but it wasn't"
+  end
+end

activerecord/test/models/bird.rb

@@ -4,6 +4,9 @@ class Bird < ActiveRecord::Base
 
   accepts_nested_attributes_for :pirate
 
+  attr_accessor :validate_pirate
+  validates :pirate, presence: true, if: :validate_pirate
+
   attr_accessor :cancel_save_from_callback
   before_save :cancel_save_callback_method, :if => :cancel_save_from_callback
   def cancel_save_callback_method

activerecord/test/models/pirate.rb

@@ -21,6 +21,7 @@ class Pirate < ActiveRecord::Base
   has_one :ship
   has_one :update_only_ship, :class_name => 'Ship'
   has_one :non_validated_ship, :class_name => 'Ship'
+  has_many :unscoped_birds, class_name: 'Bird'
   has_many :birds, -> { order('birds.id ASC') }
   has_many :birds_with_method_callbacks, :class_name => "Bird",
     :before_add    => :log_before_add,
@@ -37,6 +38,7 @@ class Pirate < ActiveRecord::Base
   has_one :foo_bulb, -> { where :name => 'foo' }, :foreign_key => :car_id, :class_name => "Bulb"
 
   accepts_nested_attributes_for :parrots, :birds, :allow_destroy => true, :reject_if => proc(&:empty?)
+  accepts_nested_attributes_for :unscoped_birds
   accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc(&:empty?)
   accepts_nested_attributes_for :update_only_ship, :update_only => true
   accepts_nested_attributes_for :parrots_with_method_callbacks, :parrots_with_proc_callbacks,
@@ -45,6 +47,9 @@ class Pirate < ActiveRecord::Base
 
   validates_presence_of :catchphrase
 
+  attr_accessor :validate_ship
+  validates :ship, presence: true, if: :validate_ship
+
   def ship_log
     @ship_log ||= []
   end

activerecord/test/models/ship.rb

@@ -13,6 +13,9 @@ class Ship < ActiveRecord::Base
 
   validates_presence_of :name
 
+  attr_accessor :validate_pirate
+  validates :pirate, presence: true, if: :validate_pirate
+
   attr_accessor :cancel_save_from_callback
   before_save :cancel_save_callback_method, :if => :cancel_save_from_callback
   def cancel_save_callback_method