From 602fe961e6834d665f2359087a1272e9f9806b71 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Fri, 9 Aug 2024 08:43:20 -0400 Subject: [PATCH] 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. --- django/db/backends/oracle/features.py | 1 + django/db/models/expressions.py | 12 +++++------- docs/releases/5.1.1.txt | 4 +++- tests/expressions/tests.py | 5 ----- tests/expressions_window/tests.py | 14 ++++++++++++++ tests/prefetch_related/tests.py | 15 +++++++++++++++ 6 files changed, 38 insertions(+), 13 deletions(-) diff --git a/django/db/backends/oracle/features.py b/django/db/backends/oracle/features.py index a83560b8929..72c6180f50c 100644 --- a/django/db/backends/oracle/features.py +++ b/django/db/backends/oracle/features.py @@ -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." diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index eee0eafc837..154e684ff04 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -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) diff --git a/docs/releases/5.1.1.txt b/docs/releases/5.1.1.txt index f307b2a0ee8..25c0b4c297c 100644 --- a/docs/releases/5.1.1.txt +++ b/docs/releases/5.1.1.txt @@ -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`). diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py index e9e41cff1eb..75aa1b0894e 100644 --- a/tests/expressions/tests.py +++ b/tests/expressions/tests.py @@ -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) diff --git a/tests/expressions_window/tests.py b/tests/expressions_window/tests.py index fd674e319bc..fd9858ccf9b 100644 --- a/tests/expressions_window/tests.py +++ b/tests/expressions_window/tests.py @@ -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( diff --git a/tests/prefetch_related/tests.py b/tests/prefetch_related/tests.py index a418beb5a52..9b1dfdd0d1a 100644 --- a/tests/prefetch_related/tests.py +++ b/tests/prefetch_related/tests.py @@ -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):