1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

Make inversed association stale after updating id

Problem
---

Prior to this commit an inversed association would always return false
for `stale_target?`. This could cause problems when updating ids. For
example, assuming that the `Post#comments` association has an inverse to
the `Comment#post` association:

```rb
comment = post.comments.first

comment.update!(post_id: some_other_post_id)

comment.post should now return the post with some_other_post_id, but
since it was inversed it doesn't go stale and the test fails
refute_equal post, comment.post
```

This commit adds a test to catch this example, as well as a test for
counter caches (we already test that this counter cache behavior works
when `inverse_of` is not set, but when `inverse_of` was set we'd
increment and decrement the counter on the same target, effectively a
no-op).

Solution
---

This commit updates the `stale_target?` method so it no longer checks
whether the association is inversed. That is enough to get the two new
tests passing.

The `@inversed` check was originally added in
https://github.com/rails/rails/pull/9499, but the test from that PR
still passes when we remove `@inversed` because of
https://github.com/rails/rails/pull/31214. Rather than tracking
`@inversed`, we set the inverse when autosaving the has one association,
updating the `@stale_state` in the process.

Removing `@inversed` broke some other tests, however:

  - test_belongs_to_with_touch_option_on_destroy_with_destroyed_parent
  - test_touch_later_an_association_dont_autosave_parent
  - test_counter_caches_are_updated_in_memory_when_the_default_value_is_nil
  - test_counters_are_updated_both_in_memory_and_in_the_database_on_create

All of these followed the same pattern of building a record, setting an
inverse to the new record, and then saving. For example:

```rb
invoice = Invoice.create!(line_items: [line_item])
```

First Rails builds the invoice. Then it sets that new invoice as the
inverse for the `line_item`s `#invoice` association (i.e. so
`line_item.invoice` is the same object as `invoice`). Setting the
inverse also involves updating `@stale_state`, but since it's a new
invoice that will be `nil`. Then everything gets saved, at which point
the `nil` `@stale_state` no longer matches `stale_state`.

Rather than relying on `@inversed` to fix this, instead this commit
takes a similar approach to https://github.com/rails/rails/pull/31214
(setting the inverse when autosaving has_one associations) and sets the
inverse when autosaving has_many associations.
This commit is contained in:
Daniel Colson 2021-10-13 15:52:05 -04:00
parent eb1f7cca3b
commit 9c8fab96be
No known key found for this signature in database
GPG key ID: 88A364BBE77B1353
4 changed files with 35 additions and 11 deletions

View file

@ -52,7 +52,6 @@ module ActiveRecord
@loaded = false @loaded = false
@target = nil @target = nil
@stale_state = nil @stale_state = nil
@inversed = false
end end
def reset_negative_cache # :nodoc: def reset_negative_cache # :nodoc:
@ -78,7 +77,6 @@ module ActiveRecord
def loaded! def loaded!
@loaded = true @loaded = true
@stale_state = stale_state @stale_state = stale_state
@inversed = false
end end
# The target is stale if the target no longer points to the record(s) that the # The target is stale if the target no longer points to the record(s) that the
@ -88,7 +86,7 @@ module ActiveRecord
# #
# Note that if the target has not been loaded, it is not considered stale. # Note that if the target has not been loaded, it is not considered stale.
def stale_target? def stale_target?
!@inversed && loaded? && @stale_state != stale_state loaded? && @stale_state != stale_state
end end
# Sets the target of this association to <tt>\target</tt>, and the \loaded flag to +true+. # Sets the target of this association to <tt>\target</tt>, and the \loaded flag to +true+.
@ -137,15 +135,11 @@ module ActiveRecord
def inversed_from(record) def inversed_from(record)
self.target = record self.target = record
@inversed = !!record
end end
def inversed_from_queries(record) def inversed_from_queries(record)
if inversable?(record) if inversable?(record)
self.target = record self.target = record
@inversed = true
else
@inversed = false
end end
end end

View file

@ -404,6 +404,8 @@ module ActiveRecord
saved = true saved = true
if autosave != false && (new_record_before_save || record.new_record?) if autosave != false && (new_record_before_save || record.new_record?)
association.set_inverse_instance(record)
if autosave if autosave
saved = association.insert_record(record, false) saved = association.insert_record(record, false)
elsif !reflection.nested? elsif !reflection.nested?
@ -447,9 +449,7 @@ module ActiveRecord
if (autosave && record.changed_for_autosave?) || record_changed?(reflection, record, key) if (autosave && record.changed_for_autosave?) || record_changed?(reflection, record, key)
unless reflection.through_reflection unless reflection.through_reflection
record[reflection.foreign_key] = key record[reflection.foreign_key] = key
if inverse_reflection = reflection.inverse_of association.set_inverse_instance(record)
record.association(inverse_reflection.name).inversed_from(self)
end
end end
saved = record.save(validate: !autosave) saved = record.save(validate: !autosave)

View file

@ -1440,7 +1440,7 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
assert_equal original_count, topic.reload.replies_count assert_equal original_count, topic.reload.replies_count
end end
def test_calling_update_changing_ids_doesnt_change_counter_cache def test_calling_update_changing_ids_changes_the_counter_cache
topic1 = Topic.find(1) topic1 = Topic.find(1)
topic2 = Topic.find(3) topic2 = Topic.find(3)
original_count1 = topic1.replies.to_a.size original_count1 = topic1.replies.to_a.size
@ -1458,6 +1458,24 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
assert_equal original_count2, topic2.reload.replies_count assert_equal original_count2, topic2.reload.replies_count
end end
def test_calling_update_changing_ids_of_inversed_association_changes_the_counter_cache
assert_predicate Post.reflect_on_association(:comments), :has_inverse?
post1 = Post.first
post2 = Post.second
original_count1 = post1.comments.count
original_count2 = post2.comments.count
post1.comments.first.update(post_id: post2.id)
assert_equal original_count1 - 1, post1.reload.comments_count
assert_equal original_count2 + 1, post2.reload.comments_count
post2.comments.first.update(post_id: post1.id)
assert_equal original_count1, post1.reload.comments_count
assert_equal original_count2, post2.reload.comments_count
end
def test_deleting_a_collection def test_deleting_a_collection
force_signal37_to_load_all_clients_of_firm force_signal37_to_load_all_clients_of_firm

View file

@ -664,6 +664,18 @@ class InverseHasManyTests < ActiveRecord::TestCase
comment.body = "OMG" comment.body = "OMG"
assert_equal comment.body, comment.children.first.parent.body assert_equal comment.body, comment.children.first.parent.body
end end
def test_changing_the_association_id_makes_the_inversed_association_target_stale
post1 = Post.first
post2 = Post.second
comment = post1.comments.first
assert_same post1, comment.post
comment.update!(post_id: post2.id)
assert_equal post2, comment.post
end
end end
class InverseBelongsToTests < ActiveRecord::TestCase class InverseBelongsToTests < ActiveRecord::TestCase