[Django] #32675: Migration autodetector detects unnecessary changes.
Django
--------------------------------------------+------------------------
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
--------------------------------------------+------------------------
Migration autodetector detects nonexistent changes in `ForeignKey`, it's
probably because previous migrations have the `to_field` parameter. For
Django itself it detects two changes:
- `django/contrib/admin/migrations/0004_alter_logentry_content_type.py`
{{{
migrations.AlterField(
model_name='logentry',
name='content_type',
field=models.ForeignKey(blank=True, null=True,
on_delete=django.db.models.deletion.SET_NULL,
to='contenttypes.contenttype', verbose_name='content type')
),
}}}
- `django/contrib/auth/migrations/0013_alter_permission_content_type.py`
{{{
migrations.AlterField(
model_name='permission',
name='content_type',
field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE,
to='contenttypes.contenttype', verbose_name='content type'),
)
}}}
Regression in aa4acc164d1247c0de515c959f7b09648b57dc42 (see #29899).
--
Ticket URL: <https://code.djangoproject.com/ticket/32675>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by David Wobrock):
Hi,
Indeed it seems that we didn't anticipate that this will generate new
migrations because of the missing `to_field`.
A lot of the modified tests in the initial PR are linked to the absence of
`to_field`
([https://github.com/django/django/commit/aa4acc164d1247c0de515c959f7b09648b57dc42
#diff-
506caa00017053ff8278de6efc2e59cc0c5cea22da9461482bdf16a9fc50af9eL1650 here
for instance])
My question would be: does this actually change any behaviour of the
existing migrations?
1/ If not, I guess we could stop taking `to_field` into account and
basically consider
{{{
models.ForeignKey(
to_field='id',
on_delete=models.SET_NULL,
blank=True, null=True,
to='contenttypes.ContentType',
verbose_name='content type',
)
}}}
and
{{{
models.ForeignKey(
on_delete=models.SET_NULL,
blank=True, null=True,
to='contenttypes.ContentType',
verbose_name='content type',
)
}}}
(without `to_field`) as identical. Which therefore wouldn't generate new
migrations.
2/ If it makes a behavioural difference, we would have to compute again
the `to_field` for the new model state.
When we dig into the technical whys, when deconstructing the old and new
fields (and
[https://github.com/django/django/commit/aa4acc164d1247c0de515c959f7b09648b57dc42
#diff-50cc08301fdd233fd64196aa90bc684d96ef557d99af67692995559646de567aL966
checking if they are different]), we have this one difference of
`to_field`:
{{{
(Pdb) p old_field_dec
('django.db.models.ForeignKey', [], {'verbose_name': 'content type',
'blank': True, 'null': True, 'on_delete': <function SET_NULL at
0x7fa5f297f9d0>, 'to': 'contenttypes.contenttype', 'to_field': 'id'})
(Pdb) p new_field_dec
('django.db.models.ForeignKey', [], {'verbose_name': 'content type',
'blank': True, 'null': True, 'on_delete': <function SET_NULL at
0x7fa5f297f9d0>, 'to': 'contenttypes.contenttype'})
}}}
So we can dig into how this `to_field` is generated for a `ForeignKey` =>
https://github.com/django/django/blob/main/django/db/models/fields/related.py#L889
{{{
to_meta = getattr(self.remote_field.model, "_meta", None)
if self.remote_field.field_name and (
not to_meta or (to_meta.pk and
self.remote_field.field_name != to_meta.pk.name)):
kwargs['to_field'] = self.remote_field.field_name
}}}
where `self.remote_field` is a `ManyToOneRel` that has a None `field_name`
attribute.
And if I follow the `ManyToOneRel` creation back correctly, we end up in
the `ForeignKey` constructor:
https://github.com/django/django/blob/main/django/db/models/fields/related.py#L786-L822
Where we can either define the `ManyToOneRel.field_name` in two ways:
- pass it explicitly (the way it is defined in the previous migration)
- or we retrieve it when `_meta` is available. However, it was the main
goal of the initial ticket #29899 to remove the rendering of models and
remove availability of `_meta`. (the code there referenced an old ticket
#12190, maybe that can help understand its purpose?)
I hope that helps in some way, and I'd be more than happy to work on a
patch if we can define what we want to do for this case :) (I mean, I
introduced this bug, so I definitely want to fix it ;-) )
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:1>
Django
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* stage: Unreviewed => Accepted
Comment:
The idea of considering `to_field='id'` equivalent to a non-specified one
is tempting but it could be wrong in theory even if it's rarely the case
in practice.
I personally don't think it's a big deal in practice since the detected
`AlterField` is basically a noop if actually applied if I understand
correctly?
I'd be up for documenting that some noop operations might be generated due
to this and we could alter the historical `admin` and `auth` ones to drop
the `to_field` which is a remnant of Django 1.7 IIRC?
If this is not deemed acceptable we could always resolve `to_field` as
David described to prevent any behavior change.
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:2>
Django
Reporter: Mariusz Felisiak | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 4.0
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
I agree with Simon. Let's add release notes about changes in the migration
autodetector with a warning that in some rare case a no-op operations
might be generated. We should also edit old migrations in Django itself.
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:3>
Django
Reporter: Mariusz Felisiak | Owner: David
| Wobrock
Type: Bug | Status: assigned
Component: Migrations | Version: 4.0
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by David Wobrock):
* owner: nobody => David Wobrock
* status: new => assigned
* has_patch: 0 => 1
Comment:
Suggested a PR to add this to the release note. Feel free to rephrase.
https://github.com/django/django/pull/14323
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:4>
Django
Reporter: Mariusz Felisiak | Owner: David
| Wobrock
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:5>
Django
Reporter: Mariusz Felisiak | Owner: David
| Wobrock
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"ee3b719a81e62751195853233813c198ab7b66bb" ee3b719a]:
{{{
#!CommitTicketReference repository=""
revision="ee3b719a81e62751195853233813c198ab7b66bb"
Refs #32675 -- Removed to_field from ForeignKeys in contrib apps'
migrations.
Refs #22889.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:6>
Django
Reporter: Mariusz Felisiak | Owner: David
| Wobrock
Component: Migrations | Version: 4.0
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"4ab3ef238edfa7c7b7c2c4420c7ae5b722bde121" 4ab3ef23]:
{{{
#!CommitTicketReference repository=""
revision="4ab3ef238edfa7c7b7c2c4420c7ae5b722bde121"
Fixed #32675 -- Doc'd that autodector changes might cause generation of
no-op migrations.
Follow up to aa4acc164d1247c0de515c959f7b09648b57dc42.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32675#comment:7>