[Django] #23790: Possible bad interaction between migration dependencies and relabeling apps
Django
-----------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.7
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+------------------------
As far as I can tell, in migrations, `dependencies` is a list of
`(app_label, migration_name)`.
I'm wondering what happens if one relabels an application with
`AppConfig.label`. How can migrations handle this case?
(I haven't tried to create such a problem. I'm just making a note before I
forget.)
--
Ticket URL: <https://code.djangoproject.com/ticket/23790>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* cc: info+coding@… (added)
Old description:
> As far as I can tell, in migrations, `dependencies` is a list of
> `(app_label, migration_name)`.
>
> I'm wondering what happens if one relabels an application with
> `AppConfig.label`. How can migrations handle this case?
>
> (I haven't tried to create such a problem. I'm just making a note before
> I forget.)
New description:
As far as I can tell, in migrations, `dependencies` is a list of
`(app_label, migration_name)`.
I'm wondering what happens if one relabels an application with
`AppConfig.label`. How can migrations handle this case?
(I haven't tried to create such a problem. I'm just making a note before I
forget.)
The only way I can think of is using the `appconfig.name` attribute
instead of `appconfig.label`. But that's hard to find a smooth migration
path.
--
Comment:
It partially works. If there are no migrations for an app yet, all
migrations will be created with the changed label in the dependencies and
all places it's being referenced. But as soon as you already have existing
migrations and then change the label of an app, the `makemigrations`
command blows up with:
{{{#!python
Traceback (most recent call last):
File "manage.py", line 10, in <module>
execute_from_command_line(sys.argv)
File "/home/markus/Coding/django/django/core/management/__init__.py",
line 338, in execute_from_command_line
utility.execute()
File "/home/markus/Coding/django/django/core/management/__init__.py",
line 330, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/home/markus/Coding/django/django/core/management/base.py", line
390, in run_from_argv
self.execute(*args, **cmd_options)
File "/home/markus/Coding/django/django/core/management/base.py", line
442, in execute
output = self.handle(*args, **options)
File
"/home/markus/Coding/django/django/core/management/commands/makemigrations.py",
line 58, in handle
loader = MigrationLoader(None, ignore_no_migrations=True)
File "/home/markus/Coding/django/django/db/migrations/loader.py", line
48, in __init__
self.build_graph()
File "/home/markus/Coding/django/django/db/migrations/loader.py", line
295, in build_graph
_reraise_missing_dependency(migration, parent, e)
File "/home/markus/Coding/django/django/db/migrations/loader.py", line
265, in _reraise_missing_dependency
raise exc
File "/home/markus/Coding/django/django/db/migrations/loader.py", line
291, in build_graph
self.graph.add_dependency(migration, key, parent)
File "/home/markus/Coding/django/django/db/migrations/graph.py", line
47, in add_dependency
parent
django.db.migrations.graph.NodeNotFoundError: Migration testapp.0002_proxy
dependencies reference nonexistent parent node ('otherapp',
'0001_initial')
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.7
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by MarkusH:
Old description:
> As far as I can tell, in migrations, `dependencies` is a list of
> `(app_label, migration_name)`.
>
> I'm wondering what happens if one relabels an application with
> `AppConfig.label`. How can migrations handle this case?
>
> (I haven't tried to create such a problem. I'm just making a note before
> I forget.)
>
> The only way I can think of is using the `appconfig.name` attribute
> instead of `appconfig.label`. But that's hard to find a smooth migration
> path.
New description:
As far as I can tell, in migrations, `dependencies` is a list of
`(app_label, migration_name)`.
I'm wondering what happens if one relabels an application with
`AppConfig.label`. How can migrations handle this case?
(I haven't tried to create such a problem. I'm just making a note before I
forget.)
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.7
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 aaugustin):
I'm afraid we messed up that part of the API and it's too late to fix :-|
Andrew, do you have any thoughts on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
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 aaugustin):
* type: Uncategorized => Bug
* component: Uncategorized => Migrations
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
To me this seems like a straightforward case of "not every edge case can
be automatic." The fix is manual, but simple, right? You just have to
replace that app label in all existing migrations that reference it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:5>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
That's inconvenient when the affected app is a third-party app.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:6>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
A third-party app with models changing its label has always meant changing
the name of all its database tables, too (unless it uses `Meta.db_table`
to preserve them). So it's always been the case that changing the label of
an app with models is a Fairly Backwards Incompatible Thing To Do. I guess
what's new in 1.7 is that a) changing app-label is easier than it used to
be (no longer implies changing import path), and b) migrations introduce a
new layer of label-dependence that isn't fixed by using a backwards-
compatible `Meta.db_table`.
I'm having trouble seeing a way to make this smoother, other than possibly
improving the error message (though the current one isn't terrible) and
probably adding a note to the docs in the `AppConfig` section clarifying
that changing `app_label` midstream is a breaking change for any existing
installs of that app. Fundamentally, migrations need some way to reliably
and persistently reference an app, and that's always been the purpose of
app-label.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:7>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* component: Migrations => Documentation
Comment:
A warning in the documentation of `AppConfig.label` is the way to go then.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:8>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
This does mean that, in the new world of required migrations, using
`AppConfig` to relabel third-party apps is pretty much impossible (due to
app-label being hardcoded in migration dependencies). So the idea that
`AppConfig.label` can allow using two third-party apps with conflicting
labels is dead (if those apps both have models/migrations).
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:9>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
In theory, it would be possible for migrations to stop using app-label for
dependencies and start using full dotted paths instead? Maybe?
And then since db table names aren't hardcoded in migrations, maybe
migrations could adapt to the app being re-labeled? (If you did the re-
labeling before running any of its migrations; re-labeling midstream would
still require manual table renames.)
This would be a big change, but if we wanted to revive the idea of using
`AppConfig.label` for third-party apps, I think that would be the
necessary path.
Using full paths of course means that if the import path for an app
changes, you have to edit all its migrations. But maybe that's better,
since import path is definitely under the control of the app author,
always, unlike app label (since the advent of `AppConfig.label`).
If we don't make this change to migrations, it seems possible that there
are no really compelling use-cases remaining for `AppConfig.label`, just
bugs-in-waiting, and we should consider deprecating and removing it.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:10>
Django
Reporter: aaugustin | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
Last thought: using `AppConfig.label` for third-party apps with migrations
is still possible, I guess - you just have to use `MIGRATION_MODULES`
also, and regenerate all its migrations. Which gets unfortunate if the app
is upgraded and ships a new migration; you'd have to always re-create
those yourself.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:11>
Django
Reporter: Aymeric Augustin | Owner: Andy Miller
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Andy Miller):
* owner: nobody => Andy Miller
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:12>
Django
Reporter: Aymeric Augustin | Owner: Andy Miller
Type: Bug | Status: assigned
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Andy Miller):
PR: https://github.com/django/django/pull/18324
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:13>
Django
Reporter: Aymeric Augustin | Owner: Andy Miller
Type: Bug | Status: assigned
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Andy Miller):
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:14>
Django
Reporter: Aymeric Augustin | Owner: Andy Miller
Type: Bug | Status: assigned
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Ryan Cheley):
* cc: Ryan Cheley (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:15>
Django
| Miller
Type: Bug | Status: assigned
Component: Documentation | Version: 1.7
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* stage: Accepted => Ready for checkin
Comment:
PR looks good, some tests are failing due to Jenkins not being able to
access GH:
{{{
> git fetch --no-tags --force --progress --depth=1 --
https://github.com/django/django.git
+refs/pull/18324/merge:refs/remotes/origin/pull/18324/merge # timeout=10
ERROR: Error fetching remote repo 'origin'
hudson.plugins.git.GitException: Failed to fetch from
https://github.com/django/django.git
}}}
Will merge when all runs are completed.
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:16>
Django
Reporter: Aymeric Augustin | Owner: Andy
| Miller
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: fixed
| checkin
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia <124304+nessita@…>):
* resolution: => fixed
* status: assigned => closed
Comment:
In [changeset:"fb140420bd54cdd3bcc1e51442d631f233213989" fb140420]:
{{{#!CommitTicketReference repository=""
revision="fb140420bd54cdd3bcc1e51442d631f233213989"
[5.1.x] Fixed #23790 -- Warned about renaming AppConfig.label in
docs/ref/applications.txt.
Backport of aa74c4083e047473ac385753e047e075e8f04890 from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:17>
Django
Reporter: Aymeric Augustin | Owner: Andy
| Miller
Type: Bug | Status: closed
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Easy pickings: 0 | UI/UX: 0
Comment (by nessita <124304+nessita@…>):
In [changeset:"aa74c4083e047473ac385753e047e075e8f04890" aa74c40]:
{{{#!CommitTicketReference repository=""
revision="aa74c4083e047473ac385753e047e075e8f04890"
docs/ref/applications.txt.
}}}
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:18>
Django
Reporter: Aymeric Augustin | Owner: Andy
| Miller
Type: Bug | Status: closed
Component: Documentation | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Easy pickings: 0 | UI/UX: 0
Comment (by Natalia <124304+nessita@…>):
In [changeset:"4cf719907812a4259cad1f86689ceea364f0a985" 4cf71990]:
{{{#!CommitTicketReference repository=""
revision="4cf719907812a4259cad1f86689ceea364f0a985"
[5.0.x] Fixed #23790 -- Warned about renaming AppConfig.label in
docs/ref/applications.txt.
Backport of aa74c4083e047473ac385753e047e075e8f04890 from main.
}}}
Ticket URL: <https://code.djangoproject.com/ticket/23790#comment:19>