Opened 10 years ago

Closed 5 months ago

Last modified 5 months ago

#23790 closed Bug (fixed)

Possible bad interaction between migration dependencies and relabeling apps

Reported by: Aymeric Augustin Owned by: Andy Miller
Component: Documentation Version: 1.7
Severity: Normal Keywords:
Cc: info+coding@…, Ryan Cheley 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 Markus Holtermann)

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

Change History (19)

comment:1 by Markus Holtermann, 10 years ago

Cc: info+coding@… added
Description: modified (diff)

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:

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

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.

Last edited 10 years ago by Markus Holtermann (previous) (diff)

comment:2 by Markus Holtermann, 10 years ago

Description: modified (diff)

comment:3 by Aymeric Augustin, 10 years ago

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?

comment:4 by Aymeric Augustin, 10 years ago

Component: UncategorizedMigrations
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:5 by Carl Meyer, 10 years ago

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.

comment:6 by Aymeric Augustin, 10 years ago

That's inconvenient when the affected app is a third-party app.

comment:7 by Carl Meyer, 10 years ago

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.

comment:8 by Aymeric Augustin, 10 years ago

Component: MigrationsDocumentation

A warning in the documentation of AppConfig.label is the way to go then.

comment:9 by Carl Meyer, 10 years ago

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

comment:10 by Carl Meyer, 10 years ago

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.

comment:11 by Carl Meyer, 10 years ago

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.

comment:12 by Andy Miller, 5 months ago

Owner: changed from nobody to Andy Miller
Status: newassigned

comment:14 by Andy Miller, 5 months ago

Has patch: set

comment:15 by Ryan Cheley, 5 months ago

Cc: Ryan Cheley added

comment:16 by Natalia Bidart, 5 months ago

Triage Stage: AcceptedReady for checkin

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.

comment:17 by Natalia <124304+nessita@…>, 5 months ago

Resolution: fixed
Status: assignedclosed

In fb140420:

[5.1.x] Fixed #23790 -- Warned about renaming AppConfig.label in docs/ref/applications.txt.

Backport of aa74c4083e047473ac385753e047e075e8f04890 from main.

comment:18 by nessita <124304+nessita@…>, 5 months ago

In aa74c40:

Fixed #23790 -- Warned about renaming AppConfig.label in docs/ref/applications.txt.

comment:19 by Natalia <124304+nessita@…>, 5 months ago

In 4cf71990:

[5.0.x] Fixed #23790 -- Warned about renaming AppConfig.label in docs/ref/applications.txt.

Backport of aa74c4083e047473ac385753e047e075e8f04890 from main.

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