From b2c3b65176c85b0d542376479705b14f727f73fa Mon Sep 17 00:00:00 2001 From: Graeme Porteous Date: Tue, 17 Mar 2009 11:34:39 +0000 Subject: [PATCH] Fix has_one with foreign_key and primary_key association bug which caused the associated object being lost when saving the owner --- .../lib/active_record/autosave_association.rb | 5 +++-- .../associations/has_one_associations_test.rb | 9 +++++++++ activerecord/test/cases/reflection_test.rb | 4 ++-- activerecord/test/fixtures/accounts.yml | 1 + activerecord/test/models/company.rb | 1 + activerecord/test/schema/schema.rb | 1 + 6 files changed, 17 insertions(+), 4 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 741aa2a..d10f699 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -311,10 +311,11 @@ module ActiveRecord # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. def save_has_one_association(reflection) if (association = association_instance_get(reflection.name)) && !association.target.nil? + primary_key = reflection.options[:primary_key] || :id if reflection.options[:autosave] && association.marked_for_destruction? association.destroy - elsif new_record? || association.new_record? || association[reflection.primary_key_name] != id || reflection.options[:autosave] - association[reflection.primary_key_name] = id + elsif new_record? || association.new_record? || association[reflection.primary_key_name] != send(primary_key) || reflection.options[:autosave] + association[reflection.primary_key_name] = send(primary_key) association.save(false) end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 1ddb3f4..aceda3f 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -36,6 +36,15 @@ class HasOneAssociationsTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index db64bbb..41b1770 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -170,9 +170,9 @@ class ReflectionTest < ActiveRecord::TestCase def test_reflection_of_all_associations # FIXME these assertions bust a lot - assert_equal 28, Firm.reflect_on_all_associations.size + assert_equal 29, Firm.reflect_on_all_associations.size assert_equal 21, Firm.reflect_on_all_associations(:has_many).size - assert_equal 7, Firm.reflect_on_all_associations(:has_one).size + assert_equal 8, Firm.reflect_on_all_associations(:has_one).size assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size end diff --git a/activerecord/test/fixtures/accounts.yml b/activerecord/test/fixtures/accounts.yml index b2d0191..3258304 100644 --- a/activerecord/test/fixtures/accounts.yml +++ b/activerecord/test/fixtures/accounts.yml @@ -2,6 +2,7 @@ signals37: id: 1 firm_id: 1 credit_limit: 50 + firm_name: 37signals unknown: id: 2 diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 02a775f..759c282 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -70,6 +70,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 end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index ea848a2..216f82d 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -23,6 +23,7 @@ ActiveRecord::Schema.define do # 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 -- 1.6.2.rc1