[Django] #25109: MigrationLoader.load_disk hides ImportError for invalid MIGRATION_MODULES
Django
----------------------------+--------------------
Reporter: blueyed | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------+--------------------
With an invalid module name in `MIGRATION_MODULES`, you will get a rather
confusing `InvalidBasesError`:
{{{
InvalidBasesError: Cannot resolve bases for [<ModelState:
'djangocms_text_ckeditor.Text'>]
This can happen if you are inheriting models from an app with migrations
(e.g. contrib.auth)
in an app with no migrations; see
https://docs.djangoproject.com/en/1.8/topics/migrations/#dependencies for
more
}}}
This is caused by ignoring the `ImportError` in
https://github.com/django/django/blob/d72f8862cb1a39934952e708c3c869be1399846e/django/db/migrations/loader.py#L70-L78,
which is meant to handle non-existent migrations.
The following patch might fix it:
{{{
diff --git i/django/db/migrations/loader.py
w/django/db/migrations/loader.py
index a8f4be4..e872294 100644
--- i/django/db/migrations/loader.py
+++ w/django/db/migrations/loader.py
@@ -72,7 +72,9 @@ def load_disk(self):
except ImportError as e:
# I hate doing this, but I don't want to squash other
import errors.
# Might be better to try a directory check directly.
- if "No module named" in str(e) and MIGRATIONS_MODULE_NAME
in str(e):
+ if ("No module named" in str(e)
+ and MIGRATIONS_MODULE_NAME in str(e)
+ and app_config.label not in
settings.MIGRATION_MODULES):
self.unmigrated_apps.add(app_config.label)
continue
raise
}}}
But then Django's tests itself fail, because they use this "hack":
{{{
settings.MIGRATION_MODULES = {
# these 'tests.migrations' modules don't actually exist, but this
lets
# us skip creating migrations for the test models.
'auth': 'django.contrib.auth.tests.migrations',
'contenttypes': 'contenttypes_tests.migrations',
}
}}}
([Source](https://github.com/django/django/blob/d72f8862cb1a39934952e708c3c869be1399846e/tests/runtests.py#L142-147))
I've seen this issue multiple times in the context of Django CMS (and its
plugins), because in the progress of migrating to Django's migrations they
were using modules like `djangocms_text_ckeditor.migrations_django` which
then were renamed.
--
Ticket URL: <https://code.djangoproject.com/ticket/25109>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: blueyed | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
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 timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:1>
Django
Reporter: blueyed | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
A related [https://github.com/django/django/pull/4950 pull request].
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:2>
Django
Reporter: blueyed | Owner: nobody
Type: Bug | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
As Berker suggested in #django-dev, a system check for invalid entries in
`MIGRATION_MODULES` might be a solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:3>
Django
Reporter: blueyed | Owner: berkerpeksag
Type: Bug | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by berkerpeksag):
* cc: berker.peksag@… (added)
* owner: nobody => berkerpeksag
* has_patch: 0 => 1
* status: new => assigned
Comment:
I've implemented the system check idea in
[https://github.com/django/django/pull/6853 PR #6853]. I'm not sure if the
check should live be part of the database tag or should be moved to a new
migrations tag.
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:4>
Django
Reporter: blueyed | Owner: berkerpeksag
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
What about letting the `ImportError` propagate if the invalid migration
module is explicitly specified through `MIGRATION_MODULES` instead?
I see the reasoning behind silencing it the default `app.migration`
package is missing but if a module is explicitly specified I would expect
an error to be raised just like any other setting.
The `MigrationLoader.migrations_module` method's return type could be
altered to be of the form `module_name, explicit` and the exception could
be re-raised if `explicit` is truthy.
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:5>
Django
Reporter: blueyed | Owner: berkerpeksag
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
If it's feasible that sounds like a simpler solution (feel free to uncheck
"patch needs improvement" if you reply with a disagreement).
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:6>
Django
Reporter: blueyed | Owner: berkerpeksag
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/7171 Proposed approach].
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:7>
Django
Reporter: blueyed | Owner: berkerpeksag
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:8>
Django
Reporter: blueyed | Owner: berkerpeksag
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"0d7929266ec9d8f6bcf9e41a40b281259cba6a79" 0d79292]:
{{{
#!CommitTicketReference repository=""
revision="0d7929266ec9d8f6bcf9e41a40b281259cba6a79"
Fixed #25109 -- Stopped silencing explicitly specified migration modules
import errors.
Thanks Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/25109#comment:9>