gin-gonic / gin

Gin is a HTTP web framework written in Go (Golang). It features a Martini-like API with much better performance -- up to 40 times faster. If you need smashing performance, get yourself some Gin.
https://gin-gonic.com/
MIT License
78.24k stars 7.98k forks source link

Could gin use a more advanced router ? #1730

Closed fungiboletus closed 3 years ago

fungiboletus commented 5 years ago

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 commented 5 years ago

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

b1scuit commented 5 years ago

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 commented 5 years ago

Same problem in my project Solitudes

route conflict

pintohutch commented 5 years ago

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

arianitu commented 4 years ago

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 commented 4 years ago

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

lggomez commented 4 years ago

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 commented 4 years ago

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

ttylta commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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:

gin-gonic/gin#2016 gin-gonic/gin#1681

csvwolf commented 3 years ago

@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 commented 3 years ago

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