Fix counter_cache double increment
#29765
Conversation
lugray
force-pushed
the
fix_counter_cache
branch
from
17f0939
to
0edf16e
Compare
|
👍 |
| @@ -180,7 +180,8 @@ def _create_record(*) | |||
| each_counter_cached_associations do |association| | |||
| if send(association.reflection.name) | |||
| association.increment_counters | |||
| @_after_create_counter_called = true | |||
| @_after_create_counter_called ||= {} | |||
There was a problem hiding this comment.
Any reason we are adding the ||= to assign this ivar?
There was a problem hiding this comment.
Because it may or may not be defined yet.
There was a problem hiding this comment.
But why the previous implementation assumed it was never defined?
There was a problem hiding this comment.
It doesn't assume anything. It just wanted to set the flag to true and it didn't matter whether it was defined or not. I just want to set the flag to true for a key, so I need to make sure the hash is defined.
|
|
||
| if (@_after_create_counter_called ||= false) | ||
| @_after_create_counter_called = false | ||
| if (@_after_create_counter_called[foreign_key] ||= false) |
There was a problem hiding this comment.
Might just be a preference question 🤷♂️ , but won't it be more readable now that we have a hash to not assign in the condition and let the condition body do it?
Feels weird that the condition body is doing the same as the condition itself.
if @_after_create_counter_called.fetch(foreign_key, false)
@_after_create_counter_called = false
endThere was a problem hiding this comment.
I agree with you. I was unwisely making decisions for an easier to read diff instead of easier to read code. I've pushed up a new version.
lugray
force-pushed
the
fix_counter_cache
branch
2 times, most recently
from
cca8f3a
to
0ddc30a
Compare
|
The test in this PR passes with or without the changes to the code (at least when I pull it into master). In fact, I can delete the ivar entirely and get no test failures. Can you confirm that the issue is still present on master? I would have expected this to be fixed on 5.1 but if not it may have been fixed by 020abad |
|
Can confirm, broken on |
|
Yes, we should keep the test provided by this PR |
lugray
force-pushed
the
fix_counter_cache
branch
from
0ddc30a
to
7c5e4c7
Compare
|
Updated. |
When an `after_create` callback did `update_attributes` on a record with multiple `belongs_to` associations with counter caches, even numbered associations would have their counters double-incremented. Fixes to `ActiveModel::Dirty` in 020abad fixed this. This adds regression tests for this bug fixed incidentally in the other commit, which also removed the need for the workaround using @_after_create_counter_called.
lugray
force-pushed
the
fix_counter_cache
branch
from
7c5e4c7
to
6137a4e
Compare
|
🤦♀️ Commit message updated. |
Fixes #29762
Summary
When anafter_createcallback didupdate_attributeson a record with multiplebelongs_toassociations with counter caches, when doingbelongs_to_counter_cache_after_updatefor the first association, it would set@_after_create_counter_calledto false to prepare for subsequent operations, but then the following association would get re-incremented. This splits out@_after_create_counter_calledas a hash keyed by the foreign key to track each separately.The need for
@_after_create_counter_calledwas removed altogether by 020abad, which also makes the test added in this PR pass.Other Information
A similar fix may be required for@_after_replace_counter_called, but I couldn't figure out a failing test case and so didn't want to do it unnecessarily.It may also be safe to remove
@_after_replace_counter_called.