Skip to content

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

Merged
lovelydinosaur merged 1 commit intomasterfrom
limit-zero-pagination-fix
Jun 13, 2016
Merged

In LimitOffsetPagination limit=0 should revert to default limit.#4194
lovelydinosaur merged 1 commit intomasterfrom
limit-zero-pagination-fix

Conversation

@lovelydinosaur
Copy link
Copy Markdown
Contributor

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

@lovelydinosaur lovelydinosaur added this to the 3.4.0 Release milestone Jun 13, 2016
@lovelydinosaur
Copy link
Copy Markdown
Contributor 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.)

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

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

@lovelydinosaur lovelydinosaur merged commit 2e7fae7 into master Jun 13, 2016
@lovelydinosaur lovelydinosaur deleted the limit-zero-pagination-fix branch June 13, 2016 15:32
@mitar
Copy link
Copy Markdown
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.

@lovelydinosaur
Copy link
Copy Markdown
Contributor 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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

mitar commented Jun 13, 2016

Hm, what is difference between max_limit and max_page_size?

@mitar
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown

yli-cpr commented Jan 15, 2018

Thanks. I am doing a similar workaround.

@crazy-canux
Copy link
Copy Markdown

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