Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

To everyone who's new here #2016

Closed
TaceyWong opened this issue Aug 15, 2019 · 37 comments · Fixed by #2663
Closed

To everyone who's new here #2016

TaceyWong opened this issue Aug 15, 2019 · 37 comments · Fixed by #2663
Labels

Comments

@TaceyWong
Copy link

TaceyWong commented Aug 15, 2019

To everyone who's new here:

If you use RESTful-Style API, router of gin will be a TROUBLEMAKER!

#205
#1681
#136
#2005

The root of the problem is httprouter

@wodim
Copy link

wodim commented Aug 20, 2019

I wrote my own "router". Example:

router.GET("/:first/:second/:third", ViewRouterThree)

Then in ViewRouterThree you access c.Param("first"), second, or third to figure out which view you want to call and call it directly. Example:

func ViewRouterTwo(c *gin.Context) {
	if c.Param("second") == "new" {
		viewNewThreadForm(c)
		return
	} else if c.Param("first") == "recent" {
		if c.Param("second") == "threads" {
			viewRecentThreads(c)
			return
		} else if c.Param("second") == "posts" {
			viewRecentPosts(c)
			return
...

Crappy but it works fine. Of course having a decent router would be nice.

@TaceyWong
Copy link
Author

TaceyWong commented Aug 27, 2019

In my point, because Gin is based on httprouter (in pursuit of speed, a special algorithm is used.), there is no elegant solution that can maintain both speed and decent routing.

@jgoc
Copy link

jgoc commented Sep 18, 2019

this is idiomatic, imo

/:second
/:second/:id
/:second/:id/:first
/:second/:id/:first/:thisshouldbethefirstinstead

reduces parsing

@LaysDragon
Copy link

LaysDragon commented Mar 25, 2020

yes,I encounter problem of conflict between path and wildcard ,and need to rewrite the openapi spec...It is annoying while gin have some limit with restful api style url path.
That really unexpected while I use this have-most-star web server framework in go world
And there is no any warning about those limitation while user expected a full function router. :(

@appleboy
Copy link
Member

related issue: julienschmidt/httprouter#73

@vishnukvmd
Copy link

@appleboy Are there plans to support alternatives to httprouter that do not have this limitation? Currently it is difficult to build REST-ful services using Gin.

@thinkerou thinkerou pinned this issue May 10, 2020
@lawrsp
Copy link

lawrsp commented May 16, 2020

yes, the conflicts issue is annoy

my solutions are:

/path/static: RouterA
/path/:id: RouterB
/path/:id/values: RouterC 

write like this:


"/path/:id":  { if get(id) == "static" { RouterA} else { RouterB } }
"/path/:id/some":  RouterC

 "/path/some/static": RouterA
 "path/:id/values": RouterB

change the route as:

 "/path-some/static": RouterA
 "/path/:id/values": RouterB

the first noe is not bad,

I did't find any good solution for the second one

I hope there would be a support of alternatives

@vietvudanh
Copy link

Well, I have never thought a simple REST could cause problems like this.
Of course I can rewrite my router, but what the hell?

wfjsw added a commit to wfjsw/go-cqhttp that referenced this issue Sep 20, 2020
@skipsizemore
Copy link

I would never have chosen Gin if I had known about this beforehand. Having to write my own router makes Gin worse than worthless. This needs to be configurable.

@ItalyPaleAle
Copy link
Contributor

I ended up here because I have a similar issue, which gave me the same error.

I took a quick look at the code and it looks like Gin is creating a tree in memory with all routes. Because of that, there can't be overlap.

A different approach that would solve both this issue and mine would be instead to keep all routes in a slice, and then iterate through them on each request. This way, the router would stop on the first match, and it would allow to have routes that partially overlap too.

For example:

router.GET("/:second/:id/:first/:thisshouldbethefirstinstead", handler)
router.GET("/:second/:id/:first", handler)
router.GET("/:second/:id", handler)
router.GET("/:second", handler)

If you navigate to /foo/bar, the router would first try to match it against /:second/:id/:first/:thisshouldbethefirstinstead, which will fail. It would then try /:second/:id/:first, and then finally find a match with /:second/:id. Because there's a match, it will stop processing the other requests. Of course, in this case order matters.

Now, I know that the current implementation, based on a tree, has a bit more performance, as it does less lookups on each route.

In practice, however, this won't matter much: most apps have a small number of routes, and the difference in time should be negligible. Additionally, because requests are matched in order, developers could put the most common routes at the top, thus making very few lookups for most requests.

There's significant prior art for doing routing this way:

@skipsizemore
Copy link

I wrote a simple router based on this principle that has the same routing interface as the Gin engine (GET, POST, etc.). My router creates a generic, catch-all handler for each method, while adding the "actual" routes as regular expressions to a slice. When a request is received, Gin calls the generic catch-all handler, which in turn cycles through the slice of regexps to find the "real" handler. The generic handler also handles adding route parameters to the Gin Context, where they can be read just as if the Gin router had gone directly to the handler.

It sucks to have to do this, but it does work.

@bluven
Copy link

bluven commented Oct 28, 2020

I would never use gin for this reason.

@tiagoacardoso
Copy link

This is a known issue and really need to be addressed, some kind of abstraction would be great.

@LaysDragon
Copy link

#1432
here is another problem if you trying to use google pattern you will encountered. Httprouter can't escaped colons which is valid url character.

@rustagram
Copy link

Guys please fix this! The library that has most stars should not have this kind of problems. My employers are considering firing me because of this bug, I really would appreciate you guys fixing this!

@dennypenta
Copy link

totally sucks, never would like to use it

@leogtzr
Copy link

leogtzr commented Dec 1, 2020

Just hit the same problem everybody is having ... time to look to a different framework.

@irisida
Copy link

irisida commented Dec 5, 2020

I ended up here because I have a similar issue, which gave me the same error.

I took a quick look at the code and it looks like Gin is creating a tree in memory with all routes. Because of that, there can't be overlap.

A different approach that would solve both this issue and mine would be instead to keep all routes in a slice, and then iterate through them on each request. This way, the router would stop on the first match, and it would allow to have routes that partially overlap too.

For example:

router.GET("/:second/:id/:first/:thisshouldbethefirstinstead", handler)
router.GET("/:second/:id/:first", handler)
router.GET("/:second/:id", handler)
router.GET("/:second", handler)

If you navigate to /foo/bar, the router would first try to match it against /:second/:id/:first/:thisshouldbethefirstinstead, which will fail. It would then try /:second/:id/:first, and then finally find a match with /:second/:id. Because there's a match, it will stop processing the other requests. Of course, in this case order matters.

Now, I know that the current implementation, based on a tree, has a bit more performance, as it does less lookups on each route.

In practice, however, this won't matter much: most apps have a small number of routes, and the difference in time should be negligible. Additionally, because requests are matched in order, developers could put the most common routes at the top, thus making very few lookups for most requests.

There's significant prior art for doing routing this way:

Is there any intention to make something like this an opt-in configuration possibility if the 'raw speed' aspect of the original source of the issue is not to be resolved?
I think it makes a strong case that for some notion of fully RESTful adherence as a perf trade-off that can be switched on is a way to make an inroad to the issue the users could live with.

@matheus-meneses
Copy link

Hi guys, any updates about this issue?

@ridwanakf
Copy link

Is this issue still persist now? just wondering. Still amazed that the most starred framework for its kind has this kinda problem.

@tiagoacardoso
Copy link

tiagoacardoso commented Dec 15, 2020 via email

@micfok
Copy link

micfok commented Dec 16, 2020

Would love to see this issue resolved, but if it's not possible, or not on the roadmap it'd be a good idea to disclose this routing limitation prominently in the README. This format of nesting is standard in RESTful APIs and I will need to consider ripping Gin out of the repo because of this.

@p3ym4n
Copy link

p3ym4n commented Dec 18, 2020

Been tracking this issue for a while, and it seems like the problem is not just changing the router, is the performance impact that can happen after that. which seems irrelevant when you cannot comply with the standards. ripping out the lovely Gin is much harder than fixing this issue, but the pain of not having a standard router is convincing me to choose the harder approach.

@ItalyPaleAle
Copy link
Contributor

I would argue that the performance impact is irrelevant regardless. Micro-optimizations in the router is likely not what will make or break your app's performance overall.

In the Node.Js space, Eran Hammer wrote this, which is definitely controversial but an interesting perspective: https://web.archive.org/web/20171017173141/https://medium.com/@eranhammer/when-500-faster-is-garbage-553121a088c3

@liov
Copy link

liov commented Feb 2, 2021

Once gave up gin because of this problem, and later chose gin again, now I am considering giving up again.

@vuonghungvinh
Copy link

vuonghungvinh commented Mar 3, 2021

Hi Guys, please suggest for me other libraries, first time learning Golang, but I see have problem with the router, this thing is not good.

@vietvudanh
Copy link

Hi Guys, please suggest for me other libraries, first time learning Golang, but I see have problem with the router, this thing is not good.

Try echo

@allesan
Copy link

allesan commented Mar 3, 2021

Or Fiber.

@agengdp
Copy link

agengdp commented Mar 21, 2021

yes, this issue still persist

@rw-access
Copy link
Contributor

rw-access commented Mar 21, 2021

@rustagram you still employed?
hope i can help save your job. it's the year of the stimulus package
julienschmidt/httprouter#329

@rw-access
Copy link
Contributor

rw-access commented Mar 26, 2021

Update: I haven't heard from @julienschmidt on that PR, but I ported it here as #2663 and it's 🟢 in CI. I'm not a maintainer here, and I hope the current ones welcome this functionality. 🤞

@mqzabin
Copy link

mqzabin commented Mar 26, 2021

I discovered this issue right now, after porting 2k+ lines to gin. Can't say how lucky i am to see @rw-access PR seconds before considering moving out from this framework. Hope PR get merged.

@mqzabin
Copy link

mqzabin commented Mar 28, 2021

People, where did you migrate from gin? To which framework?

I was in the middle of rewriting from .net core, when I found this issue. .net has an excellent routing, also like laravel, django, rails.

Any suggestion for the alternative with proper routing (also, static assets embedding) ?
Echo, fiber, chi?
I've slightly benchmarked all top, echo is very stable under the massive load (the same as gin), fiber crashed.

I would migrate to Echo.
But, to be honest, if you follow REST API routing good practices, you will rarely fall on any issue listed here.
My app have 250+ endpoints and only 2 ran into these wildcard issues, so I just refactored them.

@matheus-meneses
Copy link

Finally. Is there a date for the next release?

@appleboy
Copy link
Member

appleboy commented Apr 6, 2021

@matheus-meneses We will release the new version v1.7.0 this week

@matheus-meneses
Copy link

@appleboy thanks

@appleboy
Copy link
Member

appleboy commented Apr 8, 2021

bump to v1.7.0 version. See https://github.com/gin-gonic/gin/releases/tag/v1.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet