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

Could gin use a more advanced router ? #1730

Closed
fungiboletus opened this issue Jan 2, 2019 · 13 comments · Fixed by #2663
Closed

Could gin use a more advanced router ? #1730

fungiboletus opened this issue Jan 2, 2019 · 13 comments · Fixed by #2663

Comments

@fungiboletus
Copy link

Hei,

This issue is related to #205 #388 #1065 #1162 #1301.

Gin uses httprouter, which is not very helpful when using wildcards and a few routes. The related issue julienschmidt/httprouter#73 has been closed 4 years ago. It's by design and the suggestions are to do manual routing by hand, which I think is not acceptable for a cool framework.

Only explicit matches: With other routers, like http.ServeMux, a requested URL path could match multiple patterns. Therefore they have some awkward pattern priority rules, like longest match or first registered, first matched. By design of this router, a request can only match exactly one or no route. As a result, there are also no unintended matches, which makes it great for SEO and improves the user experience.

Therefore, gin could perhaps switch to a more advanced router. "First registered" works well with Express for example.

@davexpro
Copy link

Seconded, I believe that people can accept the flexibility with some lost.

@b1scuit
Copy link

b1scuit commented Apr 3, 2019

One of the issues is when using middleware such as:

router := gin.New()
router.GET("/login", LoginHandler)
authorised := router.Group("/")
authorised.Use(AuthRequired())
authorised.GET(":id", GetUserHandler)

Intentionally this should:

GET /login < Allow the user to login without the AuthRequired middleware being triggered
GET /111111111111 < Get a users details using the AuthRequired middleware

However this will complain with
panic: wildcard route ':id' conflicts with existing children in path '/:id'
Even though this should be a valid route and is fine with other routers, if it were possible to add regex to filter between them or prioritise them in some way, this would make the routing much easier to use in these situations.

First registered seems to be a pretty solid way to go and would fix this routing example.

@naiba
Copy link

naiba commented May 5, 2019

Same problem in my project Solitudes

  • /admin Admin dashboard router
  • /article1 Article router

route conflict

@pintohutch
Copy link

pintohutch commented May 10, 2019

First registered seems reasonable. The echo folks have a fixed ordering which IMO isn't bad either:

  • static
  • parameterized
  • match any

@arianitu
Copy link

This issue is big enough that I would never recommend gin for any serious project. The fact that you have to rename your routes because of this conflict is insanely annoying and the team constantly has to create weird routes to make everything work.

How useful is this route optimization really? Typical REST frameworks are going to hit database limits before httprouter becomes a bottleneck.

I regret using gin and you get trapped at the beginning because there isn't a big red notice saying "gin does not support classic REST apis because of its insane httprouter implementation"

@cmingxu
Copy link

cmingxu commented Mar 23, 2020

trap in same problem when trying to rewrite existing project with gin

@lggomez
Copy link
Contributor

lggomez commented May 17, 2020

I regret using gin and you get trapped at the beginning because there isn't a big red notice saying "gin does not support classic REST apis because of its insane httprouter implementation"

I strongly agree at least with this part, as long as it remains unfixed

I'm stuck with this issue and since in some project scenarios routes are already defined by spec/requirement and cannot be changed on development cycles I'll have to work around this on the traffic layer so I can still use the original routes as intended

@abhaikollara
Copy link

Is there a proper workaround for this, other than writing a manual routing function ?

@ttylta
Copy link

ttylta commented Nov 15, 2020

This seriously needs to be addressed. Having to write convoluted middlewares to make any serious (not to mention conventional) application suggests a troubling oversight in design.

@csvwolf
Copy link

csvwolf commented Jan 4, 2021

The router conflict broke my Restful tourist in my company. I really need a replace method in the project (even if only a workaround) Do anyone has an idea? THX.

@xuyang2
Copy link

xuyang2 commented Jan 5, 2021

@csvwolf you can embed a *mux.Router in your controller
and use mux.Vars in handler methods

package foo

import (
	"net/http"
	
	"github.com/gin-gonic/gin"
	"github.com/gorilla/mux"
	"go.uber.org/zap"
)

type Controller struct {
	logger *zap.Logger
	router *mux.Router
}

func (c *Controller) HandleXxx(rw http.ResponseWriter, r *http.Request) {
	params := mux.Vars(r)
	fooId := params["fooId"]
}

func NewController(logger *zap.Logger) *Controller {
	c := Controller{
		logger: logger,
		router: mux.NewRouter(),
	}
	c.internalRoutes()

	return &c
}

func (c *Controller) RegisterRoutes(r *gin.Engine) {
	r.Any("/foo", c.Dispatch)
	r.Any("/foo/*any", c.Dispatch)
	r.Any("/v2/foo", c.Dispatch)
	r.Any("/v2/foo/*any", c.Dispatch)
}

func (c *Controller) Dispatch(gc *gin.Context) {
	c.router.ServeHTTP(gc.Writer, gc.Request)
}

func (c *Controller) internalRoutes() {
	r := mux.NewRouter()

	r.NewRoute().Methods(http.MethodPost).Path("/foo").HandlerFunc(c.HandleXxxA)
	r.NewRoute().Methods(http.MethodPost).Path("/v2/foo").HandlerFunc(c.HandleXxxB)

	r.NewRoute().Methods(http.MethodPost).Path("/foo/aaaa/{fooId:[a-zA-Z0-9_]+}/").HandlerFunc(c.HandleXxxC)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/aaaa/{fooId:[a-zA-Z0-9_]+}/").HandlerFunc(c.HandleXxxD)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/aaaa").HandlerFunc(c.HandleXxxE)

	r.NewRoute().Methods(http.MethodDelete).Path("/foo/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxF)
	r.NewRoute().Methods(http.MethodDelete).Path("/v2/foo/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxG)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxH)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/bbbb/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxI)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/cccc/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxJ)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/dddd").HandlerFunc(c.HandleXxxK)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/eeee/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxL)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/ffff/gggg/{fooId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxM)

	r.NewRoute().Methods(http.MethodGet).Path("/foo/hhhh/{fooId:[a-zA-Z0-9_]+}/{barId:[a-zA-Z0-9_]+}").HandlerFunc(c.HandleXxxM)
}

Related issue:

#2016
#1681

@csvwolf
Copy link

csvwolf commented Jan 5, 2021

@xuyang2 Thanks, I have the same idea and is doing wrapper like that:

	apis := utils.Route{}
	apis.Init(e, "/api/v1")

	regions := apis.Group("/regions")
	{
                // They are gin middleware
		regions.GET("", getAllRegions)
		regions.POST("/groups", addRegionGroup)
		regions.GET("/groups", getAllRegionGroups)
	}

	languages := apis.Group("/languages")
	{
		languages.GET("", getLanguages)
	}

	banners := apis.Group("/banners")
	{
		banners.GET("", getBanners)
		banners.GET("/{id}", getBanner)
		banners.POST("", createBanner)
		banners.PUT("/{id}", updateBanner)
		banners.DELETE("/{id}", deleteBanner)
	}

	apis.Fin()

Just use mux for router but can use anything else in gin and of course, the minimum changes for code, only for the router params :id to {id}

@sgon00
Copy link

sgon00 commented Feb 18, 2021

Sorry to interrupt. Any plans on this issue? This is a showstopper to our project.

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