Opened 3 months ago

Last modified 3 months ago

#31527 new Cleanup/optimization

Admindocs' View index assumes settings.ROOT_URLCONF is an import string

Reported by: Keryn Knight Owned by: nobody
Component: contrib.admindocs Version: master
Severity: Normal 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

Currently, the code looks like:

urlconf = import_module(settings.ROOT_URLCONF)
view_functions = extract_views_from_urlpatterns(urlconf.urlpatterns)

Buuuuuuuut, it's possible to get Django to otherwise run with, for example, something like:

def urlpatterns():
    from django.urls import path
    from django.contrib import admin
    return (
        path("admin/", admin.site.urls),
    )

ROOT_URLCONF = SimpleLazyObject(urlpatterns)

which will happily pass through the checks framework, and runserver (and gunicorn), and let you use Django as you otherwise would... except for navigating to django.contrib.admindocs.views.ViewIndexView (for example, routed at /admin/doc/views/) where you'll instead encounter an exception like:

File "/path/to/python3.7/site-packages/django/contrib/admindocs/views.py" in dispatch
  46.         return super().dispatch(request, *args, **kwargs)

File "/path/to/python3.7/site-packages/django/views/generic/base.py" in dispatch
  97.         return handler(request, *args, **kwargs)

File "/path/to/python3.7/site-packages/django/views/generic/base.py" in get
  158.         context = self.get_context_data(**kwargs)

File "/path/to/python3.7/site-packages/django/contrib/admindocs/views.py" in get_context_data
  139.         urlconf = import_module(settings.ROOT_URLCONF)

File "/path/to/python3.7/importlib/__init__.py" in import_module
  118.     if name.startswith('.'):

File "/path/to/python3.7/site-packages/django/utils/functional.py" in inner
  257.         return func(self._wrapped, *args)


Exception Type: AttributeError at /admin/doc/views/
Exception Value: 'tuple' object has no attribute 'startswith'

Change History (4)

comment:1 Changed 3 months ago by Carlton Gibson

Hi.

Docs have it thus:

ROOT_URLCONF: A string representing the full Python import path to your root URLconf, for example "mydjangoapps.urls". ...

So I'm kind of inclined towards wontfix. ViewIndexView is making a reasonable assumption.

Buuuuuuuut, it's possible to get Django to otherwise run with, for example, something like...

Can I ask what the use-case here is? (Update: like, I've done similar defining ROOT_URLCONF = __name__, which then pulls urlpatterns into the resolver... — just trying to grasp if we need to allow ROOT_URLCONF as a callable, rather than an import path.)

(Also: How does it go with tests and override_settings and such? Is this literally the only place it blows up?)

Last edited 3 months ago by Carlton Gibson (previous) (diff)

comment:2 in reply to:  1 Changed 3 months ago by Keryn Knight

Docs have it thus [...]
So I'm kind of inclined towards wontfix. ViewIndexView is making a reasonable assumption.

It is making a generally reasonable assumption, but it's an assumption which is sort of wrong.
The documentation is also misleading anyway, because it says Not defined is the default which is ... true (being that it's not in global_settings), but at least on 2.2, not having a ROOT_URLCONF is your project settings is an AttributeError:

/path/to/python3.7/site-packages/django/core/handlers/base.py", line 74, in get_response
    set_urlconf(settings.ROOT_URLCONF)
AttributeError: 'Settings' object has no attribute 'ROOT_URLCONF'

So ROOT_URLCONF must be set, fine. It is on any given startproject driven package anyway. Let's set it to something which might be considered "not defined" and do ROOT_URLCONF = None because we know how Django processes a request must allow for some variation upon it... and use a middleware instead:

class URLPatternsMW(MiddlewareMixin):
    def process_request(self, request):
        request.urlconf = urlpatterns()

That works fine, and lo we can navigate to the admin and all around it as you might expect (and also most of admindocs, because in my original ticket I stupidly omitted that route from the urlpatterns() definition). But if we try to go to the same URL as before (/admin/doc/views/), we get the same principle error, but on a different type:

'NoneType' object has no attribute 'startswith'

Perhaps we should've set ROOT_URLCONF = "" and tried that instead:

Exception Type: ValueError at /admin/doc/views/
Exception Value: Empty module name

It also cannot be a "myproject.urls" where there's no urlpatterns attribute, but it can if urlpatterns = [] is within that module - that's at least dealt with by the checks framework. So I guess that's the actual intended way of having no root urlconf to speak of... Ten years in, and I didn't actually know that :)
ROOT_URLCONF can be () and it's also seemingly fine (because set_urlconf does the weaker falsy test), again except for this import expectation. Mostly because these things are hashable.

Can I ask what the use-case here is? (Also: How does it go with tests and override_settings and such? Is this literally the only place it blows up?)

It's actually specifically within tests where I found it (or remembered it, I do think I encountered it before but just moved on with life), by doing:

if __name__ == "__main__":
    def urlpatterns(...):
        return ()
    settings.configure(ROOT_URLCONF=SimpleLazyObject(urlpatterns), ...)
    django.setup()
    run the tests etc.

I happen to "know" (hah!) using the deferred object mostly seems to work from other weird hacks and things I've played around with over the years (I think for example I may've been doing something similar in #26287), but I cannot say with certainty that it's the only place it poses an error (and certainly a bunch of downstream packages would make the same assumption).
I can say that it's the only place in Django where I know the error can happen, being that it's the only place which directly tries to import the value of ROOT_URLCONF, where most everything else handles it further down the resolver stack.

A cursory glancing guess (as is always my way, I'm afraid) at a fix is something more like:

        urlconf = get_resolver(settings.ROOT_URLCONF)
        try:
            view_functions = extract_views_from_urlpatterns(urlconf.url_patterns)
        except ImproperlyConfigured:
            view_functions = []

which is more like what everything else looks to do (using the threadlocal resolver). I don't offer that up as the full solution, but a possible direction in which a correction might be found.

comment:3 Changed 3 months ago by Carlton Gibson

Triage Stage: UnreviewedAccepted

Thanks for the update Keryn. Let's accept this, since it seems we could investigate bullet proofing the admindoc view there.

Nice example.

comment:4 Changed 3 months ago by Carlton Gibson

Type: UncategorizedCleanup/optimization
Note: See TracTickets for help on using tickets.
Back to Top