Backtracking URL resolver

[Nowell Strite]

Right now the URL resolver only resolves to the first matched URL, and the view that is resolved to is unable to inspect the requested URL to see if it truly should be the view to be handling this URL. You can only have one "catch all" url pattern, however, occasionally it would make sense to allow distinct applications to receive a URL and upon inspection claim it, or allow the URL resolver to continue to find a match (i.e. CMS, legacy database driven system to handle legacy urls, etc.)

​https://github.com/django/django/pull/37

Thoughts?

If this is a desired feature, then I would be happy to write the docs or further this concept.

Thanks!
Nowell Strite

[Tai Lee]

I have had a need for multiple catch-all views in my own projects. I solve the problem in my own code by creating a single catch-all view that executes the others in sequence, continuing until one returns a valid response and raising Http404 if all fail.

This is somewhat similar to middleware that handles Http404 responses, but I needed to implement this as a catch-all URL and view because one of the catch-all views needed to determine the requested app name and namespace from the URL by looking up a ResolverMatch object in order to know which templates to load.

It would be nicer to allow any view to raise Resolver404 and have Django continue to the next matching URL if there is one. I'd like to see this feature or something similar to it make it into Django.

However, I suspect that this could cause some problems when people need to resolve a URL in their own code (not as part of the normal request/view/response machinery).

In this case, the resolved view returned may not actually be the view they want, and they may not know until it is too late -- when they attempt to execute it In many cases it may not be appropriate to execute a resolved view in order to test if it is the correct one. I don't have a specific use-case to demonstrate a problem, though.

This could probably use some discussion on django-dev, so I'm marking as DDN.

I found it difficult to review the "patch" (a pull request with many commits on GitHub), so I'm also removing the "has patch" flag. Please attach a single diff here in Trac if you would like the patch to be reviewed (e.g. I don't even know if it has tests).

[Russell Keith-Magee]

@mrmachine -- Github pull requests are an accepted form of contribution. I'm not 100% certain that this is laid out in our contribution guide; if it isn't, it should be. We have official mirrors on Github and BitBucket for a reason. The link provided can be trivially used to show a patch (click on the "diff" button above the changelist).

As for the issue itself: DDN is the right result, but I'm not sure it's for the right reason. My concern is over backwards compatibility -- a valid URLpatterns file that currently rejects a request could, if this change were adopted, result in a view actually being served. I'm not entirely certain that this is a bad thing -- it's really an ambiguity of the original URLpattern -- but it's worth some deeper consideration.

[Aymeric Augustin]

@russellm: I worked on the contributing guide in #16469 and I think it doesn't mention that pull requests are acceptable => #16777.

[Jannis Leidel]

Replying to russellm:

@mrmachine -- Github pull requests are an accepted form of contribution. I'm not 100% certain that this is laid out in our contribution guide; if it isn't, it should be. We have official mirrors on Github and BitBucket for a reason. The link provided can be trivially used to show a patch (click on the "diff" button above the changelist).

This isn't how I remembered the discussion about it (and not what I've been telling people for the last couple of months). Github and Bitbucket only host mirrors for the ease of diff creation. Trac (and patches) are still the preferred form of contribution.

[Nowell Strite]

Replying to russellm:

As for the issue itself: DDN is the right result, but I'm not sure it's for the right reason. My concern is over backwards compatibility -- a valid URLpatterns file that currently rejects a request could, if this change were adopted, result in a view actually being served. I'm not entirely certain that this is a bad thing -- it's really an ambiguity of the original URLpattern -- but it's worth some deeper consideration.

@russellm: I'm not sure I understand the backwards incompatible concern. Any view written prior to this feature will never raise a Resolver404 exception, and therefore the URL Resolver process will terminate with the existing view. Only in the case of a view being written to take advantage of this feature will trigger the URL resolver to act in the new fashion. As far as I can tell, this feature it totally backwards compatible. Here is a contrived example of how to leverage this new feature:

def legacy_url_redirect_view(request, path):
    try:
        redirect = DummyRedirectModel.objects.get(old_path=path)
    except: DummyRedirectModel.DoesNotExist:
        raise Resolver404
    else:
        return HttpResponsePermanentRedirect(redirect.new_path)

@mrmachine The default urlresolvers.resolve method still works in the backwards compatible way (it only returns the first matching regex result (and does not try and decent into calling the view, etc.) it works exactly as it did before, with no backwards incompatibility. It does add a new method urlresolvers.backtracking_resolve that an implementor can call that returns a generator that the implementor can use to leverage the backtracking ability)

Does this make sense? Do you see any wholes in the basic logic/backwards compatible construction?

Thanks!
Nowell Strite

[Jacob]

I've wanted this feature since literally before Django was open sourced. Alex agrees this is a good idea. Marking accepted.

