Opened 13 months ago

Closed 11 months ago

Last modified 11 months ago

#22920 closed Bug (fixed)

AppConfig.create swallows informative exceptions during import time

Reported by: bendavis78 Owned by: aaugustin
Component: Core (Other) Version: master
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 Changed 13 months ago by bendavis78

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 13 months ago by aaugustin

  • Keywords app-loading added; app loading removed
  • Owner changed from nobody to aaugustin
  • Severity changed from Release blocker to Normal
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 11 months ago by aaugustin

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

comment:4 Changed 11 months ago by aaugustin

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 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 Changed 11 months ago by Aymeric Augustin <aymeric.augustin@…>

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