Opened 14 months ago

Last modified 13 months ago

#22550 assigned Bug

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

Reported by: jdufresne Owned by: merll
Component: Database layer (models, ORM) Version: 1.6
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (4)

comment:1 Changed 14 months ago by merll

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 14 months ago by merll

  • Owner changed from nobody to merll
  • Status changed from new to assigned

comment:3 Changed 14 months ago by merll

  • 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 Changed 13 months ago by timo

  • Patch needs improvement set

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

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