Skip to content
  • Daniel Colson's avatar
    9c8fab96
    Make inversed association stale after updating id · 9c8fab96
    Daniel Colson authored
    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.
    9c8fab96
    Make inversed association stale after updating id
    Daniel Colson authored
    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.
Loading