-
Notifications
You must be signed in to change notification settings - Fork 406
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
Bug: Pagy seems to ignore #limit scope #461
Comments
Thank you for your report. Not sure if this is something pagy can solve: scope = User.limit(1)
scope.limit(100) # will come out with 100 records, not 1 - and pagy not involved here at all in this example It is a known "issue" with Active Record and is the intended behaviour....but should it be? All pagy does is basically apply Thoughts? |
Pagy just applies the limit to whichever scope you are paginating, hence it basically reset the AR limit to the I would say that 2 items would be "counter-intuitive" if you know how AR limit works. Besides, with the last statement, you are basically stating that you want to paginate with the defined |
Added warning info to the how-to docs. @cappe I hope this will avoid any confusion. |
@benkoshy @ddnexus Thank you for taking time to answer, and adding the warning in the docs. I think the AR behavior for (multiple) limits makes sense. When used by the developer themselves, it's usually somewhat visible and explicit. However, when Pagy does it, it's behind the scenes. I may know that Pagy (or pagination in general) is basically limit + offset but the next person may not (especially newcomers).
If I use limit in my code, I should be confident it to be applied, not reset behind the scenes. |
OK. So the alternative would be ignoring the If you want to do it automatically, it would mean checking that 2 conditions at each request, which would be an expensive performance hit without any good reason. If you want to avoid penalizing most requests (which are practically almost the totality), then you should explicitly pass some param to the constructor, which is equivalent to just explicitly set the |
Not too ideal either. I guess having the warning in the docs is enough. Considering use cases for using limit along with pagination isn't too common anyway. Thanks for helping sorting this out. |
Before submitting...
I am providing...
Simple step by step list that would work in IRB with the Pagy::Console
Plain ruby file that can run as
ruby my-problem.rb
Edited copy of the single file pagy_standalone_app.ru
Edited copy of the single file pagy_bare_rails.rb
Link of my own branch forked from pagy, which contains an added test file
Link of my own branch forked from any of the rails apps listed here.
A
docker-compose
file that uses only docker images and no local context and runs withdocker-compose up
Description
When using ActiveRecord (with Rails), Pagy seems to unscope/reset
#limit
scope:The
@users_limited
returns up toPagy::DEFAULT[:items]
amount of items. Intuitively, it should return up to 2 items.I cannot wrap my head around it if it's intended this way or not. To me it feels dangerous as I'd need to know that it behaves this way.
Here's a minimal project that reproduces the behavior, just
docker compose up
: https://github.com/cappe/pagy-bug-report-scopingThe text was updated successfully, but these errors were encountered: