Migration autodetector doesn't handle placeholders in related_name for ForeignKey.

[Andrew Chen Wang]

We have a model in a package called [django-oauth-toolkit](​https://github.com/jazzband/django-oauth-toolkit/blob/0204383f7b6f8739322f2009f2bb25e8ac9bced2/oauth2_provider/models.py#L78)

Our model for reproduction:

class RefreshToken(models.Model):
    user = models.ForeignKey(
        settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="%(app_label)s_%(class)s"
    )

Users of the package are running makemigrations and having to see something like this (ref: ​https://github.com/jazzband/django-oauth-toolkit/issues/1037):

migrations.AlterField(
            model_name='refreshtoken',
            name='user',
            field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='%(app_label)s_%(class)s', to='auth.user'),
        ),

Specifically, to='auth.user' is the issue. Our previous migrations properly had to=settings.AUTH_USER_MODEL. Please let me know how to fix this or whether this is a bug. As noted in that linked issue, it might be due to ​https://docs.djangoproject.com/en/4.0/releases/4.0/#migrations-autodetector-changes, and if this functionality is intended, then how can packages... avoid this?

Thanks!

[Pedro Schlickmann Mendes]

This is probably not a bug.

I tested your package with django 3.2 and makemigrations returned No changes detected. I also tested it with Django 4.0 and makemigrations altered all models with settings.AUTH_USER_MODEL as a ForeignKey. I am not an expert but I think this is intended by the Django team.

I also tested changing your ForeignKey references to get_user_model() instead of settings.AUTH_USER_MODEL, and the results were the same, no changes in 3.2, changes in 4.0.

If there are no objections, I'll close this in a few days.

[Mariusz Felisiak]

This is a ​documented limitation, you cannot change the AUTH_USER_MODEL setting during the lifetime of a project.

[Keryn Knight]

I'm going to risk the wrath of the fellows and re-open to get it double-checked, because I think it's reproducible:

$ git checkout 3.2.10
$ django-admin startproject test33366
$ cd test33366
$ django-admin startapp testmodels
$ vim testmodels/models.py

put the following contents in and save:

class MyModel(models.Model):
    user = models.ForeignKey(
        settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="%(app_label)s_%(class)s"
    )

add testmodels to the INSTALLED_APPS and then resume:

$ python manage.py makemigrations testmodels
Migrations for 'testmodels':
  testmodels/migrations/0001_initial.py
    - Create model MyModel
$ python manage.py makemigrations testmodels
No changes detected in app 'testmodels'

looking OK so far, here's the migration:

dependencies = [
    migrations.swappable_dependency(settings.AUTH_USER_MODEL),
]

operations = [
    migrations.CreateModel(
        name='MyModel',
        fields=[
            ('id', models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
            ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='testmodels_mymodel', to=settings.AUTH_USER_MODEL)),
        ],
    ),
]

Now moving to 4.0:

$ git checkout 4.0
Previous HEAD position was 0153a63a67 [3.2.x] Bumped version for 3.2.10 release.
HEAD is now at 67d0c4644a [4.0.x] Bumped version for 4.0 release.
$ python manage.py makemigrations testmodels
/path/to/django/django/conf/__init__.py:222: RemovedInDjango50Warning: The USE_L10N setting is deprecated. Starting with Django 5.0, localized formatting of data will always be enabled. For example Django will display numbers and dates using the format of the current locale.
  warnings.warn(USE_L10N_DEPRECATED_MSG, RemovedInDjango50Warning)
Migrations for 'testmodels':
  testmodels/migrations/0002_alter_mymodel_user.py
    - Alter field user on mymodel

and the second migration is now:

dependencies = [
    ('auth', '0012_alter_user_first_name_max_length'),
    ('testmodels', '0001_initial'),
]

operations = [
    migrations.AlterField(
        model_name='mymodel',
        name='user',
        field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='%(app_label)s_%(class)s', to='auth.user'),
    ),
]

Note that it's no longer a swappable dependency, nor is it cased the same as as the global default ('auth.User' vs 'auth.user')...

[Mariusz Felisiak]

Thanks! Sorry I should check it more carefully. This is a regression in b9df2b74b98b4d63933e8061d3cfc1f6f39eb747 which is strictly related with placeholders in related_name for ForeignKey, not with settings.AUTH_USER_MODEL

[Simon Charette]

The auto-detector does all sort special handling for swappable models and I suspect what happened here is that not rendering models and binding fields anymore broke it the existing logic that was not covered by tests.

[Simon Charette]

This ended up being a more fundamental case handling issue with apps.Apps.get_swappable_settings_name that slipped under the radar for a while due to how the auto-detector use to render models.

[Simon Charette]

Removing the patch needs improvement flag as I believe the noop AlterField for the un-interpolated related_name is expected and documented in the Django 4.0 release notes (see #32676).

I think the conclusion in comment:6 is wrong as I managed to reproduce the swappable dependency issue without involving related_name and the noop AlterField manifestation on Django 4.0 can be achieved without involving swappable models.

If we want to address the noop AlterField migration being generated for all usages of placeholders in related_name there's two way we can do that.

1. Augment the documentation to mention under which circumstances such AlterField operations can be created and mention that it is safe to adjust historic migrations to use the proper placeholder syntax to prevent it from happening (it's safe even if your library supports Django < 4.0 as well because the models were rendered and thus related_name were evaluated at field binding time).
2. Adjust the auto-detector to consider related_name='app_model and '%(app_label)s_%(class)s' equals (in the context of a app.Model model) to prevent changes from being detected. This will have the unfortunate side effect to prevent changes of a field from harcoded to placeholders based related_name to be detected by the framework which could be considered a limitation of the previous model rendering based approach.

I personally think that 1. is the way to go but I don't have a strong feeling here either way.

[Mariusz Felisiak]

If we want to address the noop AlterField migration being generated for all usages of placeholders in related_name there's two way we can do that.

We can leave it as it is. Thanks for describing possible options.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 4328970:

Fixed #33366 -- Fixed case handling with swappable setting detection in migrations autodetector.

The migration framework uniquely identifies models by case insensitive
labels composed of their app label and model names and so does the app
registry in most of its methods (e.g. AppConfig.get_model) but it
wasn't the case for get_swappable_settings_name() until this change.

This likely slipped under the radar for so long and only regressed in
b9df2b74b98b4d63933e8061d3cfc1f6f39eb747 because prior to the changes
related to the usage of model states instead of rendered models in the
auto-detector the exact value settings value was never going through a
case folding hoop.

Thanks Andrew Chen Wang for the report and Keryn Knight for the
investigation.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 7e6a2e3b:

[4.0.x] Fixed #33366 -- Fixed case handling with swappable setting detection in migrations autodetector.

The migration framework uniquely identifies models by case insensitive
labels composed of their app label and model names and so does the app
registry in most of its methods (e.g. AppConfig.get_model) but it
wasn't the case for get_swappable_settings_name() until this change.

This likely slipped under the radar for so long and only regressed in
b9df2b74b98b4d63933e8061d3cfc1f6f39eb747 because prior to the changes
related to the usage of model states instead of rendered models in the
auto-detector the exact value settings value was never going through a
case folding hoop.

Thanks Andrew Chen Wang for the report and Keryn Knight for the
investigation.

Backport of 43289707809c814a70f0db38ca4f82f35f43dbfd from main