[Django] #16774: Backtracking URL resolver
Django
-------------------------+-------------------------------
Reporter: nbstrite | Owner: nobody
Type: New feature | Status: new
Milestone: | Component: HTTP handling
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
-------------------------+-------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/16774>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: nbstrite | Owner: nobody
Type: New | Status: new
feature | Component: HTTP handling
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by mrmachine):
* needs_better_patch: => 0
* has_patch: 1 => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Design decision needed
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:1>
Django
-------------------------------------+-------------------------------------
feature | Component: HTTP handling
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:2>
Django
-------------------------------------+-------------------------------------
feature | Component: HTTP handling
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
@russellm: I worked the contributing guide in #16469 and I think it
doesn't mention that pull requests are acceptable => #16777.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:3>
Django
-------------------------------------+-------------------------------------
feature | Component: HTTP handling
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
Resolution: | Has patch: 0
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by jezdez):
Replying to [comment:2 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:4>
Django
-------------------------------------+-------------------------------------
feature | Component: HTTP handling
Milestone: | Severity: Normal
Version: 1.3 | Keywords:
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Replying to [comment:2 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
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:5>
Django
---------------------------------------+-------------------------------
Type: New feature | Status: new
Milestone: | Component: HTTP handling
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------+-------------------------------
Changes (by jacob):
* stage: Design decision needed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:6>
Django
---------------------------------------+-------------------------------
Type: New feature | Status: new
Milestone: | Component: HTTP handling
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 0
UI/UX: 0 |
---------------------------------------+-------------------------------
Comment (by nbstrite):
Replying to [comment:6 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 can take a stab at docs if you like.
Thanks!
Nowell
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:7>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by anonymous):
Replying to [comment:6 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:8>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by julien):
* needs_better_patch: 0 => 1
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:9>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* owner: nobody => meric
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:10>
Django
-------------------------------+------------------------------------
Reporter: nbstrite | Owner: meric
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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```.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:11>
Django
-------------------------------+------------------------------------
Reporter: nbstrite | Owner: meric
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by meric):
Replying to [comment:11 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:12>
Django
-------------------------------+-------------------------------------
Reporter: nbstrite | Owner: anonymous
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by meric):
* owner: meric => anonymous
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:13>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by meric):
* owner: anonymous => nobody
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:14>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Eric Man <meric.au@…>):
* owner: nobody => meric
Comment:
Replying to [comment:10 anonymous]:
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:15>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Eric Man <meric.au@…>):
* owner: meric => nobody
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:16>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by meric):
I have continued to work on the patch here:
https://github.com/meric/django/compare/ticket_16774
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:17>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by meric):
Forum thread: https://groups.google.com/d/topic/django-
developers/BAk5pGb7zE8/discussion
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:18>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* needs_docs: 0 => 1
* version: 1.3 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:19>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by timo):
https://github.com/django/django/pull/378
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:20>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Kronuz):
* cc: german.mb@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:21>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:22>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (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 pattern`r'^nestred/'` had already been extracted from the generator
by the second try (so HTTP 404 was returned instead of HttpResponse "OK"):
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:23>
Django
-----------------------------+------------------------------------
Reporter: nbstrite | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by knbk):
* component: HTTP handling => Core (URLs)
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:24>
Django
-------------------------------+--------------------------------------
Reporter: Nowell Strite | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tijani-Dia):
* owner: nobody => Tijani-Dia
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:25>
Django
-------------------------------+--------------------------------------
Reporter: Nowell Strite | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Tijani-Dia):
I've tried to put everything said here in this patch https://github.com
/Tijani-Dia/django/tree/ticket_16774.
Let me know what you think about it.
I also made the following patch https://github.com/Tijani-
Dia/django/tree/ticket_22425 which adresses
https://code.djangoproject.com/ticket/22425
I am putting it here since the two tickets are closely related.
I am ready to make any changes necessary/write docs for both tickets.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:26>
Django
-------------------------------+--------------------------------------
Reporter: Nowell Strite | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by 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 [https://groups.google.com/g/django-
developers/c/UQZmLNzlDcw/m/8NfZxMOMEQAJ 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:27>
Django
-------------------------------+--------------------------------------
Reporter: Nowell Strite | Owner: Tijani-Dia
Type: New feature | Status: assigned
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by Tijani-Dia):
Thanks for the feedback, Adam.
Alright, I liked working on it but I understand that it may no longer be a
much-desired feature.
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:28>
Django
-------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* owner: Tijani-Dia => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:29>
Django
-------------------------------+------------------------------------
Reporter: Nowell Strite | Owner: (none)
Type: New feature | Status: new
Component: Core (URLs) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------------------
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/16774#comment:30>