LimitOffsetPagination limit=0 fix by nhorelik · Pull Request #3444 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits5 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
This PR fixes an uncaught ZeroDivisionError I encountered when the limit query paramater was set to zero when using rest_framework.pagination.LimitOffsetPagination.
I figured this should return an empty set like you might expect, but otherwise revert to the defaults.
My bad. Serves me right for only pip installing requirements/requirements-testing.txt instead of the whole thing from the get-go.
Other than what's mentioned here, do you guys follow/enforce any particular style guide? (for example, the google style guide, or just pep8?)
Seems valid, though I feel like this is something which should be handled in get_limit (so it properly defaults).
If you set self.limit to the default in get_limit, paginate_queryset will not give you an empty result like you might expect from a limit of zero. I figured that this should act like the equivalent sql limit, which meanspaginate_queryset shouldn't use the default limit - it really should be zero there. Since the ZeroDivisionError happens in get_html_context in the calls to _divide_with_ceil at a different time in the flow, I figured it best not to change self.limit.
If you prefer to treat a limit of zero as an invalid query param and use the defaults for everything including when paginating the queryset, then this is a one-liner fix: just set strict=True in the call to _positive_int in get_limit.
Don't the other paginators consider limit = 0 to be unlimited ?
We shouldn't allow users to force an unlimited queryset to be returned. Agree that an empty set makes sense.
I also think that limit=0 should be unlimited (unlimited inside the max_limit limit).
So not providing limit makes queryset go for default_limit, if you provide limit=0, it goes to max_limit. I think this is reasonable.
I made an alternative fix for this in #3990. By default, if limit=0 query parameter is provided, it uses max_limit as effective limit. But this behavior can be changed in a subclass. I think it is a simpler implementation than this one. And allows both desired behaviors for 0.
(Personally, I prefer to use max_limit because one can already get default_limit by simply not providing limit query parameter.)
This was referenced
Mar 9, 2017