Erroneous handling of parameters in route matching process

[seanrmurphy]

Great work on vugu - I really like it!

I've been experimenting a little with the router to perform an authentication flow; the flow redirects to an authentication service and redirects back to an endpoint in the application with parameters in the URL as usual. A simple example of such a URL (which causes problems with the router) is http://localhost:8844/callback?code=1.

I have encountered problems with the handling of this in the route matching. More specifically, the router panics in the process2() function.

The issue seems to be that the match() function in mpath.go can return nil for the map of parameter values; but when the process2() function tries to add extra params to this map, an exception occurs here:

https://github.com/vugu/vgrouter/blob/master/router.go#L335

I'm not completely sure what the intended design is: on the one hand, it seems that the match() function should not need to deal with parameters as they seem to be separated elsewhere, but on the other hand it contains logic for handling parameters and provides return values for the parameters. For this reason, I'm not providing a PR. My (current) workaround is to have something like this:

    for _, re := range r.rlist {

        _, exact, ok := re.mpath.match(path)
        if !ok {
            continue
        }

        if !foundExact && exact {
            foundExact = true
            r.bindRouteMPath = re.mpath
        }

        //log.Printf("pvals = %v", pvals)
        pvals := make(url.Values, 2)
        // merge any other values from query into pvals
        for k, v := range query {
            log.Printf("k = %v, v = %v", k, v)
            if pvals[k] == nil {
                pvals[k] = v
            }
        }

        req := &RouteMatch{
            router:    r,
            Path:      path,
            RoutePath: re.mpath.String(),
            Params:    pvals,
            Exact:     exact,
            Request:   req,
        }

        re.rh.RouteHandle(req)

    }

[bradleypeabody]

Hi @seanrmurphy, thanks and yeah I see what you mean.

Parameters can either be in the path, e.g. "/some/path/:param" or in the query string. In order to simplify things these are treated the same by the router (i.e. a named parameter can appear in the path or in the query string and these are treated the same). The code you reference is handling the parsing on those two steps: first the path params and then the query params.

This seems like a bug and all that would be needed is a simple if pvals == nil { pvals = make(url.Values) } in the appropriate place in process2()

[seanrmurphy]

Thanks for this clarification @bradleypeabody. I will make the change you suggest locally and see if this works; if I can put together a test for it in the next few days, I will submit a PR if it helps.

[bradleypeabody]

@seanrmurphy Great! Yes, a PR is welcome.