Skip to content

Commit

Permalink
Ensure replacing has_one associations respects the supplied :dependen…
Browse files Browse the repository at this point in the history
…t option. [#1305 state:resolved]

Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
  • Loading branch information
labria authored and lifo committed Mar 6, 2009
1 parent 4863634 commit 984bc7a
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 4 deletions.
13 changes: 11 additions & 2 deletions activerecord/lib/active_record/associations/has_one_association.rb
Expand Up @@ -29,8 +29,17 @@ def replace(obj, dont_save = false)

unless @target.nil? || @target == obj
if dependent? && !dont_save
@target.destroy unless @target.new_record?
@owner.clear_association_cache
case @reflection.options[:dependent]
when :delete
@target.delete unless @target.new_record?
@owner.clear_association_cache
when :destroy
@target.destroy unless @target.new_record?
@owner.clear_association_cache

This comment has been minimized.

Copy link
@clarabstract

clarabstract May 21, 2009

Contributor

Just ran into an issue because of this - not sure how well thought out this was...

@owner.clear_association_cache will clear out all unsaved associations - which can lead to some really fun to track down mysteries if you happen to replace an association after you just built another association.

Unless there is some further, unstated reasoning behind the "screw it, let's just nuke everything"-approach, @owner.instance_variable_set "@#{@reflection.name}", nil unless @owner.new_record?, should be used instead.

when :nullify
@target[@reflection.primary_key_name] = nil
@target.save unless @owner.new_record? || @target.new_record?
end
else
@target[@reflection.primary_key_name] = nil
@target.save unless @owner.new_record? || @target.new_record?
Expand Down
18 changes: 18 additions & 0 deletions activerecord/test/cases/associations/has_one_associations_test.rb
Expand Up @@ -79,6 +79,24 @@ def test_natural_assignment_to_nil
assert_raises(ActiveRecord::RecordNotFound) { Account.find(old_account_id) }
end

def test_nullification_on_association_change
firm = companies(:rails_core)
old_account_id = firm.account.id
firm.account = Account.new
# account is dependent with nullify, therefore its firm_id should be nil
assert_nil Account.find(old_account_id).firm_id
end

def test_association_changecalls_delete
companies(:first_firm).deletable_account = Account.new
assert_equal [], Account.destroyed_account_ids[companies(:first_firm).id]
end

def test_association_change_calls_destroy
companies(:first_firm).account = Account.new
assert_equal [companies(:first_firm).id], Account.destroyed_account_ids[companies(:first_firm).id]
end

def test_natural_assignment_to_already_associated_record
company = companies(:first_firm)
account = accounts(:signals37)
Expand Down
5 changes: 3 additions & 2 deletions activerecord/test/cases/reflection_test.rb
Expand Up @@ -4,6 +4,7 @@
require 'models/company'
require 'models/company_in_module'
require 'models/subscriber'
require 'models/pirate'

class ReflectionTest < ActiveRecord::TestCase
fixtures :topics, :customers, :companies, :subscribers
Expand Down Expand Up @@ -169,9 +170,9 @@ def test_association_reflection_in_modules

def test_reflection_of_all_associations
# FIXME these assertions bust a lot
assert_equal 26, Firm.reflect_on_all_associations.size
assert_equal 27, Firm.reflect_on_all_associations.size
assert_equal 20, Firm.reflect_on_all_associations(:has_many).size
assert_equal 6, Firm.reflect_on_all_associations(:has_one).size
assert_equal 7, Firm.reflect_on_all_associations(:has_one).size
assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size
end

Expand Down
1 change: 1 addition & 0 deletions activerecord/test/models/company.rb
Expand Up @@ -69,6 +69,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 :deletable_account, :foreign_key => "firm_id", :class_name => "Account", :dependent => :delete
end

class DependentFirm < Company
Expand Down

0 comments on commit 984bc7a

Please sign in to comment.