[Django] #30966: Migration crashes due to index issue, depending on otherwise irrelevant order
Django
order
-------------------------------------+-------------------------------------
Reporter: Peter | Owner: nobody
Thomassen |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords: migration
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
In my Django app "desecapi" (reduced to a minimal example), I have two
migrations. The first migration defines the following models:
User
+ Token (Token.user relates to User.id)
+ Domain (Domain.owner relates to User.id)
...+ RRset (RRset.domain relates to Domain.id)
... where "+" denotes foreign-key relationships. Note that RRset is a
grandchild of User.
When running the two migrations together on an empty database (i.e.
running `python manage.py desecapi 0002` instead of `... 0001` and `...
0002` separately), the second migration fails with:
`django.db.utils.OperationalError: (1833, "Cannot change column 'id': used
in a foreign key constraint
'desecapi_token_user_id_bfc397b3_fk_desecapi_user_id' of table
'desec.desecapi_token'")`
When running them separately, both migrations work. I would expect that
running in bulk or each on its own should not make a difference.
The issue can be fixed by switching the order of two operations in the
first migration. One of these two operations is the one that creates the
`desecapi_token` table (mentioned in the above error), and the other
operation changes an attribute of the RRset.subname field. That model is
not related to the Token model, and also not directly related to the User
model (which is touch in the second migration).
I would not expect this ordering to influence whether the second migration
can run successfully or not.
(One can say that the operation changing RRset.subname is unnecessary, as
it could be incorporated in RRset's "CreateModel" operation. However, the
history of encountering the problem is that RRset.subname in fact was
changed in an intermediate migration. It just seems unnecessary because
this minimal example is very much reduced.)
The minimal example can be found here: https://github.com/peterthomassen
/django-bug/tree/master/api/desecapi
Note that in the interest of having a minimal example, models.py does not
contain all models that the migrations deal with. My understanding is that
that's fine (think of the missing models and fields as removed, but there
is no migration yet that reflects that).
I created a docker-compose application that sets up an empty database and
reproduces the problem. To reproduce, run the following:
git clone https://github.com/peterthomassen/django-bug.git
cd django-bug/
docker-compose build
docker-compose up -d dbapi # after this, wait for 2-3 minutes so that
the database is up
docker-compose run api bash
# now, at the container shell, compare
python manage.py migrate desecapi 0002
# vs (make sure to clean up the database)
python manage.py migrate desecapi 0001
python manage.py migrate desecapi 0002
--
Ticket URL: <https://code.djangoproject.com/ticket/30966>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
order
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migration | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "models.py" added.
Django
order
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migration | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "0001_initial.py" added.
Django
order
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migration | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "0002_alter_user_id.py" added.
Django
irrelevant order
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: migration | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:1>
Django
irrelevant order on MySQL.
---------------------------------+------------------------------------
Reporter: Peter Thomassen | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* keywords: migration => migration mySQL
* version: 2.2 => master
* component: Database layer (models, ORM) => Migrations
* stage: Unreviewed => Accepted
Comment:
Thanks for this report, I confirmed this issue on MySQL. Scenario is
really tricky, without the last operation i.e.
{{{
migrations.AlterField(
model_name='rrset',
name='subname',
field=models.CharField(blank=False, max_length=178),
),
}}}
everything works fine. When we take it into account, migrations don't drop
a `FOREIGN KEY` constraint from `desecapi_token`.
Reproduced at 85efc14a2edac532df1a9ad4dd9b6d4a4dcf583e.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:2>
Django
irrelevant order on MySQL.
---------------------------------+------------------------------------
Reporter: Peter Thomassen | Owner: nobody
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* cc: Simon Charette (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:3>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => Hasan Ramezani
Comment:
I investigate it and think the problem comes from this line:
{{{
related_objects_graph[f.remote_field.model._meta.concrete_model._meta].append(f)
}}}
of
[https://github.com/django/django/blob/30359496a3f3d9af0b02afc334710f7e24c74f5b/django/db/models/options.py#L685
django.db.models.options.Options._populate_directed_relation_graph()]:
I added some log to check `related_objects_graph` in bothe case.
When the migration runs by `./manage.py migrate desecapi 0001` and then
`./manage.py migrate desecapi 0002` (correct case), here is the result:
{{{
defaultdict(<class 'list'>, {<Options for User>:
[<django.db.models.fields.related.ForeignKey: owner>,
<django.db.models.fields.related.ForeignKey: user>], <Options for Domain>:
[<django.db.models.fields.related.ForeignKey: domain>]})
}}}
But, When the migration runs by `./manage.py migrate desecapi 0002` (case
with problem), here is the result:
{{{
defaultdict(<class 'list'>, {<Options for User>:
[<django.db.models.fields.related.ForeignKey: user>], <Options for User>:
[<django.db.models.fields.related.ForeignKey: owner>], <Options for
Domain>: [<django.db.models.fields.related.ForeignKey: domain>]})
}}}
From the result, it is obvious that in the first case we have the
`<Options for User>` key with a value of type list with two items
`[<django.db.models.fields.related.ForeignKey: owner>,
<django.db.models.fields.related.ForeignKey: user>]`.
which generate two alter commands:
{{{
ALTER TABLE `app1_domain` DROP FOREIGN KEY
`app1_domain_owner_id_cc7262a2_fk_app1_user_id`
ALTER TABLE `app1_token` DROP FOREIGN KEY
`app1_token_user_id_5ea51092_fk_app1_user_id`
}}}
But in the second case, we have two keys `<Options for User>` which one
of them has a value of type list with one item
`[<django.db.models.fields.related.ForeignKey: user>]`
and the other has a value of type list with one item
`[<django.db.models.fields.related.ForeignKey: owner>]`.
which generate just one alter command:
{{{
ALTER TABLE `app1_domain` DROP FOREIGN KEY
`app1_domain_owner_id_cc7262a2_fk_app1_user_id`
}}}
Thus, the second foreign key `app1_token_user_id_5ea51092_fk_app1_user_id`
still remains and cause the error.
Any suggestions for a fix?
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:4>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Postgres has this issue as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:5>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Hasan, the solution you proposed will likely address this issue but the
fact multiple instances of `Options` for the same model classes are mixed
in there is likely a symptom or a larger issue.
I'm not sure of the origin of it but this can only happen when model from
different `apps` boundaries are mixed together which should be disallowed
as it will cause all sort of weird issues. What I mean by that is that
there's likely ''something'' that creates models from a migration project
state and have ''real'' models reference them or vice-versa. You can
assert this doesn't happen by making sure each `model._meta.apps is
self.apps`.
In short I think this solution will only temporarily hide a much larger
problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:6>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
From giving a quick look at the current implementation it looks we're
unfortunately sharing `__fake__` models bound to different `StateApps` for
performance reason to work around the fact the schema editors are still
operating from model classes instead of model states #29898.
Since we'll likely won't be able to prevent this sharing from taking place
in the short term for performance reasons we should investigate ''why''
the models cache is not appropriately busted to avoid having two different
`Options` instances for the `User` model. In this case it looks like the
`Token` fake model is not reloaded while the `RRset`, `User`, and `Domain`
ones are when the `AlterField(model_name="RRset")` is performed. That
makes `Token.user` point to the ''stale'' `User` model and `Domain.owner`
point to the new one.
I highly suspect this ticket is related to #27737 and
[https://github.com/django/django/blob/b93a0e34d9b9b99d41103782b7e7aeabf47517e3/django/db/migrations/operations/fields.py#L231-L239
this TODO] in `AlterField.state_forwards`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:7>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Markus Holtermann (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:8>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Simon, thanks for an investigation! I agree that we should try to address
the main issue.
Hasan, Can you take a look?
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:9>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hasan Ramezani):
Sure, I can check it again.
Just for confirmation, Should I fix assertaion failures based on Simon's
patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:10>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Hasan, the changes in `django/db/migrations/state.py` from comment:7 are
likely desired but the assertion in `Options` were more of a way to test
which operation currently break this contract. From your submitted PR only
three `migrations` tests were failing with these assertions including the
one you added even on SQLite.
Again, I suspect the cause is #27737 and the `TODO` in
`AlterField.state_forwards`. The fact the rendering is delayed prevents
`Token` from being refreshed adequately.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:11>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hasan Ramezani):
Simon, I checked the `TODO` in `AlterField.state_forwards`, here is my
understanding.
When I run the test in my the patch(my patch in
[https://github.com/django/django/pull/12061 PR] + your patch in
comment:7) it fails on alter operation in `0002_alter_user_id` at line
`assert f.remote_field.model._meta.concrete_model._meta.apps is self.apps`
{{{
operations = [
migrations.AlterField(
model_name="user",
name="id",
field=models.CharField(default=uuid.uuid4, max_length=32,
primary_key=True),
),
]
}}}
It fails because there is an operation in `0001_initial` which run before
the above mentioned migration:
{{{
migrations.AlterField(
model_name="RRset",
name="subname",
field=models.CharField(blank=False, max_length=178),
),
}}}
this is an alter migration and the `AlterField.state_forwards` reloads 3
models of 4 (User, Domain, RRest) and don't reload the Token model.
If I reload the Token model this test will be passed.
Should I change the function to find proper models to reload?
Is it the right solution?
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:12>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Markus Holtermann):
As already suspected by Simon, this is indeed related to Django not
reloading related models when non-relational fields change.
I see a few ways forward within the migration system, with various pros
and cons
1. Reload all related models, regardless if the changed field is a
relational field or referenced by a relational field. This will come with
a huge run-time penalty that the delaying brought
1. Try to catch these kind of database error or do some Python-level
checking beforehand (`assert model1 is
model2._meta.get_field("foo").related_model`). If we run into any issue
there, reload all related models. Depending on how expensive that is, we
could possibly do that after each migration operation and make the delayed
model reloading based on that.
1. Reload all models after each migrations is applied when the
`ProjectState.is_delayed is False`. That would bring the benefit that it
doesn't matter if one applies migrations individually or in a batch.
1. Leave as-is; the first migration can be optimized anyway
I haven't completely made up my mind about the "best" approach here. But
ideally we'd get rid of model classes in migrations and solely work off of
the `ModelState`. My last attempt got stuck do to the backwards
compatibility in the `SchemaEditor`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:13>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Markus Holtermann):
Replying to [comment:12 Hasan Ramezani]:
> this is an alter migration and the `AlterField.state_forwards` reloads
3 models of 4 (User, Domain, RRest) and don't reload the Token model.
> If I reload the Token model this test will be passed.
>
>
> Should I change the function to find proper models to reload?
> Is it the right solution?
This is essentially approach 1 from my list in [comment:13]
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:14>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Peter Thomassen):
Replying to [comment:13 Markus Holtermann]:
> 4. Leave as-is; the first migration can be optimized anyway
The first migration can trivially be optimized in this minimum example. In
our real example, we encountered the problem where migration 0003
interferes with migration 0011, with existing deployments somewhere in the
middle.
In that case, 0003 and 0011 can be optimized/squashed more or less easily
for new deployments, but migration 0011 would have to be maintained
individually for those deployments which already have all migrations up to
that one applied. The result would be code duplication for "fresh" and
"existing" deployments, and -- if this problem reoccurs -- maybe even more
"squashed migration variations".
So far, squashing was optional, and it would become a mandatory mechanism,
while producing code duplication in cases as described above. In my
opinion, that would be the least favorable option.
Thanks a LOT for your time, and for thinking though this whole thing!
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:15>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Markus Holtermann):
This is a minimal failing test
{{{#!diff
diff --git a/tests/migrations/test_state.py
b/tests/migrations/test_state.py
index e9bdac93c5..9140609bb6 100644
--- a/tests/migrations/test_state.py
+++ b/tests/migrations/test_state.py
@@ -928,6 +928,64 @@ class StateTests(SimpleTestCase):
choices_field = Author._meta.get_field('choice')
self.assertEqual(list(choices_field.choices), choices)
+ def test_reload_related_models_on_non_relational_fields(self):
+ """
+ #30966 - Even when fields on a model change that are not involved
in
+ a relation, the model gets reloaded. Ensure that other models
pointing
+ to or from it are reloaded accordingly.
+
+ User <-- Domain <-- RRset
+ ^
+ +-- Token
+ """
+ project_state = ProjectState()
+ # Render project state to simulate initial migration state
+ project_state.apps
+ project_state.add_model(ModelState(
+ "migrations",
+ "User",
+ [
+ ("id", models.AutoField(primary_key=True)),
+ ("email", models.EmailField(max_length=191,
unique=True)),
+ ]
+ ))
+ project_state.add_model(ModelState(
+ "migrations",
+ "Domain",
+ [
+ ("id", models.AutoField(primary_key=True)),
+ ("owner", models.ForeignKey("migrations.User",
models.SET_NULL)),
+ ]
+ ))
+ project_state.add_model(ModelState(
+ "migrations",
+ "RRset",
+ [
+ ("id", models.AutoField(primary_key=True)),
+ ("subname", models.CharField(blank=True,
max_length=178)),
+ ("domain", models.ForeignKey("migrations.Domain",
models.SET_NULL)),
+ ]
+ ))
+ project_state.add_model(ModelState(
+ "migrations",
+ "Token",
+ [
+ ("id", models.AutoField(primary_key=True)),
+ ("user", models.ForeignKey("migrations.User",
models.SET_NULL)),
+ ]
+ ))
+ AlterField(
+ model_name="RRset",
+ name="subname",
+ field=models.CharField(blank=False, max_length=178),
+ ).state_forwards("migrations", project_state)
+
+ all_models = project_state.apps.all_models["migrations"]
+ assert (
+ all_models['token']._meta.get_field('user').related_model
+ is all_models['user']
+ ), "Models are not identical"
+
class ModelStateTests(SimpleTestCase):
def test_custom_model_base(self):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:16>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Markus Holtermann):
Replying to [comment:15 Peter Thomassen]:
> Replying to [comment:13 Markus Holtermann]:
> > 4. Leave as-is; the first migration can be optimized anyway
>
> The first migration can trivially be optimized in this minimum example.
In our real example, we encountered the problem where migration 0003
interferes with migration 0011, with existing deployments somewhere in the
middle.
>
> In that case, 0003 and 0011 can be optimized/squashed more or less
easily for new deployments, but migration 0011 would have to be maintained
individually for those deployments which already have all migrations up to
that one applied. The result would be code duplication for "fresh" and
"existing" deployments, and -- if this problem reoccurs -- maybe even more
"squashed migration variations".
>
> So far, squashing was optional, and it would become a mandatory
mechanism, while producing code duplication in cases as described above.
In my opinion, that would be the least favorable option.
Yeah, that'd be true. As said, none of this is really ideal.
What we're doing here are unfortunately patches upon patches upon patches.
I'd really like to move onto the "right" way.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:17>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Markus Holtermann):
I took a shot at implementing something along the lines of what I
suggested in 2. in [comment:13]:
https://github.com/MarkusH/django/pull/new/ticket30966 . At this point it
does not handle unapplying of migrations.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:18>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Thanks Markus.
The patch will likely need a bit of polishing but a solution will likely
be out best shot until we only deal with model states.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:19>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Hasan Ramezani):
Thanks Markus for the patch.
If there is something that can be done by me, please let me know.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:20>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
After doing a bit of investigation work for #31064 and revisiting #29000 I
think the solution proposed by Hasan makes the most sense for now and
here's why.
Let's take the setup defined to test #29000 as an example
{{{#!python
class Pony(models.Model):
name = models.CharField(max_length=20)
class Rider(models.Model):
pony = models.ForeignKey('test_rename_multiple.Pony', models.CASCADE)
class PonyRider(models.Model):
riders = models.ManyToManyField('Rider')
}}}
If a `RenameField('Pony', 'name', 'fancy_field)` operation is applied and
`Pony` is rendered from the new state only the `Rider.pony` pointer is
actually stale and needs to be repointed.
However we decided not to follow the repointing route because of all the
trouble it incurs. Instead we re-render only the direct relationships if
`delay=True` (only `Rider` in this case) or the full relationship tree if
`delay=False` (`Rider` and `PonyRider` in this case). A full relationship
tree re-rendering is slow hence why we try to use `delay=True` as much as
possible but when we do so references to direct relationships of the
reloaded models are left pointing at pre-re-rendering `Model` classes
which breaks `Options._populate_directed_relation_graph` mapping because
it currently relies on `Options` identity as key.
To reuse our example setup above, if `PonyRider -> Rider -> Pony` and we
`reload_model('app', 'pony', delay=True)` then `Pony` and `Rider` will be
reloaded as `Rider' -> Pony'` but `PonyRider` will still point at `Pony`
and not `Pony'` which makes any following operation relying on
`Pony'._meta.related_objects` break because `Pony'._meta is not
Pony._meta`.
If we look at how #29000 was addressed
[https://github.com/django/django/commit/fcc4e251dbc917118f73d7187ee2f4cbf3883f36
#diff-efb7d00c2383393046c9c5d842f45499R290 we can see that it works around
this issue by forcing a full relationship tree re-rendering as soon as any
model refers to a model on which a field is renamed]. This is really
wasteful but it gets the test passing in this particular case which is
more a bandaid than anything else. Fundamentally the `delay=True` model if
broken if it doesn't allow non-re-rendered models to maintain some
relationship with the re-rendered graph.
For these reasons and because model identity consistency is not necessary
beyond direct relationship by the current schema editor I think that
adjusting `Options._populate_directed_relation_graph` to use
`Options.label` as relationship key is our best way forward here as it
doesn't hurt non-`__fake__` model relationship resolution and it allows
`StateApps` to keep relying on the `delay=True` for model rendering until
schema alterations don't necessitate access to `ProjectState.apps`
anymore.
I'll submit a PR that reverts unnecessary `RenameField.state_forwards`
changes introduced by fcc4e251dbc917118f73d7187ee2f4cbf3883f36 and use
Hasan's `_populate_directed_relation_graph` adjustments to demonstrate
that the tests are passing. From that point, if everyone agrees that this
is the right way forward, I think we should keep this ticket opened as a
marker to add regression tests showing this is was properly addressed.
Thoughts on that approach Markus and Hasan?
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:21>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Submitted [https://github.com/django/django/pull/12665 a PR] to
demonstrate it properly addresses #29000 and passes the suite.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:22>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"1d16c5d562a67625f7309cc7765b8c57d49bf182" 1d16c5d]:
{{{
#!CommitTicketReference repository=""
revision="1d16c5d562a67625f7309cc7765b8c57d49bf182"
Refs #27666 -- Ensured relationship consistency on delayed reloads.
Delayed reloads of state models broke identity based relationships
between direct and non-direct ancestors.
Basing models.Options related objects map of model labels instead of
their identity ensured relationship consistency is maintained.
Refs #30966.
Co-authored-by: Hasan Ramezani <hasa...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:23>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: migration mySQL | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"db6933a032c850153a688b6c977691b37ca02745" db6933a]:
{{{
#!CommitTicketReference repository=""
revision="db6933a032c850153a688b6c977691b37ca02745"
Refs #30966 -- Added test for reloading related model state on non-
relational changes.
Thanks Markus Holtermann for initial test.
Fixed in 1d16c5d562a67625f7309cc7765b8c57d49bf182.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:24>
Django
irrelevant order on MySQL.
-------------------------------------+-------------------------------------
Reporter: Peter Thomassen | Owner: Hasan
| Ramezani
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: migration mySQL | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* needs_better_patch: 1 => 0
* resolution: => fixed
Comment:
Fixed in 1d16c5d562a67625f7309cc7765b8c57d49bf182.
--
Ticket URL: <https://code.djangoproject.com/ticket/30966#comment:25>