Code

Opened 7 years ago

Closed 7 years ago

#4453 closed (fixed)

url pattern name can contain dots, but reverse('pattern.name.with.dots') will always fail

Reported by: forest@… Owned by: jacob
Component: Uncategorized Version: master
Severity: Keywords: url reverse dot
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

It seems somewhat natural to include dots in URL pattern names in an effort to avoid name clashes. However, the reverse resolver will always fail to find the correct URL pattern if that is done.

Attachments (1)

reverse_helper.txt (4.8 KB) - added by Todd O'Bryan <toddobryan@…> 7 years ago.
patch against django.core.urlresolvers

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by anonymous

  • Keywords url reverse dot added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by SmileyChris

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

I'm not sure that there's a real problem here. It's not that "natural" to include dots in pattern names when you know dots are used to find out modules. My suggestion: use dashes or underscores. Or even spaces.

comment:3 Changed 7 years ago by mtredinnick

  • Resolution wontfix deleted
  • Status changed from closed to reopened

It's a bug.

comment:4 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Accepted

I'm still a bit confused, but Malcolm's smarter than me. Accepted it is.

comment:5 Changed 7 years ago by Forest Bond <forest@…>

Well, regardless of whether dots should be accepted in view names, it's a bug to allow them and then not deal with them effectively in reverse. If django doesn't want dots there, it would be nice to see an exception when the URLs are read.

Personally, I see no reason dots shouldn't be allowed. I looked at the code that's choking on them, and I distinctly remember thinking that the breakage is due to a pretty straight-forward shortcoming that one often sees in first-iteration code.

comment:6 Changed 7 years ago by Todd O'Bryan <toddobryan@…>

  • Summary changed from url pattern name can contain dots, but reverse('pattern.name.with.dots') will always fail to django.core.url_resolvers.reverse_url doesn't reverse escaped characters

It's actually not just dots, but any backslash-escaped character in a url. For example,

urlpatterns = patterns('',
    (r'^prices/less_than_\$(?P<price>\d+)/$', 'cost_less'),                   
    (r'^headlines/(?P<year>/d+)\.(?P<month>\d+)\.(?P<day>\d+)/$', 'daily_headlines'),
    (r'^priests/(?P<name>\w+)\+/$', 'priest_homepage'),
    (r'^windows_path/(?P<drive_name>[A-Z]):\\\\(?P<path>.+)', 'windows_path'),
)

The dollar sign, dot, plus, and backslash in each of the URL patterns match a single character, but don't get converted back to that character by the reverse function.

It seems that there aren't that many of these. Any escape sequence that doesn't match a constant string (i.e. something like \s or \d or \w) had better be part of a pattern so that it can be replaced with the right string to get the URL you're expecting. That leaves the following, I think.

Pattern Replacement
\A '' (equivalent to ^)
\Z '' (equivalent to $)
\b and \B '' (these shouldn't appear in urls, but can only match the empty string)
\., \^, \$, \*, \+, \?, \(, \), \{, \}, \[, \], and \\ the same character, without a backslash

As a first stab, I'd just get rid of \A, \Z, \b, and \B, just as the current code does for ^ and $. This is actually kind of complicated, because you have to make sure that the \ in front isn't part of a pair of backslashes. In other words, \\b should become \b, but \\\b should just become \. Also, the current code removes all ^ and $. That's wrong if they're preceded by a backslash and meant to be the actual character.

There are some gotchas--when you insert values, you have to escape characters that you'll be unescaping later. I do check for character classes that don't map to a single definite character (e.g., \d and \w) and raise an exception if they're still there when we finish (since the reverse lookup can't work). I don't check for things like [a-z] or a{2,3}, but that will almost guarantee the reversing fails, too.

Changed 7 years ago by Todd O'Bryan <toddobryan@…>

patch against django.core.urlresolvers

comment:7 Changed 7 years ago by Todd O'Bryan <toddobryan@…>

  • Component changed from Uncategorized to Template system
  • Has patch set

comment:8 Changed 7 years ago by SmileyChris

Ooh, I get it now. I was thinking of named URL patterns, not the pattern itself.

My patch in #2977 would also address this.

comment:9 Changed 7 years ago by Forest Bond <forest@…>

  • Component changed from Template system to Uncategorized
  • Summary changed from django.core.url_resolvers.reverse_url doesn't reverse escaped characters to url pattern name can contain dots, but reverse('pattern.name.with.dots') will always fail

No, no, no! I really did mean dots in pattern names:

urlpatterns = patterns('',
  (r'^xyz/$', 'some.name.with.dots'),                   
)

Todd, you've obviously spent a fair amount of time characterizing your bug, however, please open a new ticket, since it is a distinct issue, and not the originally reported one. I apologize if my original report was not clear enough.

comment:10 Changed 7 years ago by Forest Bond <forest@…>

  • Has patch unset

* Please ignore patch attached to this ticket, as it fixes the wrong bug. *

comment:11 Changed 7 years ago by Todd O'Bryan <toddobryan@…>

OK...now I don't understand what you're talking about...

What you seem to be calling a pattern name is actually the path to the view, and it works fine with dots in it, as far as I can tell. In fact, I have lots of {% url blah.blah2.blah3 %} tags in my templates and they seem to do the right thing.

Can you provide a test case that does what you're talking about?

comment:12 Changed 7 years ago by Todd O'Bryan <toddobryan@…>

Aha!!! Your example is the path to the view function--I just didn't know about the new names functionality. You're having trouble with this

urlpatterns = patterns('',
  url(r'^xyz/$', 'path.to.view', name='some.name.with.dots'),                   
)

It doesn't seem like that would be too hard to fix. Since the URL namespace is global, you'd just have to check the name dictionary before you try checking view paths. I'll see if I can figure that out.

Also, I will happily open a new ticket for my problem and apologize for grabbing your issue because I was misunderstanding.

comment:13 follow-up: Changed 7 years ago by Forest Bond <forest@…>

Ah, crud. Sorry, i mis-typed that example in a rush. Your example correctly illustrates the bug.

Misunderstanding understandable. No hard feelings on this end, and thanks sticking around :)

comment:14 in reply to: ↑ 13 Changed 7 years ago by Forest Bond <forest@…>

Replying to Forest Bond <forest@alittletooquiet.net>:

No hard feelings on this end, and thanks sticking around :)

Thanks *for* sticking around, that is.

comment:15 Changed 7 years ago by mtredinnick

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

(In [5530]) Fixed #4453 -- Allow dots in URL pattern names (although the string in that case is first tried as an import path and only then falls back to being treated as a pattern).

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.