LimitOffsetPagination limit=0 fix

[nhorelik]

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]

@nhorelik there are a few linter issues.

[nhorelik]

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]

@nhorelik mostly just keeping the linters happy.

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

[kevin-brown]

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

[nhorelik]

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]

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

[tomchristie]

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

[mitar]

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]

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]

Closed via #4194