Opened 9 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 , 9 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 9 years ago
Has patch: | set |
---|
comment:4 by , 9 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
comment:5 by , 9 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 , 9 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:7 by , 9 years ago
Cc: | added |
---|---|
Owner: | removed |
Status: | assigned → new |
comment:8 by , 9 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:9 by , 9 years ago
Easy pickings: | unset |
---|---|
Owner: | removed |
Status: | assigned → new |
comment:10 by , 9 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 , 9 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:14 by , 8 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:15 by , 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 aSystemCheckError
before 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
dependencies
in the migration file does not exist, we get a NodeNotFoundError.
comment:16 by , 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 , 8 years ago
Resolution: | → needsinfo |
---|---|
Status: | assigned → closed |
Created a pull request with a change that implements this: https://github.com/django/django/pull/5589