Opened 8 years ago

Closed 8 years ago

#25616 closed Cleanup/optimization (needsinfo)

Add note regarding missing dependencies on LookupError for migrations

Reported by: Mike C. Fletcher Owned by: Niclas Olofsson
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: 0coming.soon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

If you have a product package that includes migrations which depend on a plugin package, and you forget to include a dependency on the migration, you wind up with a confusing error message (raised from Apps.get_app_config):

 LookupError("No installed app with label '%s'"%(label,))

where label is the name of the "missing" app. Which then leads the user to trying to figure out why the app, which is in settings.py is not showing up.

It would be helpful, in django.db.migrations.state if we were to annotate the LookupError with the common issue the user is likely seeing, something like (on StateApps):

    def get_app_config(self, app_label):
        try:
            return super(StateApps, self).get_app_config(app_label)
        except LookupError as err:
            err.args += ('Possibly missing dependency on a migration?', )
            raise

I'm not attached to the particular implementation or wording, just suggesting that the error message should be cleaner in the migrations-can't-find-app case, which is different than the general "can't find an app of that name" case.

Change History (17)

comment:1 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Mark Milan, 8 years ago

Owner: changed from nobody to Mark Milan
Status: newassigned

comment:3 by Mark Milan, 8 years ago

Has patch: set

Created a pull request with a change that implements this: https://github.com/django/django/pull/5589

comment:4 by Markus Holtermann, 8 years ago

Needs tests: set
Patch needs improvement: set

comment:5 by Tim Graham, 8 years ago

Has patch: unset
Needs tests: unset
Owner: Mark Milan removed
Patch needs improvement: unset
Status: assignednew

Deassigning due to inactivity. Markus commented on the pull request, "I'd prefer overriding the StateApps class in django.db.migrations.state to catch the LookupError and reraise with an amended message."

comment:6 by Andrew Kuchev, 8 years ago

Owner: set to Andrew Kuchev
Status: newassigned

comment:7 by Andrew Kuchev, 8 years ago

Cc: 0coming.soon@… added
Owner: Andrew Kuchev removed
Status: assignednew

comment:8 by Anderson Resende, 8 years ago

Owner: set to Anderson Resende
Status: newassigned

comment:9 by Tim Graham, 8 years ago

Easy pickings: unset
Owner: Anderson Resende removed
Status: assignednew

comment:10 by Akshesh Doshi, 8 years ago

Owner: set to Akshesh Doshi
Status: newassigned

Can anyone please suggest the steps to reproduce this issue, it would be really helpful.

Thanks

comment:12 by Tim Graham, 8 years ago

Needs tests: set

We can't commit the patch without a test.

comment:13 by Akshesh Doshi, 8 years ago

Owner: Akshesh Doshi removed
Status: assignednew

comment:14 by Niclas Olofsson, 8 years ago

Owner: set to Niclas Olofsson
Status: newassigned

comment:15 by Niclas Olofsson, 8 years ago

Needs tests: unset

New patch inspired from previous patch, including test: https://github.com/django/django/pull/6699/

However, I realize that I'm not sure when this actually happens in practice, which means that the new error message may actually be more misleading then the old one.

  • When trying to migrate model with a reference to an app which is not in INSTALLED_APPS, we will typically get a SystemCheckError before the migration even starts.
  • When the app we depend upon is in INSTALLED_APPS, but the app is not included in dependencies, we seems to get a ValueError: Related model ... cannot be resolved.
  • When the name of an app in dependencies in the migration file does not exist, we get a NodeNotFoundError.

comment:16 by Tim Graham, 8 years ago

Patch needs improvement: set

If in doubt, we could close the ticket as "needsinfo" as we've not received steps to reproduce the problem.

comment:17 by Tim Graham, 8 years ago

Resolution: needsinfo
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top