From f359990e4909db8722820849d61a6f5724338723 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Tue, 23 Jul 2024 00:33:31 -0400 Subject: [PATCH] Fixed #35625 -- Fixed a crash when adding a field with db_default and check constraint. This is the exact same issue as refs #30408 but for creating a model with a constraint containing % escapes instead of column addition. All of these issues stem from a lack of SQL and parameters separation from the BaseConstraint DDL generating methods preventing them from being mixed with other parts of the schema alteration logic that do make use of parametrization on some backends (e.g. Postgres, MySQL for DEFAULT). Prior to the addition of Field.db_default and GeneratedField in 5.0 parametrization of DDL was never exercised on model creation so this is effectively a bug with db_default as the GeneratedField case was addressed by refs #35336. Thanks Julien Chaumont for the report and Mariusz Felisiak for the review. --- django/db/backends/base/schema.py | 29 +++++++++++---- docs/releases/5.0.8.txt | 4 ++ tests/migrations/test_operations.py | 58 +++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 8 deletions(-) diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py index e5f28d9c6a..3e38c56d50 100644 --- a/django/db/backends/base/schema.py +++ b/django/db/backends/base/schema.py @@ -164,7 +164,7 @@ class BaseDatabaseSchemaEditor: def __exit__(self, exc_type, exc_value, traceback): if exc_type is None: for sql in self.deferred_sql: - self.execute(sql) + self.execute(sql, None) if self.atomic_migration: self.atomic.__exit__(exc_type, exc_value, traceback) @@ -265,16 +265,29 @@ class BaseDatabaseSchemaEditor: ) if autoinc_sql: self.deferred_sql.extend(autoinc_sql) - constraints = [ - constraint.constraint_sql(model, self) - for constraint in model._meta.constraints - ] + # The BaseConstraint DDL creation methods such as constraint_sql(), + # create_sql(), and delete_sql(), were not designed in a way that + # separate SQL from parameters which make their generated SQL unfit to + # be used in a context where parametrization is delegated to the + # backend. + constraint_sqls = [] + if params: + # If parameters are present (e.g. a DEFAULT clause on backends that + # allow parametrization) defer constraint creation so they are not + # mixed with SQL meant to be parametrized. + for constraint in model._meta.constraints: + self.deferred_sql.append(constraint.create_sql(model, self)) + else: + constraint_sqls.extend( + constraint.constraint_sql(model, self) + for constraint in model._meta.constraints + ) sql = self.sql_create_table % { "table": self.quote_name(model._meta.db_table), "definition": ", ".join( - str(constraint) - for constraint in (*column_sqls, *constraints) - if constraint + str(statement) + for statement in (*column_sqls, *constraint_sqls) + if statement ), } if model._meta.db_tablespace: diff --git a/docs/releases/5.0.8.txt b/docs/releases/5.0.8.txt index 1037b78f75..7bdb5488b2 100644 --- a/docs/releases/5.0.8.txt +++ b/docs/releases/5.0.8.txt @@ -15,3 +15,7 @@ Bugfixes * Fixed a regression in Django 5.0 where ``ModelAdmin.action_checkbox`` could break the admin changelist HTML page when rendering a model instance with a ``__html__`` method (:ticket:`35606`). + +* Fixed a crash when creating a model with a ``Field.db_default`` and a + ``Meta.constraints`` constraint composed of ``__endswith``, ``__startswith``, + or ``__contains`` lookups (:ticket:`35625`). diff --git a/tests/migrations/test_operations.py b/tests/migrations/test_operations.py index f865500829..3ac813b899 100644 --- a/tests/migrations/test_operations.py +++ b/tests/migrations/test_operations.py @@ -4107,6 +4107,64 @@ class OperationTests(OperationTestBase): definition[2], {"model_name": "Pony", "constraint": gt_constraint} ) + @skipUnlessDBFeature("supports_table_check_constraints") + def test_create_model_constraint_percent_escaping(self): + app_label = "add_constraint_string_quoting" + from_state = ProjectState() + checks = [ + # "%" generated in startswith lookup should be escaped in a way + # that is considered a leading wildcard. + ( + models.Q(name__startswith="Albert"), + {"name": "Alberta"}, + {"name": "Artur"}, + ), + # Literal "%" should be escaped in a way that is not a considered a + # wildcard. + (models.Q(rebate__endswith="%"), {"rebate": "10%"}, {"rebate": "10%$"}), + # Right-hand-side baked "%" literals should not be used for + # parameters interpolation. + ( + ~models.Q(surname__startswith=models.F("name")), + {"name": "Albert"}, + {"name": "Albert", "surname": "Alberto"}, + ), + # Exact matches against "%" literals should also be supported. + ( + models.Q(name="%"), + {"name": "%"}, + {"name": "Albert"}, + ), + ] + for check, valid, invalid in checks: + with self.subTest(condition=check, valid=valid, invalid=invalid): + constraint = models.CheckConstraint(condition=check, name="constraint") + operation = migrations.CreateModel( + "Author", + fields=[ + ("id", models.AutoField(primary_key=True)), + ("name", models.CharField(max_length=100)), + ("surname", models.CharField(max_length=100, db_default="")), + ("rebate", models.CharField(max_length=100)), + ], + options={"constraints": [constraint]}, + ) + to_state = from_state.clone() + operation.state_forwards(app_label, to_state) + with connection.schema_editor() as editor: + operation.database_forwards(app_label, editor, from_state, to_state) + Author = to_state.apps.get_model(app_label, "Author") + try: + with transaction.atomic(): + Author.objects.create(**valid).delete() + with self.assertRaises(IntegrityError), transaction.atomic(): + Author.objects.create(**invalid) + finally: + with connection.schema_editor() as editor: + migrations.DeleteModel("Author").database_forwards( + app_label, editor, to_state, from_state + ) + @skipUnlessDBFeature("supports_table_check_constraints") def test_add_constraint_percent_escaping(self): app_label = "add_constraint_string_quoting"