From df236b0bcbbf1f54dfe6acc7761cd81b76ebf2cc Mon Sep 17 00:00:00 2001
From: Simon Charette <charette.s@gmail.com>
Date: Fri, 9 Aug 2024 08:43:20 -0400
Subject: [PATCH] [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.
---
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 a83560b892..72c6180f50 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 0f5e68b0d4..c24c585658 100644
--- a/django/db/models/expressions.py
+++ b/django/db/models/expressions.py
@@ -1377,16 +1377,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 f307b2a0ee..25c0b4c297 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 64103f14db..5bc98eed4d 100644
--- a/tests/expressions/tests.py
+++ b/tests/expressions/tests.py
@@ -2311,11 +2311,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 fd674e319b..fd9858ccf9 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 a418beb5a5..9b1dfdd0d1 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):