Opened 3 weeks ago

Last modified 3 days ago

#36779 assigned Bug

DeleteModel can lead to missing FK constraints

Reported by: Jamie Cockburn Owned by: Vishy Algo
Component: Migrations Version: 6.0
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I have two models, Jane and Bob. Jane has ForeignKey relationship to Bob.

I generate a migration 0001_initial:

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:

% 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().

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:

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:

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:

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

Change History (10)

comment:1 by Vishy Algo, 3 weeks ago

Owner: set to Vishy Algo
Status: new → assigned

comment:2 by Simon Charette, 3 weeks ago

This is due to the DeleteModel operation resuling in a DROP TABLE %(table)s CASCADE (​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 ​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

  • django/db/backends/base/schema.py 

    diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py
index 1f27d6a0d4..1742622683 100644
    a b class BaseDatabaseSchemaEditor:  
    8686    sql_create_table = "CREATE TABLE %(table)s (%(definition)s)"
    8787    sql_rename_table = "ALTER TABLE %(old_table)s RENAME TO %(new_table)s"
    8888    sql_retablespace_table = "ALTER TABLE %(table)s SET TABLESPACE %(new_tablespace)s"
    89     sql_delete_table = "DROP TABLE %(table)s CASCADE"
     89    sql_delete_table = "DROP TABLE %(table)s"
    9090
    9191    sql_create_column = "ALTER TABLE %(table)s ADD COLUMN %(column)s %(definition)s"
    9292    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 ​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.

comment:3 by Simon Charette, 3 weeks ago

Triage Stage: Unreviewed → Accepted

comment:4 by Vishy Algo, 3 weeks ago

So, removing the CASCADE in

django/db/backends/base/schema.py : 

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.

Version 0, edited 3 weeks ago by Vishy Algo (next)

comment:5 by Vishy Algo, 3 weeks ago

Has patch: set

comment:6 by Simon Charette, 3 weeks ago

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

  • django/db/backends/base/schema.py 

    diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py
index 1f27d6a0d4..6982d094bf 100644
    a b class BaseDatabaseSchemaEditor:  
    8686    sql_create_table = "CREATE TABLE %(table)s (%(definition)s)"
    8787    sql_rename_table = "ALTER TABLE %(old_table)s RENAME TO %(new_table)s"
    8888    sql_retablespace_table = "ALTER TABLE %(table)s SET TABLESPACE %(new_tablespace)s"
    89     sql_delete_table = "DROP TABLE %(table)s CASCADE"
     89    sql_delete_table = "DROP TABLE %(table)s"
     90    sql_delete_table_cascade = "DROP TABLE %(table)s CASCADE"
    9091
    9192    sql_create_column = "ALTER TABLE %(table)s ADD COLUMN %(column)s %(definition)s"
    9293    sql_alter_column = "ALTER TABLE %(table)s %(changes)s"
    … … class BaseDatabaseSchemaEditor:  
    538539            if field.remote_field.through._meta.auto_created:
    539540                self.create_model(field.remote_field.through)
    540541
    541     def delete_model(self, model):
     542    def delete_model(self, model, *, cascade=False):
    542543        """Delete a model from the database."""
    543544        # Handle auto-created intermediary models
    544545        for field in model._meta.local_many_to_many:
    … … class BaseDatabaseSchemaEditor:  
    546547                self.delete_model(field.remote_field.through)
    547548
    548549        # Delete the table
     550        delete_sql = self.sql_delete_table_cascade if cascade else self.sql_delete_table
    549551        self.execute(
    550             self.sql_delete_table
     552            delete_sql
    551553            % {
    552554                "table": self.quote_name(model._meta.db_table),
    553555            }

  • django/db/backends/oracle/schema.py 

    diff --git a/django/db/backends/oracle/schema.py b/django/db/backends/oracle/schema.py
index 13fa7220ce..281aa1db7b 100644
    a b class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    2323        "CONSTRAINT %(name)s REFERENCES %(to_table)s(%(to_column)s)%(on_delete_db)"
    2424        "s%(deferrable)s"
    2525    )
    26     sql_delete_table = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
     26    sql_delete_table_cascade = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
    2727    sql_create_index = "CREATE INDEX %(name)s ON %(table)s (%(columns)s)%(extra)s"
    2828
    2929    def quote_value(self, value):
    … … class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    4747            self._drop_identity(model._meta.db_table, field.column)
    4848        super().remove_field(model, field)
    4949
    50     def delete_model(self, model):
     50    def delete_model(self, model, *, cascade=False):
    5151        # Run superclass action
    52         super().delete_model(model)
     52        super().delete_model(model, cascade=cascade)
    5353        # Clean up manually created sequence.
    5454        self.execute(
    5555            """

  • django/db/backends/sqlite3/schema.py 

    diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py
index ee6163c253..6313b9be43 100644
    a b from django.db.models import CompositePrimaryKey, UniqueConstraint  
    1010
    1111
    1212class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
    13     sql_delete_table = "DROP TABLE %(table)s"
     13    # SQLite doesn't support DROP TABLE CASCADE.
     14    sql_delete_table_cascade = "DROP TABLE %(table)s"
    1415    sql_create_fk = None
    1516    sql_create_inline_fk = (
    1617        "REFERENCES %(to_table)s (%(to_column)s)%(on_delete_db)s DEFERRABLE INITIALLY "
    … … class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    278279        if restore_pk_field:
    279280            restore_pk_field.primary_key = True
    280281
    281     def delete_model(self, model, handle_autom2m=True):
     282    def delete_model(self, model, handle_autom2m=True, *, cascade=False):
    282283        if handle_autom2m:
    283             super().delete_model(model)
     284            super().delete_model(model, cascade=cascade)
    284285        else:
    285286            # Delete the table (and only that)
    286287            self.execute(

  • tests/schema/tests.py 

    diff --git a/tests/schema/tests.py b/tests/schema/tests.py
index ab8b07e9d3..2aaafc93e3 100644
    a b class SchemaTests(TransactionTestCase):  
    159159        if self.isolated_local_models:
    160160            with connection.schema_editor() as editor:
    161161                for model in self.isolated_local_models:
    162                     editor.delete_model(model)
     162                    editor.delete_model(model, cascade=True)
    163163
    164164    def delete_tables(self):
    165165        "Deletes all model tables for our models for a clean test environment"
    … … class SchemaTests(TransactionTestCase):  
    174174                if connection.features.ignores_table_name_case:
    175175                    tbl = tbl.lower()
    176176                if tbl in table_names:
    177                     editor.delete_model(model)
     177                    editor.delete_model(model, cascade=True)
    178178                    table_names.remove(tbl)
    179179            connection.enable_constraint_checking()
    180180
    … … class SchemaTests(TransactionTestCase):  
    15421542
    15431543        def drop_collation():
    15441544            with connection.cursor() as cursor:
    1545                 cursor.execute(f"DROP COLLATION IF EXISTS {ci_collation}")
     1545                cursor.execute(f"DROP COLLATION IF EXISTS {ci_collation} CASCADE")
    15461546
    15471547        with connection.cursor() as cursor:
    15481548            cursor.execute(

comment:7 by Jacob Walls, 3 weeks ago

Patch needs improvement: set

in reply to:  6 comment:8 by Vishy Algo, 6 days ago

Replying to 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

  • django/db/backends/base/schema.py 

    diff --git a/django/db/backends/base/schema.py b/django/db/backends/base/schema.py
index 1f27d6a0d4..6982d094bf 100644
    a b class BaseDatabaseSchemaEditor:  
    8686    sql_create_table = "CREATE TABLE %(table)s (%(definition)s)"
    8787    sql_rename_table = "ALTER TABLE %(old_table)s RENAME TO %(new_table)s"
    8888    sql_retablespace_table = "ALTER TABLE %(table)s SET TABLESPACE %(new_tablespace)s"
    89     sql_delete_table = "DROP TABLE %(table)s CASCADE"
     89    sql_delete_table = "DROP TABLE %(table)s"
     90    sql_delete_table_cascade = "DROP TABLE %(table)s CASCADE"
    9091
    9192    sql_create_column = "ALTER TABLE %(table)s ADD COLUMN %(column)s %(definition)s"
    9293    sql_alter_column = "ALTER TABLE %(table)s %(changes)s"
    … … class BaseDatabaseSchemaEditor:  
    538539            if field.remote_field.through._meta.auto_created:
    539540                self.create_model(field.remote_field.through)
    540541
    541     def delete_model(self, model):
     542    def delete_model(self, model, *, cascade=False):
    542543        """Delete a model from the database."""
    543544        # Handle auto-created intermediary models
    544545        for field in model._meta.local_many_to_many:
    … … class BaseDatabaseSchemaEditor:  
    546547                self.delete_model(field.remote_field.through)
    547548
    548549        # Delete the table
     550        delete_sql = self.sql_delete_table_cascade if cascade else self.sql_delete_table
    549551        self.execute(
    550             self.sql_delete_table
     552            delete_sql
    551553            % {
    552554                "table": self.quote_name(model._meta.db_table),
    553555            }

  • django/db/backends/oracle/schema.py 

    diff --git a/django/db/backends/oracle/schema.py b/django/db/backends/oracle/schema.py
index 13fa7220ce..281aa1db7b 100644
    a b class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    2323        "CONSTRAINT %(name)s REFERENCES %(to_table)s(%(to_column)s)%(on_delete_db)"
    2424        "s%(deferrable)s"
    2525    )
    26     sql_delete_table = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
     26    sql_delete_table_cascade = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
    2727    sql_create_index = "CREATE INDEX %(name)s ON %(table)s (%(columns)s)%(extra)s"
    2828
    2929    def quote_value(self, value):
    … … class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    4747            self._drop_identity(model._meta.db_table, field.column)
    4848        super().remove_field(model, field)
    4949
    50     def delete_model(self, model):
     50    def delete_model(self, model, *, cascade=False):
    5151        # Run superclass action
    52         super().delete_model(model)
     52        super().delete_model(model, cascade=cascade)
    5353        # Clean up manually created sequence.
    5454        self.execute(
    5555            """

  • django/db/backends/sqlite3/schema.py 

    diff --git a/django/db/backends/sqlite3/schema.py b/django/db/backends/sqlite3/schema.py
index ee6163c253..6313b9be43 100644
    a b from django.db.models import CompositePrimaryKey, UniqueConstraint  
    1010
    1111
    1212class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
    13     sql_delete_table = "DROP TABLE %(table)s"
     13    # SQLite doesn't support DROP TABLE CASCADE.
     14    sql_delete_table_cascade = "DROP TABLE %(table)s"
    1415    sql_create_fk = None
    1516    sql_create_inline_fk = (
    1617        "REFERENCES %(to_table)s (%(to_column)s)%(on_delete_db)s DEFERRABLE INITIALLY "
    … … class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):  
    278279        if restore_pk_field:
    279280            restore_pk_field.primary_key = True
    280281
    281     def delete_model(self, model, handle_autom2m=True):
     282    def delete_model(self, model, handle_autom2m=True, *, cascade=False):
    282283        if handle_autom2m:
    283             super().delete_model(model)
     284            super().delete_model(model, cascade=cascade)
    284285        else:
    285286            # Delete the table (and only that)
    286287            self.execute(

  • tests/schema/tests.py 

    diff --git a/tests/schema/tests.py b/tests/schema/tests.py
index ab8b07e9d3..2aaafc93e3 100644
    a b class SchemaTests(TransactionTestCase):  
    159159        if self.isolated_local_models:
    160160            with connection.schema_editor() as editor:
    161161                for model in self.isolated_local_models:
    162                     editor.delete_model(model)
     162                    editor.delete_model(model, cascade=True)
    163163
    164164    def delete_tables(self):
    165165        "Deletes all model tables for our models for a clean test environment"
    … … class SchemaTests(TransactionTestCase):  
    174174                if connection.features.ignores_table_name_case:
    175175                    tbl = tbl.lower()
    176176                if tbl in table_names:
    177                     editor.delete_model(model)
     177                    editor.delete_model(model, cascade=True)
    178178                    table_names.remove(tbl)
    179179            connection.enable_constraint_checking()
    180180
    … … class SchemaTests(TransactionTestCase):  
    15421542
    15431543        def drop_collation():
    15441544            with connection.cursor() as cursor:
    1545                 cursor.execute(f"DROP COLLATION IF EXISTS {ci_collation}")
     1545                cursor.execute(f"DROP COLLATION IF EXISTS {ci_collation} CASCADE")
    15461546
    15471547        with connection.cursor() as cursor:
    15481548            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.

comment:9 by Simon Charette, 6 days ago

Cc: Simon Charette added

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 when calling base.DatabaseSchemaEditor.delete_model though super() in your proposed alternative changes in the PR (and the quoted patch uses a different approach by redefining sql_delete_model_cascade) 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 in either versions of the patch.

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.

Last edited 6 days ago by Simon Charette (previous) (diff)

comment:10 by Vishy Algo, 3 days ago

Apologies for the oversight! You’re absolutely right. I was seeing SQLite errors in the migration test suite because test_base.cleanup_test_tables calls sql_delete_table_cascade directly. Since I had only defined that in the base editor with the CASCADE keyword, resulting in SQLite syntax errors.

I thought feature flag would be helpful to solve the discrepancy, ignoring that simply overriding sql_delete_table_cascade attribute in sqlite3/schema.py is much cleaner.

Note: See TracTickets for help on using tickets.
Back to Top