[Django] #33013: "AppConfig" subclass imports found in Django 3.2's automatic "AppConfig" discovery
Django
discovery
----------------------------------------+------------------------
Reporter: jwayodi | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
We are in the process of adding support for Django 3.2 to
[https://github.com/django-oscar/django-oscar Oscar].
Oscar [https://github.com/django-oscar/django-
oscar/blob/04dd391c900537a61dbb0ae5250ca5c2df6bbc4b/src/oscar/core/application.py#L139
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
[https://github.com/django/django/commit/3f2821af6bc48fa8e7970c1ce27bc54c3172545e
#diff-
0f8bc657bc27c9f80385c4814c2c2ebc033bda3e03285a7212965309a481cc70R110-R120
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
[https://github.com/django/django/commit/3f2821af6bc48fa8e7970c1ce27bc54c3172545e
#diff-
0f8bc657bc27c9f80385c4814c2c2ebc033bda3e03285a7212965309a481cc70R110-R120
does a check] to exclude `django.apps.config.AppConfig` from 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.
[https://github.com/django/django/commit/3f2821af6bc48fa8e7970c1ce27bc54c3172545e
#diff-
0f8bc657bc27c9f80385c4814c2c2ebc033bda3e03285a7212965309a481cc70R110-R120
adding the filter] `(not hasattr(mod, "__all__") or candidate.__name__ in
mod.__all__)`), would work, and is a bit more intuitive.
--
Ticket URL: <https://code.djangoproject.com/ticket/33013>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
discovery
Reporter: jwayodi | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Samir Shah):
* cc: Samir Shah (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/33013#comment:1>
Django
discovery
Reporter: Joseph Wayodi | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Does setting the new `AppConfig.default` to `False` not solve this?
https://docs.djangoproject.com/en/3.2/ref/applications/#django.apps.AppConfig.default
See also the [https://docs.djangoproject.com/en/3.2/ref/applications
/#configuring-applications-ref Configuring Applications] docs.
--
Ticket URL: <https://code.djangoproject.com/ticket/33013#comment:2>
Django
discovery
Reporter: Joseph Wayodi | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => invalid
Comment:
I'm going to assume this works. If not, please provide a minimal reproduce
and we can reopen. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/33013#comment:3>
Django
discovery
Reporter: Joseph Wayodi | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Joseph Wayodi):
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
[https://github.com/django/django/commit/3f2821af6bc48fa8e7970c1ce27bc54c3172545e
#diff-0f8bc657bc27c9f80385c4814c2c2ebc033bda3e03285a7212965309a481cc70R117
check for that special case]. What we are asking for is a generic solution
to that problem, which replacements for `AppConfig` can rely on.
--
Ticket URL: <https://code.djangoproject.com/ticket/33013#comment:4>
Django
discovery
Reporter: Joseph Wayodi | Owner: nobody
Type: Bug | Status: new
Component: Core (Other) | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Joseph Wayodi):
* cc: Joseph Wayodi (added)
* status: closed => new
* resolution: invalid =>
--
Ticket URL: <https://code.djangoproject.com/ticket/33013#comment:5>
Django
discovery
Reporter: Joseph Wayodi | Owner: nobody
Type: Bug | Status: closed
Component: Core (Other) | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* status: new => closed
* resolution: => wontfix
Comment:
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
[https://github.com/django/django/pull/12310 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33013#comment:6>