Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#9038 closed (fixed)

url tag not working with reverse in 1.0 as it previously did

Reported by: marystern@… Owned by: mtredinnick
Component: Core (Other) Version: 1.0
Severity: Keywords: url, reverse
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I was getting a TemplateSyntaxError caused by a NoReverseMatch error with DJango 1.0 for code which previously worked.

Exception Type:  	TemplateSyntaxError
Exception Value: 	

Caught an exception while rendering: Reverse for 'x.x_edit' with arguments '(7,)' and keyword arguments '{}' not found.
Original Traceback (most recent call last):
  File "/usr/local/lib/python2.5/site-packages/django/template/debug.py", line 71, in render_node
    result = node.render(context)
  File "/usr/local/lib/python2.5/site-packages/django/template/defaulttags.py", line 378, in render
    args=args, kwargs=kwargs)
  File "/usr/local/lib/python2.5/site-packages/django/core/urlresolvers.py", line 252, in reverse
    *args, **kwargs)))
  File "/usr/local/lib/python2.5/site-packages/django/core/urlresolvers.py", line 241, in reverse
    "arguments '%s' not found." % (lookup_view, args, kwargs))
NoReverseMatch: Reverse for 'x.x_edit' with arguments '(7,)' and keyword arguments '{}' not found.

Exception Location: 	/usr/local/lib/python2.5/site-packages/django/template/debug.py in render_node, line 81

I eventually tracked it down to this:

I was using a url template tag to match the name 'edit' (see below). The pattern was the same as a preceding pattern which matched the same prefix with an optional number after it. This first url pattern had no name (I'd originally found I needed the 2 patterns because the first pattern had 2 slashes at the end and didn't match the one slash I wanted).

    url(r'^x/edit/(?P<x_id>[0-9]*)/$', x_edit,),
    url(r'^x/edit/$',                  x_edit, name='x_edit'),

The snippet of template code was:

<form action="{% url x_edit x.id %}/" method="post" name="my_form"> 

This used to work (presumably by failing the named match and somehow matching the un-named one?).

However, in 1.0 it stopped working (I guess because the url resolver has been re-worked).

I found (after much debugging), that I could fix the problem by adding another name for the 1st pattern, eg

    url(r'^x/edit/(?P<x_id>[0-9]*)/$', x_edit, name='x_edit_id'),
    url(r'^x/edit/$',                  x_edit, name='x_edit'),

and changing my template to use it:

 <form action="{% url x_edit_id x.id %}/" method="post" name="my_form"> 

This took quite a while for me to find, and I'd like peole to not have to do the same as I have in migrating from older code to 1.0!

Can you maybe confirm the above and document this somewhere sensible for people who are migrating from 0.96+ to 1.0 (I think this was probably a backwards-incompatible change)?

Thanks!
Mary.

Attachments (1)

9038-r9084.diff (6.0 KB) - added by russellm 7 years ago.
Fix for URL reversing with tests

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to mtredinnick
  • Patch needs improvement unset
  • Status changed from new to assigned

This looks like a bug, not changed behaviour. If you're passing in a parameter, it must match the first pattern, since that's the only one that matches anything with a parameter. The fact that it isn't doing so is wrong. One of the points behind this rewrite is that similarly named but pattern-wise distinct URL patterns that are differentiated by the parameters they take should be handled correctly.

comment:2 Changed 7 years ago by wreese

We used the following code to work around this bug. The code removes items from args and kwargs where the value is empty or None, then calls the real reverse function with the correct args and kwargs.

from django.core.urlresolvers import reverse as django_reverse

def reverse(*args,**kw):
  if 'args' in kw: kw['args'] = tuple((a for a in kw['args'] if a))
  if 'kwargs' in kw: kw['kwargs'] = dict(((k,v) for k,v in kw['kwargs'].items() if v))
  return django_reverse(*args,**kw)

