[Django] #31527: Admindocs' View index assumes settings.ROOT_URLCONF is an import string
Django
---------------------------------------------+------------------------
Reporter: Keryn Knight | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admindocs | Version: master
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 |
---------------------------------------------+------------------------
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'
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31527>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Keryn Knight | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admindocs | Version: master
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):
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? (Also: How does it go with tests and
override_settings and such? Is this literally the only place it blows up?)
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:1>
Django
Reporter: Keryn Knight | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admindocs | Version: master
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 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
[https://docs.djangoproject.com/en/3.0/topics/http/urls/#how-django-
processes-a-request 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:2>
Django
Reporter: Keryn Knight | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admindocs | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Unreviewed => Accepted
Comment:
Thanks for the update Keryn. Let's accept this, since it seems we could
investigate bullet proofing the admindoc view there.
Nice example.
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:3>
Django
Reporter: Keryn Knight | Owner: nobody
Component: contrib.admindocs | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:4>
Django
Reporter: Keryn Knight | Owner:
Type: | jimxie1228
Cleanup/optimization | Status: assigned
Component: contrib.admindocs | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jimxie1228):
* owner: nobody => jimxie1228
* status: new => assigned
Comment:
Hey! I'm a first time contributor and I would like to put in a patch to
fix this issue. I'll start by understanding the source code. Any
help/guidance on which module to look for in the source code would be much
appreciated!
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:5>
Django
Reporter: Keryn Knight | Owner:
Cleanup/optimization | Status: assigned
Component: contrib.admindocs | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Hi Qihao, welcome
[https://github.com/django/django/blob/0773837e15bb632afffb6848a58c59a791008fa1/django/contrib/admindocs/views.py#L121-L124
The code in question is in here]. Once you're up and running, you can post
to the [https://forum.djangoproject.com/c/internals/mentorship/10 forum]
if you need help.
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:6>
Django
Reporter: Keryn Knight | Owner:
Type: | Qihao(Jim) Xie
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Qihao(Jim) Xie):
Replying to [comment:6 Carlton Gibson]:
> Hi Qihao, welcome
>
>
[https://github.com/django/django/blob/0773837e15bb632afffb6848a58c59a791008fa1/django/contrib/admindocs/views.py#L121-L124
The code in question is in here]. Once you're up and running, you can post
to the [https://forum.djangoproject.com/c/internals/mentorship/10 forum]
if you need help.
Hey Carlton, thank you for pointing me to the code in question! Since I'm
not familiar with Django's codebase yet, my plan is to try to reproduce
the issue suggested by Keryn in the original ticket and possibly write a
testcase for it by the end of this week. From there I'll spend a week or
two working on the patch and the documentation for the patch. I'll keep
commenting this ticket as I make progress.
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:7>
Django
Reporter: Keryn Knight | Owner:
Type: | Qihao(Jim) Xie
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Qihao(Jim) Xie):
Here is what I have for now:
PR[https://github.com/django/django/pull/13748]. Happy to see any
comments!
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:8>
Django
Reporter: Keryn Knight | Owner:
Type: | Qihao(Jim) Xie
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:9>
Django
Reporter: Keryn Knight | Owner:
Type: | Qihao(Jim) Xie
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:10>
Django
Reporter: Keryn Knight | Owner:
Type: | Qihao(Jim) Xie
Component: contrib.admindocs | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson <carlton@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ce60d28929db9f4bdd0cbdc9652808328ca37d6e" ce60d289]:
{{{
#!CommitTicketReference repository=""
revision="ce60d28929db9f4bdd0cbdc9652808328ca37d6e"
Fixed #31527 -- Allowed admindocs index to handle non-string URLconfs.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31527#comment:11>