Move middleware to interfaces. #21897
Conversation
|
ping @vdemeester @MHBauer PTAL |
| // Middleware is an interface to allow the use of ordinary functions as Docker API filters. | ||
| // Any struct that has the appropriate signature can be registered as a middleware. | ||
| type Middleware interface { | ||
| WrapHandler(func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error) func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error |
There was a problem hiding this comment.
We could still use httputils.APIFunc here.
type MiddleWare interface {
WrapHandler(httputils.APIFunc) httputils.APIFunc
}@calavera Is there a reason why not ?
There was a problem hiding this comment.
To avoid references to /pkg. There is a script that checks for that. Perhaps the APIFunc definition, or the whole httputils should move to pkg as well.
There was a problem hiding this comment.
Naive question : why avoiding to reference /pkg ?
There was a problem hiding this comment.
I misspoke above. I meant "To avoid references from pkg".
There is a build script validate-pkg that exists to prevent references from pkg to internal docker stuff. The thought is that externals pkg should stand alone.
I might be over thinking this. No ☕ this morning.
There was a problem hiding this comment.
And I'm repeating myself, great.
There was a problem hiding this comment.
middleware is in api/server/middleware and httputils is in api/server/httputils so.. there is real pkg stuff in here 👼
/me wants a bit of coffee too 😹
There was a problem hiding this comment.
No idea what this conversation is about.
There was a problem hiding this comment.
no idea either. I'd actually rather remove the APIFunc type, but I don't want to do it here/yet.
There was a problem hiding this comment.
It should be some type, it's unreadable without :( Or maybe combine variables to one struct, like a trend in modern frameworks.
|
Does |
| } | ||
|
|
||
| // WrapHandler returns a new handler function wrapping the previous one in the request chain. | ||
| func (m Middleware) WrapHandler(handler func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error) func(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error { |
There was a problem hiding this comment.
This would need to be an APIFunc, but it's in pkg, right?
|
SGTM moving to code review |
| if s.cfg.EnableCors { | ||
| handleCORS := middleware.NewCORSMiddleware(s.cfg.CorsHeaders) | ||
| next = handleCORS(next) | ||
| if len(s.middlewares) > 0 { |
There was a problem hiding this comment.
Do we need to check the length here?
There was a problem hiding this comment.
removed the check.
This makes separating middlewares from the core api easier. As an example, the authorization middleware is moved to it's own package. Initialize all static middlewares when the server is created, reducing allocations every time a route is wrapper with the middlewares. Signed-off-by: David Calavera <david.calavera@gmail.com>
calavera
force-pushed
the
remove_authorization_refs_from_api
branch
from
67e97a7
to
8d34676
Compare
|
SGTM |
|
LGTM |
1 similar comment
|
LGTM |
This makes separating middlewares from the core api easier.
As an example, the authorization middleware is moved to
it's own package.
Initialize all static middlewares when the server is created, reducing
allocations every time a route is wrapper with the middlewares.
Signed-off-by: David Calavera david.calavera@gmail.com