[Django] #33366: Foreign key to settings.AUTH_USER_MODEL causes hard-coded alteration in migration
Django
migration
-------------------------------------+-------------------------------------
Reporter: Andrew | Owner: nobody
Chen Wang |
Type: Bug | Status: new
Component: Database | Version: 4.0
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33366>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
migration
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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. Thanks!
New description:
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. Thanks!
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:1>
Django
migration
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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. Thanks!
New description:
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!
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:2>
Django
migration
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:3>
Django
migration
-------------------------------------+-------------------------------------
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
This is a [https://docs.djangoproject.com/en/stable/ref/settings/#auth-
user-model documented] limitation, you cannot change the `AUTH_USER_MODEL`
setting during the lifetime of a project.
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:4>
Django
migration
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Keryn Knight (added)
* status: closed => new
* resolution: invalid =>
Comment:
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'`)...
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:5>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: nobody
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: David Wobrock, Simon Charette (added)
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
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`
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:6>
Django
ForeignKey.
-------------------------------------+-------------------------------------
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Simon Charette
* status: new => assigned
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:7>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:8>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:9>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:10>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
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 history 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:11>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Type: Bug | Status: assigned
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:12>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Component: Database layer | Version: 4.0
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"43289707809c814a70f0db38ca4f82f35f43dbfd" 4328970]:
{{{
#!CommitTicketReference repository=""
revision="43289707809c814a70f0db38ca4f82f35f43dbfd"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:13>
Django
ForeignKey.
-------------------------------------+-------------------------------------
Reporter: Andrew Chen Wang | Owner: Simon
| Charette
Type: Bug | Status: closed
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Release blocker | Resolution: fixed
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:"7e6a2e3b4555c069c50cb949404938345bfb2ea6" 7e6a2e3b]:
{{{
#!CommitTicketReference repository=""
revision="7e6a2e3b4555c069c50cb949404938345bfb2ea6"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33366#comment:14>