From 2ce5cb0f7a4618dfdc5f5c10e53e2e9b9543d298 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20Ml=C3=A1dek?= Date: Mon, 26 May 2025 18:37:34 +0200 Subject: [PATCH 1/3] Fixed #26434 -- Removed faulty clearing of ordering field when missing from explicit grouping. Co-authored-by: Simon Charette --- django/db/models/sql/query.py | 30 +++++++++++++++++++++++++++++- tests/aggregation_regress/tests.py | 18 ++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index 84950d4ec069..8fffce5dec91 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -534,7 +534,8 @@ def get_aggregation(self, using, aggregate_exprs): # Queries with distinct_fields need ordering and when a limit is # applied we must take the slice from the ordered query. Otherwise # no need for ordering. - inner_query.clear_ordering(force=False) + if inner_query.orderby_issubset_groupby: + inner_query.clear_ordering(force=False) if not inner_query.distinct: # If the inner query uses default select and it has some # aggregate annotations, then we must make sure the inner @@ -2338,6 +2339,33 @@ def add_ordering(self, *ordering): else: self.default_ordering = False + @property + def orderby_issubset_groupby(self): + if self.extra_order_by: + # Raw SQL from extra(order_by=...) can't be reliably compared + # against resolved OrderBy/Col expressions. Treat as not a subset. + return False + if self.group_by in (None, True): + # There is either no aggregation at all (None), or the group by + # is generated automatically from model fields (True), in which + # case the order by is necessarily a subset of them. + return True + if not self.order_by: + # Although an empty set is always a subset, there's no point in + # clearing ordering when there isn't any. Avoid the clone() below. + return True + # Don't pollute the original query (might disrupt joins). + q = self.clone() + order_by_set = { + ( + order_by.resolve_expression(q) + if hasattr(order_by, "resolve_expression") + else F(order_by).resolve_expression(q) + ) + for order_by in q.order_by + } + return order_by_set.issubset(self.group_by) + def clear_ordering(self, force=False, clear_default=True): """ Remove any ordering settings if the current query allows it without diff --git a/tests/aggregation_regress/tests.py b/tests/aggregation_regress/tests.py index 3e1a6a71f9ea..c7d9271dd8ae 100644 --- a/tests/aggregation_regress/tests.py +++ b/tests/aggregation_regress/tests.py @@ -171,6 +171,24 @@ def assertObjectAttrs(self, obj, **kwargs): for attr, value in kwargs.items(): self.assertEqual(getattr(obj, attr), value) + def test_count_preserve_group_by(self): + # new release of the same book + Book.objects.create( + isbn="113235613", + name=self.b4.name, + pages=self.b4.pages, + rating=4.0, + price=Decimal("39.69"), + contact=self.a5, + publisher=self.p3, + pubdate=datetime.date(2018, 11, 3), + ) + qs = Book.objects.values("contact__name", "publisher__name").annotate( + publications=Count("id") + ) + self.assertEqual(qs.order_by("id").count(), len(qs.order_by("id"))) + self.assertEqual(qs.extra(order_by=["id"]).count(), len(qs.order_by("id"))) + def test_annotation_with_value(self): values = ( Book.objects.filter( From 0071cb1efd559d721e5882da2e18556f867eed76 Mon Sep 17 00:00:00 2001 From: Jacob Walls Date: Thu, 20 Nov 2025 13:52:24 -0500 Subject: [PATCH 2/3] Refs #36652 -- Avoided missing imports in a temporary file generated in a migration test. --- tests/migrations/test_loader.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/migrations/test_loader.py b/tests/migrations/test_loader.py index efa7a9416661..705d49d06959 100644 --- a/tests/migrations/test_loader.py +++ b/tests/migrations/test_loader.py @@ -682,8 +682,11 @@ def test_loading_order_does_not_create_circular_dependency(self): with tempfile.NamedTemporaryFile( mode="w", encoding="utf-8", suffix=".py", dir=tests_dir, delete=False ) as test_settings: + self.addCleanup(os.remove, test_settings.name) for attr, value in settings._wrapped.__dict__.items(): - if attr.isupper(): + # Only write builtin values so that any settings that reference + # a value that needs an import are omitted. + if attr.isupper() and type(value).__module__ == "builtins": test_settings.write(f"{attr} = {value!r}\n") # Provide overrides here, instead of via decorators. test_settings.write(f"DATABASES = {settings.DATABASES}\n") @@ -693,8 +696,6 @@ def test_loading_order_does_not_create_circular_dependency(self): "INSTALLED_APPS=[a for a in INSTALLED_APPS if a.startswith('django')]\n" ) test_settings.write(f"INSTALLED_APPS += {INSTALLED_APPS}\n") - test_settings_name = test_settings.name - self.addCleanup(os.remove, test_settings_name) test_environ = os.environ.copy() test_environ["PYTHONPATH"] = str(tests_dir) @@ -709,7 +710,7 @@ def test_loading_order_does_not_create_circular_dependency(self): "app1", "--skip-checks", "--settings", - Path(test_settings_name).stem, + Path(test_settings.name).stem, ] try: subprocess.run( From 922c4cf972e04b1ce7ecee592231106724dcfd09 Mon Sep 17 00:00:00 2001 From: Marc Gibbons <1726961+marcgibbons@users.noreply.github.com> Date: Mon, 8 Dec 2025 09:42:43 -0500 Subject: [PATCH 3/3] Fixed #36783 -- Ensured proper handling of multi-value QueryDicts in querystring template tag. Co-authored-by: Jacob Walls Co-authored-by: Natalia <124304+nessita@users.noreply.github.com> --- django/template/defaulttags.py | 10 +++- docs/releases/6.0.1.txt | 4 +- .../syntax_tests/test_querystring.py | 54 +++++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/django/template/defaulttags.py b/django/template/defaulttags.py index ac3d5de901c8..3348e6426312 100644 --- a/django/template/defaulttags.py +++ b/django/template/defaulttags.py @@ -1296,6 +1296,10 @@ def querystring(context, *args, **kwargs): Keyword arguments are treated as an extra, final mapping. These mappings are processed sequentially, with later arguments taking precedence. + Passing `None` as a value removes the corresponding key from the result. + For iterable values, `None` entries are ignored, but if all values are + `None`, the key is removed. + A query string prefixed with `?` is returned. Raise TemplateSyntaxError if a positional argument is not a mapping or if @@ -1327,7 +1331,8 @@ def querystring(context, *args, **kwargs): "querystring requires mappings for positional arguments (got " "%r instead)." % d ) - for key, value in d.items(): + items = d.lists() if isinstance(d, QueryDict) else d.items() + for key, value in items: if not isinstance(key, str): raise TemplateSyntaxError( "querystring requires strings for mapping keys (got %r " @@ -1336,7 +1341,8 @@ def querystring(context, *args, **kwargs): if value is None: params.pop(key, None) elif isinstance(value, Iterable) and not isinstance(value, str): - params.setlist(key, value) + # Drop None values; if no values remain, the key is removed. + params.setlist(key, [v for v in value if v is not None]) else: params[key] = value query_string = params.urlencode() if params else "" diff --git a/docs/releases/6.0.1.txt b/docs/releases/6.0.1.txt index 946102030cb6..72c35b3e6952 100644 --- a/docs/releases/6.0.1.txt +++ b/docs/releases/6.0.1.txt @@ -9,4 +9,6 @@ Django 6.0.1 fixes several bugs in 6.0. Bugfixes ======== -* ... +* Fixed a regression in Django 6.0 where :ttag:`querystring` mishandled + multi-value :class:`~django.http.QueryDict` keys, both by only preserving the + last value and by incorrectly handling ``None`` values (:ticket:`36783`). diff --git a/tests/template_tests/syntax_tests/test_querystring.py b/tests/template_tests/syntax_tests/test_querystring.py index af8dbde95597..8e0722f0fa6d 100644 --- a/tests/template_tests/syntax_tests/test_querystring.py +++ b/tests/template_tests/syntax_tests/test_querystring.py @@ -58,6 +58,22 @@ def test_querystring_multiple(self): context = RequestContext(request) self.assertRenderEqual("querystring_multiple", context, expected="?x=y&a=b") + @setup({"querystring_multiple_lists": "{% querystring %}"}) + def test_querystring_multiple_lists(self): + request = self.request_factory.get("/", {"x": ["y", "z"], "a": ["b", "c"]}) + context = RequestContext(request) + expected = "?x=y&x=z&a=b&a=c" + self.assertRenderEqual("querystring_multiple_lists", context, expected=expected) + + @setup({"querystring_lists_with_replacement": "{% querystring a=1 %}"}) + def test_querystring_lists_with_replacement(self): + request = self.request_factory.get("/", {"x": ["y", "z"], "a": ["b", "c"]}) + context = RequestContext(request) + expected = "?x=y&x=z&a=1" + self.assertRenderEqual( + "querystring_lists_with_replacement", context, expected=expected + ) + @setup({"querystring_empty_params": "{% querystring qd %}"}) def test_querystring_empty_params(self): cases = [{}, QueryDict()] @@ -111,6 +127,44 @@ def test_querystring_remove_from_dict(self): context = RequestContext(request, {"my_dict": {"test": None}}) self.assertRenderEqual("querystring_remove_dict", context, expected="?a=1") + @setup({"querystring_remove_querydict": "{% querystring request my_query_dict %}"}) + def test_querystring_remove_querydict(self): + request = self.request_factory.get("/", {"x": "1"}) + my_qd = QueryDict(mutable=True) + my_qd["x"] = None + context = RequestContext( + request, {"request": request.GET, "my_query_dict": my_qd} + ) + self.assertRenderEqual("querystring_remove_querydict", context, expected="?") + + @setup( + {"querystring_remove_querydict_many": "{% querystring request my_query_dict %}"} + ) + def test_querystring_remove_querydict_many(self): + request = self.request_factory.get( + "/", {"test": ["value1", "value2"], "a": [1, 2]} + ) + + qd_none = QueryDict(mutable=True) + qd_none["test"] = None + + qd_list_none = QueryDict(mutable=True) + qd_list_none.setlist("test", [None, None]) + + qd_empty_list = QueryDict(mutable=True) + qd_empty_list.setlist("test", []) + + for qd in (qd_none, qd_list_none, qd_empty_list): + with self.subTest(my_query_dict=qd): + context = RequestContext( + request, {"request": request.GET, "my_query_dict": qd} + ) + self.assertRenderEqual( + "querystring_remove_querydict_many", + context, + expected="?a=1&a=2", + ) + @setup({"querystring_variable": "{% querystring a=a %}"}) def test_querystring_variable(self): request = self.request_factory.get("/")