Fix has_one with foreign_key and primary_key association bug which ca…

…used the associated object being lost when saving the owner. [#1756 state:resolved]

Mixed in a bit from patch by ransom-briggs. [#2813 state:resolved]

Signed-off-by: Eloy Duran <eloy.de.enige@gmail.com>

activerecord/lib/active_record/autosave_association.rb

@@ -316,9 +316,12 @@ def save_has_one_association(reflection)
 
         if autosave && association.marked_for_destruction?
           association.destroy
-        elsif autosave != false && (new_record? || association.new_record? || association[reflection.primary_key_name] != id || autosave)
-          association[reflection.primary_key_name] = id
-          association.save(!autosave)
+        else
+          key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id
+          if autosave != false && (new_record? || association.new_record? || association[reflection.primary_key_name] != key || autosave)
+            association[reflection.primary_key_name] = key
+            association.save(!autosave)
+          end
         end
       end
     end

activerecord/test/cases/associations/has_one_associations_test.rb

@@ -36,6 +36,15 @@ def test_finding_using_primary_key
     assert_equal accounts(:rails_core_account), firm.account_using_primary_key
   end
 
+  def test_update_with_foreign_and_primary_keys
+    firm = companies(:first_firm)
+    account = firm.account_using_foreign_and_primary_keys
+    assert_equal Account.find_by_firm_name(firm.name), account
+    firm.save
+    firm.reload
+    assert_equal account, firm.account_using_foreign_and_primary_keys
+  end
+
   def test_can_marshal_has_one_association_with_nil_target
     firm = Firm.new
     assert_nothing_raised do

activerecord/test/cases/reflection_test.rb

@@ -170,9 +170,9 @@ def test_association_reflection_in_modules
 
   def test_reflection_of_all_associations
     # FIXME these assertions bust a lot
-    assert_equal 33, Firm.reflect_on_all_associations.size
+    assert_equal 34, Firm.reflect_on_all_associations.size
     assert_equal 25, Firm.reflect_on_all_associations(:has_many).size
-    assert_equal 8, Firm.reflect_on_all_associations(:has_one).size
+    assert_equal 9, Firm.reflect_on_all_associations(:has_one).size
     assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
   end
 

activerecord/test/fixtures/accounts.yml

@@ -2,6 +2,7 @@ signals37:
   id: 1
   firm_id: 1
   credit_limit: 50
+  firm_name: 37signals
 
 unknown:
   id: 2

activerecord/test/models/company.rb

@@ -72,6 +72,7 @@ class Firm < Company
   has_one :account_with_select, :foreign_key => "firm_id", :select => "id, firm_id", :class_name=>'Account'
   has_one :readonly_account, :foreign_key => "firm_id", :class_name => "Account", :readonly => true
   has_one :account_using_primary_key, :primary_key => "firm_id", :class_name => "Account"
+  has_one :account_using_foreign_and_primary_keys, :foreign_key => "firm_name", :primary_key => "name", :class_name => "Account"
   has_one :deletable_account, :foreign_key => "firm_id", :class_name => "Account", :dependent => :delete
 
   has_one :unautosaved_account, :foreign_key => "firm_id", :class_name => 'Account', :autosave => false

activerecord/test/schema/schema.rb

@@ -23,6 +23,7 @@ def create_table(*args, &block)
   # unless the ordering matters.  In which case, define them below
   create_table :accounts, :force => true do |t|
     t.integer :firm_id
+    t.string  :firm_name
     t.integer :credit_limit
   end