Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed #22920 - AppConfig.create swallows informative exceptions during import time #2861

Closed
wants to merge 1 commit into from

3 participants

@timgraham
Owner

Did you see Aymeric's questions on the ticket?

@bendavis78

I missed the comment on the ticket (as I often fail to add myself to the CC list). I'll see if I can address the questions within the next few days.

Regarding the severity, I can't remember why I made it release blocker. Makes sense that it should be minor. I'll update on that.

@timgraham
Owner

If you are the report of the ticket, you should get updates, but maybe you are missing an email address in Trac? https://www.djangoproject.com/accounts/edit/ I think

@bendavis78

Hmm, nope my email is in there, though my name wasn't populated.

@aaugustin aaugustin closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 28, 2014
  1. @bendavis78
This page is out of date. Refresh to see the latest.
View
62 django/apps/config.py
@@ -1,5 +1,6 @@
from importlib import import_module
import os
+import types
from django.core.exceptions import ImproperlyConfigured
from django.utils.module_loading import module_has_submodule
@@ -80,44 +81,41 @@ def create(cls, entry):
"""
Factory that creates an app config from an entry in INSTALLED_APPS.
"""
- try:
- # If import_module succeeds, entry is a path to an app module,
- # which may specify an app config class with default_app_config.
- # Otherwise, entry is a path to an app config class or an error.
- module = import_module(entry)
+ mod_path, _, ending = entry.rpartition('.')
- except ImportError:
- mod_path, _, cls_name = entry.rpartition('.')
+ if not mod_path:
+ # single-level module
+ mod_path = ending
- # Raise the original exception when entry cannot be a path to an
- # app config class.
- if not mod_path:
- raise
+ # Attempt to import mod_path by itself. If this fails, the exception
+ # should propagate up (nothing else can be done at that point).
- else:
- try:
- # If this works, the app module specifies an app config class.
- entry = module.default_app_config
- except AttributeError:
- # Otherwise, it simply uses the default app config class.
+ module = import_module(mod_path)
+
+ obj = getattr(module, ending, None)
+
+ if not obj or isinstance(obj, types.ModuleType):
+ # Either the path ending is a module, or the given object doesn't
+ # exist. Either way we need to perform an import here to allow
+ # import-time exceptions to propagate
+ module = import_module(entry)
+
+ if not hasattr(module, 'default_app_config'):
+ # This is a plain module entry, so create a default AppConfig
return cls(entry, module)
- else:
- mod_path, _, cls_name = entry.rpartition('.')
- # If we're reaching this point, we must load the app config class
- # located at <mod_path>.<cls_name>.
+ # otherwise, import the module path given in default_app_config
+ entry = module.default_app_config
+ mod_path, _, cls_name = entry.rpartition('.')
+ module = import_module(mod_path)
+ else:
+ # if it is an attribute, we can assume it's an AppConfig class
+ cls_name = ending
- # Avoid django.utils.module_loading.import_by_path because it
- # masks errors -- it reraises ImportError as ImproperlyConfigured.
- mod = import_module(mod_path)
- try:
- cls = getattr(mod, cls_name)
- except AttributeError:
- # Emulate the error that "from <mod_path> import <cls_name>"
- # would raise when <mod_path> exists but not <cls_name>, with
- # more context (Python just says "cannot import name ...").
- raise ImportError(
- "cannot import name '%s' from '%s'" % (cls_name, mod_path))
+ # We now know that entry *should* be a <mod_path>.<cls_name>.
+ # At this point we should allow AttributeError to propagate as normal
+ # if the given class doesn't exist.
+ cls = getattr(module, cls_name)
# Check for obvious errors. (This check prevents duck typing, but
# it could be removed if it became a problem in practice.)
View
2  tests/apps/__init__.py
@@ -0,0 +1,2 @@
+class Trouble(ImportError):
+ pass
View
1  tests/apps/misbehaving_module/__init__.py
@@ -0,0 +1 @@
+from apps.misbehaving_module import foo
View
5 tests/apps/misbehaving_module/bar.py
@@ -0,0 +1,5 @@
+from apps import Trouble
+
+
+def cause_trouble():
+ raise Trouble("No one will ever know...")
View
5 tests/apps/misbehaving_module/foo.py
@@ -0,0 +1,5 @@
+from apps.misbehaving_module import bar
+
+WHO_DUNNIT = "don't look at me"
+
+bar.cause_trouble()
View
15 tests/apps/tests.py
@@ -35,6 +35,10 @@
'django.contrib.auth',
] + SOME_INSTALLED_APPS[2:]
+EXCEPTION_RAISING_APPS = [
+ 'apps.misbehaving_module'
+] + SOME_INSTALLED_APPS[2:]
+
HERE = os.path.dirname(__file__)
@@ -200,6 +204,17 @@ def test_dynamic_load(self):
apps.get_model("apps", "SouthPonies")
self.assertEqual(new_apps.get_model("apps", "SouthPonies"), temp_model)
+ def test_importtime_exception_propagation(self):
+ """
+ Test for #22920. Make sure exceptions occuring during import time
+ propagate correctly
+ """
+ from apps import Trouble
+
+ with self.assertRaises(Trouble):
+ with self.settings(INSTALLED_APPS=EXCEPTION_RAISING_APPS):
+ pass
+
class Stub(object):
def __init__(self, **kwargs):
Something went wrong with that request. Please try again.