Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#22920 closed Bug (fixed)

AppConfig.create swallows informative exceptions during import time

Reported by: Ben Davis Owned by: Aymeric Augustin
Component: Core (Other) Version: dev
Severity: Normal Keywords: AppConfig app-loading ImportError
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If you have an INSTALLED_APPS entry that doesn't use an AppConfig, and that module has more than one level, eg "myproject.myapp", AppConfig.create will swallow any exceptions that occur during the import of that module, losing valuable traceback information. The create method makes some incorrect assumptions about the entry. Mainly it assumes that if the entry has more than one part and the import fails, that it must be an AppConfig class (which is incorrect).

I've submitted a PR with a fix for this. Basically the new algorithm is as follows:

  1. Split the entry into mod_path and ending. If it's a single-level module, ending is empty.
  2. Attempt to import mod_path as module, allowing exceptions to propagate (no try/catch)
  3. Check if ending is an attribute of module.
    • If the attribute doesn't exist OR the attribute exists and is a module, import the full entry as module (again, allowing exceptions to propagate)
    • If the imported module does NOT a default_app_config attr, return with a default AppConfig instance.
    • Otherwise, import the module path from default_app_config entry as module (again, allowing exceptions to propagate), and assume the ending is a class name.
  4. If ending was an attribute of module in step 3, we can assume it's an AppConfig class.
  5. At this point we have a legitimate module and an object which should be a class, and can continue on to validate the AppConfig class.

Change History (6)

comment:1 by Ben Davis, 10 years ago

Has patch: set

comment:2 by Aymeric Augustin, 10 years ago

Keywords: app-loading added; app loading removed
Owner: changed from nobody to Aymeric Augustin
Severity: Release blockerNormal
Status: newassigned
Triage Stage: UnreviewedAccepted

I believe the issue is valid and we should address it.

Can you confirm that the exception masking happens on Python 2 — due to the lack of exception chaining — in this section of the code?

        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))

I'm not very proud of that code and I'm not surprised someone came and flamed it ;-)


If I read your patch correctly, at some point, you redo an import that you know will fail in order to obtain the correct traceback. Two questions:

1 - Why wasn't it sufficient to use that trick in the piece of code I quoted above?
2 - Isn't it going to be confusing on Python 3, because of the exception chaining ?

I read your patch carefully but I found it hard to validate because it rewrites a section that is inherently difficult to test. Thanks for adding tests, though, that's very helpful in any case.


I consider this a minor bug because it doesn't affect functionality, only error reporting on projets that don't work anyway. Did I miss the reason why you marked it as a blocker?

comment:3 by Aymeric Augustin, 10 years ago

bendavis78, did you have the opportunity to review my questions above?

comment:4 by Aymeric Augustin, 10 years ago

I'm proposing a less invasive fix in PR 3144.

I've already rewritten that method entirely once to address issues I don't remember very well. I'd rather not restructure the logic drastically just to improve error reporting.

comment:5 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In b161c01c48b4c353d7d1a47e9097391a8eb2e047:

Fixed #22920 -- Avoid masking some exceptions.

If loading an application trigger an ImportError, the details of that
error were lost in some cases. Thanks Ben Davis for the report.

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In c981d9140d569d489d338741b6a3d878c4e712da:

[1.7.x] Fixed #22920 -- Avoid masking some exceptions.

If loading an application trigger an ImportError, the details of that
error were lost in some cases. Thanks Ben Davis for the report.

Backport of b161c01 from master

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