This works fine in the views, but it does not fix the "url" template tags which use Django's reverse function. You could update the "URLNode" class in django/template/defaulttags.py, so the "render" method redefines "reverse" using the code above.

-- Will Reese

Changed 7 years ago by russellm

Fix for URL reversing with tests

comment:3 Changed 7 years ago by russellm

Malcolm: I haven't checked this in because (1) The URL resolver is one of your regular stomping grounds, (2) I wasn't certain if you had a better way of solving this in mind.

My read on the problem - If you deploy the same view method at two different URLs, calling reverse() using the view method name becomes ambiguous. Using a MultiValueDict rather than a dict means you can find all the places that a given view is deployed.

comment:4 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

This looks like the right fix, or, at least, fixing the right problem. The implementation makes me a little nervous because it's essentially introducing an extra level of nesting in the data structure (which is shown up by the extra outer loop when working out the match). I've basically gotten into trouble in the current code by storing the pattern prefix only once for a whole bunch of possible argument combination (the new_matches list in _get_reverse_dict() is the collection of possibilities for the current pattern prefix. So Russell's patch adds extra "groups" of pattern + possibility sets, which looks fine. I want to think about whether there are alternative data structures to use there. MultiValueDict feels shady to me somehow, since I always have to think about how it works. It's not really a natural Python data structure. Maybe lists of lists or something explicit might look more comfortable.

I'll try out alternatives, but it will only be variations on this. Thanks for working this out, Russ. I missed some possibilities in the original code.

comment:5 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [9087]) Fixed #9038 -- Correctly handle URL patterns with the same name (or view name),
declared independently and that differ only by argument signatures.

Patch from Russell Keith-Magee.

comment:6 Changed 7 years ago by abhi

  • Resolution fixed deleted
  • Status changed from closed to reopened

I still face the exact same issue with build 9038. Any suggestions?

comment:7 Changed 7 years ago by ramiro

What do you mean with 'build 9038'?. If yo meant revision 9038 then that's where your problem lies, you need to be running revision 9087 or newer to see the fix that closed this ticket.

comment:8 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

The issue in this ticket has been fixed. If anybody is seeing a different problem, please open a new ticket with a repeatable test case so that we can debug that problem.

comment:9 Changed 7 years ago by abhi

Sorry.. I wanted to mean revision 9087. Even with revision 9087 I see the same exception:

TemplateSyntaxError at /customize/themeeditor/themeoftheday/

Caught an exception while rendering: Reverse for 'navburner.theme_editor' with arguments '()' and keyword arguments '{}' not found.

Original Traceback (most recent call last):

File "C:\workspaces\navburner-02\django-latest\django\template\debug.py", line 71, in render_node

result = node.render(context)

File "C:\workspaces\navburner-02\django-latest\django\template\defaulttags.py", line 378, in render

args=args, kwargs=kwargs)

File "C:\workspaces\navburner-02\django-latest\django\core\urlresolvers.py", line 254, in reverse

*args, kwargs)))

File "C:\workspaces\navburner-02\django-latest\django\core\urlresolvers.py", line 243, in reverse

"arguments '%s' not found." % (lookup_view, args, kwargs))

NoReverseMatch: Reverse for 'navburner.theme_editor' with arguments '()' and keyword arguments '{}' not found.

comment:10 Changed 7 years ago by kmtracey

You are not seeing exactly the same problem, since your exception involves no arguments and the initial report had an argument. Also you haven't provided enough information to help figure out what is going wrong, since the exception alone without some information on your url patterns is not very helpful. Finally, this is not the right place to figure out what is going wrong -- as Malcolm said the error in this ticket, which was very specific and not something that covers all "NoReverseMatch" errors -- has been fixed. The best way to figure out what is going on in your case is probably to post on django-users with enough information about your url patters and what you are doing to generate the exception to hopefully identify if the problem is in your setup or the Django code. If Django code, then a new ticket would be appropriate.

Note: See TracTickets for help on using tickets.
Back to Top