Code

Opened 3 months ago

Closed 3 months ago

#21831 closed Bug (fixed)

If contrib.auth is not in INSTALLED_APPS, any import (even indirectly) of anything within contrib.auth causes Django to fail to startup

Reported by: carljm Owned by: nobody
Component: Core (Other) Version: master
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by carljm)

This is due to d818e0c9b2b88276cc499974f9eee893170bf0a8 (the new checks framework) and the new stricter app-loading stuff. It also relates to #20915 (the dependency that django.test has on django.contrib.auth) and #19774 (the dependency that django.contrib.contenttypes has on django.contrib.admin and thus indirectly on django.contrib.auth).

The check_user_model check for contrib.auth is registered in django/contrib/auth/__init__.py as a side effect of importing anything from within django.contrib.auth.

Actually executing the check_user_model check when contrib.auth is not installed results in "LookupError: No installed app with label 'auth'." on the cls = apps.get_model(app_name, model_name) line.

Attachments (0)

Change History (11)

comment:1 Changed 3 months ago by carljm

Although it would be nice to fix #20915 at some point, this really needs to be fixed independently of that, because it's just too fragile for any import of anything from within the django.contrib.auth to be prohibited unless auth is in INSTALLED_APPS.

Admin and contenttypes may have the same problem (since they also register checks in their __init__.py), though I haven't tested yet to see if the checks they register also blow up if the app isn't installed.

I think the right solution here is for check-registration to happen in AppConfig.ready() rather than as an import-time side effect in __init__.py (yuck!); but that requires resolving the question of how to make the transition to using AppConfig for all contrib apps, which is currently under discussion.

An uglier temporary fix would be to only register the auth check (in django/contrib/auth/__init__.py) if auth is installed, or (even worse) code the check_user_model check to be robust against the app not being installed. (The latter is not bad in and of itself, but it's a poor solution to the general problem - there's no reason for that check to even be registered in the first place if auth is not installed.)

comment:2 Changed 3 months ago by Russell Keith-Magee <russell@…>

In 56516ade5ebde53af16ffdf97d3dce8d27a31e4b:

Refs #21831 -- Softened the TestClient dependency on contrib.auth.

This is to prevent an import of django.test causing an import (and thus
an implicit checks regisration) for an app that may not be in
INSTALLED_APPS.

Better fixes may be possible when #20915 and/or #21829 are addressed.

Thanks to @carljm for the report.

comment:3 Changed 3 months ago by russellm

I've left this open as a ticket because the fix provided isn't ideal (and isn't tested because it's very difficult to test); at the very least it needs to be revisited before 1.7 final.

The real underlying problem isn't entirely the checks framework (at least, not directly) - the checks framework has just made the problem obvious. The underlying problem is the increased rigor on apps being in INSTALLED_APPS, combined with dependencies on contrib apps within core (and between contrib apps). django.tests depends on contrib.auth for some features (see #20915). contrib.contenttypes depends on contrib.admin (see #19774). I haven't done a complete audit, but it's possible other examples exist in Django's own code. More broadly, it's not hard to imagine similar problems exist in third party apps (especially as dependencies on contrib apps like auth).

I think the increased rigour provided by app loading is a good idea - we just need to handle the consequences.

Resolving #21829 will go a long way to improving the situation, because it will take the implicit import behaviour out of contrib/auth/__init__.py.

comment:4 Changed 3 months ago by carljm

Some updates here: django.contrib.contenttypes.generic imports admin stuff which imports from within django.contrib.auth; and South imports django.contrib.contenttypes.generic.

So we can broaden the list of projects that are broken until #21829 is fixed to include "any project that uses South or conttenttypes.generic, and does not use contrib.auth."

Last edited 3 months ago by carljm (previous) (diff)

comment:5 Changed 3 months ago by russellm

I did some investigation last night - it looks like these problems can all be avoided (at least, at runtime) if we put an is_installed guard around the checks registration; e.g.:

from django.apps import apps
…

if apps.is_installed('django.contrib.auth'):
    checks.register(…)

The downside is that this breaks a couple of tests because the contrib apps aren't yet installed when the test framework imports the contrib apps, so the checks aren't registered. Ultimately, ready() is a much better solution for this, but if we can't come to a resolution, or we want an immediate fix for the alpha (and we're willing to live with a couple of known and understood test fails for the alpha), this could be an option.

comment:6 Changed 3 months ago by charettes

For reference, the fact that django.contrib.contenttypes.generic imports admin stuff is tracked in #19774.

Last edited 3 months ago by charettes (previous) (diff)

comment:7 Changed 3 months ago by carljm

  • Description modified (diff)
  • Summary changed from Cannot run tests without contrib.auth in INSTALLED_APPS to If contrib.auth is not in INSTALLED_APPS, any import (even indirectly) of anything within contrib.auth causes Django to fail to startup

comment:8 Changed 3 months ago by russellm

A minor clarification on the ticket title - the problem isn't just contrib.auth - it's also contrib.admin and contrib.contenttypes. If you import anything from these modules, and you don't have the corresponding app in INSTALLED_APPS, you'll have problems.

comment:9 Changed 3 months ago by aaugustin

2ff93e027c7b35378cc450a926bc2e4a446cacf0 must have solved at least part of this issue.

Can someone confirm if there's anything left to do, or simply close this ticket?

comment:10 Changed 3 months ago by carljm

It should have solved all of it I think. I can confirm and close later today.

comment:11 Changed 3 months ago by carljm

  • Resolution set to fixed
  • Status changed from new to closed

I confirm that with the move of check-registrations into AppConfig.ready in the fix for #21829, this problem is resolved.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.