Opened 10 years ago
Closed 9 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 , 10 years ago
| Triage Stage: | Unreviewed → Accepted | 
|---|
comment:2 by , 10 years ago
| Owner: | changed from to | 
|---|---|
| Status: | new → assigned | 
comment:3 by , 10 years ago
| Has patch: | set | 
|---|
comment:4 by , 10 years ago
| Needs tests: | set | 
|---|---|
| Patch needs improvement: | set | 
comment:5 by , 10 years ago
| Has patch: | unset | 
|---|---|
| Needs tests: | unset | 
| Owner: | removed | 
| Patch needs improvement: | unset | 
| Status: | assigned → new | 
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 , 10 years ago
| Owner: | set to | 
|---|---|
| Status: | new → assigned | 
comment:7 by , 10 years ago
| Cc: | added | 
|---|---|
| Owner: | removed | 
| Status: | assigned → new | 
comment:8 by , 10 years ago
| Owner: | set to | 
|---|---|
| Status: | new → assigned | 
comment:9 by , 10 years ago
| Easy pickings: | unset | 
|---|---|
| Owner: | removed | 
| Status: | assigned → new | 
comment:10 by , 10 years ago
| Owner: | set to | 
|---|---|
| Status: | new → assigned | 
Can anyone please suggest the steps to reproduce this issue, it would be really helpful.
Thanks
comment:13 by , 10 years ago
| Owner: | removed | 
|---|---|
| Status: | assigned → new | 
comment:14 by , 9 years ago
| Owner: | set to | 
|---|---|
| Status: | new → assigned | 
comment:15 by , 9 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 aSystemCheckErrorbefore the migration even starts.
- When the app we depend upon is in INSTALLED_APPS, but the app is not included independencies, we seems to get aValueError: Related model ... cannot be resolved.
- When the name of an app in dependenciesin the migration file does not exist, we get a NodeNotFoundError.
comment:16 by , 9 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 , 9 years ago
| Resolution: | → needsinfo | 
|---|---|
| Status: | assigned → closed | 
Created a pull request with a change that implements this: https://github.com/django/django/pull/5589