Allow assignment on has_one :through where the owner is a new record [#5137 state:resolved]

This required changing the code to keep the association proxy for a belongs_to around, despite its target being nil. Which in turn required various changes to the way that stale target checking is handled, in order to support various edge cases (loaded target is nil then foreign key added, foreign key is changed and then changed back, etc). A side effect is that the code is nicer and more succinct.

Note that I am removing test_no_unexpected_aliasing since that is basically checking that the proxy for a belongs_to *does* change, which is the exact opposite of the intention of this commit. Also adding various tests for various edge cases and related things.

Phew, long commit message!

15 files changed, 113 insertions, 85 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index b3d5a29b16..35fd1395f7 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1467,16 +1467,17 @@ module ActiveRecord
             force_reload = params.first unless params.empty?
             association = association_instance_get(reflection.name)
 
-            if association.nil? || force_reload || association.stale_target?
+            if association.nil?
               association = association_proxy_class.new(self, reflection)
-              retval = force_reload ? reflection.klass.uncached { association.reload } : association.reload
-              if retval.nil? and association_proxy_class == BelongsToAssociation
-                association_instance_set(reflection.name, nil)
-                return nil
-              end
               association_instance_set(reflection.name, association)
             end
 
+            if force_reload
+              reflection.klass.uncached { association.reload }
+            elsif !association.loaded? || association.stale_target?
+              association.reload
+            end
+
             association.target.nil? ? nil : association
           end
 
@@ -1485,19 +1486,19 @@ module ActiveRecord
             association && association.loaded?
           end
 
-          redefine_method("#{reflection.name}=") do |new_value|
+          redefine_method("#{reflection.name}=") do |record|
             association = association_instance_get(reflection.name)
 
-            if association.nil? || association.target != new_value
+            if association.nil?
               association = association_proxy_class.new(self, reflection)
+              association_instance_set(reflection.name, association)
             end
 
-            association.replace(new_value)
-            association_instance_set(reflection.name, new_value.nil? ? nil : association)
+            association.replace(record)
+            association.target.nil? ? nil : association
           end
 
           redefine_method("set_#{reflection.name}_target") do |target|
-            return if target.nil? and association_proxy_class == BelongsToAssociation
             association = association_proxy_class.new(self, reflection)
             association.target = target
             association_instance_set(reflection.name, association)
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index 7e68241a2c..65fef81d64 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -128,17 +128,18 @@ module ActiveRecord
 
       # Asserts the \target has been loaded setting the \loaded flag to +true+.
       def loaded
-        @loaded = true
+        @loaded      = true
+        @stale_state = stale_state
       end
 
       # The target is stale if the target no longer points to the record(s) that the
       # relevant foreign_key(s) refers to. If stale, the association accessor method
-      # on the owner will reload the target. It's up to subclasses to implement this
-      # method if relevant.
+      # on the owner will reload the target. It's up to subclasses to implement the
+      # state_state method if relevant.
       #
       # Note that if the target has not been loaded, it is not considered stale.
       def stale_target?
-        false
+        loaded? && @stale_state != stale_state
       end
 
       # Returns the target of this proxy, same as +proxy_target+.
@@ -273,16 +274,19 @@ module ActiveRecord
             @target = find_target
           end
 
-          @loaded = true
+          loaded
           @target
         rescue ActiveRecord::RecordNotFound
           reset
         end
 
-        # Can be overwritten by associations that might have the foreign key
-        # available for an association without having the object itself (and
-        # still being a new record). Currently, only +belongs_to+ presents
-        # this scenario (both vanilla and polymorphic).
+        # Should be true if there is a foreign key present on the @owner which
+        # references the target. This is used to determine whether we can load
+        # the target if the @owner is currently a new record (and therefore
+        # without a key).
+        #
+        # Currently implemented by belongs_to (vanilla and polymorphic) and
+        # has_one/has_many :through associations which go through a belongs_to
         def foreign_key_present
           false
         end
@@ -308,6 +312,15 @@ module ActiveRecord
         def invertible_for?(record)
           inverse_reflection_for(record)
         end
+
+        # This should be implemented to return the values of the relevant key(s) on the owner,
+        # so that when state_state is different from the value stored on the last find_target,
+        # the target is stale.
+        #
+        # This is only relevant to certain associations, which is why it returns nil by default.
+        def stale_state
+          nil
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index 63eb38ab34..c54e397ae4 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -29,17 +29,6 @@ module ActiveRecord
         @updated
       end
 
-      def stale_target?
-        if @target && @target.persisted?
-          target_id   = @target[@reflection.association_primary_key].to_s
-          foreign_key = @owner[@reflection.foreign_key].to_s
-
-          target_id != foreign_key
-        else
-          false
-        end
-      end
-
       private
         def update_counters(record)
           counter_cache_name = @reflection.counter_cache_column
@@ -106,6 +95,10 @@ module ActiveRecord
             @owner[@reflection.foreign_key]
           end
         end
+
+        def stale_state
+          @owner[@reflection.foreign_key].to_s
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
index 4608ffad67..4f67b02d00 100644
--- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
@@ -2,19 +2,6 @@ module ActiveRecord
   # = Active Record Belongs To Polymorphic Association
   module Associations
     class BelongsToPolymorphicAssociation < BelongsToAssociation #:nodoc:
-      def stale_target?
-        if @target && @target.persisted?
-          target_id    = @target.send(@reflection.association_primary_key).to_s
-          foreign_key  = @owner.send(@reflection.foreign_key).to_s
-          target_type  = @target.class.base_class.name
-          foreign_type = @owner.send(@reflection.foreign_type).to_s
-
-          target_id != foreign_key || target_type != foreign_type
-        else
-          false
-        end
-      end
-
       private
 
         def replace_keys(record)
@@ -38,6 +25,10 @@ module ActiveRecord
         def raise_on_type_mismatch(record)
           # A polymorphic association cannot have a type mismatch, by definition
         end
+
+        def stale_state
+          [super, @owner[@reflection.foreign_type].to_s]
+        end
     end
   end
 end
diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb
index 2348ee099c..d432e8486d 100644
--- a/activerecord/lib/active_record/associations/has_many_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_many_through_association.rb
@@ -62,7 +62,6 @@ module ActiveRecord
 
         def find_target
           return [] unless target_reflection_has_associated_record?
-          update_stale_state
           scoped.all
         end
 
diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb
index 12ccb3af8a..7bbeb8829a 100644
--- a/activerecord/lib/active_record/associations/has_one_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_association.rb
@@ -33,10 +33,8 @@ module ActiveRecord
             case @reflection.options[:dependent]
             when :delete
               @target.delete if @target.persisted?
-              @owner.clear_association_cache
             when :destroy
               @target.destroy if @target.persisted?
-              @owner.clear_association_cache
             when :nullify
               @target[@reflection.foreign_key] = nil
               @target.save if @owner.persisted? && @target.persisted?
@@ -56,7 +54,7 @@ module ActiveRecord
         end
 
         set_inverse_instance(obj)
-        @loaded = true
+        loaded
 
         unless !@owner.persisted? || obj.nil? || dont_save
           return (obj.save ? self : false)
diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb
index c9ae930e93..11fa40a5c4 100644
--- a/activerecord/lib/active_record/associations/has_one_through_association.rb
+++ b/activerecord/lib/active_record/associations/has_one_through_association.rb
@@ -7,6 +7,7 @@ module ActiveRecord
       def replace(new_value)
         create_through_record(new_value)
         @target = new_value
+        loaded
       end
 
       private
@@ -32,7 +33,6 @@ module ActiveRecord
       end
 
       def find_target
-        update_stale_state
         scoped.first
       end
     end
diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb
index 6536fb44b2..0d83b46130 100644
--- a/activerecord/lib/active_record/associations/through_association.rb
+++ b/activerecord/lib/active_record/associations/through_association.rb
@@ -10,17 +10,6 @@ module ActiveRecord
         end
       end
 
-      def stale_target?
-        if @target && @reflection.through_reflection.macro == :belongs_to && defined?(@through_foreign_key)
-          previous_key = @through_foreign_key.to_s
-          current_key  = @owner.send(@reflection.through_reflection.foreign_key).to_s
-
-          previous_key != current_key
-        else
-          false
-        end
-      end
-
       protected
 
       def construct_find_scope
@@ -157,11 +146,16 @@ module ActiveRecord
 
       alias_method :sql_conditions, :conditions
 
-      def update_stale_state
+      def stale_state
         if @reflection.through_reflection.macro == :belongs_to
-          @through_foreign_key = @owner.send(@reflection.through_reflection.foreign_key)
+          @owner[@reflection.through_reflection.foreign_key].to_s
         end
       end
+
+      def foreign_key_present
+        @reflection.through_reflection.macro == :belongs_to &&
+        !@owner[@reflection.through_reflection.foreign_key].nil?
+      end
     end
   end
 end
diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index c6c1dd8b87..70ed16eeaf 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -368,6 +368,7 @@ module ActiveRecord
           if association.updated?
             association_id = association.send(reflection.options[:primary_key] || :id)
             self[reflection.foreign_key] = association_id
+            association.loaded
           end
 
           saved if autosave
diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb
index f697fdf628..38ee4ad4e0 100644
--- a/activerecord/test/cases/associations/belongs_to_associations_test.rb
+++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb
@@ -88,19 +88,6 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     assert_not_nil citibank_result.instance_variable_get("@firm_with_primary_key_symbols")
   end
 
-  def test_no_unexpected_aliasing
-    first_firm = companies(:first_firm)
-    another_firm = companies(:another_firm)
-
-    citibank = Account.create("credit_limit" => 10)
-    citibank.firm = first_firm
-    original_proxy = citibank.firm
-    citibank.firm = another_firm
-
-    assert_equal first_firm.object_id, original_proxy.target.object_id
-    assert_equal another_firm.object_id, citibank.firm.target.object_id
-  end
-
   def test_creating_the_belonging_object
     citibank = Account.create("credit_limit" => 10)
     apple    = citibank.create_firm("name" => "Apple")
@@ -318,13 +305,21 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase
     assert_equal Firm.find(:first, :order => "id"), c.firm_with_basic_id
   end
 
-  def test_forgetting_the_load_when_foreign_key_enters_late
-    c = Client.new
-    assert_nil c.firm_with_basic_id
+  def test_setting_foreign_key_after_nil_target_loaded
+    client = Client.new
+    client.firm_with_basic_id
+    client.firm_id = 1
 
-    c.firm_id = 1
-    # sometimes tests on Oracle fail if ORDER BY is not provided therefore add always :order with :first
-    assert_equal Firm.find(:first, :order => "id"), c.firm_with_basic_id
+    assert_equal companies(:first_firm), client.firm_with_basic_id
+  end
+
+  def test_polymorphic_setting_foreign_key_after_nil_target_loaded
+    sponsor = Sponsor.new
+    sponsor.sponsorable
+    sponsor.sponsorable_id = 1
+    sponsor.sponsorable_type = "Member"
+
+    assert_equal members(:groucho), sponsor.sponsorable
   end
 
   def test_field_name_same_as_foreign_key
diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb
index 19c69ed7bc..2a9351507a 100644
--- a/activerecord/test/cases/associations/eager_test.rb
+++ b/activerecord/test/cases/associations/eager_test.rb
@@ -902,11 +902,25 @@ class EagerAssociationTest < ActiveRecord::TestCase
     end
   end
 
-  def test_preloading_empty_polymorphic_parent
+  def test_preloading_empty_belongs_to
+    c = Client.create!(:name => 'Foo', :client_of => Company.maximum(:id) + 1)
+
+    client = assert_queries(2) { Client.preload(:firm).find(c.id) }
+    assert_no_queries { assert_nil client.firm }
+  end
+
+  def test_preloading_empty_belongs_to_polymorphic
     t = Tagging.create!(:taggable_type => 'Post', :taggable_id => Post.maximum(:id) + 1, :tag => tags(:general))
 
-    assert_queries(2) { @tagging = Tagging.preload(:taggable).find(t.id) }
-    assert_no_queries { assert ! @tagging.taggable }
+    tagging = assert_queries(2) { Tagging.preload(:taggable).find(t.id) }
+    assert_no_queries { assert_nil tagging.taggable }
+  end
+
+  def test_preloading_through_empty_belongs_to
+    c = Client.create!(:name => 'Foo', :client_of => Company.maximum(:id) + 1)
+
+    client = assert_queries(2) { Client.preload(:accounts).find(c.id) }
+    assert_no_queries { assert client.accounts.empty? }
   end
 
   def test_preloading_has_many_through_with_uniq
diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb
index 7d55d909a7..0afbef5c87 100644
--- a/activerecord/test/cases/associations/has_one_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb
@@ -266,4 +266,26 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase
     assert proxy.stale_target?
     assert_equal dashboards(:second), minivan.dashboard
   end
+
+  def test_has_one_through_belongs_to_setting_belongs_to_foreign_key_after_nil_target_loaded
+    minivan = Minivan.new
+
+    minivan.dashboard
+    proxy = minivan.send(:association_instance_get, :dashboard)
+
+    minivan.speedometer_id = speedometers(:second).id
+
+    assert proxy.stale_target?
+    assert_equal dashboards(:second), minivan.dashboard
+  end
+
+  def test_assigning_has_one_through_belongs_to_with_new_record_owner
+    minivan   = Minivan.new
+    dashboard = dashboards(:cool_first)
+
+    minivan.dashboard = dashboard
+
+    assert_equal dashboard, minivan.dashboard
+    assert_equal dashboard, minivan.speedometer.dashboard
+  end
 end
diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb
index 27aee400f9..71fd3fd836 100644
--- a/activerecord/test/cases/autosave_association_test.rb
+++ b/activerecord/test/cases/autosave_association_test.rb
@@ -340,6 +340,13 @@ class TestDefaultAutosaveAssociationOnABelongsToAssociation < ActiveRecord::Test
     tags(:misc).create_tagging(:taggable => posts(:thinking))
     assert_equal num_tagging + 1, Tagging.count
   end
+
+  def test_build_and_then_save_parent_should_not_reload_target
+    client = Client.find(:first)
+    apple = client.build_firm(:name => "Apple")
+    client.save!
+    assert_no_queries { assert_equal apple, client.firm }
+  end
 end
 
 class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCase
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index 7d9b1104cd..d290afc1dd 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -411,7 +411,6 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
 
   def test_should_modify_an_existing_record_if_there_is_a_matching_composite_id
     @pirate.stubs(:id).returns('ABC1X')
-    @ship.stubs(:pirate_id).returns(@pirate.id) # prevents pirate from being reloaded due to non-matching foreign key
     @ship.pirate_attributes = { :id => @pirate.id, :catchphrase => 'Arr' }
 
     assert_equal 'Arr', @ship.pirate.catchphrase
diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb
index 7af4dfe2c9..d08e593db1 100644
--- a/activerecord/test/models/company.rb
+++ b/activerecord/test/models/company.rb
@@ -124,6 +124,7 @@ class Client < Company
   belongs_to :firm_with_primary_key, :class_name => "Firm", :primary_key => "name", :foreign_key => "firm_name"
   belongs_to :firm_with_primary_key_symbols, :class_name => "Firm", :primary_key => :name, :foreign_key => :firm_name
   belongs_to :readonly_firm, :class_name => "Firm", :foreign_key => "firm_id", :readonly => true
+  has_many :accounts, :through => :firm
 
   # Record destruction so we can test whether firm.clients.clear has
   # is calling client.destroy, deleting from the database, or setting