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

[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.

[Natalia Bidart]

Hello! Thank you for your ticket.

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

[Natalia Bidart]

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;
  

[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.

[Mariusz Felisiak]

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.

[dennisvang]

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.

[Jase Hackman]

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

[Jase Hackman]

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.

[Jase Hackman]

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

[Simon Charette]

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

@@ -421 +421 @@
                     model = new_model
                     related_key = (app_label, self.new_name_lower)
                 else:
-                    model = related_object.related_model
                     related_key = (
                         related_object.related_model._meta.app_label,
                         related_object.related_model._meta.model_name,
                     )
+                    model = to_state.apps.get_model(*related_key)
                 to_field = to_state.apps.get_model(*related_key)._meta.get_field(
                     related_object.field.name
                 )

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.