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.11k stars 7.98k forks source link

Improve middleware for router groups #3546

Open Murderlon opened 1 year ago

Murderlon commented 1 year ago

Problem

Consider this example:

group := router.Group("/group")
{
  group.Use(CORS()) // does not work. Chaining it to `Group` above also does not.
  group.POST("/", someHandler())
}

It gets worse when you'd like to use third-party middleware which expects a http.Handler. It would be great if this works:

group := router.Group("/group")
{
  group.Use(gin.wrapH(thirdparty.Middleware(group.Handler()))) // note: group.Handler() doesn't exist
  group.POST("/", someHandler())
}

Instead I have to extract the code from the library into my own project, and if I only want it to apply to a group, we get something like this:

group := router.Group("/group")
{
  group.OPTIONS("/*group", Middleware())
  group.POST("/", Middleware(), someHandler())
  group.HEAD("/:id", Middleware(), someHandler())
  group.PATCH("/:id", Middleware(), someHandler())
  group.DELETE("/:id", Middleware(), someHandler())
  group.GET("/:id", Middleware(), someHandler())
}

Solution

I'm willing to contribute these changes if we agree on this and I get some tips on how to approach it.

ShikharY10 commented 1 year ago

Can you please tell me what below line is doing. Means, what s.Router.Group is doing.

group := s.Router.Group("/group")
Murderlon commented 1 year ago

s was something from my codebase, but it's just this: https://gin-gonic.com/docs/examples/grouping-routes/

Edited the example above to be more clear.

MysticalMount commented 1 year ago

Suffering from this too, amazing we havnt encountered this up till now and the feature isnt already implemented

pscheid92 commented 1 year ago

Hmm, I am confused. Please provide a reproducible example I could run locally. Maybe I misunderstand your expectations.

When I use .Use() on a group, it works as expected. (It runs the middleware code for all routes in this group but not for other routes in other groups.)

PTAL on my example here: https://go.dev/play/p/YX5Iy5sjuXC

Ozoniuss commented 1 year ago

See my last paragraph for what I think could maybe be the problem.

I'm also confused. A similar example also works for me. Take this code for example:

func someMiddleware() gin.HandlerFunc {
    return func(c *gin.Context) {
        fmt.Println("some middleware called")
    }
}

func main() {
    r := gin.New()

    r1 := r.Group("/v1").Use(someMiddleware())
    {
        r1.GET("/hello", func(ctx *gin.Context) {
            fmt.Println("hello")
        })
    }

    r2 := r.Group("/v2")
    {
        r2.Use(someMiddleware())
        r2.GET("/hello", func(ctx *gin.Context) {
            fmt.Println("hello")
        })
    }
    r.Run(":6677")
}

I get the following output (on the server, client doesn't print anything):

$ curl localhost:6677/v1/hello
# server prints:
some middleware called
hello

$ curl localhost:6677/v2/hello
# server prints
some middleware called
hello

Changing the method to POST doesn't make a difference. In fact, when you call Use it even binds the middleware to all handlers of the group:

    r3 := r.Group("/v3")
    {
        // middleware is called
        r3.Use(someMiddleware()).GET("/hello", func(ctx *gin.Context) {
            fmt.Println("hello")
        })
        // middleware is also called
        r3.GET("/hello2", func(ctx *gin.Context) {
            fmt.Println("hello2")
        })
    }

What does not work is the following:

    // middleware is not called.
    r2 := r.Group("/v2")
    {
        r2.GET("/hello", func(ctx *gin.Context) {
            fmt.Println("hello")
        }).Use(someMiddleware())
    }

but you can get around this with the following approach, if you want it to be specific to an endpoint in the group (although I'm not sure if it's a good practice, will have to check):

    // middleware is called.
    r2 := r.Group("/v2")
    {
        r2.GET("/hello", someMiddleware(), func(ctx *gin.Context) {
            fmt.Println("hello")
        })
    }

With this approach you can even have separate "middleware" for every route:

    r2 := r.Group("/v2")
    {
        // someMiddleware is called.
        r2.GET("/hello", someMiddleware(), func(ctx *gin.Context) {
            fmt.Println("hello")
        })
        // someMiddleware2 is called.
        r2.GET("/hello2", someMiddleware2(), func(ctx *gin.Context) {
            fmt.Println("hello2")
        })
    }

How do you know that your examples are not working? Note that your middlewares might be doing some internal logic without you knowing about it. In my examples, the prints are on the server, not in the client responses. If your middlewares don't do anything to the gin Context, it's likely that they're getting called without you noticing the effect.

AshinZ commented 1 year ago

The reason of CORS middleware not working in group router is that when a path in a group router, gin will try to do route match before executing middleware. It's not middleware not working but CORS middleware not working @Murderlon In your first example, the CORS request send by browser will be aborted if you don't define router for OPTIONS method, the CORS middleware won't be called because the OPTION has been aborted. In this case, we will get 404 error because the OPTIONS request is not defined. So after you add group.OPTIONS("/*group", Middleware()), gin can accept OPTIONS request, and then the CORS middleware work. So how to use CORS middleware in group router? I don't have a better method now. If we allow every OPTION request in global middleware, the CORS mechanism will be meaningless. If we don't do this and try to do this by middleware in group router, it runs into a dead lock.

Murderlon commented 1 year ago

I think that's it yes! Thanks for walking us through it. I'm not sure what's best to do here. Perhaps some clarification in the docs could also close this.