[Django] #22425: provide ability to abort URL resolution early
Django
-----------------------------+--------------------
Reporter: cjerdonek | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
This is a feature request to provide a way to abort early from URL
resolution if a regex doesn't match. I was able to figure out a way to do
this myself using some wrapper constructs, but it seemed unnecessarily
tricky and also relied on Django internals (and so may be brittle).
To illustrate the use case, consider a typical URLconf:
{{{
urlpatterns = patterns('',
url(r'^articles/', include(article_urls)),
url(r'^blogs/', include(blog_urls)),
...
)
}}}
While the above is common, it also seems common that one would want URL
resolution to result in a 404 if an URL matches `^articles/` but does not
match the first `url()` pattern above. This would be the case, for
example, when all `article` URLs should be in the `article_urls` URLconf.
However, Django would process such an URL by unnecessarily continuing to
examine every pattern in `urlpatterns` that follows (`^blogs/`, etc). It
would be good to have a way to achieve this 404 functionality in a
performant and DRY way.
For example, the following would not be as performant or DRY as it could
be because you have to duplicate the regex and because Django's URL
resolver would unnecessarily need to match against `^articles/` twice:
{{{
urlpatterns = patterns('',
url(r'^articles/', include(article_urls)),
url(r'^articles/', page_not_found),
url(r'^blogs/', include(blog_urls)),
...
)
}}}
Modifying `account_urls` and `public_urls` etc by including
`page_not_found` is also not a good solution because it is not DRY (the
`page_not_found` addition needs to be repeated) and because those URLs
might be in third-party libraries.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: cjerdonek | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | 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
Changes (by cjerdonek):
* cc: chris.jerdonek@… (added)
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:1>
Django
Reporter: cjerdonek | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Another reason for implementing this in Django is that it would be easier
to have the nice 404 debug page that explains why the page is a 404. The
solution I came up with does not display the debug 404 because the
`page_not_found` view occurs explicitly in the URLconf.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:2>
Django
Reporter: cjerdonek | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by russellm):
* stage: Unreviewed => Accepted
Comment:
Accepting the idea as an interesting suggestion. However, by way of
design, I'd suggest a slightly different construction:
{{{
urlpatterns = patterns('',
url(r'^articles/', include(article_urls, terminal_prefix=True)),
url(r'^blogs/', include(blog_urls)),
…
)
}}}
That is - "if this prefix didn't match, a full URL, but the prefix
matched, then abort" (`terminal_prefix` is a bikeshed name - happy for an
alternative here).
The reason I'm suggesting this - I can't see a use case where you would
include page_not_found as a url pattern *without* repeating a prefix of an
included pattern. On top of that, using the include means that you don't
need to re-run the prefix match; the "include()" is already matching the
prefix, so it's a matter of aborting if the included pattern didn't match
anything.
One thing to keep in mind during design/testing -- the interaction of this
feature with the flatpages middleware (or any other response-processing
middleware).
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:3>
Django
Reporter: cjerdonek | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Thanks. Yes, enhancing `include()` in some way like that is also what I
was thinking.
Here's another scenario for consideration that is related but different.
This came up in something I'm working on. Consider an URLconf of URLs
that should only be accessible when logged in:
{{{
urlpatterns = patterns('',
url(r'^private/', include(logged_in_urls)),
...
)
}}}
In this case, if a "private" URL is requested that doesn't match, I would
like the following custom view to take effect: If the user isn't logged
in, the user is redirected to the login page with `next` properly set.
Otherwise, the user sees a 404.
This makes me wonder if the change should be more general than a simple
boolean, something like: `include(logged_in_urls,
catch_all_view=login_or_404_view)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:4>
Django
Reporter: cjerdonek | Owner: nobody
Type: New feature | Status: new
Component: Core (URLs) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by cjerdonek):
Okay, so I found a simple way to implement this and that doesn't rely on
Django internals, so maybe this feature request is no longer necessary:
{{{
def include_all(arg, namespace=None, app_name=None, catch_all_view=None):
"""Return an URLconf for inclusion that matches all URLs."""
if catch_all_view is None:
catch_all_view = page_not_found
return include(patterns('',
url('', include(arg, namespace=namespace, app_name=app_name)),
url('', catch_all_view),
))
}}}
(It's possible the code above needs to be tweaked in some way, but I
believe the general idea is sound.)
From my perspective, the only outstanding issues related to this are (1)
making the debug 404 page show up in development instead of the real one
when `page_not_found` is used explicitly in an URLconf, and (2) making the
debug 404 page show more useful info when it renders (e.g. the URL pattern
that matched and the view that executed). I can open separate issues for
these as they are orthogonal to the current one.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:5>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tijani-Dia):
Hello, I've been digging into this the past days and I would be happy to
work on it.
I've done some initial work available here [https://github.com/Tijani-
Dia/django/tree/ticket_22425].
I would like to know what do you think about it?
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:6>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tijani-Dia):
* cc: Tijani-Dia (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:7>
Django
Reporter: Chris Jerdonek | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tijani-Dia):
* owner: nobody => Tijani-Dia
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:8>
Django
Reporter: Chris Jerdonek | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
> I would like to know what do you think about it?
I looked at your branch, and it looks like you started working on the
boolean argument suggested in an earlier comment. I would suggest looking
at the later comments where I suggest an API that accepts a view.
Accepting a view is more flexible than a boolean because it lets you
specify what view should be used for the pattern's catch-all.
I would also suggest opening a PR and choosing "draft" instead of just
working on a branch in your own repo. A PR is easier to review and has
greater visibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:9>
Django
Reporter: Chris Jerdonek | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tijani-Dia):
Thanks for the feedback!
I followed you suggestion and made a draft PR here
https://github.com/django/django/pull/14342.
I also tweaked it so the API can accept view names along with a boolean.
I'm doing this because I Imagine a situation where one doen't want to
provide a fallback but is sure that if this prefix isn't met, nothing
further will be met.
What do you think about it?
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:10>
Django
Reporter: Chris Jerdonek | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
> What do you think about it?
Instead of having two very similar arguments, I would just suggest having
one. What about the following (similar to what I suggested above)?
{{{#!python
def include(arg, namespace=None, catch_all=False):
}}}
The `catch_all` argument can be either a boolean or a view. If true, it
aborts early and uses the default catch-all view (e.g. the debug 404 page
when debug is enabled). Otherwise, it would use the given view. I also
think the view argument should just be a view (like what the related
[https://docs.djangoproject.com/en/3.2/ref/urls/#django.urls.path path()]
function accepts), rather than a string. I would need to look at the code
more closely, but there's a chance a third option might be useful.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:11>
Django
Reporter: Chris Jerdonek | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tijani-Dia):
I totally agree with you on making the view argument a real actual view
and your other suggestions too.
I will be happy to refine the PR but I would first like to hear from the
core team to know their expectations on this and discuss more about
arguments naming...
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:12>
Django
Reporter: Chris Jerdonek | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Nick Pope):
* has_patch: 1 => 0
Comment:
I've unchecked ''has patch'' to remove this from the review queue as the
[https://github.com/django/django/pull/14342 PR] has been closed.
If you want to pick this up again, then I suggest taking this to the
DevelopersMailingList to get some consensus on the design.
--
Ticket URL: <https://code.djangoproject.com/ticket/22425#comment:13>