#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 )
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
- start a new project, start a new app called
myapp
, with models as above. - run
makemigrations
andmigrate
- 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}} ]
- rename the
Person
model to e.g.Foo
and update all references in code - run
makemigrations
andmigrate
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)
Change History (27)
comment:1 by , 13 months ago
Summary: | Migration fails with IntegrityError after renaming model with existing m2m relations to self → Migration fails with IntegrityError after renaming model if m2m relations to self exist |
---|
comment:2 by , 13 months ago
Summary: | Migration fails with IntegrityError after renaming model if m2m relations to self exist → Model rename fails during migration if m2m relations to self exist |
---|
comment:3 by , 13 months ago
Summary: | Model rename fails during migration if m2m relations to self exist → Model-rename migration fails with IntegrityError if m2m relations to self exist |
---|
comment:4 by , 13 months ago
Description: | modified (diff) |
---|
comment:5 by , 13 months ago
Description: | modified (diff) |
---|
comment:6 by , 13 months ago
Description: | modified (diff) |
---|
comment:7 by , 13 months ago
Description: | modified (diff) |
---|
comment:8 by , 13 months ago
comment:9 by , 13 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
toPersonFoo
)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 , 13 months ago
Attachment: | 0001_initial.py added |
---|
by , 13 months ago
Attachment: | 0002_rename_person_personfoo.py added |
---|
follow-up: 12 comment:10 by , 13 months ago
Cc: | added |
---|---|
Component: | Uncategorized → Migrations |
Version: | 4.2 → dev |
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
toSimplerPersonFoo
)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 , 13 months ago
Keywords: | sqlite added |
---|---|
Summary: | Model-rename migration fails with IntegrityError if m2m relations to self exist → migrate crashes when renaming model referenced twice by ManyToManyField.through model on SQLite. |
Triage Stage: | Unreviewed → Accepted |
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.
comment:12 by , 13 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
toSimplerPersonFoo
)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|4While 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|4It 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 , 13 months ago
Cc: | added |
---|
comment:14 by , 13 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
I'm taking part in the DjangoCon US Sprints and am going to try and fix this issue.
comment:15 by , 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 , 13 months ago
Cc: | 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.
comment:17 by , 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): 421 421 model = new_model 422 422 related_key = (app_label, self.new_name_lower) 423 423 else: 424 model = related_object.related_model425 424 related_key = ( 426 425 related_object.related_model._meta.app_label, 427 426 related_object.related_model._meta.model_name, 428 427 ) 428 model = to_state.apps.get_model(*related_key) 429 429 to_field = to_state.apps.get_model(*related_key)._meta.get_field( 430 430 related_object.field.name 431 431 )
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 , 5 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 , 5 months ago
Owner: | changed from | to
---|
comment:20 by , 5 months ago
Has patch: | set |
---|
comment:21 by , 5 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Hello! Thank you for your ticket.
Could you please attach the migrations generated by each of the
makemigrations
runs?