#36779: DeleteModel can lead to missing FK constraints
--------------------------------+--------------------------------------
Reporter: Jamie Cockburn | Owner: Vishy Algo
Type: Bug | Status: assigned
Component: Migrations | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------
Comment (by Vishy Algo):
Replying to [comment:6 Simon Charette]:
> Well I'm not sure how I missed it in my initial analysis (maybe
something got cached in my `django-docker-box` setup) but it appears that
removing the `CASCADE` from `DROM TABLE` actually breaks a ton of tests so
there might be more to it than I initially thought.
>
> Most of the failures seem to point at an innaproprite use of
`delete_model` in test teardown though and not at an actual problem in
migrations (which explicitly drop foreign keys first).
>
> The following patch might be a more realistic approach to the problem
>
> {{{#!diff
> diff --git a/django/db/backends/base/schema.py
b/django/db/backends/base/schema.py
> index 1f27d6a0d4..6982d094bf 100644
> --- a/django/db/backends/base/schema.py
> +++ b/django/db/backends/base/schema.py
> @@ -86,7 +86,8 @@ class BaseDatabaseSchemaEditor:
> sql_create_table = "CREATE TABLE %(table)s (%(definition)s)"
> sql_rename_table = "ALTER TABLE %(old_table)s RENAME TO
%(new_table)s"
> sql_retablespace_table = "ALTER TABLE %(table)s SET TABLESPACE
%(new_tablespace)s"
> - sql_delete_table = "DROP TABLE %(table)s CASCADE"
> + sql_delete_table = "DROP TABLE %(table)s"
> + sql_delete_table_cascade = "DROP TABLE %(table)s CASCADE"
>
> sql_create_column = "ALTER TABLE %(table)s ADD COLUMN %(column)s
%(definition)s"
> sql_alter_column = "ALTER TABLE %(table)s %(changes)s"
> @@ -538,7 +539,7 @@ class BaseDatabaseSchemaEditor:
> if field.remote_field.through._meta.auto_created:
> self.create_model(field.remote_field.through)
>
> - def delete_model(self, model):
> + def delete_model(self, model, *, cascade=False):
> """Delete a model from the database."""
> # Handle auto-created intermediary models
> for field in model._meta.local_many_to_many:
> @@ -546,8 +547,9 @@ class BaseDatabaseSchemaEditor:
> self.delete_model(field.remote_field.through)
>
> # Delete the table
> + delete_sql = self.sql_delete_table_cascade if cascade else
self.sql_delete_table
> self.execute(
> - self.sql_delete_table
> + delete_sql
> % {
> "table": self.quote_name(model._meta.db_table),
> }
> diff --git a/django/db/backends/oracle/schema.py
b/django/db/backends/oracle/schema.py
> index 13fa7220ce..281aa1db7b 100644
> --- a/django/db/backends/oracle/schema.py
> +++ b/django/db/backends/oracle/schema.py
> @@ -23,7 +23,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
> "CONSTRAINT %(name)s REFERENCES
%(to_table)s(%(to_column)s)%(on_delete_db)"
> "s%(deferrable)s"
> )
> - sql_delete_table = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
> + sql_delete_table_cascade = "DROP TABLE %(table)s CASCADE
CONSTRAINTS"
> sql_create_index = "CREATE INDEX %(name)s ON %(table)s
(%(columns)s)%(extra)s"
>
> def quote_value(self, value):
> @@ -47,9 +47,9 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
> self._drop_identity(model._meta.db_table, field.column)
> super().remove_field(model, field)
>
> - def delete_model(self, model):
> + def delete_model(self, model, *, cascade=False):
> # Run superclass action
> - super().delete_model(model)
> + super().delete_model(model, cascade=cascade)
> # Clean up manually created sequence.
> self.execute(
> """
> diff --git a/django/db/backends/sqlite3/schema.py
b/django/db/backends/sqlite3/schema.py
> index ee6163c253..6313b9be43 100644
> --- a/django/db/backends/sqlite3/schema.py
> +++ b/django/db/backends/sqlite3/schema.py
> @@ -10,7 +10,8 @@ from django.db.models import CompositePrimaryKey,
UniqueConstraint
>
>
> class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
> - sql_delete_table = "DROP TABLE %(table)s"
> + # SQLite doesn't support DROP TABLE CASCADE.
> + sql_delete_table_cascade = "DROP TABLE %(table)s"
> sql_create_fk = None
> sql_create_inline_fk = (
> "REFERENCES %(to_table)s (%(to_column)s)%(on_delete_db)s
DEFERRABLE INITIALLY "
> @@ -278,9 +279,9 @@ class
DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
> if restore_pk_field:
> restore_pk_field.primary_key = True
>
> - def delete_model(self, model, handle_autom2m=True):
> + def delete_model(self, model, handle_autom2m=True, *,
cascade=False):
> if handle_autom2m:
> - super().delete_model(model)
> + super().delete_model(model, cascade=cascade)
> else:
> # Delete the table (and only that)
> self.execute(
> diff --git a/tests/schema/tests.py b/tests/schema/tests.py
> index ab8b07e9d3..2aaafc93e3 100644
> --- a/tests/schema/tests.py
> +++ b/tests/schema/tests.py
> @@ -159,7 +159,7 @@ class SchemaTests(TransactionTestCase):
> if self.isolated_local_models:
> with connection.schema_editor() as editor:
> for model in self.isolated_local_models:
> - editor.delete_model(model)
> + editor.delete_model(model, cascade=True)
>
> def delete_tables(self):
> "Deletes all model tables for our models for a clean test
environment"
> @@ -174,7 +174,7 @@ class SchemaTests(TransactionTestCase):
> if connection.features.ignores_table_name_case:
> tbl = tbl.lower()
> if tbl in table_names:
> - editor.delete_model(model)
> + editor.delete_model(model, cascade=True)
> table_names.remove(tbl)
> connection.enable_constraint_checking()
>
> @@ -1542,7 +1542,7 @@ class SchemaTests(TransactionTestCase):
>
> def drop_collation():
> with connection.cursor() as cursor:
> - cursor.execute(f"DROP COLLATION IF EXISTS
{ci_collation}")
> + cursor.execute(f"DROP COLLATION IF EXISTS
{ci_collation} CASCADE")
>
> with connection.cursor() as cursor:
> cursor.execute(
> }}}
The current proposal to introduce a cascade keyword argument in
delete_model lacks a mechanism to prevent syntax errors on backends that
do not support DDL-level cascading.
As seen in the migration test failures, SQLite chokes on the CASCADE
keyword. Rather than forcing every caller to check the backend vendor, we
should introduce a new feature flag: supports_table_drop_cascade. This
allows BaseDatabaseSchemaEditor to stay backend-agnostic by conditionally
selecting the SQL template based on both the user's intent (cascade=True)
and the backend’s capability.
This approach ensures the API is extensible and backward-compatible
without breaking SQLite or third-party backends.
--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:8>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019b4dedce19-f1725247-68c1-44c8-9588-f5d83c3d1559-000000%40eu-central-1.amazonses.com.
