Re: [Django] #34881: migrate crashes when renaming model referenced twice by ManyToManyField.through model on SQLite.
Django
ManyToManyField.through model on SQLite.
----------------------------+------------------------------------
Reporter: dennisvang | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------------
Comment (by dennisvang):
Replying to [comment:10 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`)
> {{{#!sql
>
> 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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:12>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
ManyToManyField.through model on SQLite.
----------------------------+------------------------------------
Reporter: dennisvang | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+------------------------------------
* cc: David Wobrock (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:13>
Django
ManyToManyField.through model on SQLite.
Reporter: dennisvang | Owner: jasehackman
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jasehackman):
* owner: nobody => jasehackman
* status: new => assigned
Comment:
I'm taking part in the DjangoCon US Sprints and am going to try and fix
this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:14>
Django
ManyToManyField.through model on SQLite.
Reporter: dennisvang | Owner: Jase Hackman
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jase Hackman):
I spent some time digging into this today.
To account for the unique nature of sqlite a `_remake_table()`
[https://github.com/jasehackman/django/blob/2abf417c815c20f41c0868d6f66520b32347106e/django/db/backends/sqlite3/schema.py#L75
link] method was added to recreate the table under the various conditions
under which it is required for table schema changes
[https://www.sqlite.org/lang_altertable.html#caution 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`.
[https://github.com/django/django/commit/e382a37f1b90bfdac7b22ed1a4e800b7a37cd28a
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:15>
Django
ManyToManyField.through model on SQLite.
----------------------------+----------------------------------------
Reporter: dennisvang | Owner: Jase Hackman
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------------------------
* cc: Jase Hackman (added)
Comment:
I made some progress on this:
- Created a regression test for the issue
[https://github.com/django/django/commit/f3c0557dd81284e5e47bcbc0de3ca94aced13743
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.
[https://github.com/django/django/commit/a694d0a24939611b056b95a2df929d1606a69006
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.
[https://github.com/django/django/commit/2b60f6767f1ae73b9812fee228907934c5349327
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.
[https://github.com/django/django/compare/main...jasehackman:django:ticket_34881
full set of changes so far]
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:16>
Django
ManyToManyField.through model on SQLite.
----------------------------+----------------------------------------
Reporter: dennisvang | Owner: Jase Hackman
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------------------------
Comment (by 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`
[https://github.com/django/django/blob/fdd1323b9c83e56184e0c992af8faf8d54327775/django/db/migrations/operations/models.py#L421
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`
{{{#!diff
diff --git a/django/db/migrations/operations/models.py
b/django/db/migrations/operations/models.py
index d616cafb45..83e3fb772d 100644
--- a/django/db/migrations/operations/models.py
+++ b/django/db/migrations/operations/models.py
@@ -421,11 +421,11 @@ def database_forwards(self, app_label,
schema_editor, from_state, to_state):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:17>
Django
ManyToManyField.through model on SQLite.
----------------------------+----------------------------------------
Reporter: dennisvang | Owner: Jase Hackman
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+----------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:18>
Django
ManyToManyField.through model on SQLite.
----------------------------+--------------------------------------
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------------------------
* owner: Jase Hackman => Anže Pečar
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:19>
Django
ManyToManyField.through model on SQLite.
----------------------------+--------------------------------------
Reporter: dennisvang | Owner: Anže Pečar
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Keywords: sqlite | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:20>
Django
ManyToManyField.through model on SQLite.
Type: Bug | Status: assigned
Component: Migrations | Version: dev
Severity: Normal | Resolution:
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sarah Boyce):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:21>
Django
ManyToManyField.through model on SQLite.
----------------------------+---------------------------------------------
Reporter: dennisvang | Owner: Anže Pečar
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+---------------------------------------------
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"e99187e5c94516ee35f37cc41a36d906b395808d" e99187e]:
{{{#!CommitTicketReference repository=""
revision="e99187e5c94516ee35f37cc41a36d906b395808d"
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.h...@zapier.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:22>
Django
ManyToManyField.through model on SQLite.
----------------------------+---------------------------------------------
Reporter: dennisvang | Owner: Anže Pečar
Type: Bug | Status: closed
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: sqlite | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+---------------------------------------------
In [changeset:"48382a2ff63a974f92bf5c959885947211e3642c" 48382a2]:
{{{#!CommitTicketReference repository=""
revision="48382a2ff63a974f92bf5c959885947211e3642c"
[5.1.x] Fixed #34881 -- Fixed a crash when renaming a model with multiple
Thank you to dennisvang for the report and Jase Hackman for the test.
Co-authored-by: Jase Hackman <jase.h...@zapier.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:23>
Django
ManyToManyField.through model on SQLite.
----------------------------+---------------------------------------------
Reporter: dennisvang | Owner: Anže Pečar
Type: Bug | Status: closed
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: sqlite | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+---------------------------------------------
Comment (by Sarah Boyce <42296566+sarahboyce@…>):
{{{#!CommitTicketReference repository=""
revision="fa7848146738a9fe1d415ee4808664e54739eeb7"
Refs #34881 -- Fixed
OperationTests.test_rename_m2m_field_with_2_references() test on Oracle.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:24>
Django
ManyToManyField.through model on SQLite.
----------------------------+---------------------------------------------
Reporter: dennisvang | Owner: Anže Pečar
Type: Bug | Status: closed
Component: Migrations | Version: dev
Severity: Normal | Resolution: fixed
Keywords: sqlite | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+---------------------------------------------
Comment (by Sarah Boyce <42296566+sarahboyce@…>):
{{{#!CommitTicketReference repository=""
revision="a0f6835f723167768d4f44df2c840eb91dd42c7c"
[5.1.x] Refs #34881 -- Fixed
OperationTests.test_rename_m2m_field_with_2_references() test on Oracle.
Backport of fa7848146738a9fe1d415ee4808664e54739eeb7 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34881#comment:25>