Remove memoization for limited_count

[jpawlyn]

Summary

If a relation's records are not loaded and @limited_count gets set, when some of the records are subsequently deleted then the memoized value for @limited_count is inconsistent with the value returned by a call to size. In such a case calls to one? and many? can be incorrect. This is a fix for #43801.

Other Information

[jonathanhefner]

The build failures here are related. I believe this effectively reverts #42143, so perhaps it should be entirely reverted?

[rafaelfranca]

Good find. I think we can keep both if we invalidate the cache of @limited_count when mutating the relation. @jpawlyn can you take a look?

[rafaelfranca]

I'm going to revert this PR in the mean time.

[jpawlyn]

Good find. I think we can keep both if we invalidate the cache of @limited_count when mutating the relation. @jpawlyn can you take a look?

Sure. In the test:

posts.where.not(id: Post.first).destroy_all

the call to where looks to be spawning a new relation so not sure how to invalidate the cache of the original relation for this scenario. Any pointers appreciated.

If there's no obvious way to do the cache invalidation, should we keep with the memoization removal, retain the limited_count method for performance and remove the 2 failing memoization tests?

[matthewd]

Hmm, yeah, I think that test is unrepresentative of expected behaviour (even if it was the historical observable behaviour).

Relations are expected to memoize information about what's in the database after they query it, and will not notice if the database is manipulated afterwards [except very directly through that relation itself]. If you posts.each { .. } (or simply posts.load), then do that destroy_all, then loop through posts afterwards, you'll see posts that have since been destroyed. Given we make no attempt to prevent that in the general case (i.e., if you make DB changes after a relation is loaded, you need to reload it to ensure you're seeing current truth), I don't think we need to give extra consideration to one? here. (The "two methods called at different times can disagree" issue is also visible if you call size early, then later to_a after the destroy: you'll get fewer records than size predicts.)

[jpawlyn]

(The "two methods called at different times can disagree" issue is also visible if you call size early, then later to_a after the destroy: you'll get fewer records than size predicts.)

I guess the strange behaviour here though is that the calls to size and one? are made at the same time, with size returning 1 and one? returning false. I just wonder if this is an unnecessary inconsistency?

[matthewd]

Call size, perform destroy, then: relation.size == relation.to_a.size # => false

Sorry, I made the same mistake I did in #42143 -- my brain remains entirely (and falsely) convinced that size memoizes.