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!

6 files changed, 61 insertions, 23 deletions

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