Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#33366 closed Bug (fixed)

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

Reported by: Andrew Chen Wang Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 4.0
Severity: Release blocker Keywords:
Cc: Keryn Knight, David Wobrock, Simon Charette Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by 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!

Change History (14)

comment:1 by Andrew Chen Wang, 3 years ago

Description: modified (diff)

comment:2 by Andrew Chen Wang, 3 years ago

Description: modified (diff)

comment:3 by Pedro Schlickmann Mendes, 3 years ago

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.

comment:4 by Mariusz Felisiak, 3 years ago

Resolution: invalid
Status: newclosed

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

comment:5 by Keryn Knight, 3 years ago

Cc: Keryn Knight added
Resolution: invalid
Status: closednew

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

comment:6 by Mariusz Felisiak, 3 years ago

Cc: David Wobrock Simon Charette added
Severity: NormalRelease blocker
Summary: Foreign key to settings.AUTH_USER_MODEL causes hard-coded alteration in migrationMigration autodetector doesn't handle placeholders in related_name for ForeignKey.
Triage Stage: UnreviewedAccepted

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

comment:7 by Simon Charette, 3 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

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.

comment:8 by Simon Charette, 3 years ago

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.

comment:9 by Simon Charette, 3 years ago

Has patch: set
Last edited 3 years ago by Mariusz Felisiak (previous) (diff)

comment:10 by Mariusz Felisiak, 3 years ago

Patch needs improvement: set

comment:11 by Simon Charette, 3 years ago

Patch needs improvement: unset

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 happeing (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 '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 approach model rendering based approach.

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

Version 1, edited 3 years ago by Simon Charette (previous) (next) (diff)

comment:12 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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.

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

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.

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top