Opened 11 years ago
Last modified 3 years ago
#22425 assigned New feature
provide ability to abort URL resolution early
Reported by: | Chris Jerdonek | Owned by: | Tijani-Dia |
---|---|---|---|
Component: | Core (URLs) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | chris.jerdonek@…, Tijani-Dia | Triage Stage: | Accepted |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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.
Change History (13)
comment:1 by , 11 years ago
Cc: | added |
---|
comment:2 by , 11 years ago
comment:3 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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).
comment:4 by , 11 years ago
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)
.
comment:5 by , 11 years ago
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.
comment:6 by , 4 years ago
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?
comment:7 by , 4 years ago
Cc: | added |
---|
comment:8 by , 4 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:9 by , 4 years ago
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.
comment:10 by , 4 years ago
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 doesn'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?
comment:11 by , 4 years ago
What do you think about it?
Instead of having two very similar arguments, I would suggest having one. What about the following (similar to what I suggested above)?
def include(arg, namespace=None, catch_all=False):
The catch_all
argument (read as "catch all URL patterns") 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 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.
comment:12 by , 4 years ago
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 if a 'consensus' is found on this.
comment:13 by , 3 years ago
Has patch: | unset |
---|
I've unchecked has patch to remove this from the review queue as the 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.
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.