From 57c50d8c1996733cef45204ea069a2c01b2860cc Mon Sep 17 00:00:00 2001 From: Tim Graham Date: Thu, 20 Nov 2025 20:18:09 -0500 Subject: [PATCH 1/2] Refs #21961 -- Added DatabaseFeatures.supports_on_delete_db_(cascade/null) feature flags. Needed on MongoDB. Co-Authored-By: Mariusz Felisiak --- django/db/backends/base/features.py | 4 +- django/db/models/fields/related.py | 55 +++++++++----- docs/ref/checks.txt | 3 +- tests/admin_utils/models.py | 3 + tests/admin_utils/tests.py | 3 +- tests/delete/models.py | 26 +++---- tests/delete/tests.py | 18 +++-- tests/inspectdb/models.py | 6 ++ tests/inspectdb/tests.py | 6 +- tests/introspection/models.py | 11 ++- tests/introspection/tests.py | 21 +++++- .../test_relative_fields.py | 73 +++++++++++++++++++ 12 files changed, 181 insertions(+), 48 deletions(-) diff --git a/django/db/backends/base/features.py b/django/db/backends/base/features.py index ecc283ff6ba5..1e957d105ed3 100644 --- a/django/db/backends/base/features.py +++ b/django/db/backends/base/features.py @@ -393,8 +393,10 @@ class BaseDatabaseFeatures: # subqueries? supports_tuple_comparison_against_subquery = True - # Does the backend support DEFAULT as delete option? + # Does the backend support CASCADE, DEFAULT, NULL as delete options? + supports_on_delete_db_cascade = True supports_on_delete_db_default = True + supports_on_delete_db_null = True # Collation names for use by the Django test suite. test_collations = { diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index 0293c7890991..f1bc664007dd 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -12,6 +12,7 @@ from django.db.models.constants import LOOKUP_SEP from django.db.models.deletion import ( CASCADE, + DB_CASCADE, DB_SET_DEFAULT, DB_SET_NULL, DO_NOTHING, @@ -1056,9 +1057,38 @@ def check(self, **kwargs): *self._check_unique(), ] + def _check_on_delete_db_support(self, on_delete, feature_flag, databases): + for db in databases: + if not router.allow_migrate_model(db, self.model): + continue + connection = connections[db] + if feature_flag in self.model._meta.required_db_features or getattr( + connection.features, feature_flag + ): + continue + no_db_option_name = on_delete.__name__.removeprefix("DB_") + yield checks.Error( + f"{connection.display_name} does not support a {on_delete.__name__}.", + hint=f"Change the on_delete rule to {no_db_option_name}.", + obj=self, + id="fields.E324", + ) + def _check_on_delete(self, databases): on_delete = getattr(self.remote_field, "on_delete", None) errors = [] + if on_delete == DB_CASCADE: + errors.extend( + self._check_on_delete_db_support( + on_delete, "supports_on_delete_db_cascade", databases + ) + ) + if on_delete == DB_SET_NULL: + errors.extend( + self._check_on_delete_db_support( + on_delete, "supports_on_delete_db_null", databases + ) + ) if on_delete in [DB_SET_NULL, SET_NULL] and not self.null: errors.append( checks.Error( @@ -1092,25 +1122,12 @@ def _check_on_delete(self, databases): id="fields.E322", ) ) - for db in databases: - if not router.allow_migrate_model(db, self.model): - continue - connection = connections[db] - if not ( - "supports_on_delete_db_default" - in self.model._meta.required_db_features - or connection.features.supports_on_delete_db_default - ): - errors.append( - checks.Error( - f"{connection.display_name} does not support a " - "DB_SET_DEFAULT.", - hint="Change the on_delete rule to SET_DEFAULT.", - obj=self, - id="fields.E324", - ), - ) - elif not isinstance(self.remote_field.model, str) and on_delete != DO_NOTHING: + errors.extend( + self._check_on_delete_db_support( + on_delete, "supports_on_delete_db_default", databases + ) + ) + if not isinstance(self.remote_field.model, str) and on_delete != DO_NOTHING: # Database and Python variants cannot be mixed in a chain of # model references. is_db_on_delete = isinstance(on_delete, DatabaseOnDelete) diff --git a/docs/ref/checks.txt b/docs/ref/checks.txt index 22ebfed2ca4e..7dfa7c61d24b 100644 --- a/docs/ref/checks.txt +++ b/docs/ref/checks.txt @@ -309,7 +309,8 @@ Related fields ``db_default`` value. * **fields.E323**: Field specifies database/Python-level on_delete variant, but referenced model uses python/database-level variant. -* **fields.E324**: ```` does not support ``DB_SET_DEFAULT``. +* **fields.E324**: ```` does not support + ````. * **fields.E330**: ``ManyToManyField``\s cannot be unique. * **fields.E331**: Field specifies a many-to-many relation through model ````, which has not been installed. diff --git a/tests/admin_utils/models.py b/tests/admin_utils/models.py index e5d2b6788722..55810f0b50a8 100644 --- a/tests/admin_utils/models.py +++ b/tests/admin_utils/models.py @@ -55,6 +55,9 @@ class DBCascade(models.Model): def __str__(self): return str(self.num) + class Meta: + required_db_features = {"supports_on_delete_db_cascade"} + class Event(models.Model): date = models.DateTimeField(auto_now_add=True) diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py index ce32535c52f0..81c6f495f8ef 100644 --- a/tests/admin_utils/tests.py +++ b/tests/admin_utils/tests.py @@ -21,7 +21,7 @@ from django.contrib.auth.templatetags.auth import render_password_as_hash from django.core.validators import EMPTY_VALUES from django.db import DEFAULT_DB_ALIAS, models -from django.test import SimpleTestCase, TestCase, override_settings +from django.test import SimpleTestCase, TestCase, override_settings, skipUnlessDBFeature from django.test.utils import isolate_apps from django.utils.formats import localize from django.utils.safestring import mark_safe @@ -115,6 +115,7 @@ def test_relation_on_abstract(self): n.collect([Vehicle.objects.first()]) +@skipUnlessDBFeature("supports_on_delete_db_cascade") class DBNestedObjectsTests(NestedObjectsTests): """ Exercise NestedObjectsTests but with a model that makes use of DB_CASCADE diff --git a/tests/delete/models.py b/tests/delete/models.py index bd9caf42a7ae..d8f81827be66 100644 --- a/tests/delete/models.py +++ b/tests/delete/models.py @@ -49,17 +49,26 @@ class RelatedDbOptionParent(models.Model): p = models.ForeignKey(RelatedDbOptionGrandParent, models.DB_CASCADE, null=True) -class RelatedDbOption(models.Model): +class CascadeDbModel(models.Model): name = models.CharField(max_length=30) + db_cascade = models.ForeignKey( + RelatedDbOptionParent, models.DB_CASCADE, related_name="db_cascade_set" + ) + + class Meta: + required_db_features = {"supports_on_delete_db_cascade"} + + +class SetNullDbModel(models.Model): db_setnull = models.ForeignKey( RelatedDbOptionParent, models.DB_SET_NULL, null=True, related_name="db_setnull_set", ) - db_cascade = models.ForeignKey( - RelatedDbOptionParent, models.DB_CASCADE, related_name="db_cascade_set" - ) + + class Meta: + required_db_features = {"supports_on_delete_db_null"} class SetDefaultDbModel(models.Model): @@ -159,15 +168,6 @@ def create_a(name): return a -def create_related_db_option(name): - a = RelatedDbOption(name=name) - for name in ["db_setnull", "db_cascade"]: - r = RelatedDbOptionParent.objects.create() - setattr(a, name, r) - a.save() - return a - - class M(models.Model): m2m = models.ManyToManyField(R, related_name="m_set") m2m_through = models.ManyToManyField(R, through="MR", related_name="m_through_set") diff --git a/tests/delete/tests.py b/tests/delete/tests.py index 8d525d1e5ff3..664cdfb440f2 100644 --- a/tests/delete/tests.py +++ b/tests/delete/tests.py @@ -15,6 +15,7 @@ Avatar, B, Base, + CascadeDbModel, Child, DeleteBottom, DeleteTop, @@ -34,16 +35,15 @@ RChild, RChildChild, Referrer, - RelatedDbOption, RelatedDbOptionGrandParent, RelatedDbOptionParent, RProxy, S, SetDefaultDbModel, + SetNullDbModel, T, User, create_a, - create_related_db_option, get_default_r, ) @@ -81,10 +81,13 @@ def test_setnull(self): a = A.objects.get(pk=a.pk) self.assertIsNone(a.setnull) + @skipUnlessDBFeature("supports_on_delete_db_null") def test_db_setnull(self): - a = create_related_db_option("db_setnull") + a = SetNullDbModel.objects.create( + db_setnull=RelatedDbOptionParent.objects.create() + ) a.db_setnull.delete() - a = RelatedDbOption.objects.get(pk=a.pk) + a = SetNullDbModel.objects.get(pk=a.pk) self.assertIsNone(a.db_setnull) def test_setdefault(self): @@ -394,20 +397,21 @@ def test_bulk(self): self.assertNumQueries(5, s.delete) self.assertFalse(S.objects.exists()) + @skipUnlessDBFeature("supports_on_delete_db_cascade") def test_db_cascade(self): related_db_op = RelatedDbOptionParent.objects.create( p=RelatedDbOptionGrandParent.objects.create() ) - RelatedDbOption.objects.bulk_create( + CascadeDbModel.objects.bulk_create( [ - RelatedDbOption(db_cascade=related_db_op) + CascadeDbModel(db_cascade=related_db_op) for _ in range(2 * GET_ITERATOR_CHUNK_SIZE) ] ) with self.assertNumQueries(1): results = related_db_op.delete() self.assertEqual(results, (1, {"delete.RelatedDbOptionParent": 1})) - self.assertFalse(RelatedDbOption.objects.exists()) + self.assertFalse(CascadeDbModel.objects.exists()) self.assertFalse(RelatedDbOptionParent.objects.exists()) def test_instance_update(self): diff --git a/tests/inspectdb/models.py b/tests/inspectdb/models.py index fbe1df8a9531..489704879f35 100644 --- a/tests/inspectdb/models.py +++ b/tests/inspectdb/models.py @@ -178,3 +178,9 @@ class DbOnDeleteModel(models.Model): fk_set_null = models.ForeignKey( DigitsInColumnName, on_delete=models.DB_SET_NULL, null=True ) + + class Meta: + required_db_features = { + "supports_on_delete_db_cascade", + "supports_on_delete_db_null", + } diff --git a/tests/inspectdb/tests.py b/tests/inspectdb/tests.py index c16258b0eb67..8175c52e4ec9 100644 --- a/tests/inspectdb/tests.py +++ b/tests/inspectdb/tests.py @@ -301,7 +301,11 @@ def test_foreign_key_to_field(self): out.getvalue(), ) - @skipUnlessDBFeature("can_introspect_foreign_keys") + @skipUnlessDBFeature( + "can_introspect_foreign_keys", + "supports_on_delete_db_cascade", + "supports_on_delete_db_null", + ) def test_foreign_key_db_on_delete(self): out = StringIO() call_command("inspectdb", "inspectdb_dbondeletemodel", stdout=out) diff --git a/tests/introspection/models.py b/tests/introspection/models.py index 6c94f5212c9d..8ced619b08b2 100644 --- a/tests/introspection/models.py +++ b/tests/introspection/models.py @@ -112,11 +112,20 @@ class Meta: required_db_features = {"supports_comments"} -class DbOnDeleteModel(models.Model): +class DbOnDeleteCascadeModel(models.Model): fk_do_nothing = models.ForeignKey(Country, on_delete=models.DO_NOTHING) fk_db_cascade = models.ForeignKey(City, on_delete=models.DB_CASCADE) + + class Meta: + required_db_features = {"supports_on_delete_db_cascade"} + + +class DbOnDeleteSetNullModel(models.Model): fk_set_null = models.ForeignKey(Reporter, on_delete=models.DB_SET_NULL, null=True) + class Meta: + required_db_features = {"supports_on_delete_db_null"} + class DbOnDeleteSetDefaultModel(models.Model): fk_db_set_default = models.ForeignKey( diff --git a/tests/introspection/tests.py b/tests/introspection/tests.py index 1f7f22b2dc16..3a2c6112c874 100644 --- a/tests/introspection/tests.py +++ b/tests/introspection/tests.py @@ -10,8 +10,9 @@ Comment, Country, DbCommentModel, - DbOnDeleteModel, + DbOnDeleteCascadeModel, DbOnDeleteSetDefaultModel, + DbOnDeleteSetNullModel, District, Reporter, UniqueConstraintConditionModel, @@ -244,11 +245,11 @@ def test_get_relations(self): editor.add_field(Article, body) self.assertEqual(relations, expected_relations) - @skipUnlessDBFeature("can_introspect_foreign_keys") - def test_get_relations_db_on_delete(self): + @skipUnlessDBFeature("can_introspect_foreign_keys", "supports_on_delete_db_cascade") + def test_get_relations_db_on_delete_cascade(self): with connection.cursor() as cursor: relations = connection.introspection.get_relations( - cursor, DbOnDeleteModel._meta.db_table + cursor, DbOnDeleteCascadeModel._meta.db_table ) if connection.vendor == "mysql" and connection.mysql_is_mariadb: @@ -259,6 +260,18 @@ def test_get_relations_db_on_delete(self): expected_relations = { "fk_db_cascade_id": ("id", City._meta.db_table, DB_CASCADE), "fk_do_nothing_id": ("id", Country._meta.db_table, no_db_on_delete), + } + self.assertEqual(relations, expected_relations) + + @skipUnlessDBFeature("can_introspect_foreign_keys", "supports_on_delete_db_null") + def test_get_relations_db_on_delete_null(self): + with connection.cursor() as cursor: + relations = connection.introspection.get_relations( + cursor, DbOnDeleteSetNullModel._meta.db_table + ) + + # {field_name: (field_name_other_table, other_table, db_on_delete)} + expected_relations = { "fk_set_null_id": ("id", Reporter._meta.db_table, DB_SET_NULL), } self.assertEqual(relations, expected_relations) diff --git a/tests/invalid_models_tests/test_relative_fields.py b/tests/invalid_models_tests/test_relative_fields.py index e73f22ab41fe..51b1de149427 100644 --- a/tests/invalid_models_tests/test_relative_fields.py +++ b/tests/invalid_models_tests/test_relative_fields.py @@ -2288,6 +2288,41 @@ class FooBar(models.Model): @isolate_apps("invalid_models_tests") class DatabaseLevelOnDeleteTests(TestCase): + def test_db_cascade_support(self): + class Parent(models.Model): + pass + + class Child(models.Model): + parent = models.ForeignKey(Parent, models.DB_CASCADE) + + field = Child._meta.get_field("parent") + expected = ( + [] + if connection.features.supports_on_delete_db_cascade + else [ + Error( + f"{connection.display_name} does not support a DB_CASCADE.", + hint="Change the on_delete rule to CASCADE.", + obj=field, + id="fields.E324", + ) + ] + ) + self.assertEqual(field.check(databases=self.databases), expected) + + def test_db_cascade_required_db_features(self): + class Parent(models.Model): + pass + + class Child(models.Model): + parent = models.ForeignKey(Parent, models.DB_CASCADE) + + class Meta: + required_db_features = {"supports_on_delete_db_cascade"} + + field = Child._meta.get_field("parent") + self.assertEqual(field.check(databases=self.databases), []) + def test_db_set_default_support(self): class Parent(models.Model): pass @@ -2349,6 +2384,41 @@ class Child(models.Model): ], ) + def test_db_set_null_support(self): + class Parent(models.Model): + pass + + class Child(models.Model): + parent = models.ForeignKey(Parent, models.DB_SET_NULL, null=True) + + field = Child._meta.get_field("parent") + expected = ( + [] + if connection.features.supports_on_delete_db_null + else [ + Error( + f"{connection.display_name} does not support a DB_SET_NULL.", + hint="Change the on_delete rule to SET_NULL.", + obj=field, + id="fields.E324", + ) + ] + ) + self.assertEqual(field.check(databases=self.databases), expected) + + def test_db_set_null_required_db_features(self): + class Parent(models.Model): + pass + + class Child(models.Model): + parent = models.ForeignKey(Parent, models.DB_SET_NULL, null=True) + + class Meta: + required_db_features = {"supports_on_delete_db_null"} + + field = Child._meta.get_field("parent") + self.assertEqual(field.check(databases=self.databases), []) + def test_python_db_chain(self): class GrandParent(models.Model): pass @@ -2376,6 +2446,7 @@ class Child(models.Model): ], ) + @skipUnlessDBFeature("supports_on_delete_db_null") def test_db_python_chain(self): class GrandParent(models.Model): pass @@ -2403,6 +2474,7 @@ class Child(models.Model): ], ) + @skipUnlessDBFeature("supports_on_delete_db_cascade") def test_db_python_chain_auto_created(self): class GrandParent(models.Model): pass @@ -2430,6 +2502,7 @@ class Child(models.Model): ], ) + @skipUnlessDBFeature("supports_on_delete_db_null") def test_db_do_nothing_chain(self): class GrandParent(models.Model): pass From 2a6e0bd72d4a69725b957d6748a4b834f21b12b5 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Mon, 24 Nov 2025 06:14:38 -0500 Subject: [PATCH 2/2] Fixed #36751 -- Fixed empty filtered aggregation crash over annotated queryset. Regression in b8e5a8a9a2a767f584cbe89a878a42363706f939. Refs #36404. The replace_expressions method was innapropriately dealing with falsey but not None source expressions causing them to also be potentially evaluated when __bool__ was invoked (e.g. QuerySet.__bool__ evaluates the queryset). The changes introduced in b8e5a8a9a2, which were to deal with a similar issue, surfaced the problem as aggregation over an annotated queryset requires an inlining (or pushdown) of aggregate references which is achieved through replace_expressions. In cases where an empty Q object was provided as an aggregate filter, such as when the admin facetting feature was used as reported, it would wrongly be turned into None, instead of an empty WhereNode, causing a crash at aggregate filter compilation. Note that the crash signature differed depending on whether or not the backend natively supports aggregate filtering (supports_aggregate_filter_clause) as the fallback, which makes use Case / When expressions, would result in a TypeError instead of a NoneType AttributeError. Thanks Rafael Urben for the report, Antoliny and Youngkwang Yang for the triage. --- django/db/models/expressions.py | 2 +- docs/releases/5.2.9.txt | 4 ++++ tests/aggregation/test_filter_argument.py | 11 +++++++++++ tests/expressions/tests.py | 18 ++++++++++++++++++ 4 files changed, 34 insertions(+), 1 deletion(-) diff --git a/django/db/models/expressions.py b/django/db/models/expressions.py index 0d47366d2c03..6b90a42cf1d2 100644 --- a/django/db/models/expressions.py +++ b/django/db/models/expressions.py @@ -426,7 +426,7 @@ def replace_expressions(self, replacements): clone = self.copy() clone.set_source_expressions( [ - expr.replace_expressions(replacements) if expr else None + None if expr is None else expr.replace_expressions(replacements) for expr in source_expressions ] ) diff --git a/docs/releases/5.2.9.txt b/docs/releases/5.2.9.txt index 588c278be5e9..515b233b8a99 100644 --- a/docs/releases/5.2.9.txt +++ b/docs/releases/5.2.9.txt @@ -16,3 +16,7 @@ Bugfixes * Fixed a bug in Django 5.2 on PostgreSQL where ``bulk_create()`` did not apply a field's custom query placeholders (:ticket:`36748`). + +* Fixed a regression in Django 5.2.2 that caused a crash when using aggregate + functions with an empty ``Q`` filter over a queryset with annotations + (:ticket:`36751`). diff --git a/tests/aggregation/test_filter_argument.py b/tests/aggregation/test_filter_argument.py index 1a17703a8640..59588d30d05e 100644 --- a/tests/aggregation/test_filter_argument.py +++ b/tests/aggregation/test_filter_argument.py @@ -88,6 +88,17 @@ def test_empty_filtered_aggregates(self): agg = Count("pk", filter=Q()) self.assertEqual(Author.objects.aggregate(count=agg)["count"], 3) + def test_empty_filtered_aggregates_with_annotation(self): + agg = Count("pk", filter=Q()) + self.assertEqual( + Author.objects.annotate( + age_annotation=F("age"), + ).aggregate( + count=agg + )["count"], + 3, + ) + def test_double_filtered_aggregates(self): agg = Sum("age", filter=Q(Q(name="test2") & ~Q(name="test"))) self.assertEqual(Author.objects.aggregate(age=agg)["age"], 60) diff --git a/tests/expressions/tests.py b/tests/expressions/tests.py index 6f18321aa7b3..981d84e9e8ec 100644 --- a/tests/expressions/tests.py +++ b/tests/expressions/tests.py @@ -1571,6 +1571,24 @@ def test_get_expression_for_validation_only_one_source_expression(self): with self.assertRaisesMessage(ValueError, msg): expression.get_expression_for_validation() + def test_replace_expressions_falsey(self): + class AssignableExpression(Expression): + def __init__(self, *source_expressions): + super().__init__() + self.set_source_expressions(list(source_expressions)) + + def get_source_expressions(self): + return self.source_expressions + + def set_source_expressions(self, exprs): + self.source_expressions = exprs + + expression = AssignableExpression() + falsey = Q() + expression.set_source_expressions([falsey]) + replaced = expression.replace_expressions({"replacement": Expression()}) + self.assertEqual(replaced.get_source_expressions(), [falsey]) + class ExpressionsNumericTests(TestCase): @classmethod