Route params mixup

[CaptainCodeman]

First of all, love the project & the philosophy behind it - it's the perfect approach for Go 1.7+ IMO 👍

But I'm having an issue with some supporting some legacy route formats. Requests map to the correct handlers but the parameters don't match up correctly. Here's a simplified example replacing the routes in TestTree to show the issue I'm having:

func TestTree(t *testing.T) {
    hDate := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
    hCat := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})

    tr := &node{}

    tr.InsertRoute(mGET, "/items/:year/:month", hDate)
    tr.InsertRoute(mGET, "/items/:category", hCat)

    tests := []struct {
        r string            // input request path
        h http.Handler      // output matched handler
        p map[string]string // output params
    }{
        {r: "/items/2016/08", h: hDate, p: map[string]string{"year": "2016", "month": "08"}},
        {r: "/items/things", h: hCat, p: map[string]string{"category": "things"}},
    }

That results in:

--- FAIL: TestTree (0.00s)
    tree_test.go:59: input [1]: find '/items/things' expecting params:map[category:things] , got:map[year:things]

Note that the /items/:category handler is correctly used but the parameter is called :year instead from the previous route.

It looks like it's due to how the tree is built with the name of the parameter ignored so it then gets the name from the previous node:

2016/08/23 10:40:56 [node 0 parent:0] typ:0 prefix: label: numEdges:1 isLeaf:false
2016/08/23 10:40:56 [node 1 parent:0] typ:0 prefix:/items/ label:/ numEdges:1 isLeaf:false
2016/08/23 10:40:56 [node 2 parent:1] typ:2 prefix::year label:: numEdges:1 isLeaf:true handler:map[512:<nil> 4:0x90450]
2016/08/23 10:40:56 [node 3 parent:2] typ:0 prefix:/ label:/ numEdges:1 isLeaf:false
2016/08/23 10:40:56 [node 4 parent:3] typ:2 prefix::month label:: numEdges:0 isLeaf:true handler:map[512:<nil> 4:0x90440]

Is this simply not supported (I know many Go routers are strict about the route variations, others such as Echo cope better with suffix variations) or is it a bug?

Many thanks.

[pkieltyka]

@CaptainCodeman thanks man! this is definitely a bug and I know why it's happening. Thanks for the report, I'll fix it soon.

[CaptainCodeman]

Brilliant, thanks !

[pkieltyka]

hey @CaptainCodeman - I was looking to solve this problem when I realized its logically difficult. Did you say other routers can support this?

The reason I ask is, I can make the particular case you're describing work..

/items/:category => categoryHandler (urlParams: category)
/items/:year/:month => yearMonthHandler (urlParams: year, month)

but, imagine the routes now look like

/items/:category => categoryHandler
/items/:year/:month => yearMonthHandler
/items/:x/:y => xyHandler

.. for a request to /items/2016/09 would we expect to reach the yearMonthHandler or the xyHandler? the answer is, the /items/:x/:y route will overwrite the route and handler to xyHandler where the last one takes precedence.

The best we could, from someone definging those three routes is ending up with..

/items/:category => categoryHandler (urlParams: category)
/items/:x/:y => xyHandler (urlParams: x, y)

let me know what you think. but I'm still not convinced which is better / less confusing

[CaptainCodeman]

Yeah, I totally get how things can quickly drift into being ambiguous and unclear. The httprouter and many of it's variants don't handle the suffix differences but Echo especially does a great job of getting the matching right (so can support routes that Gin, for instance, can't). I think if the routes are truly ambiguous as your yearMonth/xy example then all bets are off (without some additional constraint such as regex matches for a param).

The main use case I have actually has a static suffix (like /thumb.jpg) which disambiguates the route. I guess that could boil down to having something like:

/:name/user
/:id/order

That seems to match the type of thing you say could be supported which would be great if that's possible. Like I said, the route did match correctly in terms of getting to the right handler, it just didn't have the right param names set.

So yes, if that last example you give could be supported, that would be ideal.

Thanks again for looking into this!

[pkieltyka]

can you show me some sample code of your main use case like you mentioned with the suffix..? I don't fully understand that part yet.

also, any chance you can show me a gist of how echo works for these cases too..? I'm just on the road for the rest of the week but I'd like to come to the best possible solution

[jhngrant]

I appreciate there may be strong opposition to this, but +1 for regular expression matching. I'm finding this essential when migrating legacy code.

[pkieltyka]

@jhngrant yea, I don't see that getting implemented soon by me, I'm quite busy with work on our actual platform, but hopefully someone submits a PR for that one day. I have some stubbed in code for regexp matching in the tree already

[jhngrant]

Good to hear that Chi does not rule out regexp - I wasn't aware. I'll endeavor to make a contribution.

[pkieltyka]

@jhngrant :) https://github.com/pressly/chi/blob/master/tree.go#L47 and https://github.com/pressly/chi/blob/master/tree.go#L326-L327

for url param syntax, I was thinking either /:id([0-9]+) or #id^[0-9]+$ or /#id([0-9]+) .. I think the first or last one is best. example would be ... /users/#id([0-9]+) and /users/#username([a-z]+)

[pkieltyka]

if you get any success tho, perhaps open a new issue where we can discuss it

[CaptainCodeman]

@pkieltyka Sure, here's an example. I have some images that are stored in either AWS or GCS. The files in AWS are in a year / month structure and the ones in GCS are just based solely on the id. The routes using Echo (e being the echo router) would be:

e.Get("/:year/:month/:id/thumb.jpg", awsThumbnailHandler)
e.Get("/:id/thumb.jpg", gcsThumbnailHandler)

Both handlers are correctly called for the different URLs but the gcsThumbnailHandler currently gets a year parameter instead of the id which is confusing.

The suffix also comes into play with other handlers for things like ../preview.jpg URLs, tiles etc...

Maybe I should add a PR with some failing tests? I know, also including the code that then fixes them would be better ...

[pkieltyka]

well, for you to just get by, I recommend just keep the same name for the url param and keep going. like /:year_id/:month/:id/thumb.jpg and /:year_id/thumb.jpg`.

no need for test case, I have it already, Im just trying to still figure out if its worth supporting the edge case. I'll try out echo to see how they'd work with the /:x/:y case we talked about above

[CaptainCodeman]

yeah, that's what I'm doing ... just figured it may save someone the same initial confusion when just one route param didn't seem to work (esp. as it could be adding another route, someplace else that breaks it).

[pkieltyka]

Awesome

On Aug 25, 2016, at 9:50 AM, Simon Green notifications@github.com wrote:

yeah, that's what I'm doing ... just figured it may save someone the same initial confusion when just one route param didn't seem to work (esp. as it could be adding another route, someplace else that breaks it).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.