Opened 9 years ago

Closed 8 years ago

#25109 closed Bug (fixed)

MigrationLoader.load_disk hides ImportError for invalid MIGRATION_MODULES

Reported by: Daniel Hahler Owned by: Berker Peksag
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: berker.peksag@… 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

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.

Change History (9)

comment:1 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Tim Graham, 9 years ago

A related pull request.

comment:3 by Tim Graham, 8 years ago

As Berker suggested in #django-dev, a system check for invalid entries in MIGRATION_MODULES might be a solution.

comment:4 by Berker Peksag, 8 years ago

Cc: berker.peksag@… added
Has patch: set
Owner: changed from nobody to Berker Peksag
Status: newassigned

I've implemented the system check idea in PR #6853. I'm not sure if the check should be part of the database tag or should be moved to a new migrations tag.

Last edited 8 years ago by Berker Peksag (previous) (diff)

comment:5 by Simon Charette, 8 years ago

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.

comment:6 by Tim Graham, 8 years ago

Patch needs improvement: set

If it's feasible that sounds like a simpler solution (feel free to uncheck "patch needs improvement" if you reply with a disagreement).

comment:7 by Simon Charette, 8 years ago

Patch needs improvement: unset

comment:8 by Tim Graham, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Simon Charette <charette.s@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 0d79292:

Fixed #25109 -- Stopped silencing explicitly specified migration modules import errors.

Thanks Tim for the review.

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