From ab5087e215271bb9b6d64f84ee7d69194eec2827 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Mon, 7 Dec 2015 23:17:05 +0100 Subject: [PATCH 1/4] Gzip: support for min_length. --- caddy/setup/gzip.go | 30 ++++++-- caddy/setup/gzip_test.go | 12 ++++ middleware/gzip/gzip.go | 20 ++++-- middleware/gzip/gzip_test.go | 2 +- .../gzip/{filter.go => request_filter.go} | 8 +-- ...{filter_test.go => request_filter_test.go} | 4 +- middleware/gzip/response_filter.go | 62 ++++++++++++++++ middleware/gzip/response_filter_test.go | 70 +++++++++++++++++++ 8 files changed, 192 insertions(+), 16 deletions(-) rename middleware/gzip/{filter.go => request_filter.go} (91%) rename middleware/gzip/{filter_test.go => request_filter_test.go} (95%) create mode 100644 middleware/gzip/response_filter.go create mode 100644 middleware/gzip/response_filter_test.go diff --git a/caddy/setup/gzip.go b/caddy/setup/gzip.go index a81c5f170..40c81252f 100644 --- a/caddy/setup/gzip.go +++ b/caddy/setup/gzip.go @@ -27,9 +27,13 @@ func gzipParse(c *Controller) ([]gzip.Config, error) { for c.Next() { config := gzip.Config{} + // Request Filters pathFilter := gzip.PathFilter{IgnoredPaths: make(gzip.Set)} extFilter := gzip.ExtFilter{Exts: make(gzip.Set)} + // Response Filters + lengthFilter := gzip.LengthFilter(0) + // No extra args expected if len(c.RemainingArgs()) > 0 { return configs, c.ArgErr() @@ -68,24 +72,42 @@ func gzipParse(c *Controller) ([]gzip.Config, error) { } level, _ := strconv.Atoi(c.Val()) config.Level = level + case "min_length": + if !c.NextArg() { + return configs, c.ArgErr() + } + length, err := strconv.ParseInt(c.Val(), 10, 64) + if err != nil { + return configs, err + } else if length == 0 { + return configs, fmt.Errorf(`gzip: min_length must be greater than 0`) + } + lengthFilter = gzip.LengthFilter(length) default: return configs, c.ArgErr() } } - config.Filters = []gzip.Filter{} + // Request Filters + config.RequestFilters = []gzip.RequestFilter{} // If ignored paths are specified, put in front to filter with path first if len(pathFilter.IgnoredPaths) > 0 { - config.Filters = []gzip.Filter{pathFilter} + config.RequestFilters = []gzip.RequestFilter{pathFilter} } // Then, if extensions are specified, use those to filter. // Otherwise, use default extensions filter. if len(extFilter.Exts) > 0 { - config.Filters = append(config.Filters, extFilter) + config.RequestFilters = append(config.RequestFilters, extFilter) } else { - config.Filters = append(config.Filters, gzip.DefaultExtFilter()) + config.RequestFilters = append(config.RequestFilters, gzip.DefaultExtFilter()) + } + + // Response Filters + // If min_length is specified, use it. + if int64(lengthFilter) != 0 { + config.ResponseFilters = append(config.ResponseFilters, lengthFilter) } configs = append(configs, config) diff --git a/caddy/setup/gzip_test.go b/caddy/setup/gzip_test.go index 22d01d7a1..36eeb0aea 100644 --- a/caddy/setup/gzip_test.go +++ b/caddy/setup/gzip_test.go @@ -73,6 +73,18 @@ func TestGzip(t *testing.T) { level 1 } `, false}, + {`gzip { not /file + ext * + level 1 + min_length ab + } + `, true}, + {`gzip { not /file + ext * + level 1 + min_length 1000 + } + `, false}, } for i, test := range tests { c := NewTestController(test.input) diff --git a/middleware/gzip/gzip.go b/middleware/gzip/gzip.go index e2d447753..b5866f682 100644 --- a/middleware/gzip/gzip.go +++ b/middleware/gzip/gzip.go @@ -23,8 +23,9 @@ type Gzip struct { // Config holds the configuration for Gzip middleware type Config struct { - Filters []Filter // Filters to use - Level int // Compression level + RequestFilters []RequestFilter + ResponseFilters []ResponseFilter + Level int // Compression level } // ServeHTTP serves a gzipped response if the client supports it. @@ -36,8 +37,8 @@ func (g Gzip) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { outer: for _, c := range g.Configs { - // Check filters to determine if gzipping is permitted for this request - for _, filter := range c.Filters { + // Check request filters to determine if gzipping is permitted for this request + for _, filter := range c.RequestFilters { if !filter.ShouldCompress(r) { continue outer } @@ -56,8 +57,17 @@ outer: defer gzipWriter.Close() gz := gzipResponseWriter{Writer: gzipWriter, ResponseWriter: w} + var rw http.ResponseWriter + // if no response filter is used + if len(c.ResponseFilters) == 0 { + rw = gz + } else { + // wrap gzip writer with ResponseFilterWriter + rw = NewResponseFilterWriter(c.ResponseFilters, gz) + } + // Any response in forward middleware will now be compressed - status, err := g.Next.ServeHTTP(gz, r) + status, err := g.Next.ServeHTTP(rw, r) // If there was an error that remained unhandled, we need // to send something back before gzipWriter gets closed at diff --git a/middleware/gzip/gzip_test.go b/middleware/gzip/gzip_test.go index 3e7bed996..11ce6b209 100644 --- a/middleware/gzip/gzip_test.go +++ b/middleware/gzip/gzip_test.go @@ -21,7 +21,7 @@ func TestGzipHandler(t *testing.T) { extFilter.Exts.Add(e) } gz := Gzip{Configs: []Config{ - {Filters: []Filter{pathFilter, extFilter}}, + {RequestFilters: []RequestFilter{pathFilter, extFilter}}, }} w := httptest.NewRecorder() diff --git a/middleware/gzip/filter.go b/middleware/gzip/request_filter.go similarity index 91% rename from middleware/gzip/filter.go rename to middleware/gzip/request_filter.go index f4039da35..76a9ef83c 100644 --- a/middleware/gzip/filter.go +++ b/middleware/gzip/request_filter.go @@ -7,8 +7,8 @@ import ( "github.com/mholt/caddy/middleware" ) -// Filter determines if a request should be gzipped. -type Filter interface { +// RequestFilter determines if a request should be gzipped. +type RequestFilter interface { // ShouldCompress tells if gzip compression // should be done on the request. ShouldCompress(*http.Request) bool @@ -26,7 +26,7 @@ func DefaultExtFilter() ExtFilter { return m } -// ExtFilter is Filter for file name extensions. +// ExtFilter is RequestFilter for file name extensions. type ExtFilter struct { // Exts is the file name extensions to accept Exts Set @@ -43,7 +43,7 @@ func (e ExtFilter) ShouldCompress(r *http.Request) bool { return e.Exts.Contains(ExtWildCard) || e.Exts.Contains(ext) } -// PathFilter is Filter for request path. +// PathFilter is RequestFilter for request path. type PathFilter struct { // IgnoredPaths is the paths to ignore IgnoredPaths Set diff --git a/middleware/gzip/filter_test.go b/middleware/gzip/request_filter_test.go similarity index 95% rename from middleware/gzip/filter_test.go rename to middleware/gzip/request_filter_test.go index f6537b9e2..ce31d7faf 100644 --- a/middleware/gzip/filter_test.go +++ b/middleware/gzip/request_filter_test.go @@ -47,7 +47,7 @@ func TestSet(t *testing.T) { } func TestExtFilter(t *testing.T) { - var filter Filter = ExtFilter{make(Set)} + var filter RequestFilter = ExtFilter{make(Set)} for _, e := range []string{".txt", ".html", ".css", ".md"} { filter.(ExtFilter).Exts.Add(e) } @@ -86,7 +86,7 @@ func TestPathFilter(t *testing.T) { paths := []string{ "/a", "/b", "/c", "/de", } - var filter Filter = PathFilter{make(Set)} + var filter RequestFilter = PathFilter{make(Set)} for _, p := range paths { filter.(PathFilter).IgnoredPaths.Add(p) } diff --git a/middleware/gzip/response_filter.go b/middleware/gzip/response_filter.go new file mode 100644 index 000000000..8793dabe1 --- /dev/null +++ b/middleware/gzip/response_filter.go @@ -0,0 +1,62 @@ +package gzip + +import ( + "net/http" + "strconv" +) + +// ResponseFilter determines if the response should be gzipped. +type ResponseFilter interface { + ShouldCompress(http.ResponseWriter) bool +} + +// LengthFilter is ResponseFilter for minimum content length. +type LengthFilter int64 + +// ShouldCompress returns if content length is greater than or +// equals to minimum length. +func (l LengthFilter) ShouldCompress(w http.ResponseWriter) bool { + contentLength := (w.Header().Get("Content-Length")) + length, err := strconv.ParseInt(contentLength, 10, 64) + if err != nil || length == 0 { + return false + } + return l != 0 && int64(l) <= length +} + +// ResponseFilterWriter validates ResponseFilters. It writes +// gzip compressed data if ResponseFilters are satisfied or +// uncompressed data otherwise. +type ResponseFilterWriter struct { + filters []ResponseFilter + validated bool + shouldCompress bool + gzipResponseWriter +} + +// NewResponseFilterWriter creates and initializes a new ResponseFilterWriter. +func NewResponseFilterWriter(filters []ResponseFilter, gz gzipResponseWriter) *ResponseFilterWriter { + return &ResponseFilterWriter{filters: filters, gzipResponseWriter: gz} +} + +// Write wraps underlying Write method and compresses if filters +// are satisfied +func (r *ResponseFilterWriter) Write(b []byte) (int, error) { + // One time validation to determine if compression should + // be used or not. + if !r.validated { + r.shouldCompress = true + for _, filter := range r.filters { + if !filter.ShouldCompress(r) { + r.shouldCompress = false + break + } + } + r.validated = true + } + + if r.shouldCompress { + return r.gzipResponseWriter.Write(b) + } + return r.ResponseWriter.Write(b) +} diff --git a/middleware/gzip/response_filter_test.go b/middleware/gzip/response_filter_test.go new file mode 100644 index 000000000..1a5a1b4f3 --- /dev/null +++ b/middleware/gzip/response_filter_test.go @@ -0,0 +1,70 @@ +package gzip + +import ( + "compress/gzip" + "fmt" + "net/http/httptest" + "testing" +) + +func TestLengthFilter(t *testing.T) { + var filters []ResponseFilter = []ResponseFilter{ + LengthFilter(100), + LengthFilter(1000), + LengthFilter(0), + } + + var tests = []struct { + length int64 + shouldCompress [3]bool + }{ + {20, [3]bool{false, false, false}}, + {50, [3]bool{false, false, false}}, + {100, [3]bool{true, false, false}}, + {500, [3]bool{true, false, false}}, + {1000, [3]bool{true, true, false}}, + {1500, [3]bool{true, true, false}}, + } + + for i, ts := range tests { + for j, filter := range filters { + r := httptest.NewRecorder() + r.Header().Set("Content-Length", fmt.Sprint(ts.length)) + if filter.ShouldCompress(r) != ts.shouldCompress[j] { + t.Errorf("Test %v: Expected %v found %v", i, ts.shouldCompress[j], filter.ShouldCompress(r)) + } + } + } +} + +func TestResponseFilterWriter(t *testing.T) { + tests := []struct { + body string + shouldCompress bool + }{ + {"Hello\t\t\t\n", false}, + {"Hello the \t\t\t world is\n\n\n great", true}, + {"Hello \t\t\nfrom gzip", true}, + {"Hello gzip\n", false}, + } + filters := []ResponseFilter{ + LengthFilter(15), + } + for i, ts := range tests { + w := httptest.NewRecorder() + w.Header().Set("Content-Length", fmt.Sprint(len(ts.body))) + gz := gzipResponseWriter{gzip.NewWriter(w), w} + rw := NewResponseFilterWriter(filters, gz) + rw.Write([]byte(ts.body)) + resp := w.Body.String() + if !ts.shouldCompress { + if resp != ts.body { + t.Errorf("Test %v: No compression expected, found %v", i, resp) + } + } else { + if resp == ts.body { + t.Errorf("Test %v: Compression expected, found %v", i, resp) + } + } + } +} From 8631f33940a8c50b154da2756ce7d4954b925f0a Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Mon, 7 Dec 2015 23:27:57 +0100 Subject: [PATCH 2/4] remove minor ugly parenthesis --- middleware/gzip/response_filter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/middleware/gzip/response_filter.go b/middleware/gzip/response_filter.go index 8793dabe1..fcfc8be51 100644 --- a/middleware/gzip/response_filter.go +++ b/middleware/gzip/response_filter.go @@ -16,7 +16,7 @@ type LengthFilter int64 // ShouldCompress returns if content length is greater than or // equals to minimum length. func (l LengthFilter) ShouldCompress(w http.ResponseWriter) bool { - contentLength := (w.Header().Get("Content-Length")) + contentLength := w.Header().Get("Content-Length") length, err := strconv.ParseInt(contentLength, 10, 64) if err != nil || length == 0 { return false From 23631cfaca8526192a7d18ce619a4fd8e46b4289 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Tue, 8 Dec 2015 12:01:24 +0100 Subject: [PATCH 3/4] Fix deleted Content-Length header bug. --- middleware/gzip/gzip.go | 17 ++++++++--- middleware/gzip/gzip_test.go | 2 ++ middleware/gzip/response_filter.go | 38 ++++++++++++++++--------- middleware/gzip/response_filter_test.go | 30 +++++++++++++++---- 4 files changed, 65 insertions(+), 22 deletions(-) diff --git a/middleware/gzip/gzip.go b/middleware/gzip/gzip.go index b5866f682..65256b2ff 100644 --- a/middleware/gzip/gzip.go +++ b/middleware/gzip/gzip.go @@ -3,6 +3,7 @@ package gzip import ( + "bytes" "compress/gzip" "fmt" "io" @@ -47,9 +48,13 @@ outer: // Delete this header so gzipping is not repeated later in the chain r.Header.Del("Accept-Encoding") - w.Header().Set("Content-Encoding", "gzip") - w.Header().Set("Vary", "Accept-Encoding") - gzipWriter, err := newWriter(c, w) + // gzipWriter modifies underlying writer at init, + // use a buffer instead to leave ResponseWriter in + // original form. + var buf = &bytes.Buffer{} + defer buf.Reset() + + gzipWriter, err := newWriter(c, buf) if err != nil { // should not happen return http.StatusInternalServerError, err @@ -60,6 +65,8 @@ outer: var rw http.ResponseWriter // if no response filter is used if len(c.ResponseFilters) == 0 { + // replace buffer with ResponseWriter + gzipWriter.Reset(w) rw = gz } else { // wrap gzip writer with ResponseFilterWriter @@ -88,7 +95,7 @@ outer: // newWriter create a new Gzip Writer based on the compression level. // If the level is valid (i.e. between 1 and 9), it uses the level. // Otherwise, it uses default compression level. -func newWriter(c Config, w http.ResponseWriter) (*gzip.Writer, error) { +func newWriter(c Config, w io.Writer) (*gzip.Writer, error) { if c.Level >= gzip.BestSpeed && c.Level <= gzip.BestCompression { return gzip.NewWriterLevel(w, c.Level) } @@ -108,6 +115,8 @@ type gzipResponseWriter struct { // be wrong because it doesn't know it's being gzipped. func (w gzipResponseWriter) WriteHeader(code int) { w.Header().Del("Content-Length") + w.Header().Set("Content-Encoding", "gzip") + w.Header().Set("Vary", "Accept-Encoding") w.ResponseWriter.WriteHeader(code) } diff --git a/middleware/gzip/gzip_test.go b/middleware/gzip/gzip_test.go index 11ce6b209..c35c99c63 100644 --- a/middleware/gzip/gzip_test.go +++ b/middleware/gzip/gzip_test.go @@ -80,6 +80,8 @@ func TestGzipHandler(t *testing.T) { func nextFunc(shouldGzip bool) middleware.Handler { return middleware.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { + w.WriteHeader(200) + w.Write([]byte("test")) if shouldGzip { if r.Header.Get("Accept-Encoding") != "" { return 0, fmt.Errorf("Accept-Encoding header not expected") diff --git a/middleware/gzip/response_filter.go b/middleware/gzip/response_filter.go index fcfc8be51..034fd6123 100644 --- a/middleware/gzip/response_filter.go +++ b/middleware/gzip/response_filter.go @@ -1,6 +1,7 @@ package gzip import ( + "compress/gzip" "net/http" "strconv" ) @@ -29,7 +30,6 @@ func (l LengthFilter) ShouldCompress(w http.ResponseWriter) bool { // uncompressed data otherwise. type ResponseFilterWriter struct { filters []ResponseFilter - validated bool shouldCompress bool gzipResponseWriter } @@ -40,21 +40,33 @@ func NewResponseFilterWriter(filters []ResponseFilter, gz gzipResponseWriter) *R } // Write wraps underlying Write method and compresses if filters -// are satisfied -func (r *ResponseFilterWriter) Write(b []byte) (int, error) { - // One time validation to determine if compression should - // be used or not. - if !r.validated { - r.shouldCompress = true - for _, filter := range r.filters { - if !filter.ShouldCompress(r) { - r.shouldCompress = false - break - } +// are satisfied. +func (r *ResponseFilterWriter) WriteHeader(code int) { + // Determine if compression should be used or not. + r.shouldCompress = true + for _, filter := range r.filters { + if !filter.ShouldCompress(r) { + r.shouldCompress = false + break } - r.validated = true } + if r.shouldCompress { + // replace buffer with ResponseWriter + if gzWriter, ok := r.gzipResponseWriter.Writer.(*gzip.Writer); ok { + gzWriter.Reset(r.ResponseWriter) + } + // use gzip WriteHeader to include and delete + // necessary headers + r.gzipResponseWriter.WriteHeader(code) + } else { + r.ResponseWriter.WriteHeader(code) + } +} + +// Write wraps underlying Write method and compresses if filters +// are satisfied +func (r *ResponseFilterWriter) Write(b []byte) (int, error) { if r.shouldCompress { return r.gzipResponseWriter.Write(b) } diff --git a/middleware/gzip/response_filter_test.go b/middleware/gzip/response_filter_test.go index 1a5a1b4f3..cd7c71917 100644 --- a/middleware/gzip/response_filter_test.go +++ b/middleware/gzip/response_filter_test.go @@ -3,8 +3,11 @@ package gzip import ( "compress/gzip" "fmt" + "net/http" "net/http/httptest" "testing" + + "github.com/mholt/caddy/middleware" ) func TestLengthFilter(t *testing.T) { @@ -30,7 +33,8 @@ func TestLengthFilter(t *testing.T) { for j, filter := range filters { r := httptest.NewRecorder() r.Header().Set("Content-Length", fmt.Sprint(ts.length)) - if filter.ShouldCompress(r) != ts.shouldCompress[j] { + wWriter := NewResponseFilterWriter([]ResponseFilter{filter}, gzipResponseWriter{gzip.NewWriter(r), r}) + if filter.ShouldCompress(wWriter) != ts.shouldCompress[j] { t.Errorf("Test %v: Expected %v found %v", i, ts.shouldCompress[j], filter.ShouldCompress(r)) } } @@ -47,16 +51,32 @@ func TestResponseFilterWriter(t *testing.T) { {"Hello \t\t\nfrom gzip", true}, {"Hello gzip\n", false}, } + filters := []ResponseFilter{ LengthFilter(15), } + + server := Gzip{Configs: []Config{ + {ResponseFilters: filters}, + }} + for i, ts := range tests { + server.Next = middleware.HandlerFunc(func(w http.ResponseWriter, r *http.Request) (int, error) { + w.Header().Set("Content-Length", fmt.Sprint(len(ts.body))) + w.WriteHeader(200) + w.Write([]byte(ts.body)) + return 200, nil + }) + + r := urlRequest("/") + r.Header.Set("Accept-Encoding", "gzip") + w := httptest.NewRecorder() - w.Header().Set("Content-Length", fmt.Sprint(len(ts.body))) - gz := gzipResponseWriter{gzip.NewWriter(w), w} - rw := NewResponseFilterWriter(filters, gz) - rw.Write([]byte(ts.body)) + + server.ServeHTTP(w, r) + resp := w.Body.String() + if !ts.shouldCompress { if resp != ts.body { t.Errorf("Test %v: No compression expected, found %v", i, resp) From a44d59f1e553ed23a489da63c64f0932c7ccc295 Mon Sep 17 00:00:00 2001 From: Abiola Ibrahim Date: Wed, 9 Dec 2015 18:44:25 +0100 Subject: [PATCH 4/4] Use ioutil.Discard instead for unneeded bytes. --- middleware/gzip/gzip.go | 11 ++++------- middleware/gzip/response_filter.go | 2 +- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/middleware/gzip/gzip.go b/middleware/gzip/gzip.go index 65256b2ff..147d739f8 100644 --- a/middleware/gzip/gzip.go +++ b/middleware/gzip/gzip.go @@ -3,10 +3,10 @@ package gzip import ( - "bytes" "compress/gzip" "fmt" "io" + "io/ioutil" "net/http" "strings" @@ -49,12 +49,9 @@ outer: r.Header.Del("Accept-Encoding") // gzipWriter modifies underlying writer at init, - // use a buffer instead to leave ResponseWriter in + // use a discard writer instead to leave ResponseWriter in // original form. - var buf = &bytes.Buffer{} - defer buf.Reset() - - gzipWriter, err := newWriter(c, buf) + gzipWriter, err := newWriter(c, ioutil.Discard) if err != nil { // should not happen return http.StatusInternalServerError, err @@ -65,7 +62,7 @@ outer: var rw http.ResponseWriter // if no response filter is used if len(c.ResponseFilters) == 0 { - // replace buffer with ResponseWriter + // replace discard writer with ResponseWriter gzipWriter.Reset(w) rw = gz } else { diff --git a/middleware/gzip/response_filter.go b/middleware/gzip/response_filter.go index 034fd6123..c599b3e1b 100644 --- a/middleware/gzip/response_filter.go +++ b/middleware/gzip/response_filter.go @@ -52,7 +52,7 @@ func (r *ResponseFilterWriter) WriteHeader(code int) { } if r.shouldCompress { - // replace buffer with ResponseWriter + // replace discard writer with ResponseWriter if gzWriter, ok := r.gzipResponseWriter.Writer.(*gzip.Writer); ok { gzWriter.Reset(r.ResponseWriter) }