Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#26290 closed Cleanup/optimization (fixed)

Pagination module should warn about unordered query set

Reported by: Kartik Anand Owned by: Tim Graham <timograham@…>
Component: Core (Other) Version: dev
Severity: Normal Keywords: pagination
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (16)

comment:1 by Claude Paroz, 8 years ago

Triage Stage: UnreviewedAccepted
Version: 1.9master

Documentation is a minimum, I would also consider a runtime warning.

comment:2 by Simon Charette, 8 years ago

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.

comment:3 by Josh Smeaton, 8 years ago

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.

comment:4 by Duane Hilton, 8 years ago

Owner: changed from nobody to Duane Hilton
Status: newassigned

comment:6 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 0dc3822f:

[1.9.x] Fixed #26290 -- Documented that a QuerySet for pagination should be ordered.

Backport of f8b23e52e86307428da2cf928bf4f1d9fdbd2694 from master

comment:7 by Tim Graham <timograham@…>, 8 years ago

In f8b23e5:

Fixed #26290 -- Documented that a QuerySet for pagination should be ordered.

comment:8 by Claude Paroz, 8 years ago

Resolution: fixed
Status: closednew

Was the runtime warning excluded somewhere? I still think it might be a good idea.

comment:9 by Emad Mokhtar, 8 years ago

Owner: changed from Duane Hilton to Emad Mokhtar
Status: newassigned

comment:10 by Tim Graham, 8 years ago

Component: DocumentationCore (Other)
Has patch: set
Needs tests: set

PR (currently lacks a test)

comment:11 by Tim Graham, 8 years ago

Needs tests: unset
Patch needs improvement: set

Left some comments for improvement and a few test failures remain.

comment:12 by Carl Meyer, 8 years ago

Owner: Emad Mokhtar removed
Status: assignednew

De-assigning to make it clear that this is available for a PyCon sprinter to finish up.

comment:13 by Tim Graham, 8 years ago

Easy pickings: unset

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.

comment:14 by Tim Graham <timograham@…>, 8 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In c4980e28:

Fixed #26290 -- Warned that paginating an unordered QuerySet may result in inconsistent results.

comment:15 by Simon Charette <charette.s@…>, 7 years ago

In c0f12a0:

Fixed #28109 -- Corrected the stack level of unordered queryset pagination warnings.

Refs #26290.

Thanks Tim for the review.

comment:16 by Simon Charette <charette.s@…>, 7 years ago

In 395df007:

[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

Note: See TracTickets for help on using tickets.
Back to Top