Opened 14 months ago

Closed 5 months ago

Last modified 5 months ago

#34881 closed Bug (fixed)

migrate crashes when renaming model referenced twice by ManyToManyField.through model on SQLite.

Reported by: dennisvang Owned by: Anže Pečar
Component: Migrations Version: dev
Severity: Normal Keywords: sqlite
Cc: Simon Charette, Mariusz Felisiak, David Wobrock, Jase Hackman Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by dennisvang)

Description

Please consider the (contrived) minimal example below, part of an app called myapp:

class Person(models.Model):
    name = models.CharField(max_length=255)
    parents_or_children = models.ManyToManyField(
        to='self', through='myapp.Relation', blank=True
    )


class Relation(models.Model):
    parent = models.ForeignKey(
        to='myapp.Person',
        related_name='relations_as_parent',
        on_delete=models.CASCADE,
    )
    child = models.ForeignKey(
        to='myapp.Person',
        related_name='relations_as_child',
        on_delete=models.CASCADE,
    )

Now suppose I rename the Person model to Foo and update corresponding references.

Then I run manage.py makemigrations, which correctly recognizes that the model has been renamed.

Now, applying this migration to an empty database works, without issue, but applying the migration to a database with existing data fails with an IntegrityError.

Steps to reproduce

  1. start a new project, start a new app called myapp, with models as above.
  2. run makemigrations and migrate
  3. Load (valid) data from the following fixture:
    [
      {"model": "myapp.person", "pk": 1, "fields": {"name": "Jenny"}}, 
      {"model": "myapp.person", "pk": 2, "fields": {"name": "Johnny"}}, 
      {"model": "myapp.person", "pk": 3, "fields": {"name": "Mom"}}, 
      {"model": "myapp.person", "pk": 4, "fields": {"name": "Dad"}}, 
      {"model": "myapp.relation", "pk": 1, "fields": {"parent": 3, "child": 1}},
      {"model": "myapp.relation", "pk": 2, "fields": {"parent": 3, "child": 2}},
      {"model": "myapp.relation", "pk": 3, "fields": {"parent": 4, "child": 1}}, 
      {"model": "myapp.relation", "pk": 4, "fields": {"parent": 4, "child": 2}}
    ]
    
  4. rename the Person model to e.g. Foo and update all references in code
  5. run makemigrations and migrate again

What happens

The migrate command fails with

django.db.utils.IntegrityError: The row in table 'myapp_relation' with primary key '1' has an invalid foreign key: myapp_relation.child_id contains a value '1' that does not have a corresponding value in myapp_person.id.

But a Person with id=1 does exist in the database.

What I would expect to happen

I would expect this to work without any problems.

Notes

I also tried the same steps with an implicit through model , i.e. children = models.ManyToManyField(to='self', blank=True).
This works without issue.

Attachments (2)

0001_initial.py (2.0 KB ) - added by Natalia Bidart 14 months ago.
0002_rename_person_personfoo.py (342 bytes ) - added by Natalia Bidart 14 months ago.

Download all attachments as: .zip

Change History (27)

comment:1 by dennisvang, 14 months ago

Summary: Migration fails with IntegrityError after renaming model with existing m2m relations to selfMigration fails with IntegrityError after renaming model if m2m relations to self exist

comment:2 by dennisvang, 14 months ago

Summary: Migration fails with IntegrityError after renaming model if m2m relations to self existModel rename fails during migration if m2m relations to self exist

comment:3 by dennisvang, 14 months ago

Summary: Model rename fails during migration if m2m relations to self existModel-rename migration fails with IntegrityError if m2m relations to self exist

comment:4 by dennisvang, 14 months ago

Description: modified (diff)

comment:5 by dennisvang, 14 months ago

Description: modified (diff)

comment:6 by dennisvang, 14 months ago

Description: modified (diff)

comment:7 by dennisvang, 14 months ago

Description: modified (diff)

comment:8 by Natalia Bidart, 14 months ago

Hello! Thank you for your ticket.

Could you please attach the migrations generated by each of the makemigrations runs?

comment:9 by Natalia Bidart, 14 months ago

