[Django] #22550: QuerySet last() and reverse() confusing when used with ordered slices
Django
----------------------------------------------+--------------------
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
Component: Database layer (models, ORM) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
I ran all tests below with MySQL backend.
Given the following model:
{{{
class MyModel(models.Model):
name = models.CharField(max_length=100)
class Meta:
ordering = ['name']
def __unicode__(self):
return self.name
}}}
From the following test script I receive the output:
{{{
import os
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "djtest.settings")
from myapp.models import MyModel
MyModel.objects.all().delete()
for i in range(10):
m = MyModel(name='model%d' % i)
m.save()
print MyModel.objects.all()[0:5]
print MyModel.objects.all()[0:5].last()
print MyModel.objects.all()[0:5].reverse()
}}}
{{{
[<MyModel: model0>, <MyModel: model1>, <MyModel: model2>, <MyModel:
model3>, <MyModel: model4>]
model9
[<MyModel: model9>, <MyModel: model8>, <MyModel: model7>, <MyModel:
model6>, <MyModel: model5>, <MyModel: model4>, <MyModel: model3>,
<MyModel: model2>, <MyModel: model1>, <MyModel: model0>]
}}}
In my opinion, the lines that show the output of {{{last()}}} and
{{{reverse()}}} are confusing. My naive expectation was that each of these
would be operating on the slice. That is, the last value from the slice or
the reverse order of the slice.
{{{
model4
[<MyModel: model4>, <MyModel: model3>, ...
}}}
Of course, after thinking about how this might get translated to SQL I
understand why this happens. Making this a leaky abstraction. In my
opinion, if expected behavior can't be realized an exception preventing
the operation makes more sense.
The original reason I bumped into this is I was creating a custom
Paginator to create page labels based on the first and last item on the
page. As Paginator uses slices, calling {{{last()}}} produced unexpected
(to me) results.
--
Ticket URL: <https://code.djangoproject.com/ticket/22550>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jdufresne | Owner: nobody
Type: Bug | Status: new
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by merll):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
When retrying this using SQLite in the current master, additional sorting
on the sliced queryset returns the same order. `last()` returns the first
element from the default order, just like `first()`.
Anyhow, as the queryset is already sliced, it can no longer be filtered or
sorted. Explicit sorting with `order_by()` or using `reverse()`/`last()`
on a sliced, unordered queryset raises an error message pointing this out.
For consistency, the latter should also be implemented for a sliced
queryset with a default sort order.
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:1>
Django
Reporter: jdufresne | Owner: merll
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by merll):
* owner: nobody => merll
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:2>
Django
Reporter: jdufresne | Owner: merll
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by merll):
* has_patch: 0 => 1
Comment:
A PR has been sent, which fixes the behavior as described in ticket 22550.
It also includes an addition to the docs, describing that sorting and
filtering are not possible after the slicing operation:
https://github.com/django/django/pull/2677
'''Note:''' It is potentially breaking backwards compatibility in
`QuerySet.last()` and `QuerySet.reverse()`. Using `reverse()` and `last()`
on querysets was programmatically possible even after they had been
sliced, provided that
* models have defined `ordering`,
* no further filters or sorting was used.
Although it has never delivered correct nor consistent results, these two
functions may have been used under aforementioned conditions. After
applying this patch, this will raise an error. In this process, the test
for ticket 7235 had to be revised, as it was relying on the wrong order of
operations.
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:3>
Django
Reporter: jdufresne | Owner: merll
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
I left comments for improvement on PR. Please uncheck "Patch needs
improvement" when you update it, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:4>
Django
Reporter: Jon Dufresne | Owner: Matthias
| Erll
Type: Bug | Status: assigned
Component: Database layer | Version: 1.6
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Michael Käufl):
I've sent a new PR: https://github.com/django/django/pull/8060
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:5>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Käufl):
* owner: Matthias Erll => Michael Käufl
* needs_better_patch: 1 => 0
* version: 1.6 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:6>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:7>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Michael Käufl):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:8>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Claude Paroz):
#27997 was closed as a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:9>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael Käufl):
@Tim: Can you give my pull request another review?
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:10>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Type: Bug | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:11>
Django
Reporter: Jon Dufresne | Owner: Michael
| Käufl
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"eee34ef64c026e3274c6d1b4fa2baffbc956b954" eee34ef]:
{{{
#!CommitTicketReference repository=""
revision="eee34ef64c026e3274c6d1b4fa2baffbc956b954"
Fixed #22550 -- Prohibited QuerySet.last()/reverse() after slicing.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22550#comment:12>