Browse files

Remove undocumented feature from has_one where you could pass false a…

…s the second parameter to build_assoc or create_assoc, and the existing associated object would be untouched (the foreign key would not be nullified, and it would not be deleted). If you want behaviour similar to this you can do the following things:

* Use :dependent => :nullify (or don't specify :dependent) if you want to prevent the existing associated object from being deleted
* Use has_many if you actually want multiple associated objects
* Explicitly set the foreign key if, for some reason, you really need to have multiple objects associated with the same has_one. E.g.

    previous = obj.assoc
    obj.create_assoc
    previous.update_attributes(:obj_id => obj.id)
  • Loading branch information...
1 parent 2120da7 commit 40afcade0dc1450e765a91fc15a6ac6d442c9826 @jonleighton jonleighton committed with tenderlove Jan 3, 2011
View
11 activerecord/lib/active_record/associations.rb
@@ -1556,20 +1556,15 @@ def collection_accessor_methods(reflection, association_proxy_class, writer = tr
def association_constructor_method(constructor, reflection, association_proxy_class)
redefine_method("#{constructor}_#{reflection.name}") do |*params|
- attributees = params.first unless params.empty?
- replace_existing = params[1].nil? ? true : params[1]
- association = association_instance_get(reflection.name)
+ attributes = params.first unless params.empty?
+ association = association_instance_get(reflection.name)
unless association
association = association_proxy_class.new(self, reflection)
association_instance_set(reflection.name, association)
end
- if association_proxy_class == HasOneAssociation
- association.send(constructor, attributees, replace_existing)
- else
- association.send(constructor, attributees)
- end
+ association.send(constructor, attributes)
end
end
View
28 activerecord/lib/active_record/associations/has_one_association.rb
@@ -4,22 +4,22 @@ module Associations
class HasOneAssociation < AssociationProxy #:nodoc:
include HasAssociation
- def create(attrs = {}, replace_existing = true)
- new_record(replace_existing) do |reflection|
+ def create(attrs = {})
+ new_record do |reflection|
attrs = merge_with_conditions(attrs)
reflection.create_association(attrs)
end
end
- def create!(attrs = {}, replace_existing = true)
- new_record(replace_existing) do |reflection|
+ def create!(attrs = {})
+ new_record do |reflection|
attrs = merge_with_conditions(attrs)
reflection.create_association!(attrs)
end
end
- def build(attrs = {}, replace_existing = true)
- new_record(replace_existing) do |reflection|
+ def build(attrs = {})
+ new_record do |reflection|
attrs = merge_with_conditions(attrs)
reflection.build_association(attrs)
end
@@ -82,21 +82,9 @@ def creation_attributes
construct_owner_attributes
end
- def new_record(replace_existing)
- # Make sure we load the target first, if we plan on replacing the existing
- # instance. Otherwise, if the target has not previously been loaded
- # elsewhere, the instance we create will get orphaned.
- load_target if replace_existing
+ def new_record
record = scoped.scoping { yield @reflection }
-
- if replace_existing
- replace(record, true)
- else
- record[@reflection.foreign_key] = @owner.id if @owner.persisted?
- self.target = record
- set_inverse_instance(record)
- end
-
+ replace(record, true)
record
end
View
29 activerecord/test/cases/associations/has_one_associations_test.rb
@@ -116,35 +116,6 @@ def test_natural_assignment_to_already_associated_record
assert_equal company.account, account
end
- def test_assignment_without_replacement
- apple = Firm.create("name" => "Apple")
- citibank = Account.create("credit_limit" => 10)
- apple.account = citibank
- assert_equal apple.id, citibank.firm_id
-
- hsbc = apple.build_account({ :credit_limit => 20}, false)
- assert_equal apple.id, hsbc.firm_id
- hsbc.save
- assert_equal apple.id, citibank.firm_id
-
- nykredit = apple.create_account({ :credit_limit => 30}, false)
- assert_equal apple.id, nykredit.firm_id
- assert_equal apple.id, citibank.firm_id
- assert_equal apple.id, hsbc.firm_id
- end
-
- def test_assignment_without_replacement_on_create
- apple = Firm.create("name" => "Apple")
- citibank = Account.create("credit_limit" => 10)
- apple.account = citibank
- assert_equal apple.id, citibank.firm_id
-
- hsbc = apple.create_account({:credit_limit => 10}, false)
- assert_equal apple.id, hsbc.firm_id
- hsbc.save
- assert_equal apple.id, citibank.firm_id
- end
-
def test_dependence
num_accounts = Account.count
View
14 activerecord/test/cases/associations/inverse_associations_test.rb
@@ -146,9 +146,9 @@ def test_parent_instance_should_be_shared_with_newly_created_child_via_bang_meth
assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance"
end
- def test_parent_instance_should_be_shared_with_newly_built_child_when_we_dont_replace_existing
+ def test_parent_instance_should_be_shared_with_newly_built_child
m = Man.find(:first)
- f = m.build_face({:description => 'haunted'}, false)
+ f = m.build_face(:description => 'haunted')
assert_not_nil f.man
assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance"
m.name = 'Bongo'
@@ -157,9 +157,9 @@ def test_parent_instance_should_be_shared_with_newly_built_child_when_we_dont_re
assert_equal m.name, f.man.name, "Name of man should be the same after changes to just-built-child-owned instance"
end
- def test_parent_instance_should_be_shared_with_newly_created_child_when_we_dont_replace_existing
+ def test_parent_instance_should_be_shared_with_newly_created_child
m = Man.find(:first)
- f = m.create_face({:description => 'haunted'}, false)
+ f = m.create_face(:description => 'haunted')
assert_not_nil f.man
assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance"
m.name = 'Bongo'
@@ -168,9 +168,9 @@ def test_parent_instance_should_be_shared_with_newly_created_child_when_we_dont_
assert_equal m.name, f.man.name, "Name of man should be the same after changes to newly-created-child-owned instance"
end
- def test_parent_instance_should_be_shared_with_newly_created_child_via_bang_method_when_we_dont_replace_existing
+ def test_parent_instance_should_be_shared_with_newly_created_child_via_bang_method
m = Man.find(:first)
- f = m.face.create!({:description => 'haunted'}, false)
+ f = m.face.create!(:description => 'haunted')
assert_not_nil f.man
assert_equal m.name, f.man.name, "Name of man should be the same before changes to parent instance"
m.name = 'Bongo'
@@ -203,7 +203,7 @@ def test_parent_instance_should_be_shared_with_replaced_via_method_child
assert_equal m.name, f.man.name, "Name of man should be the same after changes to replaced-child-owned instance"
end
- def test_parent_instance_should_be_shared_with_replaced_via_method_child_when_we_dont_replace_existing
+ def test_parent_instance_should_be_shared_with_replaced_via_method_child
m = Man.find(:first)
f = Face.new(:description => 'haunted')
m.face.replace(f, false)

0 comments on commit 40afcad

Please sign in to comment.