Code

Opened 6 years ago

Closed 6 years ago

Last modified 2 years ago

#9176 closed Uncategorized (wontfix)

NoReverseMatch for optional arguments

Reported by: to.roma.from.djbug@… Owned by: mtredinnick
Component: Core (Other) Version: 1.0
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A number of URLs of my site have the form /<object_name>/something. The objects can have several representations, one of which is canonical, so I have a decorator that redirects requests with non-canonical names. The decorator (simplified) looks like this:

def url_from_view(view, **kwargs):
    return django.core.urlresolvers.reverse(view, kwargs=kwargs)

def decorator(view):
    def wrapper(request, obj_name, **kwargs):
        obj = load_object(obj_name)
        if canonical_name(obj) != obj_name:
            return HttpResponseRedirect(url_from_view(wrapper, obj_name=canonical_name(obj), **kwargs)
        return view(request, obj, obj_name, **kwargs)
    return wrapper

Everything worked excellently, until I added something like this to my urlpatterns:

(r"^(?P<parent>[a-zA-Z0-9_-])/something(?:/(?P<child>\d+))?/*$", something),

For /parent_noncanonical/something/42 it calls something(parent="parent_noncanonical", child="42") which then reverses the URL and redirects correctly to /parent_canonical/something/42.
However, if the child part is omitted, because it’s optional in the regex, as in /parent_noncanonical/something, the view gets called as something(parent="parent_noncanonical", child=None) which then fails to reverse.

The problem is that the reverse operation turns out not to be defined for some of data that the regular expressions can produce: when I pass it the same view and the same data that have been chosen by the urlpatterns, it fails to produce a reverse match.

The reverse matching engine should handle None parameters by finding matches which do not contain the corresponding groups. For example, for r"(?P<first>[a-z])?(?P<second>[0-9])?(?P<third>[A-Z])?" and first="d", second=None, third="J" the correct reverse match is "dJ".

Here’s the relevant Django code (core/urlresolvers.py, 227–241):

        for result, params in possibilities:
            if args:
                # skipped
            else:
                if set(kwargs.keys()) != set(params):
                    continue
                unicode_kwargs = dict([(k, force_unicode(v)) for (k, v) in kwargs.items()])
                candidate = result % unicode_kwargs
            if re.search(u'^%s' % pattern, candidate, re.UNICODE):
                return candidate
        raise NoReverseMatch("Reverse for '%s' with arguments '%s' and keyword "
                "arguments '%s' not found." % (lookup_view, args, kwargs))

For the URL pattern above, the possibilities are correctly enumerated:
[(u'%(parent)s/something', parent?), (u'%(parent)s/something/%(child)s', ['parent', 'child'])]
But then it fails to take into account the fact child is None, and chooses the wrong possibility.

Perhaps simply eliminating None by changing set(kwargs.keys()) != set(params) to set([k for (k, v) in kwargs.items() if v]) != set(params) could do the job.

Attachments (0)

Change History (9)

comment:1 Changed 6 years ago by to.roma.from.djbug@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

In fact, by modifying my url_from_view function this way:

def url_from_view(view, **kwargs):
    return reverse(view, kwargs=dict([(k, v) for (k, v) in kwargs.items() if v]))

I can solve the problem. Something like that should be added to Django.

comment:2 Changed 6 years ago by to.roma.from.djbug@…

Of course, that should be ‘...if v is not None]’. Otherwise, zeroes cause problems, into which I’ve just run ignorantly.

comment:3 Changed 6 years ago by russellm

  • Resolution set to duplicate
  • Status changed from new to closed

I'm pretty certain this is a dupe of #9038.

comment:4 Changed 6 years ago by mtredinnick

  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

This isn't a dupe of #9038. It's asking that if you pass in extra arguments (in this case child=None), the reverse() call shouldn't error out by saying that you've passed in extra arguments. So it's a different situation.

It's probably a reasonable exception to the "must pass in the precise arguments", since the groupdict() function on a reg-exp Match object does contain None for optional groups, so, as the reporter mentions, this makes things truly reversible.

comment:5 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick
  • Status changed from reopened to new

comment:6 Changed 6 years ago by mtredinnick

  • Triage Stage changed from Accepted to Design decision needed

Hmm ... thinking about this some more, it's not so clear that it is worth doing for this one particular case. It's a bit of an edge case because it's not guaranteed to work in all cases: nested groups won't work for either positional arguments or keyword arguments, for example. So it's not necessarily true that just because your view received a particular set of keyword or positional arguments that you can pass all of those back to reverse() and have it resolve to something. Implementing just this one case (None values) will then lead to people trying it with nested values and other odd combinations and we'll be at this forever.

Putting in "design decision needed", because I don't want to rush to (a different) judgement. But having played with writing some tests for a few minutes, I'm inclined to think it's not a good idea.

comment:7 Changed 6 years ago by mtredinnick

  • Resolution set to wontfix
  • Status changed from new to closed

Due to the nested groups issue mentioned in the previous comment, it isn't possible to always pass the arguments you receive from URL resolution back to reverse without looking at them. The particular issue describe in the initial description here is easy enough to work around in the calling code (delete any keys with None as the value) and I don't really want to add special cases that aren't universally applicable to the reverse-handling -- it will lead to lots of questions about why one case is supported and another isn't, etc.

So the rule is that just as it is now: you must pass in the exact set of arguments to match against and they must have valid values. Sorry, I realise this isn't ideal, but we can't have a perfect solution here without adding lots of extra code and it doesn't seem worth it to me.

comment:8 Changed 5 years ago by pragmar

I hit this too, I don't think it's as fringe as it appears. To fully take advantage of caching, I tend to use the path to create a breadcrumb trail and have many variants to the same content. This admittedly creates many urls of the same page content with minor variations, but the caching flexibility of anonymous views more than makes up for that and the dupes can be disallowed in robots.txt. Of course, I do understand the can of worms in terms of more advanced regex in urls so a templatetag solution seemed like a reasonable approach. In any case, I put a custom tag together that seems to solve it - I just use {% url_optional %} in place of {% url %} where needed. Perhaps someone will find it a useful workaround for the templates.

"""
     exactly the same as from django.template.defaulttags.url EXCEPT kwargs equal to None are removed
     this allows a bit more flexibility than the use of {% url %} where nesting is rested on optional 
     base kw arguments.
     
     see http://code.djangoproject.com/ticket/9176     
"""

from django import template
from django.template import TemplateSyntaxError
from django.template.defaulttags import URLNode, url

register = template.Library()

class URLNodeOptional(URLNode):
    
    """
    identical to django.template.defaulttags.URLNode
    but removes kwargs equal to None before resolving the url
    """
    
    def render(self, context):
        for k, v in self.kwargs.items():
            if v.resolve(context) is None:            
                self.kwargs.pop(k)
        return super(URLNodeOptional, self).render(context)

def url_optional(parser, token): 
    
    """
     creates the default URLNode, then routes it to the Optional resolver with the same properties
     by first creating the URLNode, the parsing stays in django core where it belongs.     
    """ 
    
    urlnode = url(parser, token)
    return URLNodeOptional(urlnode.view_name, urlnode.args, urlnode.kwargs, urlnode.asvar)
    

url_optional = register.tag(url_optional)

comment:9 Changed 2 years ago by anonymous

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

I noticed that this might be a workaround to the ticket. Basically being smarter with your URL regexs:
http://c4urself.posterous.com/optional-arguments-with-django-url-patterns

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.