Fix counter caches to work for models with composite primary keys #50983
Conversation
| Cpk::Order.increment_counter(:books_count, order.id) | ||
|
|
||
| # check that it gets reset | ||
| assert_difference "order.reload.books_count", -1 do |
There was a problem hiding this comment.
Are you sure eval is cool here? It might be slow. How about lambda?
| assert_difference "order.reload.books_count", -1 do | |
| assert_difference ->{ order.reload.books_count }, -1 do |
There was a problem hiding this comment.
That is the style used within this file. The slowness won't be noticeable, but I also don't like this pattern, so time to break it.
fatkodima
force-pushed
the
fix-counter-caches-for-cpk
branch
from
36a6a00
to
f5f4bca
Compare
| unscoped.where(primary_key => object.id).update_all(updates) if updates.any? | ||
| if updates.any? | ||
| id = object.id | ||
| id = [id] if composite_primary_key? && id.is_a?(Array) && !id[0].is_a?(Array) |
There was a problem hiding this comment.
I think we're way past the point where we need to take a step back and figure out how to not have to check for CPK in such a ad hoc way everywhere.
Even if we ignore how ugly this is, and fix all the call sites we may have missed, we're likely to constantly break CPK support when adding new features.
There was a problem hiding this comment.
Generally we expected that where(primary_key => id) will "just work", or at least where(primary_key => [id]) will work regardless of what shape id has.
But it looks like we end up in a situation where only "multi" where(primary_key => [id]) where [id] is an array of arrays for composite primary key models and array of plan ids for regular models.
One of the possible options that was considered is to introduce some kind of a Model.primary_key_scope(id_or_ids) helper (at least for internal use) which will always know how to build a correct relation. The helper implementation may end up having similar if checks but at least consumers won't need to bother.
It may be a bit harder but in the long term if we had primary_key_scope concept we could make sure that CPK relations and regular model relations have different implementation of the method so there will be no branching
There was a problem hiding this comment.
Hey @fatkodima, do you remember why this line needed the array wrap as well?
In this case we know that id = object.id will always represent one identifier so unscoped.where(primary_key => [id]) should just work. I tried it quickly and none of the tests seem to fail
The update_counters does need at least something as indeed we can't know whether passed id is an array of ids or just a composite primary key array
There was a problem hiding this comment.
Yeah, correct 🤦 We had object.id, not just id.
fatkodima
force-pushed
the
fix-counter-caches-for-cpk
branch
from
f5f4bca
to
ea38048
Compare
| @@ -113,6 +113,7 @@ def reset_counters(id, *counters, touch: nil) | |||
| # # `updated_at` = '2016-10-13T09:59:23-05:00' | |||
| # # WHERE id IN (10, 15) | |||
| def update_counters(id, counters) | |||
| id = [id] if composite_primary_key? && id.is_a?(Array) && !id[0].is_a?(Array) | |||
There was a problem hiding this comment.
@casperisfine So after a small tweak this is the only place where we end up with a "not so pretty" handling. How would you feel about merging this as a bugfix but we will still work on a solution to make sure we get rid of composite_primary_key? branches and everything that comes with it in a separate PR. Because even if we come up with something I'd prefer to ship it separately as this bugfix will need to be backported while a refactoring can be released in the next major
There was a problem hiding this comment.
Yeah, that was my idea all along, I'm suggesting we clean this on on main but we won't be backporting it.
And yes 7-1 need a fix for this, it's still much uglier than I would like, but a bug is a bug.
Fix counter caches to work for models with composite primary keys
|
Backported as 26aa77d |
Fixes #50968.