Opened 11 years ago

Closed 8 years ago

#22550 closed Bug (fixed)

QuerySet last() and reverse() confusing when used with ordered slices

Reported by: Jon Dufresne Owned by: Michael Käufl
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (12)

comment:1 by Matthias Erll, 11 years ago

Triage Stage: UnreviewedAccepted

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.

comment:2 by Matthias Erll, 11 years ago

Owner: changed from nobody to Matthias Erll
Status: newassigned

comment:3 by Matthias Erll, 11 years ago

Has patch: set

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.

comment:4 by Tim Graham, 10 years ago

Patch needs improvement: set

I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.

comment:6 by Michael Käufl, 8 years ago

Owner: changed from Matthias Erll to Michael Käufl
Patch needs improvement: unset
Version: 1.6master

comment:7 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:8 by Michael Käufl, 8 years ago

Patch needs improvement: unset

comment:9 by Claude Paroz, 8 years ago

#27997 was closed as a duplicate.

comment:10 by Michael Käufl, 8 years ago

@Tim: Can you give my pull request another review?

comment:11 by Mariusz Felisiak, 8 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In eee34ef:

Fixed #22550 -- Prohibited QuerySet.last()/reverse() after slicing.

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