Permalink
Browse files

Improved strategy for updating a belongs_to association when the fore…

…ign key changes. Rather than resetting each affected association when the foreign key changes, we should lazily check for 'staleness' (where fk does not match target id) when the association is accessed.
  • Loading branch information...
1 parent 3f17ed4 commit 2d9626fc74c2d57f90c856c37e5967bbe6651bd8 @jonleighton jonleighton committed with tenderlove Dec 22, 2010
@@ -1229,14 +1229,12 @@ def belongs_to(association_id, options = {})
if reflection.options[:polymorphic]
association_accessor_methods(reflection, BelongsToPolymorphicAssociation)
- association_foreign_type_setter_method(reflection)
else
association_accessor_methods(reflection, BelongsToAssociation)
association_constructor_method(:build, reflection, BelongsToAssociation)
association_constructor_method(:create, reflection, BelongsToAssociation)
end
- association_foreign_key_setter_method(reflection)
add_counter_cache_callbacks(reflection) if options[:counter_cache]
add_touch_callbacks(reflection, options[:touch]) if options[:touch]
@@ -1456,7 +1454,7 @@ 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
+ if association.nil? || force_reload || association.stale_target?
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
@@ -1556,45 +1554,6 @@ def association_constructor_method(constructor, reflection, association_proxy_cl
end
end
- def association_foreign_key_setter_method(reflection)
- setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection|
- if belongs_to_reflection.primary_key_name == reflection.primary_key_name
- "association_instance_set(:#{belongs_to_reflection.name}, nil);"
- end
- end.compact.join
-
- if method_defined?(:"#{reflection.primary_key_name}=")
- undef_method :"#{reflection.primary_key_name}="
- end
-
- class_eval <<-FILE, __FILE__, __LINE__ + 1
- def #{reflection.primary_key_name}=(new_id)
- write_attribute :#{reflection.primary_key_name}, new_id
- if #{reflection.primary_key_name}_changed?
- #{ setters }
- end
- end
- FILE
- end
-
- def association_foreign_type_setter_method(reflection)
- setters = reflect_on_all_associations(:belongs_to).map do |belongs_to_reflection|
- if belongs_to_reflection.options[:foreign_type] == reflection.options[:foreign_type]
- "association_instance_set(:#{belongs_to_reflection.name}, nil);"
- end
- end.compact.join
-
- field = reflection.options[:foreign_type]
- class_eval <<-FILE, __FILE__, __LINE__ + 1
- def #{field}=(new_id)
- write_attribute :#{field}, new_id
- if #{field}_changed?
- #{ setters }
- end
- end
- FILE
- end
-
def add_counter_cache_callbacks(reflection)
cache_column = reflection.counter_cache_column
@@ -130,6 +130,16 @@ def loaded
@loaded = true
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.
+ #
+ # Note that if the target has not been loaded, it is not considered stale.
+ def stale_target?
+ false
+ end
+
# Returns the target of this proxy, same as +proxy_target+.
def target
@target
@@ -42,6 +42,14 @@ def updated?
@updated
end
+ def stale_target?
+ if @target && @target.persisted?
+ @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i
+ else
+ false
+ end
+ end
+
private
def find_target
find_method = if @reflection.options[:primary_key]
@@ -23,6 +23,15 @@ def updated?
@updated
end
+ def stale_target?
+ if @target && @target.persisted?
+ @target.send(@reflection.association_primary_key).to_i != @owner.send(@reflection.primary_key_name).to_i ||
+ @target.class.base_class.name.to_s != @owner.send(@reflection.options[:foreign_type]).to_s
+ else
+ false
+ end
+ end
+
private
# NOTE - for now, we're only supporting inverse setting from belongs_to back onto
@@ -209,7 +209,10 @@ def association_foreign_key
end
def association_primary_key
- @association_primary_key ||= options[:primary_key] || klass.primary_key
+ @association_primary_key ||=
+ options[:primary_key] ||
+ !options[:polymorphic] && klass.primary_key ||
+ 'id'
end
def active_record_primary_key
@@ -298,7 +298,7 @@ def test_should_accept_update_only_option
def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
@ship.delete
-
+
@pirate.reload.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' })
assert_not_nil @pirate.ship
@@ -411,6 +411,7 @@ 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
@@ -451,7 +452,7 @@ def test_should_work_with_update_attributes_as_well
def test_should_not_destroy_the_associated_model_until_the_parent_is_saved
pirate = @ship.pirate
-
+
@ship.attributes = { :pirate_attributes => { :id => pirate.id, '_destroy' => true } }
assert_nothing_raised(ActiveRecord::RecordNotFound) { Pirate.find(pirate.id) }
@ship.save
@@ -7,6 +7,7 @@
require 'models/ship'
require 'models/pirate'
require 'models/price_estimate'
+require 'models/tagging'
class ReflectionTest < ActiveRecord::TestCase
include ActiveRecord::Reflection
@@ -194,6 +195,7 @@ def test_has_many_through_reflection
def test_association_primary_key
assert_equal "id", Author.reflect_on_association(:posts).association_primary_key.to_s
assert_equal "name", Author.reflect_on_association(:essay).association_primary_key.to_s
+ assert_equal "id", Tagging.reflect_on_association(:taggable).association_primary_key.to_s
end
def test_active_record_primary_key

0 comments on commit 2d9626f

Please sign in to comment.