From d15cbe4b4ec15873c9b0b171b9ca4845060c5aaf Mon Sep 17 00:00:00 2001 From: Serge Zaitsev Date: Mon, 30 Aug 2021 11:48:34 +0200 Subject: [PATCH] Macaron cleanup (#37795) * simplify some dependency injection in macaron * remove unused internal server error handler from macaron * remove internal server error handler from the router * remove unused combo router api * remove unused parts of the macaron router --- pkg/api/app_routes.go | 2 +- pkg/api/http_server.go | 2 - pkg/api/routing/route_register.go | 4 +- pkg/api/routing/route_register_test.go | 8 +- pkg/api/static/static.go | 15 +- pkg/macaron/inject.go | 16 --- pkg/macaron/macaron.go | 42 +----- pkg/macaron/return_handler.go | 2 +- pkg/macaron/router.go | 182 +++---------------------- 9 files changed, 38 insertions(+), 235 deletions(-) diff --git a/pkg/api/app_routes.go b/pkg/api/app_routes.go index 21478a19aa9..875c17d61f3 100644 --- a/pkg/api/app_routes.go +++ b/pkg/api/app_routes.go @@ -47,7 +47,7 @@ func (hs *HTTPServer) initAppPluginRoutes(r *macaron.Macaron) { } } handlers = append(handlers, AppPluginRoute(route, plugin.Id, hs)) - r.Route(url, route.Method, handlers...) + r.Handle(route.Method, url, handlers) log.Debugf("Plugins: Adding proxy route %s", url) } } diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 5f6c3e117e1..23b291c8ec3 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -136,8 +136,6 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi socialService social.Service, oauthTokenService oauthtoken.OAuthTokenService) (*HTTPServer, error) { macaron.Env = cfg.Env m := macaron.New() - // automatically set HEAD for every GET - m.SetAutoHead(true) hs := &HTTPServer{ Cfg: cfg, diff --git a/pkg/api/routing/route_register.go b/pkg/api/routing/route_register.go index e699733071c..102d5c3670c 100644 --- a/pkg/api/routing/route_register.go +++ b/pkg/api/routing/route_register.go @@ -10,8 +10,8 @@ import ( ) type Router interface { - Handle(method, pattern string, handlers []macaron.Handler) *macaron.Route - Get(pattern string, handlers ...macaron.Handler) *macaron.Route + Handle(method, pattern string, handlers []macaron.Handler) + Get(pattern string, handlers ...macaron.Handler) } // RouteRegister allows you to add routes and macaron.Handlers diff --git a/pkg/api/routing/route_register_test.go b/pkg/api/routing/route_register_test.go index cf67abf06dc..285340935d7 100644 --- a/pkg/api/routing/route_register_test.go +++ b/pkg/api/routing/route_register_test.go @@ -12,24 +12,20 @@ type fakeRouter struct { route []route } -func (fr *fakeRouter) Handle(method, pattern string, handlers []macaron.Handler) *macaron.Route { +func (fr *fakeRouter) Handle(method, pattern string, handlers []macaron.Handler) { fr.route = append(fr.route, route{ pattern: pattern, method: method, handlers: handlers, }) - - return &macaron.Route{} } -func (fr *fakeRouter) Get(pattern string, handlers ...macaron.Handler) *macaron.Route { +func (fr *fakeRouter) Get(pattern string, handlers ...macaron.Handler) { fr.route = append(fr.route, route{ pattern: pattern, method: http.MethodGet, handlers: handlers, }) - - return &macaron.Route{} } func emptyHandlers(n int) []macaron.Handler { diff --git a/pkg/api/static/static.go b/pkg/api/static/static.go index e372a7c6c9b..98b2c5d7222 100644 --- a/pkg/api/static/static.go +++ b/pkg/api/static/static.go @@ -17,7 +17,6 @@ package httpstatic import ( "fmt" - "log" "net/http" "os" "path" @@ -26,6 +25,7 @@ import ( "strings" "sync" + "github.com/grafana/grafana/pkg/infra/log" "gopkg.in/macaron.v1" ) @@ -115,7 +115,7 @@ func prepareStaticOptions(dir string, options []StaticOptions) StaticOptions { return prepareStaticOption(dir, opt) } -func staticHandler(ctx *macaron.Context, log *log.Logger, opt StaticOptions) bool { +func staticHandler(ctx *macaron.Context, log log.Logger, opt StaticOptions) bool { if ctx.Req.Method != "GET" && ctx.Req.Method != "HEAD" { return false } @@ -138,7 +138,7 @@ func staticHandler(ctx *macaron.Context, log *log.Logger, opt StaticOptions) boo } defer func() { if err := f.Close(); err != nil { - log.Printf("Failed to close file: %s\n", err) + log.Error("Failed to close file", "error", err) } }() @@ -171,7 +171,7 @@ func staticHandler(ctx *macaron.Context, log *log.Logger, opt StaticOptions) boo } defer func() { if err := indexFile.Close(); err != nil { - log.Printf("Failed to close file: %s", err) + log.Error("Failed to close file", "error", err) } }() @@ -182,7 +182,7 @@ func staticHandler(ctx *macaron.Context, log *log.Logger, opt StaticOptions) boo } if !opt.SkipLogging { - log.Printf("[Static] Serving %s\n", file) + log.Info("[Static] Serving", "file", file) } // Add an Expires header to the static content @@ -198,7 +198,8 @@ func staticHandler(ctx *macaron.Context, log *log.Logger, opt StaticOptions) boo func Static(directory string, staticOpt ...StaticOptions) macaron.Handler { opt := prepareStaticOptions(directory, staticOpt) - return func(ctx *macaron.Context, log *log.Logger) { - staticHandler(ctx, log, opt) + logger := log.New("static") + return func(ctx *macaron.Context) { + staticHandler(ctx, logger, opt) } } diff --git a/pkg/macaron/inject.go b/pkg/macaron/inject.go index 5a7cf9c9bc6..1cc4f988c43 100644 --- a/pkg/macaron/inject.go +++ b/pkg/macaron/inject.go @@ -26,10 +26,6 @@ import ( type Injector interface { Invoker TypeMapper - // SetParent sets the parent of the injector. If the injector cannot find a - // dependency in its Type map it will check its parent before returning an - // error. - SetParent(Injector) } // Invoker represents an interface for calling functions via reflection. @@ -84,7 +80,6 @@ type TypeMapper interface { type injector struct { values map[reflect.Type]reflect.Value - parent Injector } // InterfaceOf dereferences a pointer to an Interface type. @@ -193,16 +188,5 @@ func (i *injector) GetVal(t reflect.Type) reflect.Value { } } } - - // Still no type found, try to look it up on the parent - if !val.IsValid() && i.parent != nil { - val = i.parent.GetVal(t) - } - return val - -} - -func (i *injector) SetParent(parent Injector) { - i.parent = parent } diff --git a/pkg/macaron/macaron.go b/pkg/macaron/macaron.go index ce44a6ebf26..782d2b403f6 100644 --- a/pkg/macaron/macaron.go +++ b/pkg/macaron/macaron.go @@ -19,9 +19,7 @@ package macaron // import "gopkg.in/macaron.v1" import ( "context" - "log" "net/http" - "os" "reflect" "strings" ) @@ -56,14 +54,6 @@ func (invoke handlerFuncInvoker) Invoke(params []interface{}) ([]reflect.Value, return nil, nil } -// internalServerErrorInvoker is an inject.FastInvoker wrapper of func(rw http.ResponseWriter, err error). -type internalServerErrorInvoker func(rw http.ResponseWriter, err error) - -func (invoke internalServerErrorInvoker) Invoke(params []interface{}) ([]reflect.Value, error) { - invoke(params[0].(http.ResponseWriter), params[1].(error)) - return nil, nil -} - // validateAndWrapHandler makes sure a handler is a callable function, it panics if not. // When the handler is also potential to be any built-in inject.FastInvoker, // it wraps the handler automatically to have some performance gain. @@ -78,8 +68,6 @@ func validateAndWrapHandler(h Handler) Handler { return ContextInvoker(v) case func(http.ResponseWriter, *http.Request): return handlerFuncInvoker(v) - case func(http.ResponseWriter, error): - return internalServerErrorInvoker(v) } } return h @@ -99,32 +87,18 @@ func validateAndWrapHandlers(handlers []Handler) []Handler { // Macaron represents the top level web application. // Injector methods can be invoked to map services on a global level. type Macaron struct { - Injector - befores []BeforeHandler handlers []Handler - hasURLPrefix bool - urlPrefix string // For suburl support. + urlPrefix string // For suburl support. *Router - - logger *log.Logger } // New creates a bare bones Macaron instance. // Use this method if you want to have full control over the middleware that is used. func New() *Macaron { - m := &Macaron{ - Injector: NewInjector(), - Router: NewRouter(), - logger: log.New(os.Stdout, "[Macaron] ", 0), - } + m := &Macaron{Router: NewRouter()} m.Router.m = m - m.Map(m.logger) - m.Map(defaultReturnHandler()) m.NotFound(http.NotFound) - m.InternalServerError(func(rw http.ResponseWriter, err error) { - http.Error(rw, err.Error(), 500) - }) return m } @@ -185,7 +159,7 @@ func (m *Macaron) createContext(rw http.ResponseWriter, req *http.Request) *Cont Data: make(map[string]interface{}), } req = req.WithContext(context.WithValue(req.Context(), macaronContextKey{}, c)) - c.SetParent(m) + c.Map(defaultReturnHandler()) c.Map(c) c.MapTo(c.Resp, (*http.ResponseWriter)(nil)) c.Map(req) @@ -197,19 +171,11 @@ func (m *Macaron) createContext(rw http.ResponseWriter, req *http.Request) *Cont // Useful if you want to control your own HTTP server. // Be aware that none of middleware will run without registering any router. func (m *Macaron) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - if m.hasURLPrefix { - req.URL.Path = strings.TrimPrefix(req.URL.Path, m.urlPrefix) - } - for _, h := range m.befores { - if h(rw, req) { - return - } - } + req.URL.Path = strings.TrimPrefix(req.URL.Path, m.urlPrefix) m.Router.ServeHTTP(rw, req) } // SetURLPrefix sets URL prefix of router layer, so that it support suburl. func (m *Macaron) SetURLPrefix(prefix string) { m.urlPrefix = prefix - m.hasURLPrefix = len(m.urlPrefix) > 0 } diff --git a/pkg/macaron/return_handler.go b/pkg/macaron/return_handler.go index 51a1d5aa23b..efef439dd97 100644 --- a/pkg/macaron/return_handler.go +++ b/pkg/macaron/return_handler.go @@ -53,7 +53,7 @@ func defaultReturnHandler() ReturnHandler { if isError(respVal) { err := respVal.Interface().(error) if err != nil { - ctx.internalServerError(ctx, err) + http.Error(resp, err.Error(), 500) } return } else if canDeref(respVal) { diff --git a/pkg/macaron/router.go b/pkg/macaron/router.go index 7d3d457bebf..497fef85178 100644 --- a/pkg/macaron/router.go +++ b/pkg/macaron/router.go @@ -40,7 +40,7 @@ type routeMap struct { } // NewRouteMap initializes and returns a new routeMap. -func NewRouteMap() *routeMap { +func newRouteMap() *routeMap { rm := &routeMap{ routes: make(map[string]map[string]*Leaf), } @@ -73,61 +73,37 @@ type group struct { // Router represents a Macaron router layer. type Router struct { - m *Macaron - autoHead bool - routers map[string]*Tree + m *Macaron + routers map[string]*Tree *routeMap namedRoutes map[string]*Leaf - groups []group - notFound http.HandlerFunc - internalServerError func(*Context, error) + groups []group + notFound http.HandlerFunc } func NewRouter() *Router { return &Router{ routers: make(map[string]*Tree), - routeMap: NewRouteMap(), + routeMap: newRouteMap(), namedRoutes: make(map[string]*Leaf), } } -// SetAutoHead sets the value who determines whether add HEAD method automatically -// when GET method is added. -func (r *Router) SetAutoHead(v bool) { - r.autoHead = v -} - type Params map[string]string // Handle is a function that can be registered to a route to handle HTTP requests. // Like http.HandlerFunc, but has a third parameter for the values of wildcards (variables). type Handle func(http.ResponseWriter, *http.Request, Params) -// Route represents a wrapper of leaf route and upper level router. -type Route struct { - router *Router - leaf *Leaf -} - -// Name sets name of route. -func (r *Route) Name(name string) { - if len(name) == 0 { - panic("route name cannot be empty") - } else if r.router.namedRoutes[name] != nil { - panic("route with given name already exists: " + name) - } - r.router.namedRoutes[name] = r.leaf -} - // handle adds new route to the router tree. -func (r *Router) handle(method, pattern string, handle Handle) *Route { +func (r *Router) handle(method, pattern string, handle Handle) { method = strings.ToUpper(method) var leaf *Leaf // Prevent duplicate routes. if leaf = r.getLeaf(method, pattern); leaf != nil { - return &Route{r, leaf} + return } // Validate HTTP methods. @@ -156,11 +132,10 @@ func (r *Router) handle(method, pattern string, handle Handle) *Route { } r.add(m, pattern, leaf) } - return &Route{r, leaf} } // Handle registers a new request handle with the given pattern, method and handlers. -func (r *Router) Handle(method string, pattern string, handlers []Handler) *Route { +func (r *Router) Handle(method string, pattern string, handlers []Handler) { if len(r.groups) > 0 { groupPattern := "" h := make([]Handler, 0) @@ -175,7 +150,7 @@ func (r *Router) Handle(method string, pattern string, handlers []Handler) *Rout } handlers = validateAndWrapHandlers(handlers) - return r.handle(method, pattern, func(resp http.ResponseWriter, req *http.Request, params Params) { + r.handle(method, pattern, func(resp http.ResponseWriter, req *http.Request, params Params) { c := r.m.createContext(resp, req) c.params = params c.handlers = make([]Handler, 0, len(r.m.handlers)+len(handlers)) @@ -192,64 +167,31 @@ func (r *Router) Group(pattern string, fn func(), h ...Handler) { } // Get is a shortcut for r.Handle("GET", pattern, handlers) -func (r *Router) Get(pattern string, h ...Handler) (leaf *Route) { - leaf = r.Handle("GET", pattern, h) - if r.autoHead { - r.Head(pattern, h...) - } - return leaf +func (r *Router) Get(pattern string, h ...Handler) { + r.Handle("GET", pattern, h) + r.Head(pattern, h...) } // Patch is a shortcut for r.Handle("PATCH", pattern, handlers) -func (r *Router) Patch(pattern string, h ...Handler) *Route { - return r.Handle("PATCH", pattern, h) -} +func (r *Router) Patch(pattern string, h ...Handler) { r.Handle("PATCH", pattern, h) } // Post is a shortcut for r.Handle("POST", pattern, handlers) -func (r *Router) Post(pattern string, h ...Handler) *Route { - return r.Handle("POST", pattern, h) -} +func (r *Router) Post(pattern string, h ...Handler) { r.Handle("POST", pattern, h) } // Put is a shortcut for r.Handle("PUT", pattern, handlers) -func (r *Router) Put(pattern string, h ...Handler) *Route { - return r.Handle("PUT", pattern, h) -} +func (r *Router) Put(pattern string, h ...Handler) { r.Handle("PUT", pattern, h) } // Delete is a shortcut for r.Handle("DELETE", pattern, handlers) -func (r *Router) Delete(pattern string, h ...Handler) *Route { - return r.Handle("DELETE", pattern, h) -} +func (r *Router) Delete(pattern string, h ...Handler) { r.Handle("DELETE", pattern, h) } // Options is a shortcut for r.Handle("OPTIONS", pattern, handlers) -func (r *Router) Options(pattern string, h ...Handler) *Route { - return r.Handle("OPTIONS", pattern, h) -} +func (r *Router) Options(pattern string, h ...Handler) { r.Handle("OPTIONS", pattern, h) } // Head is a shortcut for r.Handle("HEAD", pattern, handlers) -func (r *Router) Head(pattern string, h ...Handler) *Route { - return r.Handle("HEAD", pattern, h) -} +func (r *Router) Head(pattern string, h ...Handler) { r.Handle("HEAD", pattern, h) } // Any is a shortcut for r.Handle("*", pattern, handlers) -func (r *Router) Any(pattern string, h ...Handler) *Route { - return r.Handle("*", pattern, h) -} - -// Route is a shortcut for same handlers but different HTTP methods. -// -// Example: -// m.Route("/", "GET,POST", h) -func (r *Router) Route(pattern, methods string, h ...Handler) (route *Route) { - for _, m := range strings.Split(methods, ",") { - route = r.Handle(strings.TrimSpace(m), pattern, h) - } - return route -} - -// Combo returns a combo router. -func (r *Router) Combo(pattern string, h ...Handler) *ComboRouter { - return &ComboRouter{r, pattern, h, map[string]bool{}, nil} -} +func (r *Router) Any(pattern string, h ...Handler) { r.Handle("*", pattern, h) } // NotFound configurates http.HandlerFunc which is called when no matching route is // found. If it is not set, http.NotFound is used. @@ -265,19 +207,6 @@ func (r *Router) NotFound(handlers ...Handler) { } } -// InternalServerError configurates handler which is called when route handler returns -// error. If it is not set, default handler is used. -// Be sure to set 500 response code in your handler. -func (r *Router) InternalServerError(handlers ...Handler) { - handlers = validateAndWrapHandlers(handlers) - r.internalServerError = func(c *Context, err error) { - c.index = 0 - c.handlers = handlers - c.Map(err) - c.run() - } -} - func (r *Router) ServeHTTP(rw http.ResponseWriter, req *http.Request) { if t, ok := r.routers[req.Method]; ok { // Fast match for static routes @@ -299,74 +228,3 @@ func (r *Router) ServeHTTP(rw http.ResponseWriter, req *http.Request) { r.notFound(rw, req) } - -// URLFor builds path part of URL by given pair values. -func (r *Router) URLFor(name string, pairs ...string) string { - leaf, ok := r.namedRoutes[name] - if !ok { - panic("route with given name does not exists: " + name) - } - return leaf.URLPath(pairs...) -} - -// ComboRouter represents a combo router. -type ComboRouter struct { - router *Router - pattern string - handlers []Handler - methods map[string]bool // Registered methods. - - lastRoute *Route -} - -func (cr *ComboRouter) checkMethod(name string) { - if cr.methods[name] { - panic("method '" + name + "' has already been registered") - } - cr.methods[name] = true -} - -func (cr *ComboRouter) route(fn func(string, ...Handler) *Route, method string, h ...Handler) *ComboRouter { - cr.checkMethod(method) - cr.lastRoute = fn(cr.pattern, append(cr.handlers, h...)...) - return cr -} - -func (cr *ComboRouter) Get(h ...Handler) *ComboRouter { - if cr.router.autoHead { - cr.Head(h...) - } - return cr.route(cr.router.Get, "GET", h...) -} - -func (cr *ComboRouter) Patch(h ...Handler) *ComboRouter { - return cr.route(cr.router.Patch, "PATCH", h...) -} - -func (cr *ComboRouter) Post(h ...Handler) *ComboRouter { - return cr.route(cr.router.Post, "POST", h...) -} - -func (cr *ComboRouter) Put(h ...Handler) *ComboRouter { - return cr.route(cr.router.Put, "PUT", h...) -} - -func (cr *ComboRouter) Delete(h ...Handler) *ComboRouter { - return cr.route(cr.router.Delete, "DELETE", h...) -} - -func (cr *ComboRouter) Options(h ...Handler) *ComboRouter { - return cr.route(cr.router.Options, "OPTIONS", h...) -} - -func (cr *ComboRouter) Head(h ...Handler) *ComboRouter { - return cr.route(cr.router.Head, "HEAD", h...) -} - -// Name sets name of ComboRouter route. -func (cr *ComboRouter) Name(name string) { - if cr.lastRoute == nil { - panic("no corresponding route to be named") - } - cr.lastRoute.Name(name) -}