Following the ticket description, I have been able to reproduce. Migrations are attached, and SQLs are:

  • 0001
    BEGIN;
    --
    -- Create model Person
    --
    CREATE TABLE "ticket_34881_person" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(255) NOT NULL);
    --
    -- Create model Relation
    --
    CREATE TABLE "ticket_34881_relation" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "child_id" bigint NOT NULL REFERENCES "ticket_34881_person" ("id") DEFERRABLE INITIALLY DEFERRED, "parent_id" bigint NOT NULL REFERENCES "ticket_34881_person" ("id") DEFERRABLE INITIALLY DEFERRED);
    --
    -- Add field parents_or_children to person
    --
    CREATE TABLE "new__ticket_34881_person" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(255) NOT NULL);
    INSERT INTO "new__ticket_34881_person" ("id", "name") SELECT "id", "name" FROM "ticket_34881_person";
    DROP TABLE "ticket_34881_person";
    ALTER TABLE "new__ticket_34881_person" RENAME TO "ticket_34881_person";
    CREATE INDEX "ticket_34881_relation_child_id_77021f7a" ON "ticket_34881_relation" ("child_id");
    CREATE INDEX "ticket_34881_relation_parent_id_20447c41" ON "ticket_34881_relation" ("parent_id");
    COMMIT;
    
  • 0002 (I renamed Person to PersonFoo)
    BEGIN;
    --
    -- Rename model Person to PersonFoo
    --
    ALTER TABLE "ticket_34881_person" RENAME TO "ticket_34881_personfoo";
    CREATE TABLE "new__ticket_34881_relation" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "child_id" bigint NOT NULL REFERENCES "ticket_34881_personfoo" ("id") DEFERRABLE INITIALLY DEFERRED, "parent_id" bigint NOT NULL REFERENCES "ticket_34881_person" ("id") DEFERRABLE INITIALLY DEFERRED);
    INSERT INTO "new__ticket_34881_relation" ("id", "parent_id", "child_id") SELECT "id", "parent_id", "child_id" FROM "ticket_34881_relation";
    DROP TABLE "ticket_34881_relation";
    ALTER TABLE "new__ticket_34881_relation" RENAME TO "ticket_34881_relation";
    CREATE INDEX "ticket_34881_relation_child_id_77021f7a" ON "ticket_34881_relation" ("child_id");
    CREATE INDEX "ticket_34881_relation_parent_id_20447c41" ON "ticket_34881_relation" ("parent_id");
    CREATE TABLE "new__ticket_34881_relation" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "parent_id" bigint NOT NULL REFERENCES "ticket_34881_personfoo" ("id") DEFERRABLE INITIALLY DEFERRED, "child_id" bigint NOT NULL REFERENCES "ticket_34881_person" ("id") DEFERRABLE INITIALLY DEFERRED);
    INSERT INTO "new__ticket_34881_relation" ("id", "child_id", "parent_id") SELECT "id", "child_id", "parent_id" FROM "ticket_34881_relation";
    DROP TABLE "ticket_34881_relation";
    ALTER TABLE "new__ticket_34881_relation" RENAME TO "ticket_34881_relation";
    CREATE INDEX "ticket_34881_relation_parent_id_20447c41" ON "ticket_34881_relation" ("parent_id");
    CREATE INDEX "ticket_34881_relation_child_id_77021f7a" ON "ticket_34881_relation" ("child_id");
    COMMIT;
    

by Natalia Bidart, 14 months ago

Attachment: 0001_initial.py added

by Natalia Bidart, 14 months ago

comment:10 by Natalia Bidart, 14 months ago

Cc: Simon Charette Mariusz Felisiak added
Component: UncategorizedMigrations
Version: 4.2dev

Assuming a simpler model for Person where the M2M is implicit, and doing the rename, does indeed work. The SQL for the migration is:

  • 0003 (renamed SimplerPerson to SimplerPersonFoo)
    BEGIN;
    --
    -- Rename model SimplerPerson to SimplerPersonFoo
    --
    ALTER TABLE "ticket_34881_simplerperson" RENAME TO "ticket_34881_simplerpersonfoo";
    CREATE TABLE "ticket_34881_simplerpersonfoo_parents_or_children" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "from_simplerpersonfoo_id" bigint NOT NULL REFERENCES "ticket_34881_simplerpersonfoo" ("id") DEFERRABLE INITIALLY DEFERRED, "to_simplerpersonfoo_id" bigint NOT NULL REFERENCES "ticket_34881_simplerpersonfoo" ("id") DEFERRABLE INITIALLY DEFERRED);
    INSERT INTO "ticket_34881_simplerpersonfoo_parents_or_children" (id, from_simplerpersonfoo_id, to_simplerpersonfoo_id) SELECT id, from_simplerperson_id, to_simplerperson_id FROM "ticket_34881_simplerperson_parents_or_children";
    DROP TABLE "ticket_34881_simplerperson_parents_or_children";
    CREATE UNIQUE INDEX "ticket_34881_simplerpersonfoo_parents_or_children_from_simplerpersonfoo_id_to_simplerpersonfoo_id_f05f0b12_uniq" ON "ticket_34881_simplerpersonfoo_parents_or_children" ("from_simplerpersonfoo_id", "to_simplerpersonfoo_id");
    CREATE INDEX "ticket_34881_simplerpersonfoo_parents_or_children_from_simplerpersonfoo_id_6d3cdfb4" ON "ticket_34881_simplerpersonfoo_parents_or_children" ("from_simplerpersonfoo_id");
    CREATE INDEX "ticket_34881_simplerpersonfoo_parents_or_children_to_simplerpersonfoo_id_83aff647" ON "ticket_34881_simplerpersonfoo_parents_or_children" ("to_simplerpersonfoo_id");
    COMMIT;
    

