Opened 4 years ago
Closed 4 years ago
#33013 closed Bug (wontfix)
"AppConfig" subclass imports found in Django 3.2's automatic "AppConfig" discovery
| Reported by: | Joseph Wayodi | Owned by: | nobody |
|---|---|---|---|
| Component: | Core (Other) | Version: | 3.2 |
| Severity: | Normal | Keywords: | |
| Cc: | Samir Shah, Joseph Wayodi | Triage Stage: | Unreviewed |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
We are in the process of adding support for Django 3.2 to Oscar.
Oscar mixes in its apps' configurations into Django's AppConfig class, and developers using Oscar then subclass these, to make their own modifications:
# my_app/apps.py
from oscar.core.application import OscarConfig
class MyAppConfig(OscarConfig):
# my app modifications
With Django 3.2, for apps added to INSTALLED_APPS using the dotted Python path to the app package, Django finds all AppConfig subclasses in the apps.py (using Python's inspect.getmembers), including ones imported to be subclassed. django.apps.config.AppConfig itself does get excluded, but this is not sufficient, as we and the developers using Oscar do a lot of sub classing of these classes.
To solve this, we would have have to do:
# my_app/apps.py
from oscar.core import application
class MyAppConfig(application.OscarConfig):
# my app modifications
And then ask the developers using Oscar to also do the same.
The problem with this is:
- it is not intuitive - Python developers would not typically expect imports to cause side effects like this, which creates the potential for confusing and hard-to-debug issues
- it is flaky - a new developer may innocently refactor imports not realising why they were done that way, and then encounter the confusing issues
- explicit class imports are normally preferred in Python, unless they cause local name clashes, so requiring them for this reason is a bit odd
- Django itself does a check to exclude
django.apps.config.AppConfigfrom being detected, which is indicative of the fundamental issue which remains
It would be nice if Django could mitigate this by having a mechanism for deciding which classes are considered (e.g. using something like the abstract metadata option used for models). In the short term, restricting the scanning for AppConfig subclasses to those named in the app.py's __all__ variable, if it has one (by e.g. adding the filter (not hasattr(mod, "__all__") or candidate.__name__ in mod.__all__)), would work, and is a bit more intuitive.
Change History (6)
comment:1 by , 4 years ago
| Cc: | added |
|---|
comment:2 by , 4 years ago
comment:3 by , 4 years ago
| Resolution: | → invalid |
|---|---|
| Status: | new → closed |
I'm going to assume this works. If not, please provide a minimal reproduce and we can reopen. Thanks!
comment:4 by , 4 years ago
Oscar provides a drop-in replacement for Django's default AppConfig, for use by Oscar itself, and by any projects that depend on it:
# oscar/core/application.py (Oscar's base "AppConfig" subclass)
class OscarConfig(AppConfig):
# Oscar's extensions to "AppConfig"
# oscar/apps/basket/apps.py (one of Oscar's apps)
from oscar.core.application import OscarConfig
class BasketConfig(OscarConfig):
# Oscar's "basket" app configurations
# project/settings.py (a project that depends on Oscar)
INSTALLED_APPS = [
...
'oscar.apps.basket',
...
]
With default_app_config for oscar.apps.basket set, the following deprecation warning is displayed:
/project-venv/lib/python3.8/site-packages/django/apps/registry.py:91: RemovedInDjango41Warning: 'oscar.apps.basket' defines default_app_config = 'oscar.apps.basket.apps.BasketConfig'. However, Django's automatic detection did not find this configuration. You should move the default config class to the apps submodule of your application and, if this module defines several config classes, mark the default one with default = True. app_config = AppConfig.create(entry)
If default_app_config for oscar.apps.basket is removed, Django finds multiple non-default AppConfig subclasses (the imported OscarConfig, and its subclass BasketConfig), and ends up using its default AppConfig class.
If OscarConfig.default is set to False, this attribute gets inherited by its BasketConfig subclass, and Django finds no default AppConfig subclass, and ends up using its default AppConfig class. This would require us to switch to specifying the app in INSTALLED_APPS using only oscar.apps.basket.apps.BasketConfig. Or for projects using Oscar to subclass BasketConfig just to set BasketConfig.default to True.
We could set BasketConfig.default to True, but this then causes a problem for any project using Oscar, which should in turn be able to subclass BasketConfig:
# project/basket/apps.py (overriden app in a project that depends on Oscar)
from oscar.apps.basket.apps import BasketConfig
class OverriddenBasketConfig(BasketConfig):
# overridden "basket" app configurations
# project/settings.py (a project that depends on Oscar)
INSTALLED_APPS = [
...
'project.basket',
...
]
The default attribute gets inherited by its OverriddenBasketConfig subclass, and Django finds multiple default AppConfig subclasses (the imported BasketConfig, and its subclass OverriddenBasketConfig):
RuntimeError: 'project.basket.apps' declares more than one default AppConfig: 'OverriddenBasketConfig', 'BasketConfig'.
Note that the same underlying issue exists with Django's own AppConfig, which is why it is necessary to check for that special case. What we are asking for is a generic solution to that problem, which replacements for AppConfig can rely on.
comment:5 by , 4 years ago
| Cc: | added |
|---|---|
| Resolution: | invalid |
| Status: | closed → new |
comment:6 by , 4 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
Hi Joseph. Thanks for the additional details.
If OscarConfig.default is set to False, this attribute gets inherited by its BasketConfig subclass, and Django finds no default AppConfig subclass, and ends up using its default AppConfig class. This would require us to switch to specifying the app in INSTALLED_APPS using only oscar.apps.basket.apps.BasketConfig. Or for projects using Oscar to subclass BasketConfig just to set BasketConfig.default to True.
This is the right approach in your case, where you have multiple AppConfig classes in play. Set default = False on the superclass and have users config the explicit app config they want, or set default = True on the one subclass they need.
It's worth reviewing the discussion on the original PR where this kind of thing was discussed.
If you wish to propose a change it would need to go via the DevelopersMailingList to seek consensus there.
Does setting the new
AppConfig.defaulttoFalsenot solve this?https://docs.djangoproject.com/en/3.2/ref/applications/#django.apps.AppConfig.default
See also the Configuring Applications docs.