Skip to content

Conversation

composerinteralia
Copy link
Member

@composerinteralia composerinteralia commented Oct 13, 2021

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:

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 this 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 #9499, but the test from that PR still passes when we remove @inversed because of #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:

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 #31214 (setting the inverse when autosaving has_one associations) and sets the inverse when autosaving has_many associations. That way we update @stale_state after the owner is saved and has an id available.

if inverse_reflection = reflection.inverse_of
record.association(inverse_reflection.name).inversed_from(self)
end
association.set_inverse_instance(record)
Copy link
Member Author

@composerinteralia composerinteralia Oct 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary for this PR, but it's the method we use everywhere else for this, and it lets this file know a bit less about how inverse_of works. It essentially the same thing, although this does add one additional check to make sure the record has the right foreign key attribute.

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
rails#9499, but the test from that PR
still passes when we remove `@inversed` because of
rails#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 rails#31214
(setting the inverse when autosaving has_one associations) and sets the
inverse when autosaving has_many associations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants