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

In LimitOffsetPagination limit=0 should revert to default limit. #4194

Merged
merged 1 commit into from
Jun 13, 2016

Conversation

tomchristie
Copy link
Member

Treat limit=0 in the same way as an other invalid amount in thelimit` parameter.

@tomchristie tomchristie added this to the 3.4.0 Release milestone Jun 13, 2016
@tomchristie
Copy link
Member Author

Closes #3990.
Closes #3444.

Not that this also removes a now defunct test, that it would return empty results. (Which didn't cover the DivideByZero that it also causes elsewhere.)

@tomchristie tomchristie changed the title limit=0 should revert to default limit. In LimitOffsetPagination limit=0 should revert to default limit. Jun 13, 2016
@tomchristie
Copy link
Member Author

Thanks to @mitar and @nhorelik for the input on this! 😄

@tomchristie tomchristie merged commit 2e7fae7 into master Jun 13, 2016
@tomchristie tomchristie deleted the limit-zero-pagination-fix branch June 13, 2016 15:32
@mitar
Copy link
Contributor

mitar commented Jun 13, 2016

Hm, but with this change it is not possible to ask for maximum allowed number of requests? Client has to know how much is that.

@tomchristie
Copy link
Member Author

I don't think 0==max is particularly intuitive behavior. If someone wants that, then sure override the class and use your own desired behavior. Treating 0 the same as any other invalid value seems a pretty reasonable default.

@kevin-brown
Copy link
Member

Hm, but with this change it is not possible to ask for maximum allowed number of requests?

There's a note in the docs about how you might go about doing this.

http://www.django-rest-framework.org/api-guide/pagination/#drf-extensions

The DRF-extensions package includes a PaginateByMaxMixin mixin class that allows your API clients to specify ?page_size=max to obtain the maximum allowed page size.

@mitar
Copy link
Contributor

mitar commented Jun 13, 2016

Hm, what is difference between max_limit and max_page_size?

@mitar
Copy link
Contributor

mitar commented Jun 13, 2016

So it seems that that extension works only when you have pagination, not when you have offset/limit approach. But maybe a similar mixin can be make for offset/limit as well.

helgi added a commit to helgi/workflow-e2e that referenced this pull request Jul 24, 2016
helgi added a commit to helgi/workflow-e2e that referenced this pull request Jul 24, 2016
@stschindler
Copy link

stschindler commented Aug 1, 2017

Unfortunately this prevents my frontend client to get the count of a particular resource. What I've been doing is sending a GET request to /resource?limit=0 to fetch the total amount of existing records.

Is there any other good way of doing that?

@yli-cpr
Copy link

yli-cpr commented Jan 15, 2018

This breaks old behavior. Clients may have been using ?limit=0 to count objects. Is there any benefit of treating zero as invalid input?

@carltongibson
Copy link
Collaborator

...old behaviour...

That's quite old 😀 v3.4 was mid-2016.

A work around would be to subclass LimitOffsetPagination to check for 0 in get_limit before calling super.

@yli-cpr
Copy link

yli-cpr commented Jan 15, 2018

Thanks. I am doing a similar workaround.

@crazy-canux
Copy link

How did you guys fix this?
I upgrade from 3.3 to 3.8, but frontend use &limit=0 not working.

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.

7 participants