Opened 7 years ago

Closed 7 years ago

#28241 closed Bug (fixed)

module_has_submodule() doesn't work correctly if the module_name argument is a dotted path

Reported by: tkhyn Owned by: tkhyn
Component: Utilities Version: 1.11
Severity: Normal Keywords: module_loading python3
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

Hello,

Porting a django project from python 2.7 to 3.6, I noticed an issue with utils.module_loading.module_has_submodule. At some stage my project makes use of autodiscover_modules('module.submodule') to try and discover modules that are nested in my app.

I'm using django 1.11.1, but this bug probably also affects 1.8 as the code of module_has_submodule is the same.

So here we go, with python 2.7 we have the expected behavior (taking contenttypes as an example app):

python2.7
>>> from django.utils.module_loading import module_has_submodule
>>> from django.contrib import contenttypes
>>> module_has_submodule(contenttypes, 'invalid_module.submodule')
False
>>> module_has_submodule(contenttypes, 'checks')
True
>>> module_has_submodule(contenttypes, 'checks.submodule')
False

But, with python 3.6

python3.6
>>> from django.utils.module_loading import module_has_submodule
>>> from django.contrib import contenttypes
>>> module_has_submodule(contenttypes, 'invalid_module.submodule')
  File "<console>", line 1, in <module>
  File "d:\dev\.env\buildout\eggs\django-1.11.1-py3.6.egg\django\utils\module_loading.py", line 79, in module_has_submodule
    return importlib_find(full_module_name, package_path) is not None
  File "D:\dev\.env\venv\buildout\lib\importlib\util.py", line 88, in find_spec
    parent = __import__(parent_name, fromlist=['__path__'])
ModuleNotFoundError: No module named 'django.contrib.contenttypes.invalid_module'
>>> module_has_submodule(contenttypes, 'checks')
True
>>> module_has_submodule(contenttypes, 'checks.submodule')
  File "<console>", line 1, in <module>
  File "d:\dev\.env\buildout\eggs\django-1.11.1-py3.6.egg\django\utils\module_loading.py", line 79, in module_has_submodule
    return importlib_find(full_module_name, package_path) is not None
  File "D:\dev\.env\venv\buildout\lib\importlib\util.py", line 89, in find_spec
    return _find_spec(fullname, parent.__path__)
AttributeError: module 'django.contrib.contenttypes.checks' has no attribute '__path__'

From the replies I got on the python bug tracker (http://bugs.python.org/issue30436) the AttributeError will be converted to ModuleNotFoundError only in Python 3.7, but that behavior is apparently not expected to be fixed in previous versions.

We'll definitely need to catch ModuleNotFoundError, and to have a proper fix for python < 3.7 we'll need to:

  • either catch AttributeError as well. The problem I see is AttributeError is too broad and may result in 'false catches'
  • or convert a dotted path to [package, name] (basically do what find_spec actually does) but detect if the module has a __path__ attribute before calling find_spec so that it can not raise exceptions

What are your thoughts ?

Change History (9)

comment:1 by tkhyn, 7 years ago

Summary: module_has_submodule behavior in Python 3module_has_submodule raises exceptions in Python 3

comment:2 by tkhyn, 7 years ago

Description: modified (diff)

comment:3 by Tim Graham, 7 years ago

Description: modified (diff)
Easy pickings: unset
Summary: module_has_submodule raises exceptions in Python 3module_has_submodule() doesn't work correctly if the module_name argument is a dotted path

I don't think the module_name argument of module_has_submodule() is expected to be a dotted path. For example, this test (incorrectly, as far as I see) passes on Python 2:

  • tests/utils_tests/test_module_loading.py

    diff --git a/tests/utils_tests/test_module_loading.py b/tests/utils_tests/test_module_loading.py
    index 2a524a2..70047b2 100644
    a b class DefaultLoader(unittest.TestCase):  
    2626        test_no_submodule = import_module(
    2727            'utils_tests.test_no_submodule')
    2828
     29        self.assertTrue(module_has_submodule(test_module, 'invalid.good_module'))
     30
    2931        # An importable child
    3032        self.assertTrue(module_has_submodule(test_module, 'good_module'))
    3133        mod = import_module('utils_tests.test_module.good_module')

Is Django making any calls like that or is this function used like this outside of Django?

comment:4 by tkhyn, 7 years ago

That's what I said, it works on Python 2, not on Python 3.

The problem I have with that is I need to use autodiscover_modules('path_to.my_module'), which calls module_has_submodule in every app. In Python 2 it attempts to discover this module in all apps. In Python 3 it raises an exception the first time path_to is not a package, preventing loading any module in subsequent apps.

I know one solution is to re-organize my apps structure to put my_module at the apps' root level, but:

  • that does mess up with the organization of my code
  • this used to work in Python 2 and could easily be made to work in Python 3, and I would like to avoid having to monkey-patch django just to add a try / catch block ...
Version 0, edited 7 years ago by tkhyn (next)

comment:5 by tkhyn, 7 years ago

In addition, trying to reproduce your example with python 2.7.13 I get:

>>> from importlib import import_module
>>> from django.utils.module_loading import module_has_submodule
>>> test_module = import_module('utils_tests.test_module')
>>> module_has_submodule(test_module, 'invalid.good_module')
False

comment:6 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted

I don't know if module_has_submodule() should not accept dotted paths, however, as far as I can tell, the behavior is undocumented and untested. We could either fix it or raise a more helpful error message.

comment:7 by tkhyn, 7 years ago

Owner: changed from nobody to tkhyn
Status: newassigned

Great, I'll submit a PR with testcases + solution for all currently supported versions.

comment:8 by tkhyn, 7 years ago

Has patch: set

Here is a patch: PR 8611 PR 8614

Last edited 7 years ago by tkhyn (previous) (diff)

comment:9 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In f6bd0013:

Fixed #28241 -- Allowed module_has_submodule()'s module_name arg to be a dotted path.

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