It's worth noting that the content of the explicit M2M (Relation) has these rows:

sqlite> SELECT * FROM ticket_34881_relation;
1|1|3
2|2|3
3|1|4
4|2|4

While the rows for the implicit one include:

sqlite> SELECT * FROM ticket_34881_simplerperson_parents_or_children;
1|3|1
2|3|2
3|1|3
4|2|3
5|4|1
6|4|2
7|1|4
8|2|4

It may look like a valid issue but I'll cc Simon and Mariusz for a second opinion.

comment:11 by Mariusz Felisiak, 14 months ago

Keywords: sqlite added
Summary: Model-rename migration fails with IntegrityError if m2m relations to self existmigrate crashes when renaming model referenced twice by ManyToManyField.through model on SQLite.
Triage Stage: UnreviewedAccepted

Thanks for the report. As far as I'm aware, this is an issue only on SQLite, caused by remaking a table twice, where the second remake doesn't take into account the first alteration.

in reply to:  10 comment:12 by dennisvang, 14 months ago

Replying to Natalia Bidart:

Assuming a simpler model for Person where the M2M is implicit, and doing the rename, does indeed work. The SQL for the migration is:

  • 0003 (renamed SimplerPerson to SimplerPersonFoo)
    BEGIN;
    --
    -- Rename model SimplerPerson to SimplerPersonFoo
    --
    ALTER TABLE "ticket_34881_simplerperson" RENAME TO "ticket_34881_simplerpersonfoo";
    CREATE TABLE "ticket_34881_simplerpersonfoo_parents_or_children" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "from_simplerpersonfoo_id" bigint NOT NULL REFERENCES "ticket_34881_simplerpersonfoo" ("id") DEFERRABLE INITIALLY DEFERRED, "to_simplerpersonfoo_id" bigint NOT NULL REFERENCES "ticket_34881_simplerpersonfoo" ("id") DEFERRABLE INITIALLY DEFERRED);
    INSERT INTO "ticket_34881_simplerpersonfoo_parents_or_children" (id, from_simplerpersonfoo_id, to_simplerpersonfoo_id) SELECT id, from_simplerperson_id, to_simplerperson_id FROM "ticket_34881_simplerperson_parents_or_children";
    DROP TABLE "ticket_34881_simplerperson_parents_or_children";
    CREATE UNIQUE INDEX "ticket_34881_simplerpersonfoo_parents_or_children_from_simplerpersonfoo_id_to_simplerpersonfoo_id_f05f0b12_uniq" ON "ticket_34881_simplerpersonfoo_parents_or_children" ("from_simplerpersonfoo_id", "to_simplerpersonfoo_id");
    CREATE INDEX "ticket_34881_simplerpersonfoo_parents_or_children_from_simplerpersonfoo_id_6d3cdfb4" ON "ticket_34881_simplerpersonfoo_parents_or_children" ("from_simplerpersonfoo_id");
    CREATE INDEX "ticket_34881_simplerpersonfoo_parents_or_children_to_simplerpersonfoo_id_83aff647" ON "ticket_34881_simplerpersonfoo_parents_or_children" ("to_simplerpersonfoo_id");
    COMMIT;
    

It's worth noting that the content of the explicit M2M (Relation) has these rows:

sqlite> SELECT * FROM ticket_34881_relation;
1|1|3
2|2|3
3|1|4
4|2|4

While the rows for the implicit one include:

sqlite> SELECT * FROM ticket_34881_simplerperson_parents_or_children;
1|3|1
2|3|2
3|1|3
4|2|3
5|4|1
6|4|2
7|1|4
8|2|4

It may look like a valid issue but I'll cc Simon and Mariusz for a second opinion.

Thanks for reproducing this. :-)

You are right, for the implicit M2M relation, I should have set symmetrical=False in order to get the equivalent of my explicit Relation.

However, the rename also works fine for the implicit case with symmetrical=False.

comment:13 by David Wobrock, 14 months ago

Cc: David Wobrock added

comment:14 by Jase Hackman, 13 months ago

Owner: changed from nobody to Jase Hackman
Status: newassigned

I'm taking part in the DjangoCon US Sprints and am going to try and fix this issue.

