Macaron: Strip down renderer middleware (#37627)

* strip down macaron renderer

* inline renderHTML

* remove IndentJSON parameter

* replace renderer with a html/template set

* fix failing test

* fix renderer paths in tests

* make template reloading even simpler

* unify ignored gzip path lookup

* fix csp middleware usage

pkg/middleware/gziper.go

@@ -1,43 +1,81 @@
 package middleware
 
 import (
+	"bufio"
+	"compress/gzip"
+	"fmt"
+	"net"
+	"net/http"
 	"strings"
 
-	"github.com/go-macaron/gzip"
-	"github.com/grafana/grafana/pkg/infra/log"
-	"gopkg.in/macaron.v1"
+	macaron "gopkg.in/macaron.v1"
 )
 
-const resourcesPath = "/resources"
-
-var gzipIgnoredPathPrefixes = []string{
-	"/api/datasources/proxy", // Ignore datasource proxy requests.
-	"/api/plugin-proxy/",
-	"/metrics",
-	"/api/live/ws",   // WebSocket does not support gzip compression.
-	"/api/live/push", // WebSocket does not support gzip compression.
+type gzipResponseWriter struct {
+	w *gzip.Writer
+	macaron.ResponseWriter
 }
 
-func Gziper() macaron.Handler {
-	gziperLogger := log.New("gziper")
-	gziper := gzip.Gziper()
+func (grw *gzipResponseWriter) WriteHeader(c int) {
+	grw.Header().Del("Content-Length")
+	grw.ResponseWriter.WriteHeader(c)
+}
 
-	return func(ctx *macaron.Context) {
-		requestPath := ctx.Req.URL.RequestURI()
+func (grw gzipResponseWriter) Write(p []byte) (int, error) {
+	if grw.Header().Get("Content-Type") == "" {
+		grw.Header().Set("Content-Type", http.DetectContentType(p))
+	}
+	grw.Header().Del("Content-Length")
+	return grw.w.Write(p)
+}
 
-		for _, pathPrefix := range gzipIgnoredPathPrefixes {
-			if strings.HasPrefix(requestPath, pathPrefix) {
+func (grw gzipResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+	if hijacker, ok := grw.ResponseWriter.(http.Hijacker); ok {
+		return hijacker.Hijack()
+	}
+	return nil, nil, fmt.Errorf("GZIP ResponseWriter doesn't implement the Hijacker interface")
+}
+
+type matcher func(s string) bool
+
+func prefix(p string) matcher { return func(s string) bool { return strings.HasPrefix(s, p) } }
+func substr(p string) matcher { return func(s string) bool { return strings.Contains(s, p) } }
+
+var gzipIgnoredPaths = []matcher{
+	prefix("/api/datasources"),
+	prefix("/api/plugins"),
+	prefix("/api/plugin-proxy/"),
+	prefix("/metrics"),
+	prefix("/api/live/ws"),   // WebSocket does not support gzip compression.
+	prefix("/api/live/push"), // WebSocket does not support gzip compression.
+	substr("/resources"),
+}
+
+func Gziper() func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+			requestPath := req.URL.RequestURI()
+
+			for _, pathMatcher := range gzipIgnoredPaths {
+				if pathMatcher(requestPath) {
+					fmt.Println("skip path", requestPath)
+					next.ServeHTTP(rw, req)
+					return
+				}
+			}
+
+			if !strings.Contains(req.Header.Get("Accept-Encoding"), "gzip") {
+				next.ServeHTTP(rw, req)
 				return
 			}
-		}
 
-		// ignore resources
-		if (strings.HasPrefix(requestPath, "/api/datasources/") || strings.HasPrefix(requestPath, "/api/plugins/")) && strings.Contains(requestPath, resourcesPath) {
-			return
-		}
+			grw := &gzipResponseWriter{gzip.NewWriter(rw), rw.(macaron.ResponseWriter)}
+			grw.Header().Set("Content-Encoding", "gzip")
+			grw.Header().Set("Vary", "Accept-Encoding")
 
-		if _, err := ctx.Invoke(gziper); err != nil {
-			gziperLogger.Error("Invoking gzip handler failed", "err", err)
-		}
+			next.ServeHTTP(grw, req)
+			// We can't really handle close errors at this point and we can't report them to the caller
+			_ = grw.w.Close()
+		})
 	}
 }

pkg/middleware/middleware_test.go

@@ -111,7 +111,7 @@ func TestMiddlewareContext(t *testing.T) {
 				Settings: map[string]interface{}{},
 				NavTree:  []*dtos.NavLink{},
 			}
-			t.Log("Calling HTML", "data", data, "render", c.Render)
+			t.Log("Calling HTML", "data", data)
 			c.HTML(200, "index-template", data)
 			t.Log("Returned HTML with code 200")
 		}
