Improved strategy for updating a belongs_to association when the foreign key changes. Rather than resetting each affected association when the foreign key changes, we should lazily check for 'staleness' (where fk does not match target id) when the association is accessed.

7 files changed, 37 insertions, 45 deletions

diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb
index e4dff69efe..c6c41a7a12 100644
--- a/activerecord/lib/active_record/associations.rb
+++ b/activerecord/lib/active_record/associations.rb
@@ -1229,14 +1229,12 @@ module ActiveRecord
 
         if reflection.options[:polymorphic]
           association_accessor_methods(reflection, BelongsToPolymorphicAssociation)
-          association_foreign_type_setter_method(reflection)
         else
           association_accessor_methods(reflection, BelongsToAssociation)
           association_constructor_method(:build,  reflection, BelongsToAssociation)
           association_constructor_method(:create, reflection, BelongsToAssociation)
         end
 
-        association_foreign_key_setter_method(reflection)
         add_counter_cache_callbacks(reflection)          if options[:counter_cache]
         add_touch_callbacks(reflection, options[:touch]) if options[:touch]
 
@@ -1456,7 +1454,7 @@ module ActiveRecord
             force_reload = params.first unless params.empty?
             association = association_instance_get(reflection.name)
 
-            if association.nil? || force_reload
+            if association.nil? || force_reload || association.stale_target?
               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
@@ -1556,45 +1554,6 @@ module ActiveRecord
           end
         end
 
-        def association_foreign_key_setter_method(reflection)
-          setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection|
-            if belongs_to_reflection.primary_key_name == reflection.primary_key_name
-              "association_instance_set(:#{belongs_to_reflection.name}, nil);"
-            end
-          end.compact.join
-
-          if method_defined?(:"#{reflection.primary_key_name}=")
-            undef_method :"#{reflection.primary_key_name}="
-          end
-
-          class_eval <<-FILE, __FILE__, __LINE__ + 1
-            def #{reflection.primary_key_name}=(new_id)
-              write_attribute :#{reflection.primary_key_name}, new_id
-              if #{reflection.primary_key_name}_changed?
-                #{ setters }
-              end
-            end
-          FILE
-        end
-
-        def association_foreign_type_setter_method(reflection)
-          setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection|
-            if belongs_to_reflection.options[:foreign_type] == reflection.options[:foreign_type]
-              "association_instance_set(:#{belongs_to_reflection.name}, nil);"
-            end
-          end.compact.join
-
-          field = reflection.options[:foreign_type]
-          class_eval <<-FILE, __FILE__, __LINE__ + 1
-            def #{field}=(new_id)
-              write_attribute :#{field}, new_id
-              if #{field}_changed?
-                #{ setters }
-              end
-            end
-          FILE
-        end
-
         def add_counter_cache_callbacks(reflection)
           cache_column = reflection.counter_cache_column
 
diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb
index 252ff7e7ea..f4eceeed8c 100644
--- a/activerecord/lib/active_record/associations/association_proxy.rb
+++ b/activerecord/lib/active_record/associations/association_proxy.rb
@@ -130,6 +130,16 @@ module ActiveRecord
         @loaded = true
       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.
+      #
+      # Note that if the target has not been loaded, it is not considered stale.
+      def stale_target?
+        false
+      end
+
       # Returns the target of this proxy, same as +proxy_target+.
       def target
         @target
diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb
index b438620c8f..c9abfe36e8 100644
--- a/activerecord/lib/active_record/associations/belongs_to_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_association.rb
@@ -42,6 +42,14 @@ module ActiveRecord
         @updated
       end
 
+      def stale_target?
+        if @target && @target.persisted?
+          @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i
+        else
+          false
+        end
+      end
+
       private
         def find_target
           find_method = if @reflection.options[:primary_key]
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 a0df860623..844ae94c3d 100644
--- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
+++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb
@@ -23,6 +23,15 @@ module ActiveRecord
         @updated
       end
 
+      def stale_target?
+        if @target && @target.persisted?
+          @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i ||
+          @target.class.base_class.name.to_s != @owner.send(@reflection.options[:foreign_type]).to_s
+        else
+          false
+        end
+      end
+
       private
 
         # NOTE - for now, we're only supporting inverse setting from belongs_to back onto
diff --git a/activerecord/lib/active_record/reflection.rb b/activerecord/lib/active_record/reflection.rb
index 7a7f7812df..87a842bc6b 100644
--- a/activerecord/lib/active_record/reflection.rb
+++ b/activerecord/lib/active_record/reflection.rb
@@ -209,7 +209,10 @@ module ActiveRecord
       end
 
       def association_primary_key
-        @association_primary_key ||= options[:primary_key] || klass.primary_key
+        @association_primary_key ||=
+          options[:primary_key] ||
+          !options[:polymorphic] && klass.primary_key ||
+          'id'
       end
 
       def active_record_primary_key
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index ffcc7a081a..7d9b1104cd 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -298,7 +298,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
 
   def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
     @ship.delete
-    
+
     @pirate.reload.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' })
 
     assert_not_nil @pirate.ship
@@ -411,6 +411,7 @@ 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
@@ -451,7 +452,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
 
   def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
     pirate = @ship.pirate
-    
+
     @ship.attributes = { :pirate_attributes => { :id => pirate.id, '_destroy' => true } }
     assert_nothing_raised(ActiveRecord::RecordNotFound) { Pirate.find(pirate.id) }
     @ship.save
diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb
index 1e205714a7..901c12b26c 100644
--- a/activerecord/test/cases/reflection_test.rb
+++ b/activerecord/test/cases/reflection_test.rb
@@ -7,6 +7,7 @@ require 'models/subscriber'
 require 'models/ship'
 require 'models/pirate'
 require 'models/price_estimate'
+require 'models/tagging'
 
 class ReflectionTest < ActiveRecord::TestCase
   include ActiveRecord::Reflection
@@ -194,6 +195,7 @@ class ReflectionTest < ActiveRecord::TestCase
   def test_association_primary_key
     assert_equal "id", Author.reflect_on_association(:posts).association_primary_key.to_s
     assert_equal "name", Author.reflect_on_association(:essay).association_primary_key.to_s
+    assert_equal "id", Tagging.reflect_on_association(:taggable).association_primary_key.to_s
   end
 
   def test_active_record_primary_key