Migration autodetector detects unnecessary changes.

[Mariusz Felisiak]

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

[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 (​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 ​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 ;-) )

[Simon Charette]

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.

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

[David Wobrock]

Suggested a PR to add this to the release note. Feel free to rephrase.
​https://github.com/django/django/pull/14323

[Mariusz Felisiak <felisiak.mariusz@…>]

In ee3b719a:

Refs #32675 -- Removed to_field from ForeignKeys in contrib apps' migrations.

Refs #22889.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4ab3ef23:

Fixed #32675 -- Doc'd that autodector changes might cause generation of no-op migrations.

Follow up to aa4acc164d1247c0de515c959f7b09648b57dc42.