[Django] #26290: Pagination module should warn about unordered query set
Django
--------------------------------------+------------------------
Reporter: kartikanand | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 1.9
Severity: Normal | Keywords: pagination
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------
Let's say somebody is using Gunicorn with more than one worker process.
Let's say contact_list is the following: (Paginating by 3)
{{{
[123, 456, 789, 345, 567, 909, 102]
}}}
When the first worker process handles request for page 1, it is possible
that it gets the first three
{{{
[123, 456, 789],
}}}
but when the second worker process handles the request for page 2, it is
very much possible that it returns,
{{{
[456, 789, 345]
}}}
See the duplicate results above. This is because the query set is
'''unordered'''.
The above "may" also happen with just one worker process.
The documentation should warn about inconsistent pagination when no
ordering is specified.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: kartikanand | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by claudep):
* needs_better_patch: => 0
* version: 1.9 => master
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
Documentation is a minimum, I would also consider a runtime warning.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:1>
Django
Reporter: kartikanand | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by charettes):
If we were to document or raise a warning in this case it must also be
done when a queryset is ordered but not by any unique expression.
Else I don't see much benefit as ordering by a non unique expression can
lead to the same undefined ordering reported here.
Since I can't think of any reliable way of determining if a queryset is
uniquely ordered (matching ordered fields against `unique` and
`unique_together` can yields false positive as users might have defined
unique functional indexes using `RunSQL`) I think we should favor a
documentation patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:2>
Django
Reporter: kartikanand | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by jarshwah):
Hmm I don't agree Simon. There's a vast difference between not ordered and
mostly ordered. If a column is not unique you can still order by a second
clause which will get you even closer to a unique ordering (and so on).
Any duplicates in the final ordering will be randomly ordered (database
dependent), but non duplicates will still be in the correct order. For
most applications I would expect that to be OK.
Documentation should definitely be added. I would probably also like to
see a warning about a completely unordered paginated queryset for the
users sake.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:3>
Django
Reporter: kartikanand | Owner: duane9
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by duane9):
* owner: nobody => duane9
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:4>
Django
Reporter: kartikanand | Owner: duane9
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by duane9):
https://github.com/django/django/pull/6296
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:5>
Django
Reporter: kartikanand | Owner: duane9
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0dc3822f56a0db83f9fc0b1ed320bbeb477eca63" 0dc3822f]:
{{{
#!CommitTicketReference repository=""
revision="0dc3822f56a0db83f9fc0b1ed320bbeb477eca63"
[1.9.x] Fixed #26290 -- Documented that a QuerySet for pagination should
be ordered.
Backport of f8b23e52e86307428da2cf928bf4f1d9fdbd2694 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:6>
Django
Reporter: kartikanand | Owner: duane9
Type: Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"f8b23e52e86307428da2cf928bf4f1d9fdbd2694" f8b23e5]:
{{{
#!CommitTicketReference repository=""
revision="f8b23e52e86307428da2cf928bf4f1d9fdbd2694"
Fixed #26290 -- Documented that a QuerySet for pagination should be
ordered.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:7>
Django
Reporter: kartikanand | Owner: duane9
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by claudep):
* status: closed => new
* resolution: fixed =>
Comment:
Was the runtime warning excluded somewhere? I still think it might be a
good idea.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:8>
Django
Reporter: kartikanand | Owner: duane9
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by duane9):
Okay, I will do that. I will have it done in two weeks.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:9>
Django
Reporter: kartikanand | Owner: duane9
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by duane9):
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:10>
Django
Reporter: kartikanand | Owner:
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by duane9):
* status: assigned => new
* owner: duane9 =>
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:11>
Django
Reporter: kartikanand | Owner:
Type: | EmadMokhtar
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by EmadMokhtar):
* status: new => assigned
* owner: duane9 => EmadMokhtar
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:9>
Django
Reporter: kartikanand | Owner:
Type: | EmadMokhtar
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* has_patch: 0 => 1
* component: Documentation => Core (Other)
* needs_tests: 0 => 1
Comment:
[https://github.com/django/django/pull/6384 PR] (currently lacks a test)
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:10>
Django
Reporter: kartikanand | Owner:
Type: | EmadMokhtar
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
Comment:
Left some comments for improvement and a few test failures remain.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:11>
Django
Reporter: kartikanand | Owner:
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Changes (by carljm):
* status: assigned => new
* owner: EmadMokhtar =>
Comment:
De-assigning to make it clear that this is available for a PyCon sprinter
to finish up.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:12>
Django
Reporter: kartikanand | Owner:
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: pagination | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
--------------------------------------+------------------------------------
Changes (by timgraham):
* easy: 1 => 0
Comment:
It looks like the patch was waiting for a review actually (which I've just
done), so I'd give a chance to let the original owner finish it up.
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:13>
Django
Reporter: kartikanand | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Keywords: pagination | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"c4980e28e57f385d8ffed5e32ce373e52ce61049" c4980e28]:
{{{
#!CommitTicketReference repository=""
revision="c4980e28e57f385d8ffed5e32ce373e52ce61049"
Fixed #26290 -- Warned that paginating an unordered QuerySet may result in
inconsistent results.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:14>
Django
Reporter: Kartik Anand | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Keywords: pagination | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette <charette.s@…>):
In [changeset:"c0f12a098c0258eef3e9af982c17f5ef7f6c927d" c0f12a0]:
{{{
#!CommitTicketReference repository=""
revision="c0f12a098c0258eef3e9af982c17f5ef7f6c927d"
Fixed #28109 -- Corrected the stack level of unordered queryset pagination
warnings.
Refs #26290.
Thanks Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:15>
Django
Reporter: Kartik Anand | Owner: Tim
Type: | Graham <timograham@…>
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Keywords: pagination | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette <charette.s@…>):
In [changeset:"395df007f4151390c614866d99827bbed1795a43" 395df007]:
{{{
#!CommitTicketReference repository=""
revision="395df007f4151390c614866d99827bbed1795a43"
[1.11.x] Fixed #28109 -- Corrected the stack level of unordered queryset
pagination warnings.
Refs #26290.
Thanks Tim for the review.
Backport of c0f12a098c0258eef3e9af982c17f5ef7f6c927d from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26290#comment:16>