Fix AR::Relation#last bugs instroduced in 7705fc #23377
Conversation
|
r? @eileencodes (@rails-bot has picked a reviewer for you, use r? to override) |
|
|
||
| result = limit(sql_limit) | ||
| if initial_limit | ||
| result.offset!((offset_value || 0) + initial_limit - sql_limit) |
There was a problem hiding this comment.
Should we just stick with to_a here? This assumes there are at least offset_value + initial_limit rows in the original relation, doesn't it?
There was a problem hiding this comment.
In this patch implementation people have a choice between relation.last and relation.to_a.last. In your variant, there will be no such choice. I vote for this patch version, but open for an argument.
There was a problem hiding this comment.
I guess my argument is that before we started changing it, it always returned the right answer. Now it doesn't, and people may be forced to call to_a to make it work. Performance is nice and all, but that doesn't seem like a win. 😕
There was a problem hiding this comment.
The problem with behaviour comes from lack of tests.
I tried some combinations of offset and limit on a relation where last is called and didn't find a different behaviour from previous version. Can you guess what else it could be?
It would be cool to add a test anyway even we go with to_a.last when limit present on basic relation at the end anyway.
|
This fixes the issue I'm seeing in our app 👍 |
| def find_last(limit) | ||
| limit ? to_a.last(limit) : to_a.last | ||
| end | ||
|
|
There was a problem hiding this comment.
Minor nitpick, can you remove this extra line.
|
@eileencodes removed whitespace. Anything else? |
This reverts commit 9f3730a, reversing changes made to 2637fb7. There are additional issues with this commit that need to be addressed before this change is ready (see #23377). This is a temporary revert in order for us to have more time to address the issues with that PR, without blocking the release of beta2.
|
Updated PR. Current version contains:
@dmathieu if you have a test that we can add to a test suite, please suggest. I still didn't a use case that is not covered. |
|
@eileencodes can you verify if this is good to go back in? |
|
Hey @bogdan there's a conflict in the last method when I rebase. Can you take a look at it so I can test this against basecamp? Thanks! |
|
I tested and this looks good. If you can rebase we can merge this again. |
bogdan
force-pushed
the
last-with-sql
branch
6 times, most recently
from
9f0d765
to
cedc362
Compare
instead of loading the relation into memory
bogdan
force-pushed
the
last-with-sql
branch
from
cedc362
to
b679916
Compare
|
@eileencodes done |
Fix AR::Relation#last bugs instroduced in 7705fc
Fix bugs identifier in #16400 (comment)
I realized that
lastshould work pretty different on a relation withlimitand without it.So the fix is pretty different from what we all expected:
We need to use
offsetinstead ofreverse_orderon such a relation.cc @eileencodes @rafaelfranca @sgrif