provide ability to abort URL resolution early

[Chris Jerdonek]

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.

[Chris Jerdonek]

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.

[Russell Keith-Magee]

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).

[Chris Jerdonek]

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).

[Chris Jerdonek]

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.

[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?

[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.

[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 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?

[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)?

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 ​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.

[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 if a 'consensus' is found on this.

[Nick Pope]

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.