Add wrapper for using middleware #178
Conversation
server/server.go
Outdated
| var handler http.Handler | ||
| if s.middleware != nil { | ||
| handler = s.middleware(mux) | ||
| } else { | ||
| handler = mux | ||
| } | ||
| s.HTTPServer.Handler = handler |
There was a problem hiding this comment.
It can probably be simplified:
if s.middleware != nil {
s.HTTPServer.Handler = s.middleware(s.HTTPServer.Handler)
}
There was a problem hiding this comment.
As I mentioned in comment above, we should consider defining multiple middleware not only one.
for _, m := range s.middleware {
s.HTTPServer.Handler = m(s.HTTPServer.Handler)
}
server/server.go
Outdated
| @@ -31,6 +31,7 @@ type Server struct { | |||
| initializers []InitializerFunc | |||
| initializeTimeout time.Duration | |||
| registrars []func(mux *http.ServeMux) error | |||
| middleware Middleware | |||
There was a problem hiding this comment.
From my point of view field "middleware" should be repeated to allow use to define chain of middleware, solution with one middleware from my point of view isn't enough flexible
server/server.go
Outdated
| var handler http.Handler | ||
| if s.middleware != nil { | ||
| handler = s.middleware(mux) | ||
| } else { | ||
| handler = mux | ||
| } | ||
| s.HTTPServer.Handler = handler |
There was a problem hiding this comment.
As I mentioned in comment above, we should consider defining multiple middleware not only one.
for _, m := range s.middleware {
s.HTTPServer.Handler = m(s.HTTPServer.Handler)
}
|
As discussed with @burov, it's useful to add support for chaining middlewares. The preferable approach would probably be to add a utility function to be called on the client side, similar to grpc_middleware.ChainUnaryServer. func ChainMiddlewares(middlewares ...Middleware) Middleware {
...
}@rdv821 Could you add it to your PR? |
server/server.go
Outdated
| @@ -31,6 +31,7 @@ type Server struct { | |||
| initializers []InitializerFunc | |||
| initializeTimeout time.Duration | |||
| registrars []func(mux *http.ServeMux) error | |||
| middleware []Middleware | |||
server/server.go
Outdated
| @@ -67,6 +71,10 @@ func NewServer(opts ...Option) (*Server, error) { | |||
| } | |||
| s.HTTPServer.Handler = mux | |||
|
|
|||
| for _, m := range s.middleware { | |||
There was a problem hiding this comment.
You should probably wrap them backwards, so that the order of chain execution matches the order they are provided by the client.
server/server_test.go
Outdated
| t.Error("Expected another status") | ||
| } | ||
|
|
||
| resp2, err := http.Get(fmt.Sprint("http://", httpL.Addr().String(), "/v1/hello?name=test")) |
There was a problem hiding this comment.
Probably it doesn't make sense to make the same request twice.
server/server.go
Outdated
| } | ||
|
|
||
| // Sort args for middleware | ||
| func (s *Server) Sort() { |
server/server.go
Outdated
| // Sort args for middleware | ||
| func (s *Server) Sort() { | ||
| for i, j := 0, len(s.middlewares)-1; i < j; i, j = i+1, j-1 { | ||
| s.middlewares[i], s.middlewares[j] = s.middlewares[j], s.middlewares[i] |
There was a problem hiding this comment.
Could you also update the tests to check that the resulting order is right?
server/server_test.go
Outdated
| }) | ||
| } | ||
|
|
||
| func newTestParamVerify(t *testing.T, h http.Handler) http.Handler { |
There was a problem hiding this comment.
This seems not be used anywhere.
|
@Evgeniy-L @burov any more feedback from your side? |
No description provided.