Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#35665 closed Bug (fixed)

Prefetch() fails with "OrderByList" when queryset has no order_by and is sliced

Reported by: Andrew Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 5.1
Severity: Release blocker Keywords: prefetch slice order_by
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

In v5.1 the Prefetch operator causes an issue when no order_by() is specified, and the query contains a slice.

  • Must have no order by (either explict order_by() or model Meta contains no order by)
  • Must be sliced - any form works, :10, 10:, 10:20

Does not crash in v5.0.7. I did not see this mentioned in the Prefetch documentation, or the changelog.

short_list = Prefetch(
    "products",
    queryset=Product.objects.order_by()[:10],
    to_attr="short_list",
)
query = Store.objects.prefetch_related(short_list)
print(query) # errors

The stack trace:

  File "python3.12/site-packages/django/db/models/expressions.py", line 1896, in __init__
    self.order_by = OrderByList(*self.order_by)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "python3.12/site-packages/django/db/models/expressions.py", line 1414, in __init__
    super().__init__(*expressions, **extra)
  File "python3.12/site-packages/django/db/models/expressions.py", line 1382, in __init__
    raise ValueError(
ValueError: OrderByList requires at least one expression.

I was able to replicate this in a clean project, against sqlite, with the following 2 simple models, used in the example above:

class Store(models.Model):
    name = models.CharField(max_length=100)

class Product(models.Model):
    store = models.ForeignKey(Store, on_delete=models.CASCADE, related_name='products')
    name = models.CharField(max_length=100)
    class Meta:
        ordering = ['name']

Change History (6)

comment:1 by Simon Charette, 4 months ago

Keywords: order_by added
Owner: set to Simon Charette
Severity: NormalRelease blocker
Status: newassigned
Triage Stage: UnreviewedAccepted

I managed to reproduce and the issue the issue is relatively simple to address.

It should obviously not crash but just so you know using slicing without ordering generally makes little sense as the backend is allowed to return rows in any order.

comment:2 by Simon Charette, 4 months ago

Has patch: set

in reply to:  2 comment:3 by Andrew, 4 months ago

Replying to Simon Charette:

Edit: Thanks for the super fast fix. I noticed just now that you said "Supporting explicit empty ordering on Window functions and slicing" and I wanted to call this bit out, as it's a *bit* less of a footgun, as it could be the default and was my actual original issue.

Yes, it returns them in database order, which is acceptable in this case.

The original issue was a Model having no Meta.ordering. Since both cause the issue, and we require order_by() to be explicit at the query level, I used the .order_by() example. The following will also fail, as the products model has no default ordering:

short_list = Prefetch(
    "products",
    queryset=Product.objects.all()[:10],
    to_attr="short_list",
)
class Product(models.Model):
    class Meta:
        # ordering = ['name']       
        verbose_name_plural = "Products"
Version 3, edited 4 months ago by Andrew (previous) (next) (diff)

comment:4 by Sarah Boyce, 4 months ago

Triage Stage: AcceptedReady for checkin

comment:5 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

Resolution: fixed
Status: assignedclosed

In 602fe96:

Fixed #35665 -- Fixed a crash when passing an empty order_by to Window.

This also caused un-ordered sliced prefetches to crash as they rely on Window.

Regression in e16d0c176e9b89628cdec5e58c418378c4a2436a that made OrderByList
piggy-back ExpressionList without porting the empty handling that the latter
provided.

Supporting explicit empty ordering on Window functions and slicing is arguably
a foot-gun design due to how backends will return undeterministic results but
this is a problem that requires a larger discussion.

Refs #35064.

Thanks Andrew Backer for the report and Mariusz for the review.

comment:6 by Sarah Boyce <42296566+sarahboyce@…>, 4 months ago

In df236b0:

[5.1.x] Fixed #35665 -- Fixed a crash when passing an empty order_by to Window.

This also caused un-ordered sliced prefetches to crash as they rely on Window.

Regression in e16d0c176e9b89628cdec5e58c418378c4a2436a that made OrderByList
piggy-back ExpressionList without porting the empty handling that the latter
provided.

Supporting explicit empty ordering on Window functions and slicing is arguably
a foot-gun design due to how backends will return undeterministic results but
this is a problem that requires a larger discussion.

Refs #35064.

Thanks Andrew Backer for the report and Mariusz for the review.

Backport of 602fe961e6834d665f2359087a1272e9f9806b71 from main.

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