comment:15 by Jase Hackman, 13 months ago

I spent some time digging into this today.

To account for the unique nature of sqlite a _remake_table() link method was added to recreate the table under the various conditions under which it is required for table schema changes sqlite docs.

The problem is that the code is not aware of previous calls of _remake_table() or if multiple will be required for a single table.

To fix this I am going to try to aggregate all of the alter statements into a single call, resulting in a single create statement.

To accomplish this I added an alter_fields() method to BaseDatabaseSchemaEditor that is passed a list of tuples to of the fields to be updated. It then loops over those fields to call the original alter_field() method. So far the new method is only being called in RenameModel. See commit with current progress

I next plan override this new method in the sqlite DatabaseSchemaEditor to allow for new logic that aggregates the alter statements into a single remake of the table. I'm going to continue down this path unless anyone has feedback, or recommendations towards a different approach.

comment:16 by Jase Hackman, 13 months ago

Cc: Jase Hackman added

I made some progress on this:

  • Created a regression test for the issue commit link
  • In the SQLite DatabaseSchemaEditor I laid out all the code paths that are possible. I still need to implement the one that will be the new single table rebuild. commit link
  • I have the beginnings of tests for all the code paths for alter_fields(). I'm still working out how to properly assert that I am getting the results I want, but I validated that each case is hitting to correct conditional via breakpoint. commit link containing current alter_fields code

Right now it is looking like I will need to repeat a lot of the logic super().alter_field() and from self._alter_field() to ensure all of the validation is consistent. So next I'm going to look into breaking some of that out into private method to keep things a bit more dry.

full set of changes so far

comment:17 by Simon Charette, 13 months ago

Thanks for working on this patch during DjangConUS Jase!

From reviewing your changes and tests I get a sense that there might be an even less invasive way to address this particular issue.

The crux of the problem here, as you've identified it, is the SQLite backend needs to rebuild the table entirely and it cannot be done from an old representation of the model iteratively.

By the time alter_field is called the columns of the tables related to the one being renamed are already altered in the database (a RENAME operation repoints all referencing columns) so it should be safe to provide the model originating from to_state like we do when dealing with self-referencing related objects.

By retrieving the model meant to be passed to alter_field from to to_state

  • django/db/migrations/operations/models.py

    diff --git a/django/db/migrations/operations/models.py b/django/db/migrations/operations/models.py
    index d616cafb45..83e3fb772d 100644
    a b def database_forwards(self, app_label, schema_editor, from_state, to_state):  
    421421                    model = new_model
    422422                    related_key = (app_label, self.new_name_lower)
    423423                else:
    424                     model = related_object.related_model
    425424                    related_key = (
    426425                        related_object.related_model._meta.app_label,
    427426                        related_object.related_model._meta.model_name,
    428427                    )
     428                    model = to_state.apps.get_model(*related_key)
    429429                to_field = to_state.apps.get_model(*related_key)._meta.get_field(
    430430                    related_object.field.name
    431431                )

The tests seem to be passing.

I think that your work on alter_fields might be beneficial to support tickets like #24203 but might not be strictly necessary to address this particular issue.

comment:18 by Anže Pečar, 6 months ago

I have picked this ticket up at the DjangoCon Europe sprint and opened a PR that adds Jase's test and Simon's solution: https://github.com/django/django/pull/18252

comment:19 by Anže Pečar, 6 months ago

Owner: changed from Jase Hackman to Anže Pečar

comment:20 by Simon Charette, 5 months ago

Has patch: set

comment:21 by Sarah Boyce, 5 months ago

Triage Stage: AcceptedReady for checkin

comment:22 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In e99187e:

Fixed #34881 -- Fixed a crash when renaming a model with multiple ManyToManyField.through references on SQLite.

Thank you to dennisvang for the report and Jase Hackman for the test.

Co-authored-by: Jase Hackman <jase.hackman@…>

comment:23 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In 48382a2:

[5.1.x] Fixed #34881 -- Fixed a crash when renaming a model with multiple ManyToManyField.through references on SQLite.

Thank you to dennisvang for the report and Jase Hackman for the test.

Co-authored-by: Jase Hackman <jase.hackman@…>

Backport of e99187e5c94516ee35f37cc41a36d906b395808d from main.

comment:24 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In fa78481:

Refs #34881 -- Fixed OperationTests.test_rename_m2m_field_with_2_references() test on Oracle.

comment:25 by Sarah Boyce <42296566+sarahboyce@…>, 5 months ago

In a0f6835f:

[5.1.x] Refs #34881 -- Fixed OperationTests.test_rename_m2m_field_with_2_references() test on Oracle.

Backport of fa7848146738a9fe1d415ee4808664e54739eeb7 from main.

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