Code

Opened 3 years ago

Last modified 5 months ago

#16774 new New feature

Backtracking URL resolver

Reported by: nbstrite Owned by: nobody
Component: HTTP handling Version: master
Severity: Normal Keywords:
Cc: german.mb@… 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 (4)

backtracking_resolver.diff (18.0 KB) - added by nbstrite 3 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 3 years ago.
overlapping_urls.diff (15.0 KB) - added by meric 22 months ago.
Patch for overlapping urls
#16774-Backtracking_URL_resolver.diff (9.8 KB) - added by Kronuz 5 months ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 3 years ago by mrmachine

  • Has patch unset
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design 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 follow-ups: Changed 3 years ago by 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).

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 Changed 3 years ago by aaugustin

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

Last edited 3 years ago by aaugustin (previous) (diff)

comment:4 in reply to: ↑ 2 Changed 3 years ago by jezdez

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.

comment:5 in reply to: ↑ 2 Changed 3 years ago by nbstrite

  • 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 follow-ups: Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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.

comment:7 in reply to: ↑ 6 Changed 3 years ago by nbstrite

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 3 years ago by nbstrite (previous) (diff)

Changed 3 years ago by nbstrite

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.

comment:8 in reply to: ↑ 6 Changed 3 years ago by 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?

Changed 3 years ago by julien

comment:9 Changed 3 years ago by julien

  • 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 follow-up: Changed 22 months ago by anonymous

  • Owner changed from nobody to meric

Changed 22 months ago by meric

Patch for overlapping urls

comment:11 follow-up: Changed 22 months ago by 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.

comment:12 in reply to: ↑ 11 Changed 22 months ago by 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.

comment:13 Changed 22 months ago by meric

  • Owner changed from meric to anonymous

comment:14 Changed 22 months ago by meric

  • Owner changed from anonymous to nobody

comment:15 in reply to: ↑ 10 Changed 22 months ago by Eric Man <meric.au@…>

  • Owner changed from nobody to meric

Replying to anonymous:

comment:16 Changed 22 months ago by Eric Man <meric.au@…>

  • Owner changed from meric to nobody

comment:17 Changed 22 months ago by meric

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

comment:19 Changed 19 months ago by apollo13

  • Needs documentation set
  • Version changed from 1.3 to master

comment:21 Changed 6 months ago by Kronuz

  • Cc german.mb@… added

comment:22 Changed 5 months ago by Kronuz

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 Changed 5 months ago by Kronuz

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.

Changed 5 months ago by Kronuz

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.