Permalink
Browse files

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!
  • Loading branch information...
1 parent c47f802 commit a0be389d39b790e0625339251d2674b8250b16b1 @jonleighton jonleighton committed with tenderlove Jan 2, 2011
@@ -1467,16 +1467,17 @@ def association_accessor_methods(reflection, association_proxy_class)
force_reload = params.first unless params.empty?
association = association_instance_get(reflection.name)
- if association.nil? || force_reload || association.stale_target?
+ if association.nil?
association = association_proxy_class.new(self, reflection)
- retval = force_reload ? reflection.klass.uncached { association.reload } : association.reload
- if retval.nil? and association_proxy_class == BelongsToAssociation
- association_instance_set(reflection.name, nil)
- return nil
- end
association_instance_set(reflection.name, association)
end
+ if force_reload
+ reflection.klass.uncached { association.reload }
+ elsif !association.loaded? || association.stale_target?
+ association.reload
+ end
+
association.target.nil? ? nil : association
end
@@ -1485,19 +1486,19 @@ def association_accessor_methods(reflection, association_proxy_class)
association && association.loaded?
end
- redefine_method("#{reflection.name}=") do |new_value|
+ redefine_method("#{reflection.name}=") do |record|
association = association_instance_get(reflection.name)
- if association.nil? || association.target != new_value
+ if association.nil?
association = association_proxy_class.new(self, reflection)
+ association_instance_set(reflection.name, association)
end
- association.replace(new_value)
- association_instance_set(reflection.name, new_value.nil? ? nil : association)
+ association.replace(record)
+ association.target.nil? ? nil : association
end
redefine_method("set_#{reflection.name}_target") do |target|
- return if target.nil? and association_proxy_class == BelongsToAssociation
association = association_proxy_class.new(self, reflection)
association.target = target
association_instance_set(reflection.name, association)
@@ -128,17 +128,18 @@ def loaded?
# Asserts the \target has been loaded setting the \loaded flag to +true+.
def loaded
- @loaded = true
+ @loaded = true
+ @stale_state = stale_state
end
# The target is stale if the target no longer points to the record(s) that the
# relevant foreign_key(s) refers to. If stale, the association accessor method
- # on the owner will reload the target. It's up to subclasses to implement this
- # method if relevant.
+ # on the owner will reload the target. It's up to subclasses to implement the
+ # state_state method if relevant.
#
# Note that if the target has not been loaded, it is not considered stale.
def stale_target?
- false
+ loaded? && @stale_state != stale_state
end
# Returns the target of this proxy, same as +proxy_target+.
@@ -273,16 +274,19 @@ def load_target
@target = find_target
end
- @loaded = true
+ loaded
@target
rescue ActiveRecord::RecordNotFound
reset
end
- # Can be overwritten by associations that might have the foreign key
- # available for an association without having the object itself (and
- # still being a new record). Currently, only +belongs_to+ presents
- # this scenario (both vanilla and polymorphic).
+ # Should be true if there is a foreign key present on the @owner which
+ # references the target. This is used to determine whether we can load
+ # the target if the @owner is currently a new record (and therefore
+ # without a key).
+ #
+ # Currently implemented by belongs_to (vanilla and polymorphic) and
+ # has_one/has_many :through associations which go through a belongs_to
def foreign_key_present
false
end
@@ -308,6 +312,15 @@ def inverse_reflection_for(record)
def invertible_for?(record)
inverse_reflection_for(record)
end
+
+ # This should be implemented to return the values of the relevant key(s) on the owner,
+ # so that when state_state is different from the value stored on the last find_target,
+ # the target is stale.
+ #
+ # This is only relevant to certain associations, which is why it returns nil by default.
+ def stale_state
+ nil
+ end
end
end
end
@@ -29,17 +29,6 @@ def updated?
@updated
end
- def stale_target?
- if @target && @target.persisted?
- target_id = @target[@reflection.association_primary_key].to_s
- foreign_key = @owner[@reflection.foreign_key].to_s
-
- target_id != foreign_key
- else
- false
- end
- end
-
private
def update_counters(record)
counter_cache_name = @reflection.counter_cache_column
@@ -106,6 +95,10 @@ def target_id
@owner[@reflection.foreign_key]
end
end
+
+ def stale_state
+ @owner[@reflection.foreign_key].to_s
+ end
end
end
end
@@ -2,19 +2,6 @@ module ActiveRecord
# = Active Record Belongs To Polymorphic Association
module Associations
class BelongsToPolymorphicAssociation < BelongsToAssociation #:nodoc:
- def stale_target?
- if @target && @target.persisted?
- target_id = @target.send(@reflection.association_primary_key).to_s
- foreign_key = @owner.send(@reflection.foreign_key).to_s
- target_type = @target.class.base_class.name
- foreign_type = @owner.send(@reflection.foreign_type).to_s
-
- target_id != foreign_key || target_type != foreign_type
- else
- false
- end
- end
-
private
def replace_keys(record)
@@ -38,6 +25,10 @@ def target_klass
def raise_on_type_mismatch(record)
# A polymorphic association cannot have a type mismatch, by definition
end
+
+ def stale_state
+ [super, @owner[@reflection.foreign_type].to_s]
+ end
end
end
end
@@ -62,7 +62,6 @@ def delete_records(records)
def find_target
return [] unless target_reflection_has_associated_record?
- update_stale_state
scoped.all
end
@@ -33,10 +33,8 @@ def replace(obj, dont_save = false)
case @reflection.options[:dependent]
when :delete
@target.delete if @target.persisted?
- @owner.clear_association_cache
when :destroy
@target.destroy if @target.persisted?
- @owner.clear_association_cache
when :nullify
@target[@reflection.foreign_key] = nil
@target.save if @owner.persisted? && @target.persisted?
@@ -56,7 +54,7 @@ def replace(obj, dont_save = false)
end
set_inverse_instance(obj)
- @loaded = true
+ loaded
unless !@owner.persisted? || obj.nil? || dont_save
return (obj.save ? self : false)
@@ -7,6 +7,7 @@ class HasOneThroughAssociation < HasOneAssociation
def replace(new_value)
create_through_record(new_value)
@target = new_value
+ loaded
end
private
@@ -32,7 +33,6 @@ def create_through_record(new_value)
end
def find_target
- update_stale_state
scoped.first
end
end
@@ -10,17 +10,6 @@ def scoped
end
end
- def stale_target?
- if @target && @reflection.through_reflection.macro == :belongs_to && defined?(@through_foreign_key)
- previous_key = @through_foreign_key.to_s
- current_key = @owner.send(@reflection.through_reflection.foreign_key).to_s
-
- previous_key != current_key
- else
- false
- end
- end
-
protected
def construct_find_scope
@@ -157,11 +146,16 @@ def build_sti_condition
alias_method :sql_conditions, :conditions
- def update_stale_state
+ def stale_state
if @reflection.through_reflection.macro == :belongs_to
- @through_foreign_key = @owner.send(@reflection.through_reflection.foreign_key)
+ @owner[@reflection.through_reflection.foreign_key].to_s
end
end
+
+ def foreign_key_present
+ @reflection.through_reflection.macro == :belongs_to &&
+ !@owner[@reflection.through_reflection.foreign_key].nil?
+ end
end
end
end
@@ -368,6 +368,7 @@ def save_belongs_to_association(reflection)
if association.updated?
association_id = association.send(reflection.options[:primary_key] || :id)
self[reflection.foreign_key] = association_id
+ association.loaded
end
saved if autosave
@@ -88,19 +88,6 @@ def test_eager_loading_with_primary_key_as_symbol
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 @@ def test_new_record_with_foreign_key_but_no_object
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
@@ -902,11 +902,25 @@ def test_include_has_one_using_primary_key
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
@@ -266,4 +266,26 @@ def test_has_one_through_belongs_to_should_update_when_the_through_foreign_key_c
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
@@ -340,6 +340,13 @@ def test_store_association_with_a_polymorphic_relationship
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
@@ -411,7 +411,6 @@ def test_should_take_a_hash_with_string_keys_and_update_the_associated_model
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
@@ -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

0 comments on commit a0be389

Please sign in to comment.