[Django] #36779: DeleteModel can lead to missing FK constraints

[Django]

#36779: DeleteModel can lead to missing FK constraints
--------------------------------+--------------------------------------
Reporter: Jamie Cockburn | Type: Bug
Status: new | Component: Migrations
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------
I have two models, `Jane` and `Bob`. `Jane` has ForeignKey relationship to
`Bob`.

I generate a migration 0001_initial:

{{{#!python
class Migration(migrations.Migration):
operations = [
migrations.CreateModel(
name='Bob',
fields=[
('id', models.BigAutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
],
),
migrations.CreateModel(
name='Jane',
fields=[
('id', models.BigAutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
('bob',
models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
to='constraintbug.bob')),
],
),
]
}}}

`Jane` is rendered in postgres as:

{{{#!sql
% docker compose exec db psql -U constraintbug
psql (18.0 (Debian 18.0-1.pgdg13+3))
Type "help" for help.

constraintbug=# \d constraintbug_jane
Table "public.constraintbug_jane"
Column | Type | Collation | Nullable | Default
--------+--------+-----------+----------+----------------------------------
id | bigint | | not null | generated by default as identity
bob_id | bigint | | not null |
Indexes:
"constraintbug_jane_pkey" PRIMARY KEY, btree (id)
"constraintbug_jane_bob_id_35b76f6b" btree (bob_id)
Foreign-key constraints:
"constraintbug_jane_bob_id_35b76f6b_fk_constraintbug_bob_id" FOREIGN
KEY (bob_id) REFERENCES constraintbug_bob(id) DEFERRABLE INITIALLY
DEFERRED
}}}

Note the FK constraint.

Now, I decide to manually write a migration, because I want to remove
`Bob` and replace it with a new thing. In my case, I was introducing model
inheritance, and didn't want to revise my code to remove everything,
`makemigrations`, add everything back, `makemigrations` again, and I
decided to just manually type `migrations.DeleteModel()`.

{{{#!python
class Migration(migrations.Migration):
operations = [
migrations.DeleteModel( # this should fail?
name='Bob',
),
]
}}}

Now things start to go wrong. I would expect at this point that the
migration should fail, because I'm trying to delete a table that is
referred to by the FK constraint. Instead, is just deletes the constraint:

{{{#!sql
constraintbug=# \d constraintbug_jane
Table "public.constraintbug_jane"
Column | Type | Collation | Nullable | Default
--------+--------+-----------+----------+----------------------------------
id | bigint | | not null | generated by default as identity
bob_id | bigint | | not null |
Indexes:
"constraintbug_jane_pkey" PRIMARY KEY, btree (id)
"constraintbug_jane_bob_id_35b76f6b" btree (bob_id)
}}}

I suppose we could say that this is an "intermediate" state, but then, I
go and add the model back:

{{{#!python
class Migration(migrations.Migration):
operations = [
migrations.CreateModel( # maybe this should re-create the FK
constraint on jane?
name='Bob',
fields=[
('id', models.BigAutoField(auto_created=True,
primary_key=True, serialize=False, verbose_name='ID')),
],
),
]
}}}

But `Jane` is still missing her constraint, and worse, I can do things
like:
{{{#!python
Jane.objects.create(
bob_id=1234, # there is no such bob with pk 1234, this should fail!!!
)
}}}

Here's a wee example repo showing the issue:
https://github.com/daggaz/django-deletemodel/tree/master

I think that either:
* the `DeleteModel()` migration should fail to delete a model that is
referenced
* the 2nd `CreateModel()` migration should recreate the FK constraint on
`Jane` that `DeleteModel()` deleted
* at the very least the docs should have very big warning
--
Ticket URL: <https://code.djangoproject.com/ticket/36779>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#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: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Changes (by Vishy Algo):

* owner: (none) => Vishy Algo
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:1>

[Django]

#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: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Comment (by Simon Charette):

This is due to the `DeleteModel` operation resuling in a `DROP TABLE
%(table)s CASCADE`
([https://github.com/django/django/blob/e726254a380f2a35a2fcf71143e96cb5987d8102/django/db/backends/base/schema.py#L89
source]).

There is documented risk in crafting your own migration but I don't think
it's fair to expect foreign key constraints to be implicitly dropped.

Given the recent decision to remove `CASCADE` from `RemoveField` on
PostgreSQL #35487 (2a5aca38bbb37f6e7590ac6e68912bfbefb17dae) which landed
in 6.0 I think it's worth accepting this ticket for
[https://forum.djangoproject.com/t/removefield-dropping-views-without-
notice/31409 the same arguments] around implicit deletion of references
(views and foreing key references).

FWIW I ran the full `schema` and `migrations` test suite on Postgres,
SQLite, and MySQL against the following patch

{{{#!diff
diff --git a/django/db/backends/base/schema.py
b/django/db/backends/base/schema.py
index 1f27d6a0d4..1742622683 100644
--- a/django/db/backends/base/schema.py
+++ b/django/db/backends/base/schema.py
@@ -86,7 +86,7 @@ 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_create_column = "ALTER TABLE %(table)s ADD COLUMN %(column)s
%(definition)s"
sql_alter_column = "ALTER TABLE %(table)s %(changes)s"
}}}

and everything passed so I dug more and it seems the usage of `CASCADE`
comes from
[https://github.com/django/django/commit/8ba5bf31986fa746ecc81683c64999dcea4f8e0a
#diff-4c84fbb83165bd5643606b3712b80a88a7c235235a8b61981de5ca30cea8d0faR30
the very begining of the migrations framework] and was likely ported from
South which had a more rudementary way of tracking references between
models and might explain the usage of cascade.
--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:2>

[Django]

#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: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Changes (by Simon Charette):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:3>

[Django]

#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: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Comment (by Vishy Algo):

So, removing the CASCADE in

''django/db/backends/base/schema.py'' :
{{{#!diff
class BaseDatabaseSchemaEditor:
+ sql_delete_table = "DROP TABLE %(table)s CASCADE"

- sql_delete_table = "DROP TABLE %(table)s"
}}}

, would cause the migration to fail with an integrity-related database
error, preventing the migration from completing successfully.

Hence, removing the divergence happening in the ProjectState object.
--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:4>

[Django]

#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: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Changes (by Vishy Algo):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:5>

[Django]

#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: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------+--------------------------------------

Comment (by 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(
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:6>

[Django]

#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
--------------------------------+--------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:7>

[Django]

#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]:

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]

#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
--------------------------------+--------------------------------------

Changes (by Simon Charette):

* cc: Simon Charette (added)

Comment:

I'm not against a feature flag at all if we have a use for it but I can't
reproduce the test failures you are alluding to by running the full SQLite
test suite and it seems to be a propery required by third party backends
to pass the full schema and migrations test suites.

Given `sqlite3.DatabaseSchemaEditor.delete_model` completely ignores the
`cascade` argument by when calling
`base.DatabaseSchemaEditor.delete_model` though `super()` in the proposed
patch and that only the later makes use of `sql_delete_table_cascade` I
don't see how any code path can issue a `DROP TABLE {table} CASCADE` on
SQLite.

> This approach ensures the API is extensible and backward-compatible
without breaking SQLite or third-party backends.

Could you elaborate on what you mean by that? SQLite doesn't seem to be
broken here (as pointed out above) and the default on all other core
backends use to be to cascade by default (Oracle, Postgres, MySQL). All we
did here is to change the default of `DropModel` to not cascade and allow
the schema editor to take a more informed decision.

Given the usage and expectations that `cascade=True` is implemented to get
the schema and migrations test suite passing I don't see how
`supports_table_drop_cascade = False` would be useful; third-party
database backends must emulate it even if they don't support it natively.
--
Ticket URL: <https://code.djangoproject.com/ticket/36779#comment:9>