jameshfisher commented 7 years ago

Steps to reproduce

Use version 4563b0b of httprouter
Compile the following program

package main

import (
    "github.com/julienschmidt/httprouter"
    "net/http"
)

func main() {
    router := httprouter.New()
    router.GET("/user/:user", nil)
    router.GET("/user/gordon/:profile", nil)
    http.ListenAndServe(":8080", router)
}

Run it

Expected behavior

Process starts web server. Requests with a two-segment path, if beginning with /user/, go to the first handler. Requests with a three-segment path, if beginning with /user/gordon/, go to the second handler. All other requests error.

Actual behavior

Process panics:

panic: 'gordon' in new path '/user/gordon/:profile' conflicts with existing wildcard ':user' in existing prefix '/user/:user'

goroutine 1 [running]:
panic(0x20c240, 0xc42000d200)
    /usr/local/Cellar/go/1.7.3/libexec/src/runtime/panic.go:500 +0x1a1
github.com/pusher/httproutertest/vendor/github.com/julienschmidt/httprouter.(*node).addRoute(0xc420012730, 0x2676c4, 0x15, 0x0)
    /Users/jhf/go/src/github.com/pusher/httproutertest/vendor/github.com/julienschmidt/httprouter/tree.go:159 +0x73e
github.com/pusher/httproutertest/vendor/github.com/julienschmidt/httprouter.(*Router).Handle(0xc420010500, 0x263045, 0x3, 0x2676c4, 0x15, 0x0)
    /Users/jhf/go/src/github.com/pusher/httproutertest/vendor/github.com/julienschmidt/httprouter/router.go:236 +0xcc
github.com/pusher/httproutertest/vendor/github.com/julienschmidt/httprouter.(*Router).GET(0xc420010500, 0x2676c4, 0x15, 0x0)
    /Users/jhf/go/src/github.com/pusher/httproutertest/vendor/github.com/julienschmidt/httprouter/router.go:180 +0x5e
main.main()
    /Users/jhf/go/src/github.com/pusher/httproutertest/main.go:11 +0xba

Thomasdezeeuw commented 7 years ago

This is because :user in the first route is a wild card, which match gordon/:profile of the second route as well. See #175.

jameshfisher commented 7 years ago

@Thomasdezeeuw @julienschmidt thanks. :user certainly shouldn't match gordon/:profile, because "Named parameters only match a single path segment". So I think this should be considered a bug (or a documentation bug).

From #175, I see this is because httprouter uses a prefix tree. I think there's a simple algorithmic fix: first branch based on the length in number of path segments, before branching on the individual segments. So the prefix tree in my example should look like this:

O --+--2--> O --"user"--> O --any--> O
    |
    +--3--> O --"user"--> O --"gordon"--> O --any--> O

liminalitythree commented 6 years ago

Has any progress been made on this issue? This is a very common pattern in websites, even the page you're probably on right now.

github.com/julienschmidt/ and github.com/julienschmidt/httprouter/

would something simple like this not be possible with httprouter?

if this is about the "Only explicit matches" thing, I think this is still explicit.

one route is /* and the other is /*/*. I don't think the wildcard from the first route should match the second route, because there's another path/subdirectory there.

yaron2 commented 5 years ago

Hi, interested in knowing whether this is going to be addressed soon?

julienschmidt commented 5 years ago

For now the following should work. If necessary, one would have to check that :user is gordon. Relaxed routing, where static segments have preference over dynamic segements (wildcards), is planned for v2.

package main

import (
    "net/http"

    "github.com/julienschmidt/httprouter"
)

func main() {
    router := httprouter.New()
    router.GET("/user/:user", handleUser)
    router.GET("/user/:user/:profile", handleUserProfile)
    http.ListenAndServe(":8080", router)
}

See also #73

arianitu commented 4 years ago

PLEASE fix this so users of frameworks like Gin do not have to rip out the entire framework. That's what we are about to do. We are about to create our own package called http that has a sane router and some basic other things we need like middleware.go and binding.go and be done with this nonsense.

Honestly, is it just me or is it confusing how Gin has 32k+ stars and this has 10k+ stars when you can't even make usable routes? I mean sure, you can claim it's fast, but it comes at a cost that most people don't know about until they're in deep since there was no notice and no indication that this is a limitation. I know I am complaining on this library instead of gin, but the gin guys haven't fixed it in 4 years and if this library could have some flag like httprouter.SaneRoutes(true) it would be awesome.

By the way, there should be a massive notice at the top of both Gin and this library that talks about this. It should be:

NOTICE: You cannot make a typical REST API because you WILL run into wildcard conflicts. Use at your OWN risk

AGAIN the issue goes back 4 YEARS and why is no one doing anything about it?

SamWhited commented 4 years ago

I would like to disagree with the assertion that creating ambiguous routes this way is "sane". If you want ambiguous routes or need them to meet legacy requirements, there are plenty of other routers you can use. However, for new products where you get to design the routes, I encourage you to just work within the constraints. I can't tell you the number of times I've seen a new web service broken because someone didn't vet their routes properly and someone created a user called "new" that broke the page "/new" or something along those lines. Or, in this case, someone wants to add a /user/:user/:profile route later and can't because no one had thought that far ahead.

While this may be an acceptable risk for you, or you may have taken great care to avoid these sorts of issues and trust that it won't happen in the future, I find it much safer and easier to just use a router that doesn't allow this sort of ambiguity. It influences how I design my routes and means I end up with a better product in the future with fewer chances for ambiguity. In fact, for me personally, one of my requirements when selecting a new router is that it doesn't let me mix variable route parameters and static routes.

ImVexed commented 4 years ago

I would like to disagree with the assertion that mixing static segments and wildcard segments is somehow legacy or "risky". And that I should "work within the constraints" that are arbitrary to this package and not standard across the industry.

Examples of static and wildcard segment mixing: https://discord.com/developers/docs/resources/user#get-current-user

https://dev.twitch.tv/docs/v5/reference/videos#get-video + https://dev.twitch.tv/docs/v5/reference/videos#get-top-videos

https://www.reddit.com/dev/api/#GET_api_live_happening_now

There are plenty more examples, but I feel that this sort of functionality is not at all ambiguous and the onus is not on the package to withhold features because it's possible to misuse them. Even if you don't trust yourself to be disciplined enough not to misuse the tools given to you, doesn't mean they aren't useful for others.