@@ -633,10 +633,7 @@ func middlewareScenario(t *testing.T, desc string, fn scenarioFunc, cbs ...func(
 		sc.m = macaron.New()
 		sc.m.Use(AddDefaultResponseHeaders(cfg))
 		sc.m.UseMiddleware(AddCSPHeader(cfg, logger))
-		sc.m.Use(macaron.Renderer(macaron.RenderOptions{
-			Directory: viewsPath,
-			Delims:    macaron.Delims{Left: "[[", Right: "]]"},
-		}))
+		sc.m.UseMiddleware(macaron.Renderer(viewsPath, "[[", "]]"))
 
 		ctxHdlr := getContextHandler(t, cfg)
 		sc.sqlStore = ctxHdlr.SQLStore

pkg/middleware/org_redirect.go

@@ -37,7 +37,7 @@ func OrgRedirect(cfg *setting.Cfg) macaron.Handler {
 			if ctx.IsApiRequest() {
 				ctx.JsonApiErr(404, "Not found", nil)
 			} else {
-				ctx.Error(404, "Not found")
+				http.Error(ctx.Resp, "Not found", http.StatusNotFound)
 			}
 
 			return

pkg/middleware/rate_limit_test.go

@@ -32,10 +32,7 @@ func rateLimiterScenario(t *testing.T, desc string, rps int, burst int, fn rateL
 		cfg := setting.NewCfg()
 
 		m := macaron.New()
-		m.Use(macaron.Renderer(macaron.RenderOptions{
-			Directory: "",
-			Delims:    macaron.Delims{Left: "[[", Right: "]]"},
-		}))
+		m.UseMiddleware(macaron.Renderer("../../public/views", "[[", "]]"))
 		m.Use(getContextHandler(t, cfg).Middleware)
 		m.Get("/foo", RateLimit(rps, burst, func() time.Time { return currentTime }), defaultHandler)
 

pkg/middleware/recovery.go

@@ -158,7 +158,7 @@ func Recovery(cfg *setting.Cfg) macaron.Handler {
 
 					c.JSON(500, resp)
 				} else {
-					c.HTML(500, cfg.ErrTemplateName)
+					c.HTML(500, cfg.ErrTemplateName, c.Data)
 				}
 			}
 		}()

pkg/middleware/recovery_test.go

@@ -65,10 +65,7 @@ func recoveryScenario(t *testing.T, desc string, url string, fn scenarioFunc) {
 		sc.m.Use(Recovery(cfg))
 
 		sc.m.Use(AddDefaultResponseHeaders(cfg))
-		sc.m.Use(macaron.Renderer(macaron.RenderOptions{
-			Directory: viewsPath,
-			Delims:    macaron.Delims{Left: "[[", Right: "]]"},
-		}))
+		sc.m.UseMiddleware(macaron.Renderer(viewsPath, "[[", "]]"))
 
 		sc.userAuthTokenService = auth.NewFakeUserAuthTokenService()
 		sc.remoteCacheService = remotecache.NewFakeStore(t)