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 e16d0c176e 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.
This commit is contained in:
Simon Charette 2024-08-09 08:43:20 -04:00 committed by Sarah Boyce
parent 5ae9922666
commit 602fe961e6
6 changed files with 38 additions and 13 deletions

View File

@ -116,6 +116,7 @@ class DatabaseFeatures(BaseDatabaseFeatures):
"Oracle requires ORDER BY in row_number, ANSI:SQL doesn't.": {
"expressions_window.tests.WindowFunctionTests."
"test_row_number_no_ordering",
"prefetch_related.tests.PrefetchLimitTests.test_empty_order",
},
"Oracle doesn't support changing collations on indexed columns (#33671).": {
"migrations.test_operations.OperationTests."

View File

@ -1425,16 +1425,14 @@ class ExpressionList(Func):
template = "%(expressions)s"
def __init__(self, *expressions, **extra):
if not expressions:
raise ValueError(
"%s requires at least one expression." % self.__class__.__name__
)
super().__init__(*expressions, **extra)
def __str__(self):
return self.arg_joiner.join(str(arg) for arg in self.source_expressions)
def as_sql(self, *args, **kwargs):
if not self.source_expressions:
return "", ()
return super().as_sql(*args, **kwargs)
def as_sqlite(self, compiler, connection, **extra_context):
# Casting to numeric is unnecessary.
return self.as_sql(compiler, connection, **extra_context)

View File

@ -9,4 +9,6 @@ Django 5.1.1 fixes several bugs in 5.1.
Bugfixes
========
* ...
* Fixed a regression in Django 5.1 that caused a crash of ``Window()`` when
passing an empty sequence to the ``order_by`` parameter, and a crash of
``Prefetch()`` for a sliced queryset without ordering (:ticket:`35665`).

View File

@ -2315,11 +2315,6 @@ class ValueTests(TestCase):
self.assertNotEqual(value, other_value)
self.assertNotEqual(value, no_output_field)
def test_raise_empty_expressionlist(self):
msg = "ExpressionList requires at least one expression"
with self.assertRaisesMessage(ValueError, msg):
ExpressionList()
def test_compile_unresolved(self):
# This test might need to be revisited later on if #25425 is enforced.
compiler = Time.objects.all().query.get_compiler(connection=connection)

View File

@ -928,6 +928,20 @@ class WindowFunctionTests(TestCase):
),
)
def test_empty_ordering(self):
"""
Explicit empty ordering makes little sense but it is something that
was historically allowed.
"""
qs = Employee.objects.annotate(
sum=Window(
expression=Sum("salary"),
partition_by="department",
order_by=[],
)
).order_by("department", "sum")
self.assertEqual(len(qs), 12)
def test_related_ordering_with_count(self):
qs = Employee.objects.annotate(
department_sum=Window(

View File

@ -1999,6 +1999,21 @@ class PrefetchLimitTests(TestDataMixin, TestCase):
with self.assertRaisesMessage(NotSupportedError, msg):
list(Book.objects.prefetch_related(Prefetch("authors", authors[1:])))
@skipUnlessDBFeature("supports_over_clause")
def test_empty_order(self):
authors = Author.objects.order_by()
with self.assertNumQueries(3):
books = list(
Book.objects.prefetch_related(
Prefetch("authors", authors),
Prefetch("authors", authors[:1], to_attr="authors_sliced"),
)
)
for book in books:
with self.subTest(book=book):
self.assertEqual(len(book.authors_sliced), 1)
self.assertIn(book.authors_sliced[0], list(book.authors.all()))
class DeprecationTests(TestCase):
def test_get_current_queryset_warning(self):