Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better traceback for bad imports in custom path #735

Merged
merged 1 commit into from Nov 22, 2018

Conversation

Projects
None yet
6 participants
@dset0x
Copy link

commented May 22, 2015

No description provided.

except ImportError:
# support importing modules not yet set up by the parent module
# (or package for that matter)
module = import_string(module_name)

This comment has been minimized.

Copy link
@untitaker

untitaker May 22, 2015

Member

The tests don't fail because of your PR, but I think this line is there for a reason (no tests there for it though?)

This comment has been minimized.

Copy link
@dset0x

dset0x May 22, 2015

Author

This also made me raise an eyebrow, but I could not understand what "not yet set up" implied either. However, the case that I introduced in the tests, does cause the ImportError of line 428 to be raised.

FWIW I use this patch with inveniosoftware/flask-registry and had no obvious issues.

Edit: Here is the patch that added these lines: ec4f326

This comment has been minimized.

Copy link
@untitaker

untitaker May 22, 2015

Member

I can't find a way to break your code either. The original code dates back to 9aa53b8, but there's insufficient explanation there.

This comment has been minimized.

Copy link
@untitaker

untitaker May 22, 2015

Member

I'll assign this to @mitsuhiko, maybe he (still) can come up with a testcase for the linked commit.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Contributor

commented May 26, 2015

i suspect a potential use case lingering in lazy code loaders that inject modules into sys.modules at their own import

on closer examination the internal logic is grown a bit strange and should probably do a different loop altogether

@kaplun

This comment has been minimized.

Copy link

commented on 2e9c820 Aug 28, 2015

@dset0x: when are you sending a PR to werkzeug for this?

This comment has been minimized.

Copy link

replied Aug 28, 2015

I see though why: you are removing a behavior of werkzeug. Maybe you can re-raise the previous exception? Additionally why are you passing globals() and locals() and the original code didn't?

This comment has been minimized.

Copy link
Owner

replied Aug 28, 2015

@kaplun

when are you sending a PR to werkzeug for this?

pallets#735

you are removing a behavior of werkzeug. Maybe you can re-raise the previous exception?

Not from what Werkzeug folks can tell, but can't fully confirm yet. https://github.com/mitsuhiko/werkzeug/pull/735/files

Additionally why are you passing globals() and locals() and the original code didn't?

Because there is no context without them, and without context we fail too early, if memory serves me right.


As far as flask-registry is concerned, we can ship our own method.

@davidism davidism added this to the 0.15 milestone Nov 19, 2018

@davidism

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

We can preserve the "not set up by parent" case by saving the exception and raising it in the AttributeError block. However, this just adds more complexity to a code path no one can reproduce. And it makes the chained traceback in Python 3 look like it's coming from the wrong place. I'm inclined to merge this PR as-is, leaving this diff as a note in case someone comes up with a failing test case.

diff --git a/werkzeug/utils.py b/werkzeug/utils.py
index 5c8f5125..b3e859f7 100644
--- a/werkzeug/utils.py
+++ b/werkzeug/utils.py
@@ -423,10 +423,17 @@ def import_string(import_name, silent=False):
             return sys.modules[import_name]
 
         module_name, obj_name = import_name.rsplit('.', 1)
-        module = __import__(module_name, globals(), locals(), [obj_name])
+        preserved_import_error = None
+        try:
+            module = __import__(module_name, globals(), locals(), [obj_name])
+        except ImportError as e:
+            preserved_import_error = e
+            module = import_string(module_name)
         try:
             return getattr(module, obj_name)
         except AttributeError as e:
+            if preserved_import_error is not None:
+                raise preserved_import_error
             raise ImportError(e)
 
     except ImportError as e:

@davidism davidism merged commit 01f54c5 into pallets:master Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

davidism added a commit that referenced this pull request Nov 22, 2018

@dset0x

This comment has been minimized.

Copy link
Author

commented Nov 22, 2018

Nearly three and a half years later, I still vividly recall the dozens of hours of pain that this was helping solve for the (now-deprecated) referenced module. Thanks for the merge. I hope it helps people get useful tracebacks.

@davidism

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Sorry it took so long 😞 It came up in a much more recent issue, so it will still help. Thanks for working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.