Opened 13 years ago

Last modified 11 days ago

#16774 new New feature

Backtracking URL resolver

Reported by: Nowell Strite Owned by:
Component: Core (URLs) Version: dev
Severity: Normal Keywords:
Cc: german.mb@…, Ülgen Sarıkavak Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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

Attachments (5)

backtracking_resolver.diff (18.0 KB ) - added by Nowell Strite 13 years ago.
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.
16774.diff (18.2 KB ) - added by Julien Phalip 12 years ago.
overlapping_urls.diff (15.0 KB ) - added by meric 12 years ago.
Patch for overlapping urls
#16774-Backtracking_URL_resolver.diff (9.8 KB ) - added by German M. Bravo 10 years ago.
#16774-Backtracking_URL_resolver.2.diff (9.9 KB ) - added by German M. Bravo 9 years ago.
Updated for Django v1.7, also it keeps track of already tried urls

Download all attachments as: .zip

Change History (35)

comment:1 by Tai Lee, 13 years ago

Has patch: unset
Triage Stage: UnreviewedDesign decision needed

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

comment:2 by Russell Keith-Magee, 13 years ago

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

comment:3 by Aymeric Augustin, 13 years ago

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

Last edited 13 years ago by Aymeric Augustin (previous) (diff)

in reply to:  2 comment:4 by Jannis Leidel, 13 years ago

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.

in reply to:  2 comment:5 by Nowell Strite, 13 years ago

Has patch: set

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

comment:6 by Jacob, 13 years ago

Triage Stage: Design decision neededAccepted

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.

in reply to:  6 comment:7 by Nowell Strite, 13 years ago

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

Last edited 13 years ago by Nowell Strite (previous) (diff)

by Nowell Strite, 13 years ago

Attachment: backtracking_resolver.diff added

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.

in reply to:  6 comment:8 by anonymous, 12 years ago

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?

by Julien Phalip, 12 years ago

Attachment: 16774.diff added

comment:9 by Julien Phalip, 12 years ago

Patch needs improvement: set

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!

comment:10 by anonymous, 12 years ago

Owner: changed from nobody to meric

by meric, 12 years ago

Attachment: overlapping_urls.diff added

Patch for overlapping urls

comment:11 by anonymous, 12 years ago

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.

in reply to:  11 comment:12 by meric, 12 years ago

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.

comment:13 by meric, 12 years ago

Owner: changed from meric to anonymous

comment:14 by meric, 12 years ago

Owner: changed from anonymous to nobody

in reply to:  10 comment:15 by Eric Man <meric.au@…>, 12 years ago

Owner: changed from nobody to meric

Replying to anonymous:

comment:16 by Eric Man <meric.au@…>, 12 years ago

Owner: changed from meric to nobody

comment:17 by meric, 12 years ago

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

comment:19 by Florian Apolloner, 11 years ago

Needs documentation: set
Version: 1.3master

comment:21 by German M. Bravo, 10 years ago

Cc: german.mb@… added

comment:22 by German M. Bravo, 10 years ago

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.

comment:23 by German M. Bravo, 10 years ago

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.

by German M. Bravo, 10 years ago

by German M. Bravo, 9 years ago

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

comment:24 by Marten Kenbeek, 9 years ago

Component: HTTP handlingCore (URLs)

comment:25 by Tijani-Dia, 3 years ago

Owner: changed from nobody to Tijani-Dia
Status: newassigned

comment:26 by Tijani-Dia, 3 years ago

I've tried to put everything said here in the following patch https://github.com/Tijani-Dia/django/tree/ticket_16774

Let me know what do you think about it!

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

Last edited 3 years ago by Tijani-Dia (previous) (diff)

comment:27 by Adam Johnson, 3 years ago

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.

comment:28 by Tijani-Dia, 3 years ago

Thanks for the feedback, Adam.

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

Last edited 3 years ago by Tijani-Dia (previous) (diff)

comment:29 by Tijani-Dia, 3 years ago

Owner: Tijani-Dia removed
Status: assignednew

comment:30 by Ülgen Sarıkavak, 11 days ago

Cc: Ülgen Sarıkavak added
Note: See TracTickets for help on using tickets.
Back to Top