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

Paginators always repeat the query even if the count is 0 #4201

Closed
4 of 6 tasks
gcbirzan opened this issue Jun 16, 2016 · 3 comments
Closed
4 of 6 tasks

Paginators always repeat the query even if the count is 0 #4201

gcbirzan opened this issue Jun 16, 2016 · 3 comments
Milestone

Comments

@gcbirzan
Copy link
Contributor

gcbirzan commented Jun 16, 2016

Checklist

  • I have verified that that issue exists against the master branch of Django REST framework.
  • I have searched for similar issues in both open and closed tickets and cannot find a duplicate.
  • This is not a usage question. (Those should be directed to the discussion group instead.)
  • This cannot be dealt with as a third party library. (We prefer new functionality to be in the form of third party libraries where possible.) - This can be done with a third party library, but I feel that it's a sensible improvement.
  • I have reduced the issue to the simplest possible case.
  • I have included a failing test as a pull request. (If you are unable to do so we can still accept the issue.) - I haven't, since I'm not sure this is considered a bug/desired feature.

Steps to reproduce

  • Add a paginated view for an empty model (Only applies to LimitOffsetPagination)
  • Make a GET request.

Expected behavior

  • If the count query returns 0, the actual query shouldn't be re-run

Actual behavior

  • The query is re-run.

This isn't a problem in the case I described, but in some cases the query can get quite complex and slow. Even if the view isn't wrapped in a transaction and it is possible to get rows on the second run, the results will be inconsistent, but the maybe expensive query is re-run.

@xordoquy
Copy link
Collaborator

Do you have an analysis in regard to where the queryset is re-evaluated ?

@gcbirzan
Copy link
Contributor Author

It depends on the paginator, but, for LimitOffsetPagination it's rest_framework/pagination.py:307 (first one is line 303->53). For CursorPagination, this isn't an issue, since it doesn't do the count. For PageNumberPagination, this is done by DjangoPaginator (first one is 59->78).

While Django behaves the same, that's a separate issue and I will probably put in a ticket there as well. My main concern is whether this is a behaviour that seems sensible (it does to me).

@gcbirzan
Copy link
Contributor Author

For PageNumberPagination, this is done by DjangoPaginator (first one is 59->78).

I've actually went and tested that, and it's not true. Django has an extra bit which caps the upper limit to the count and in qs.query.set_limits, if low and high are equal, it returns an empty QS, without going to the database.

So, I guess that answers my question whether this is intended behaviour or not, I'll make a PR tonight when I get home to unify the behaviour.

@tomchristie tomchristie added this to the 3.4.0 Release milestone Jun 24, 2016
rlucioni pushed a commit to openedx/course-discovery that referenced this issue Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include: support for Django 1.10 (encode/django-rest-framework#4158 - sigh), support for schema generation (encode/django-rest-framework#4179 - required for newer versions of django-rest-swagger), and a change that prevents paginated views from re-running queries when count queries return 0 (encode/django-rest-framework#4201 - this explains the expected query count changes made in tests).

LEARNER-1590
rlucioni pushed a commit to openedx/course-discovery that referenced this issue Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series.

Highlights include:
    - [Support for Django 1.10](encode/django-rest-framework#4158) - sigh
    - [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger
    - A [change](encode/django-rest-framework#4201) that prevents paginated views from re-running queries when count queries return 0 - this explains the expected query count changes made in tests

LEARNER-1590
rlucioni pushed a commit to openedx/course-discovery that referenced this issue Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include:

1. [Support for Django 1.10](encode/django-rest-framework#4158) - sigh
2. [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger
3. A change that [prevents paginated views from re-running queries](encode/django-rest-framework#4201) when count queries return 0 - this explains the expected query count changes made in tests

LEARNER-1590
rlucioni pushed a commit to openedx/course-discovery that referenced this issue Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include:

1. [Support for Django 1.10](encode/django-rest-framework#4158) - sigh
2. [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger
3. A change that [prevents paginated views from re-running queries](encode/django-rest-framework#4201) when count queries return 0 - this explains the expected query count changes made in tests

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

No branches or pull requests

3 participants