diff options
15 files changed, 113 insertions, 85 deletions
diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index b3d5a29b16..35fd1395f7 100644 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -1467,16 +1467,17 @@ module ActiveRecord 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 @@ module ActiveRecord 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) diff --git a/activerecord/lib/active_record/associations/association_proxy.rb b/activerecord/lib/active_record/associations/association_proxy.rb index 7e68241a2c..65fef81d64 100644 --- a/activerecord/lib/active_record/associations/association_proxy.rb +++ b/activerecord/lib/active_record/associations/association_proxy.rb @@ -128,17 +128,18 @@ module ActiveRecord # 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 @@ module ActiveRecord @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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/belongs_to_association.rb b/activerecord/lib/active_record/associations/belongs_to_association.rb index 63eb38ab34..c54e397ae4 100644 --- a/activerecord/lib/active_record/associations/belongs_to_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_association.rb @@ -29,17 +29,6 @@ module ActiveRecord @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 @@ module ActiveRecord @owner[@reflection.foreign_key] end end + + def stale_state + @owner[@reflection.foreign_key].to_s + end end end end diff --git a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb index 4608ffad67..4f67b02d00 100644 --- a/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb +++ b/activerecord/lib/active_record/associations/belongs_to_polymorphic_association.rb @@ -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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/associations/has_many_through_association.rb b/activerecord/lib/active_record/associations/has_many_through_association.rb index 2348ee099c..d432e8486d 100644 --- a/activerecord/lib/active_record/associations/has_many_through_association.rb +++ b/activerecord/lib/active_record/associations/has_many_through_association.rb @@ -62,7 +62,6 @@ module ActiveRecord def find_target return [] unless target_reflection_has_associated_record? - update_stale_state scoped.all end diff --git a/activerecord/lib/active_record/associations/has_one_association.rb b/activerecord/lib/active_record/associations/has_one_association.rb index 12ccb3af8a..7bbeb8829a 100644 --- a/activerecord/lib/active_record/associations/has_one_association.rb +++ b/activerecord/lib/active_record/associations/has_one_association.rb @@ -33,10 +33,8 @@ module ActiveRecord 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 @@ module ActiveRecord end set_inverse_instance(obj) - @loaded = true + loaded unless !@owner.persisted? || obj.nil? || dont_save return (obj.save ? self : false) diff --git a/activerecord/lib/active_record/associations/has_one_through_association.rb b/activerecord/lib/active_record/associations/has_one_through_association.rb index c9ae930e93..11fa40a5c4 100644 --- a/activerecord/lib/active_record/associations/has_one_through_association.rb +++ b/activerecord/lib/active_record/associations/has_one_through_association.rb @@ -7,6 +7,7 @@ module ActiveRecord def replace(new_value) create_through_record(new_value) @target = new_value + loaded end private @@ -32,7 +33,6 @@ module ActiveRecord end def find_target - update_stale_state scoped.first end end diff --git a/activerecord/lib/active_record/associations/through_association.rb b/activerecord/lib/active_record/associations/through_association.rb index 6536fb44b2..0d83b46130 100644 --- a/activerecord/lib/active_record/associations/through_association.rb +++ b/activerecord/lib/active_record/associations/through_association.rb @@ -10,17 +10,6 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index c6c1dd8b87..70ed16eeaf 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -368,6 +368,7 @@ module ActiveRecord if association.updated? association_id = association.send(reflection.options[:primary_key] || :id) self[reflection.foreign_key] = association_id + association.loaded end saved if autosave diff --git a/activerecord/test/cases/associations/belongs_to_associations_test.rb b/activerecord/test/cases/associations/belongs_to_associations_test.rb index f697fdf628..38ee4ad4e0 100644 --- a/activerecord/test/cases/associations/belongs_to_associations_test.rb +++ b/activerecord/test/cases/associations/belongs_to_associations_test.rb @@ -88,19 +88,6 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase 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 @@ class BelongsToAssociationsTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/associations/eager_test.rb b/activerecord/test/cases/associations/eager_test.rb index 19c69ed7bc..2a9351507a 100644 --- a/activerecord/test/cases/associations/eager_test.rb +++ b/activerecord/test/cases/associations/eager_test.rb @@ -902,11 +902,25 @@ class EagerAssociationTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/associations/has_one_through_associations_test.rb b/activerecord/test/cases/associations/has_one_through_associations_test.rb index 7d55d909a7..0afbef5c87 100644 --- a/activerecord/test/cases/associations/has_one_through_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_through_associations_test.rb @@ -266,4 +266,26 @@ class HasOneThroughAssociationsTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 27aee400f9..71fd3fd836 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -340,6 +340,13 @@ class TestDefaultAutosaveAssociationOnABelongsToAssociation < ActiveRecord::Test 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 diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index 7d9b1104cd..d290afc1dd 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -411,7 +411,6 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase 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 diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 7af4dfe2c9..d08e593db1 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -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 |