URL variables bug for multiple patterns with relatively complex regexp about mux HOT 18 CLOSED

Actually, this will pass:

{
    title:       "Path route with multiple patterns with pipe, match",
    route:       new(Route).Path("/{category:a|b/c}/{product}/{id:[0-9]+}"),
    request:     newRequest("GET", "http://localhost/a/product_name/1"),
    vars:        map[string]string{"category": "a", "product": "product_name", "id": "1"},
    host:        "",
    path:        "/a/product_name/1",
    shouldMatch: true,
},

We could probably close this.

from mux.

Any news regarding this?

from mux.

In fact, the issue comes from the parentheses and the backreference generated.
In the example I gave, they are useless (a|b/c is equivalent to a|(b/c)) but for more complex URL variable regexp with parentheses, it might be necessary to have them.

The generated backreferences mess with the variables (hence the weird order we see with failing tests). To avoid that, you must use (?:) to set a group without backreference.

If I update the regexp of previously failing tests, it works :

{
    title:       "Path route with multiple patterns with pipe, match",
    route:       new(Route).Path("/{category:a|(?:b/c)}/{product}/{id:[0-9]+}"),
    request:     newRequest("GET", "http://localhost/a/product_name/1"),
    vars:        map[string]string{"category": "a", "product": "product_name", "id": "1"},
    host:        "",
    path:        "/a/product_name/1",
    shouldMatch: true,
},
{
    title:       "Path route with multiple patterns with pipe, match",
    route:       new(Route).Path("/{category:a|(?:b/c)}/{product}/{id:[0-9]+}"),
    request:     newRequest("GET", "http://localhost/b/c/product_name/1"),
    vars:        map[string]string{"category": "b/c", "product": "product_name", "id": "1"},
    host:        "",
    path:        "/b/c/product_name/1",
    shouldMatch: true,
},

In the end, it was just simple regexp logic.
@kisielk, do you think a warning should be added about that in the documentation ?

from mux.

I don't really think a warning would help since it's a pretty obscure case that would require a fair bit of effort on the developer's part to debug.

I'd rather see us redesign the route matching algorithm so that this kind of thing wasn't a problem, I think the current method is a bit brittle and suboptimal.

from mux.

Just ran into this.

With the following

router.HandleFunc("/{assetType:(js|css|fonts)}/{filepath:.*}", serveAssets)

and url

http://127.0.0.1/css/foo

vars := mux.Vars(r)
fmt.Println(vars)

would produce

map[assetType:css filepath:css]

It would be nice if this bug was fixed.

from mux.

Bump! Please reopen this issue... also ran into this issue that joegrasse describes very well above.

from mux.

How do you propose to fix it?

If I added a note "backreferences should not be used in regexp patterns" would that have been helpful in avoiding the problem?

from mux.

Yes, communication in documentation would help for those that RTFM, however not fool proof :) But in general, the path of least surprise is the correct choice (and preferably not at runtime)... That said, I haven't looked that closely as to how/where/why it is happening... Any pointers on which area of code (assuming around regexp.go:166 or where varsR is filled) you believe to be the offending lines? ... and I'm happy to take a look...

from mux.

Finally got a free moment to look... the code appears to rewrite the regex in question with parenthesis... My reduced repro was this subroute:

sub.Path("/not/{name:(.+)}/{profile:.+}").Methods("GET").HandlerFunc(profile)

Added some printlines and found that regexp.go had this:

fmt.Fprintf(pattern, "%s(%s)", regexp.QuoteMeta(raw), patt)

Causing the regex of (.+) to be ((.+)), meaning this has two groups in it... Not exactly clear why but current hypothesis is that there is an iteration later on w/ two group matches for the {name} template portion of the path and forward poisons the {profile} template...

This is admittedly a hack and wouldn't work for xdmnl's example above of a|(bc), but...

        // Build the regexp pattern.
        // don't create another nested regex group if already supplied
        if strings.HasPrefix(patt, "(") && strings.HasSuffix(patt, ")") {
            fmt.Fprintf(pattern, "%s%s", regexp.QuoteMeta(raw), patt)
            fmt.Println(fmt.Sprintf("%s%s", regexp.QuoteMeta(raw), patt))
        } else {
            fmt.Fprintf(pattern, "%s(%s)", regexp.QuoteMeta(raw), patt)
            fmt.Println(fmt.Sprintf("%s(%s)", regexp.QuoteMeta(raw), patt))
        }

Thoughts ?

from mux.

There's still a lot of ways that could break. The addition of braces around the variables is required so that they can be pulled out in to mux.Vars. You're right that having nested braces causes the same match to appear in two subsequent indices of the submatch result. mux isn't expecting this to happen when unpacking, so it uses the same pattern match twice.

Maybe the solution would be to use SubexpNames and only extract vars with that? All the route vars should be named so any unnamed results could be ignored.

from mux.

Okay, I believe the patch linked in #117 will fix the problem with subexpression. Before merging I'd like people to try it out with some of the patterns they've had problems with and let me know if it solves the issue.

from mux.

So I pulled the subexp-fix branch however it still fails for the following paths and corresponding curl...Looks to still be forward poisoning the next template because regex group inn group...

    sub.Path("/not/{name:(.+)}/{profile:.+}").Methods("GET").HandlerFunc(profile)
    sub.Path("/again/{name:bl(ah|at)}/{profile:.+}").Methods("GET").HandlerFunc(profile)
...
...
    w.Write([]byte(fmt.Sprintf("\n\nHello %s with profile %s params: %v\n", name, profile, params)))

curl -v "http://127.0.0.1:3000/v3/again/blah/up"
Hello blah with profile ah params: map[name:blah profile:ah]

and

curl -v "http://127.0.0.1:3000/v3/not/blah/up"
Hello blah with profile blah params: map[name:blah profile:blah]

from mux.

Are you sure you ran it with the correct version? I tried adding the following test, and it worked correctly:

        {
            title:       "subexp",
            route:       new(Route).Path("/again/{name:bl(ah|at)}/{profile:.+}").Methods("GET"),
            request:     newRequest("GET", "http://localhost/again/blah/up"),
            vars:        map[string]string{"name": "blah", "profile": "up"},
            host:        "",
            path:        "/again/blah/up",
            shouldMatch: true,
        },

Otherwise can you provide a full runnable example so I can test it?

from mux.

oh hrm, yea... let me double check...

from mux.

aha, you are correct... my bad on that.. it is legit for above use cases...

from mux.

@joegrasse any chance you can try it for your use cases?

from mux.

Works for my case as well.

from mux.

Great. Hopefully that works for everyone :)

from mux.