Opened 3 years ago

Closed 3 years ago

Last modified 21 months ago

#21674 closed Bug (fixed)

django.utils.module_loading.import_by_path considered harmful

Reported by: Aymeric Augustin Owned by: Berker Peksag
Component: Utilities Version: master
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

The purpose of this function is to import whatever its argument points to.

If it fails, it should raise ImportError, signalling that an import failed.

Unfortunately, it catches exceptions, including ImportErrors, and re-raises ImproperlyConfigured. Such exception masking makes it needlessly hard to diagnose circular import problems, because it makes it look like the problem comes from inside Django. It becomes supremely perverse when some code in Django catches ImproperlyConfigured and things go wrong further down the line.

I understand that the original intent was to provide more frienly error messages, but I believe that ImportError is a perfectly fine and suitable exception and that replacing it with a more generic one is a net loss.

(I know I'm attacking an old dogma, but this is an easy step in the long standing "improved error reporting" project.)

Change History (15)

comment:1 Changed 3 years ago by Tim Graham

Do you see any way we can make the change backwards compatible?

comment:2 Changed 3 years ago by Aymeric Augustin

Well, it's a private API...

comment:3 Changed 3 years ago by Aymeric Augustin

Hum, no, it's documented. Well we could provide a replacement in Django and then deprecate it. It was only added in 1.6.

comment:4 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:5 Changed 3 years ago by Berker Peksag

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

I've opened a pull request on GitHub: https://github.com/django/django/pull/2187

I borrowed the name "import_string" from the Peak project:

http://svn.eby-sarna.com/Importing/peak/util/imports.py?view=markup

comment:6 Changed 3 years ago by Claude Paroz

I think that the ImproperlyConfigured exception might make sense in some cases, especially when dotted_path is coming from a setting. What about adding a new keyword argument to import_by_path, something like: def import_by_path(dotted_path, error_prefix='', error_class=ImproperlyConfigured). Then it would be possible to overwrite the exception raised by that function, on a case-by-case basis.

comment:7 Changed 3 years ago by Berker Peksag

Ah, good idea. Thanks claudep.

How about adding a new keyword argument named suppress_import_error? (somewhat similar to http://docs.python.org/3.4/library/contextlib.html#contextlib.suppress)

def import_by_path(dotted_path, error_prefix='', suppress_import_error=True):  # or raise_import_error=False
    # snip
    except ImportError as e:
        if not suppress_import_error:
            raise
        raise ImproperlyConfigured
    # snip

comment:8 Changed 3 years ago by Aymeric Augustin

Really, I'm not eager to add an error masking API such as error_class. What's wrong with a plain ImportError? Usually it's pretty clear.

If a _caller_ can provide additional information for its specific use-case, it can catch and re-raise an exception. Exception chaining makes this less of a problem on Python 3. As long as we support Python 2, I'm -1 on catching an exception and raising another one.

I would suggest adding a new function that simply perfoms the import and doesn't handle exceptions. We would use it wherever the error_prefix argument of import_by_path isn't needed. We would make this function public and recommend it rather than import_by_path. Regarding its name, import_string sounds reasonable.

comment:9 Changed 3 years ago by Tim Graham

Patch needs improvement: set

Patch needs improvement as noted by Aymeric. Usage of the deprecated function also needs to be removed as I noted on the PR.

comment:10 Changed 3 years ago by Berker Peksag

Patch needs improvement: unset

I've updated my pull request to adress Aymeric and Tim's feedbacks: https://github.com/django/django/pull/2187

Thanks!

comment:11 Changed 3 years ago by Aymeric Augustin

Triage Stage: AcceptedReady for checkin

The PR has been heavily reviewed by Timo and improved by Berker. I think it's ready. Thank you very much.

Timo, I'll let you do the merge, in case you want to review Berker's latest changes first.

comment:12 Changed 3 years ago by Tim Graham

My only reservation is that I think it's a bit confusing to have to catch AttributeError and ValueError (besides ImportError) for all usage of import_string. Do you think catching these exceptions inside import_string and re-raising ImportError for these cases would be considered harmful?

comment:13 Changed 3 years ago by Aymeric Augustin

That would be acceptable.

The biggest problem is that Django raises and catches ImproperlyConfigured too liberally. As long as we avoid that, I'm happy.

comment:14 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 5d263dee304fdaf95e18d2f0619d6925984a7f02:

Fixed #21674 -- Deprecated the import_by_path() function in favor of import_string().

Thanks Aymeric Augustin for the suggestion and review.

comment:15 Changed 21 months ago by Tim Graham <timograham@…>

In d029fafea1a1f2f257397fbcd0fa3b531e09854f:

Removed utils.module_loading.import_by_path() per deprecation timeline; refs #21674.

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