I should note that we probably should use a completely new exception to avoid any possible backwards incompatibilities (and provide a better-reading API. Maybe something along the lines of ContinueResolving or something.

[Nowell Strite]

Replying to jacob:

I've wanted this feature since literally before Django was open sourced. Alex agrees this is a good idea. Marking accepted.

I should note that we probably should use a completely new exception to avoid any possible backwards incompatibilities (and provide a better-reading API. Maybe something along the lines of ContinueResolving or something.

Hey Jacob, I Implemented your suggestion and separated the exception off into it's own subclass of Resolver404 called ContinueResolving to provide a cleaner API as well as ensure no possible backwards incompatibility. Any suggestions on where you would like this functionality documented? In docs/topics/http/urls.txt? I took an initial stab at adding the ContinueResolving exception documentation into the documentation there. Can you provide any guidance or suggestion on a better format or approach to documenting this feature or where/how you would like to call it out. I have seen "versionadded:: 1.4" markers elsewhere but I wasn't sure how to fold in that indication within the current doc structure for this feature.

Thanks!
Nowell

[Nowell Strite]

Implemented jacobm's suggestion and separated the backtracking resolver exception off into it's own subclass of Resolver404 exception to provide a cleaner API as well as ensure no possible backwards incompatibility.

[anonymous]

Replying to jacob:

I've wanted this feature since literally before Django was open sourced. Alex agrees this is a good idea. Marking accepted.

I should note that we probably should use a completely new exception to avoid any possible backwards incompatibilities (and provide a better-reading API. Maybe something along the lines of ContinueResolving or something.

Any chance of this being accepted and merged?

[Julien Phalip]

The patch looks great. Nice work! I've updated it to current trunk.

I had a few remarks and questions regarding the implementation:

• Would it be possible to implement the generator as a class rather than a private method of RegexURLResolver, or even merge it with ResolverMatches? That would seem cleaner.
• Shouldn't the backtracking functionality be enabled by default, i.e. done by RegexURLResolver.resolve() itself, since it is meant to be backwards-compatible? I'm not sure we need two separate methods (resolve() and backtracking_resolve()).
• The 'peeking' implementation in ResolverMatches feels a bit convoluted. Could it be simplified by using a buffer? See: ​http://stackoverflow.com/questions/1517862/using-lookahead-with-generators/1517965#1517965
• I'm not sure ContinueResolving is a good name for the exception, since the exception is meant to be raised by the view and the name is quite suggestive about the implementation that happens outside of the view (i.e. in the resolver). As far as the view is concerned, the only thing it cares to say is "Do not use me", or "Skip me", so a name like SkipView, for example, would seem more appropriate.

Other than that, I think it's already in great shape and it's getting pretty close!

[meric]

Patch for overlapping urls

[anonymous]

This patch is naive and cannot get around this problem:

"However, I suspect that this could cause some problems when people need to resolve a URL in their own code (not as part of the normal request/view/response machinery)."

But it uses generators and has the functionality enabled by default and named the exception to be raised as urlresolvers.DoesNotResolve.

[meric]

Replying to anonymous:

This patch is naive and cannot get around this problem:

"However, I suspect that this could cause some problems when people need to resolve a URL in their own code (not as part of the normal request/view/response machinery)."

But it uses generators and has the functionality enabled by default and named the exception to be raised as urlresolvers.DoesNotResolve.

Forgot to username of comment. meric made this comment.

[Eric Man <meric.au@…>]

Replying to anonymous:

[meric]

I have continued to work on the patch here: ​https://github.com/meric/django/compare/ticket_16774

[meric]

Forum thread: ​https://groups.google.com/d/topic/django-developers/BAk5pGb7zE8/discussion

[Tim Graham]

​https://github.com/django/django/pull/378

[German M. Bravo]

The problem with using a generator is when the url patterns are themselves resolvers (includes) and one wants to skip, for example, the first match in that nested resolver, but not the second. At this point, when the DoesNotResolve exception is caught the generator would not allow trying the nested "unfinished" resolver once again, where it left and where it would march the second url.

[German M. Bravo]

When using a generator, as it did before, patch for #16774 wasn't resolving nested patterns like the following because the generator for the first patternr'^nestred/' had already been extracted from the generator by the second try (so HTTP 404 was returned instead of HttpResponse "OK"):

def response_view():
    return HttpResponse("OK")

def does_not_resolve_view():
    raise DoesNotResolve

urlpatterns += patterns('',
    (r'^nested/', include(patterns('',
        (r'^overlapping_view/$', does_not_resolve_view),
        (r'^overlapping_view/$', response_view),
    ))),
)

I'm attaching a patch with tests.

[German M. Bravo]

Updated for Django v1.7, also it keeps track of already tried urls

[Tijani-Dia]

I've tried to put everything said here in this patch ​https://github.com/Tijani-Dia/django/tree/ticket_16774( messed up with the branch, will fix soon).

These tests are the only failing one - in my environment at least -:
​https://github.com/django/django/blob/7582d913e7db7f32e4cdcfafc177aa77cbbf4332/tests/handlers/tests.py#L214-L222
​https://github.com/django/django/blob/7582d913e7db7f32e4cdcfafc177aa77cbbf4332/tests/handlers/tests.py#L269-L275

Let me know what do you think about it!

I am ready to make any changes necessary - write docs.

[Adam Johnson]

Well done for reviving an old ticket Tijani-Dia

I think I'm against this feature. It won't interact work with Middleware.process_view() particularly well at all. For example, atomic requests, as ​Florian mentioned on django-developers, or @csrf_exempt.

Also there's a potential footgun: if a view raises DoesNotResolve at a very late stage, the view could have had many side effects but then be less visible in logging or APM traces.

I think the already-suggested solution of one view that calls other subviews is sensible for most situations.

[Tijani-Dia]

Thanks for the feedback, Adam.

Alright, I liked working on it and I understand that it may no longer be a much-desired feature.