Consistent handling for app modules with no (or empty) __path__
|Reported by:||carljm||Owned by:||carljm|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
This ticket is a followup from discussion that occurred on https://github.com/django/django/pull/2212 in the course of resolving #17304. The description here is lengthy as I've tried to be comprehensive, but really the issue impacts only very unusual edge-case app modules.
Currently in master, if an app module has no __path__ attribute, its appconfig gets a path of None. This is intended to indicate that the app has no directory path that can be used for finding things like templates, static assets, etc. It is likely to break some naive code that iterates over app-configs using their paths. (For instance, the current code in AppDirectoriesFinder breaks with a TypeError if any app-config has a path of None).
If an app module would have an empty __path__, currently an obscure IndexError would be raised.
If an app module has a __path__ with more than one element in it (namespace package), ImproperlyConfigured is raised.
All of the above is bypassed if the app uses an AppConfig subclass that has an explicit path as a class attribute - in that case, that path is unconditionally used and no path detection occurs.
The __file__ attribute is not currently used at all for determining an app's filesystem path.
Here are some types of app-modules that could be referenced in INSTALLED_APPS, and the contents of their __path__ and __file__ attributes:
A) An ordinary package, e.g 'myapp' where myapp is a directory containing an __init__.py. In this case, the module object's __path__ will be ['/path/to/myapp/'] and its __file__ will be '/path/to/myapp/__init__.pyc'
B) A namespace package with only a single location. In this case, __path__ is length-one, just as in (A), and there is no __file__ attribute.
C) A namespace package with multiple locations. __path__ is length > 1, and there is no __file__.
D) A single-file module, e.g. 'myapp' where there is just myapp.py. In this case, the module will have no __path__ attribute, and __file__ will be /path/to/myapp.pyc.
E) A package inside a zipped egg, e.g. 'myapp' where there is a zipped egg myapp-0.1-py2.7.egg added to sys.path in a pth file, and the zipfile internally contains a file myapp/__init__.py. In this case, __path__ will contain the bogus non-path ['/path/to/myapp-0.1-py2.7.egg/myapp/'] and __file__ will similarly be a bogus non-path '/path/to/myapp-0.1-py2.7.egg/myapp/__init__.pyc'. (A package or module within an unzipped egg is no different from case (A) or (D).)
F) A module loaded by some hypothetical custom loader which leaves __path__ empty.
So currently for cases (A), (B), and (E) we use the single entry in __path__ (even though in case (E) that path doesn't exist), and for case (C) we raise ImproperlyConfigured and require the user to explicitly supply a path on the AppConfig. For case (D) we set appconfig.path to None, and for case (F) we raise an obscure IndexError. I consider our handling of cases (D) and (F) sub-optimal.
I think it would be preferable if appconfig.path were never set to None. The algorithm I would propose for appconfig.path is this: (i) if AppConfig.path is set explicitly, use it, (ii) If app_module.__path__ exists and is length 1, use its only element, (iii) if app_module.__file__ exists, use os.path.dirname(__file__), and (iv) if no path can be deduced from the __path__ and __file__ attributes, then raise ImproperlyConfigured and tell the user they need to set the path explicitly on their AppConfig.
Raising ImproperlyConfigured for any type of app module that we supported in previous versions of Django would be a backwards-incompatible change. As far as I am aware, only cases (A) and (E) were previously explicitly supported, and neither of those would raise ImproperlyConfigured with my proposal.
Case (D) seems to work in previous versions of Django, though I don't believe that it was ever explicitly supported. In any case, it would be covered in my proposal by the fallback to os.path.dirname(__file__).
Case (F) is a bit vague, and I don't believe is a backwards-compatibility concern - I think my proposal provides reasonable handling of any combination of __path__ and __file__ that any custom loader might set.
If my proposal is rejected for whatever reason and we opt to stick with something closer to the status quo in master, then the following actions should be taken to resolve this ticket: set appconfig.path to None when __path__ exists but is empty to avoid the IndexError in case (F), and update AppDirectoriesFinder in contrib.staticfiles to not blow up if it its an appconfig.path that is None (and perhaps audit other uses of appconfig.path in Django to ensure they can handle it being None).
One issue that will need to be resolved for my proposal to move forward is that currently the test suite instantiates AppConfig in a number of places with app_module=None, which currently passes silently but under my proposal would cause ImproperlyConfigured to be raised. I plan to look into this issue and propose a patch.
Feedback on the concept welcome, in particular if I've missed any important cases.
Change History (7)
comment:1 follow-up: ↓ 2 Changed 19 months ago by aaugustin
- Triage Stage changed from Unreviewed to Accepted
comment:4 Changed 19 months ago by carljm
- Has patch set
- Owner changed from nobody to carljm
- Status changed from new to assigned