Skip to content
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

LimitOffsetPagination limit=0 fix #3444

Closed
wants to merge 5 commits into from

Conversation

nhorelik
Copy link

@nhorelik nhorelik commented Sep 24, 2015

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.

@jpadilla
Copy link
Member

jpadilla commented Sep 25, 2015

@nhorelik there are a few linter issues.

@nhorelik
Copy link
Author

nhorelik commented Sep 25, 2015

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?)

@jpadilla
Copy link
Member

jpadilla commented Sep 25, 2015

@nhorelik mostly just keeping the linters happy.

@tomchristie @xordoquy thoughts on this? Seems like a valid enhancement to me.

@kevin-brown
Copy link
Member

kevin-brown commented Sep 25, 2015

Seems valid, though I feel like this is something which should be handled in get_limit (so it properly defaults).

@nhorelik
Copy link
Author

nhorelik commented Sep 25, 2015

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.

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 25, 2015

Don't the other paginators consider limit = 0 to be unlimited ?

@tomchristie
Copy link
Member

tomchristie commented Sep 26, 2015

We shouldn't allow users to force an unlimited queryset to be returned. Agree that an empty set makes sense.

@mitar
Copy link
Contributor

mitar commented Mar 13, 2016

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.

@mitar
Copy link
Contributor

mitar commented Mar 13, 2016

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.)

@tomchristie
Copy link
Member

tomchristie commented Jun 13, 2016

Closed via #4194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants