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.93k stars 8.02k forks source link

Middleware must be added before endpoints #1224

Open japettyjohn opened 6 years ago

japettyjohn commented 6 years ago

If you use RouterGroup.handle it makes a set of handlers for that path/method based on what is in the RouterGroup.Handlers at that time and it will not use any added later.

You can see why that it is in the routergroup.go.

What that means is if you have:

router := gin.New()
router.GET("/test.html",handler)
router.USE(middleware)
router.GET("/test2.html",handler)

Requests to /test.html will run with no middleware while requets to test2.html will go through middleware.

This makes the functions sort of 'non-idempotent' and based on what I've seen in the documentation this is not the intended behavior.

rvillablanca commented 6 years ago

So, is this considered a bug ? @thinkerou, @appleboy

thinkerou commented 6 years ago

Hi, @japettyjohn @rvillablanca firstly please see https://github.com/gin-gonic/gin/pull/1521/files but the document not prefect and my learn experience, if it have mistake please you point, thanks. middleware includes three types: global middleware, group middleware and single middleware, global middleware should init at the main begin.

japettyjohn commented 6 years ago

@thinkerou Thank you for the document, it explains some features I was not aware of.

But to your point that:

global middleware should init at the main begin

I think you mean that it is incorrect usage to execute router.USE(global) after the use of router.GET(...) for example. If this is the case then router.USE() should panic - but I think this is an unnecessary solution.

The problem is that RouterGroup.Use should be idempotent if it's supposed to add global middleware. One way of doing it would be to separate out Global from the rest and always iterate through them before going through the rest of the potential endpoints and middleware.

thinkerou commented 6 years ago

Hi, @japettyjohn

global middleware should init at the main begin.

I think it's usage suggestion not rule, so it should not panic.

for example:

...
r.Use(globalMiddleware1)
r.GET("/a", ...)
r.Use(globalMiddleware2)
r.GET("/b", ...)

/b can use two global middleware, but /a only can use globalMiddleware1. In other words, middleware is order.

sorry, what's the problem about RouterGroup.Use? can you post some sample code? thanks!

japettyjohn commented 6 years ago

Thanks @thinkerou - the example you gave there is all that I am referring to. I quickly looked at some of the other libraries using fasthttprouter and they are doing the same thing as gin - I can only presume other users expect this behavior.

I was definitely not expecting that calls to Get must be after Use. As I was dynamically enabling middleware and endpoints I had to implement dynamic sequencing on top of it in order to catch this.

At the very least, I would make a note of this in the documentation on Handle and Use.

0xd61 commented 5 years ago

It would be great to have a comment about this behavior in the middleware example.