Opened 3 years ago

Closed 3 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.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. 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 Samir Shah, 3 years ago

Cc: Samir Shah added

comment:2 by Carlton Gibson, 3 years ago

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 Configuring Applications docs.

comment:3 by Carlton Gibson, 3 years ago

Resolution: invalid
Status: newclosed

I'm going to assume this works. If not, please provide a minimal reproduce and we can reopen. Thanks!

comment:4 by Joseph Wayodi, 3 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 Joseph Wayodi, 3 years ago

Cc: Joseph Wayodi added
Resolution: invalid
Status: closednew

comment:6 by Carlton Gibson, 3 years ago

Resolution: wontfix
Status: newclosed

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.

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