Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix association loading behavior when counter cache is zero #35127

Merged
merged 1 commit into from Feb 5, 2019

Conversation

bogdan
Copy link
Contributor

@bogdan bogdan commented Feb 1, 2019

Suppose we have a has_many association with a counter cache.
In this case, when counter cache is zero, it is often used to avoid certain SQL queries.

Example:

Reward.belogns_to :coupon, counter_cache: true
coupon = Coupon.create!
coupon.rewards_count # => 0 - required

coupon.rewards.size # => 0 no query
coupon.rewards.to_a # => [] no query

However, this behavior doesn't happen consistently. If we change the method call order the number of SQL queries is different:

coupon.rewards.to_a # => [] with query
coupon.rewards.size # => [] no query

It happens because #size makes use of the counter cache and assumes the association is loaded when it is zero. I think that this behavior should be consistent with direct association loading.

A lot of additional tests were added to ensure no regression occur.

cc @kamipo

@rafaelfranca
Copy link
Member

There is a random failure being caused by the new tests being added. Can you fix it?

@kamipo kamipo merged commit eec3e28 into rails:master Feb 5, 2019
@Edouard-chin
Copy link
Member

👋 Thanks for opening this PR ! We are targeting HEAD in our monolith and found that this commit is breaking few of our tests.
TL;DR Model.destroy_all doesn't do anything when fixtures gets loaded and the parent try to destroy all children, this is because the counter_cache_value is 0 when fixtures gets loaded, we end up not loading the target https://github.com/rails/rails/pull/35127/files#diff-7eee2aa7b5cdbffb0bc3ef2eb7d0b76fR70

Although the counter_cache value not getting set seems to be an issue only in tests, isn't it risky that if for any reasons the counter_cache is desync we can end up not deleted any records?

I created a reproduction script here https://gist.github.com/Edouard-chin/df15e2f11a9129880d264c8d861a11ae

Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Feb 13, 2019
- rails#35127 introduced a new change to
  not make a query when the counter cache value of the parent is 0.

  The implementation was overwriting the `find_target?` method from
  `AR::Associaton`.
  This had the side effect of making `Topic.comments.destroy_all` to
  not destroy anything in some circumstances.

  The bug was mainly happening during test because the counter_cache
  value isn't set when fixtures load. This resulted in the
  `find_target` method to return the in memory target (empty array)
  instead of getting it from the DB.

  This PR removes the `find_target?` overwritten method in
  HasManyAssociation and inline the condition inside methods that
  needs it (such as `size` and `load_target` which is used by
  `to_ary`).

  Even though the root cause of the problem was mainly happening in
  test, I decided to opt-out the `destroy_all` from checking the
  counter cache value has this could cause potential issue if fonr any
  reason the counter cacher get desync.
kamipo added a commit that referenced this pull request Feb 13, 2019
This reverts commit eec3e28, reversing
changes made to 5588fb4.

Reason: Marking as loaded without actual loading is too greedy optimization.

See more context #35239.

Closes #35239.

[Edouard CHIN & Ryuta Kamizono]
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.

None yet

4 participants