From a352357cd08e82184065e4d337b2a25ceb994952 Mon Sep 17 00:00:00 2001 From: Johannes Westphal Date: Tue, 14 Oct 2025 19:40:24 +0100 Subject: [PATCH 1/6] Add test for django 5.2 broken aggregate queries --- tests/test_cte.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/test_cte.py b/tests/test_cte.py index 1c28d4c..3984cc6 100644 --- a/tests/test_cte.py +++ b/tests/test_cte.py @@ -711,3 +711,14 @@ def test_django52_ambiguous_column_names(self): ('venus', 22, "admin"), ('venus', 23, "admin"), ]) + + def test_django52_queryset_aggregates_klass_error(self): + cte = CTE( + Order.objects.annotate(user_name=F("user__name")) + .values("user_name") + .annotate(c=Count("user_name")) + .values("user_name", "c") + ) + qs = with_cte(cte, select=cte) + # Executing the query should not raise TypeError: 'NoneType' object is not subscriptable + self.assertEqual(list(qs), [{"user_name": "admin", "c": 22}]) From d952ce202fd9bdb3f8841ce59ef1b0d2faa905ab Mon Sep 17 00:00:00 2001 From: Johannes Westphal Date: Tue, 14 Oct 2025 19:57:33 +0100 Subject: [PATCH 2/6] Add test for allowing reannotation of any fields when querying a CTE --- tests/test_cte.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/test_cte.py b/tests/test_cte.py index 3984cc6..a31e972 100644 --- a/tests/test_cte.py +++ b/tests/test_cte.py @@ -722,3 +722,26 @@ def test_django52_queryset_aggregates_klass_error(self): qs = with_cte(cte, select=cte) # Executing the query should not raise TypeError: 'NoneType' object is not subscriptable self.assertEqual(list(qs), [{"user_name": "admin", "c": 22}]) + + def test_django52_annotate_model_field_name_after_queryset(self): + # Select the `id` field in one CTE + cte = CTE(Order.objects.values("id", "region", "user_id")) + # In the next query, when querying from the CTE we reassign the `id` field + # Previously, this would have thrown an exception + qs = ( + with_cte(cte, select=cte) + .annotate(id=F('user_id')) + .values_list('id', 'region') + .order_by('id', 'region') + .distinct() + ) + self.assertEqual(list(qs), [ + (1, 'earth'), + (1, 'mars'), + (1, 'mercury'), + (1, 'moon'), + (1, 'proxima centauri'), + (1, 'proxima centauri b'), + (1, 'sun'), + (1, 'venus'), + ]) From e69b1b3deae7222a61059659819a2b1bfc2670e0 Mon Sep 17 00:00:00 2001 From: Johannes Westphal Date: Tue, 14 Oct 2025 20:03:10 +0100 Subject: [PATCH 3/6] Allow any field names to be reannotated when querying from a CTE --- django_cte/cte.py | 1 + 1 file changed, 1 insertion(+) diff --git a/django_cte/cte.py b/django_cte/cte.py index 11743df..07d8d53 100644 --- a/django_cte/cte.py +++ b/django_cte/cte.py @@ -124,6 +124,7 @@ def queryset(self): """ cte_query = self.query qs = cte_query.model._default_manager.get_queryset() + qs._fields = () # Allow any field names to be used in further annotations query = jit_mixin(sql.Query(cte_query.model), CTEQuery) query.join(BaseTable(self.name, None)) From e2717983cee414a1cad35e42881fa07ece2c018d Mon Sep 17 00:00:00 2001 From: Johannes Westphal Date: Tue, 14 Oct 2025 19:50:13 +0100 Subject: [PATCH 4/6] Add tests for django 5.2 queryset() after values_list() in CTE --- tests/test_cte.py | 49 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/test_cte.py b/tests/test_cte.py index a31e972..5092cfc 100644 --- a/tests/test_cte.py +++ b/tests/test_cte.py @@ -1,4 +1,5 @@ import pytest +import django from django.db.models import IntegerField, TextField from django.db.models.aggregates import Count, Max, Min, Sum from django.db.models.expressions import ( @@ -745,3 +746,51 @@ def test_django52_annotate_model_field_name_after_queryset(self): (1, 'sun'), (1, 'venus'), ]) + + @pytest.mark.skipif(django.VERSION < (5, 2), reason="Requires Django 5.2+") + def test_queryset_after_values_list(self): + cte = CTE(Order.objects.values_list("region", "amount").order_by("region", "amount")) + qs = with_cte(cte, select=cte) + self.assertEqual(list(qs), [ + ('earth', 30), + ('earth', 31), + ('earth', 32), + ('earth', 33), + ('mars', 40), + ('mars', 41), + ('mars', 42), + ('mercury', 10), + ('mercury', 11), + ('mercury', 12), + ('moon', 1), + ('moon', 2), + ('moon', 3), + ('proxima centauri', 2000), + ('proxima centauri b', 10), + ('proxima centauri b', 11), + ('proxima centauri b', 12), + ('sun', 1000), + ('venus', 20), + ('venus', 21), + ('venus', 22), + ('venus', 23), + ]) + + @pytest.mark.skipif(django.VERSION < (5, 2), reason="Requires Django 5.2+") + def test_queryset_after_values_list_flat(self): + cte = CTE( + Order.objects.values_list("region", flat=True) + .order_by("region") + .distinct() + ) + qs = with_cte(cte, select=cte) + self.assertEqual(list(qs), [ + 'earth', + 'mars', + 'mercury', + 'moon', + 'proxima centauri', + 'proxima centauri b', + 'sun', + 'venus' + ]) From 30b8dcc04deba3131cc14b46b60298ef31fb5940 Mon Sep 17 00:00:00 2001 From: Johannes Westphal Date: Tue, 14 Oct 2025 19:52:32 +0100 Subject: [PATCH 5/6] Add tests for django 5.2 correct field order when selecting from CTE --- tests/test_cte.py | 51 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/test_cte.py b/tests/test_cte.py index 5092cfc..d17e222 100644 --- a/tests/test_cte.py +++ b/tests/test_cte.py @@ -5,6 +5,7 @@ from django.db.models.expressions import ( Exists, ExpressionWrapper, F, OuterRef, Subquery, ) +from django.db.models.query import ValuesListIterable from django.db.models.sql.constants import LOUTER from django.db.utils import OperationalError, ProgrammingError from django.test import TestCase @@ -794,3 +795,53 @@ def test_queryset_after_values_list_flat(self): 'sun', 'venus' ]) + + @pytest.mark.skipif(django.VERSION < (5, 2), reason="Requires Django 5.2+") + def test_queryset_values_list_order1(self): + cte = CTE( + Order.objects.values("region") + .annotate(c=Count("region")) + .values_list("c", "region") + .order_by("region") + ) + qs = with_cte(cte, select=cte) + # Use the `ValuesListIterable` for being able to inspect the order of the fields + # in the resulting query + qs._iterable_class = ValuesListIterable + # Ensure the column order of queried fields is the specified one: c, region + # Before the fix, the order would have been this one: region, c + self.assertEqual(list(qs), [ + (4, 'earth'), + (3, 'mars'), + (3, 'mercury'), + (3, 'moon'), + (1, 'proxima centauri'), + (3, 'proxima centauri b'), + (1, 'sun'), + (4, 'venus'), + ]) + + @pytest.mark.skipif(django.VERSION < (5, 2), reason="Requires Django 5.2+") + def test_queryset_values_list_order2(self): + cte = CTE( + Order.objects.values("region") + .annotate(r=F("region"), c=Count("region")) + .values_list("c", "r") + .order_by("r") + ) + qs = with_cte(cte, select=cte) + # Use the `ValuesListIterable` for being able to inspect the order of the fields + # in the resulting query + qs._iterable_class = ValuesListIterable + # Ensure the column order of queried fields is the specified one: c, r + # Before the fix, the order would have been this one: r, c + self.assertEqual(list(qs), [ + (4, 'earth'), + (3, 'mars'), + (3, 'mercury'), + (3, 'moon'), + (1, 'proxima centauri'), + (3, 'proxima centauri b'), + (1, 'sun'), + (4, 'venus'), + ]) From 67353a7b2b53ce38110850205b513b1379580e6d Mon Sep 17 00:00:00 2001 From: Johannes Westphal Date: Tue, 14 Oct 2025 20:09:10 +0100 Subject: [PATCH 6/6] Propagate correct values_list() when querying from a CTE --- django_cte/cte.py | 37 ++++++++++++++++++++++++++----------- tests/test_cte.py | 7 ------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/django_cte/cte.py b/django_cte/cte.py index 07d8d53..7faad61 100644 --- a/django_cte/cte.py +++ b/django_cte/cte.py @@ -1,5 +1,6 @@ from copy import copy +import django from django.db.models import Manager, sql from django.db.models.expressions import Ref from django.db.models.query import Q, QuerySet, ValuesIterable @@ -45,21 +46,30 @@ class CTE: """ def __init__(self, queryset, name="cte", materialized=False): - self.query = None if queryset is None else queryset.query + self._set_queryset(queryset) self.name = name self.col = CTEColumns(self) self.materialized = materialized def __getstate__(self): - return (self.query, self.name, self.materialized) + return (self.query, self.name, self.materialized, self._iterable_class) def __setstate__(self, state): - self.query, self.name, self.materialized = state + if len(state) == 3: + # Keep compatibility with the previous serialization method + self.query, self.name, self.materialized = state + self._iterable_class = ValuesIterable + else: + self.query, self.name, self.materialized, self._iterable_class = state self.col = CTEColumns(self) def __repr__(self): return f"<{type(self).__name__} {self.name}>" + def _set_queryset(self, queryset): + self.query = None if queryset is None else queryset.query + self._iterable_class = getattr(queryset, "_iterable_class", ValuesIterable) + @classmethod def recursive(cls, make_cte_queryset, name="cte", materialized=False): """Recursive Common Table Expression @@ -73,7 +83,7 @@ def recursive(cls, make_cte_queryset, name="cte", materialized=False): :returns: The fully constructed recursive cte object. """ cte = cls(None, name, materialized) - cte.query = make_cte_queryset(cte).query + cte._set_queryset(make_cte_queryset(cte)) return cte def join(self, model_or_queryset, *filter_q, **filter_kw): @@ -124,25 +134,30 @@ def queryset(self): """ cte_query = self.query qs = cte_query.model._default_manager.get_queryset() + qs._iterable_class = self._iterable_class qs._fields = () # Allow any field names to be used in further annotations query = jit_mixin(sql.Query(cte_query.model), CTEQuery) query.join(BaseTable(self.name, None)) query.default_cols = cte_query.default_cols query.deferred_loading = cte_query.deferred_loading - if cte_query.values_select: + + if django.VERSION < (5, 2) and cte_query.values_select: query.set_values(cte_query.values_select) - qs._iterable_class = ValuesIterable + if cte_query.annotations: for alias, value in cte_query.annotations.items(): col = CTEColumnRef(alias, self.name, value.output_field) query.add_annotation(col, alias) query.annotation_select_mask = cte_query.annotation_select_mask - for alias in getattr(cte_query, "selected", None) or (): - if alias not in cte_query.annotations: - output_field = cte_query.resolve_ref(alias).output_field - col = CTEColumnRef(alias, self.name, output_field) - query.add_annotation(col, alias) + + if selected := getattr(cte_query, "selected", None): + for alias in selected: + if alias not in cte_query.annotations: + output_field = cte_query.resolve_ref(alias).output_field + col = CTEColumnRef(alias, self.name, output_field) + query.add_annotation(col, alias) + query.selected = {alias: alias for alias in selected} qs.query = query return qs diff --git a/tests/test_cte.py b/tests/test_cte.py index d17e222..f3831a9 100644 --- a/tests/test_cte.py +++ b/tests/test_cte.py @@ -5,7 +5,6 @@ from django.db.models.expressions import ( Exists, ExpressionWrapper, F, OuterRef, Subquery, ) -from django.db.models.query import ValuesListIterable from django.db.models.sql.constants import LOUTER from django.db.utils import OperationalError, ProgrammingError from django.test import TestCase @@ -805,9 +804,6 @@ def test_queryset_values_list_order1(self): .order_by("region") ) qs = with_cte(cte, select=cte) - # Use the `ValuesListIterable` for being able to inspect the order of the fields - # in the resulting query - qs._iterable_class = ValuesListIterable # Ensure the column order of queried fields is the specified one: c, region # Before the fix, the order would have been this one: region, c self.assertEqual(list(qs), [ @@ -830,9 +826,6 @@ def test_queryset_values_list_order2(self): .order_by("r") ) qs = with_cte(cte, select=cte) - # Use the `ValuesListIterable` for being able to inspect the order of the fields - # in the resulting query - qs._iterable_class = ValuesListIterable # Ensure the column order of queried fields is the specified one: c, r # Before the fix, the order would have been this one: r, c self.assertEqual(list(qs), [