Repository: beatlabs/patron
Pull request #305 (2020-12-04 15:37:08)

Parse Accept-Encoding header, and set Content-Encoding correctly
Issue #300, #301

<!--
Thanks for taking precious time for making a PR.

Before creating a pull request, please make sure:
- Your PR solves one problem for which a issue exist and a solution has been discussed
- You have read the guide for contributing
  - See https://github.com/beatlabs/patron/blob/master/README.md#how-to-contribute
- You signed all your commits (otherwise we won't be able to merge the PR)
  - See https://github.com/beatlabs/patron/blob/master/SIGNYOURWORK.md
- You added unit tests for the new functionality
- You mention in the PR description which issue it is addressing, e.g. "Resolves #123"
-->

## Which problem is this PR solving?

Resolves issues https://github.com/beatlabs/patron/issues/300 and https://github.com/beatlabs/patron/issues/301

## Short description of the changes

`Accept-Encoding` header is now parsed and respected in compression middleware
`Content-Encoding` header is not correctly set


codecov[bot]: # [Codecov](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=h1) Report
> Merging [#305](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=desc) (9da8c50) into [master](https://codecov.io/gh/beatlabs/patron/commit/c0556559f4a96c5cc1aa7d2c7fe651e58715e540?el=desc) (c055655) will **increase** coverage by `0.18%`.
> The diff coverage is `84.61%`.

[![Impacted file tree graph](https://codecov.io/gh/beatlabs/patron/pull/305/graphs/tree.svg?width=650&height=150&src=pr&token=sxY15rXW1X)](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
+ Coverage   72.23%   72.42%   +0.18%     
==========================================
  Files          61       61              
  Lines        3288     3325      +37     
==========================================
+ Hits         2375     2408      +33     
- Misses        820      822       +2     
- Partials       93       95       +2     
```


| [Impacted Files](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [component/http/middleware.go](https://codecov.io/gh/beatlabs/patron/pull/305/diff?src=pr&el=tree#diff-Y29tcG9uZW50L2h0dHAvbWlkZGxld2FyZS5nbw==) | `83.76% <84.61%> (+<0.01%)` | :arrow_up: |
| [component/async/kafka/simple/duration\_client.go](https://codecov.io/gh/beatlabs/patron/pull/305/diff?src=pr&el=tree#diff-Y29tcG9uZW50L2FzeW5jL2thZmthL3NpbXBsZS9kdXJhdGlvbl9jbGllbnQuZ28=) | `96.36% <0.00%> (+3.63%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=footer). Last update [c055655...9da8c50](https://codecov.io/gh/beatlabs/patron/pull/305?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/component/http/middleware.go b/component/http/middleware.go
index 5f7c0d503..98016fd46 100644
--- a/component/http/middleware.go
+++ b/component/http/middleware.go
@@ -4,10 +4,14 @@ import (
 	"compress/flate"
 	"compress/gzip"
 	"errors"
+	"fmt"
 	"io"
+	"math"
 	"net/http"
 	"net/url"
 	"runtime/debug"
+	"sort"
+	"strconv"
 	"strings"
 
 	"github.com/beatlabs/patron/component/http/auth"
@@ -25,8 +29,12 @@ import (
 const (
 	serverComponent = "http-server"
 	fieldNameError  = "error"
-	gzipHeader      = "gzip"
-	deflateHeader   = "deflate"
+
+	// compression algorithms
+	gzipHeader     = "gzip"
+	deflateHeader  = "deflate"
+	identityHeader = "identity"
+	anythingHeader = "*"
 )
 
 type responseWriter struct {
@@ -153,39 +161,116 @@ func ignore(ignoreRoutes []string, url string) bool {
 	return false
 }
 
+func parseAcceptEncoding(header string) (string, error) {
+	if header == "" {
+		return identityHeader, nil
+	}
+
+	if header == anythingHeader {
+		return anythingHeader, nil
+	}
+
+	weighted := make(map[float64]string)
+
+	algorithms := strings.Split(header, ",")
+	for _, a := range algorithms {
+		algAndWeight := strings.Split(a, ";")
+		algorithm := strings.TrimSpace(algAndWeight[0])
+
+		if notSupportedCompression(algorithm) {
+			continue
+		}
+
+		if len(algAndWeight) != 2 {
+			addWithWeight(weighted, 1.0, algorithm)
+			continue
+		}
+
+		weight := parseWeight(algAndWeight[1])
+		addWithWeight(weighted, weight, algorithm)
+	}
+
+	return selectByWeight(weighted)
+}
+
+func addWithWeight(mapWeighted map[float64]string, weight float64, algorithm string) {
+	if _, ok := mapWeighted[weight]; !ok {
+		mapWeighted[weight] = algorithm
+	}
+}
+
+func notSupportedCompression(algorithm string) bool {
+	return gzipHeader != algorithm && deflateHeader != algorithm && anythingHeader != algorithm
+}
+
+// When not present, the default value is 1 according to https://developer.mozilla.org/en-US/docs/Glossary/Quality_values
+// q not present or can’t be parsed -> 1.0
+// q is < 0 -> 0.0
+// q is > 1 -> 1.0
+func parseWeight(qStr string) float64 {
+	qAndWeight := strings.Split(qStr, "=")
+	if len(qAndWeight) != 2 {
+		return 1.0
+	}
+
+	parsedWeight, err := strconv.ParseFloat(qAndWeight[1], 32)
+	if err != nil {
+		return 1.0
+	}
+
+	return math.Min(1.0, math.Max(0.0, parsedWeight))
+}
+
+func selectByWeight(weighted map[float64]string) (string, error) {
+	if len(weighted) == 0 {
+		return "", fmt.Errorf("no valid compression encoding accepted by client")
+	}
+
+	keys := make([]float64, 0, len(weighted))
+	for k := range weighted {
+		keys = append(keys, k)
+	}
+	sort.Float64s(keys)
+	return weighted[keys[len(keys)-1]], nil
+}
+
 // NewCompressionMiddleware initializes a compression middleware.
 // As per Section 3.5 of the HTTP/1.1 RFC, we support GZIP and Deflate as compression methods.
-// https://tools.ietf.org/html/rfc2616#section-3.5
+// https://tools.ietf.org/html/rfc2616#section-14.3
 func NewCompressionMiddleware(deflateLevel int, ignoreRoutes ...string) MiddlewareFunc {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			hdr := r.Header.Get(encoding.AcceptEncodingHeader)
-
-			if !isCompressionHeader(hdr) || ignore(ignoreRoutes, r.URL.String()) {
+			if ignore(ignoreRoutes, r.URL.String()) {
 				next.ServeHTTP(w, r)
 				log.Debugf("url %s skipped from compression middleware", r.URL.String())
 				return
 			}
-			// explicitly specify encoding in header
-			w.Header().Set(encoding.ContentEncodingHeader, hdr)
 
-			// keep content type intact
-			respHeader := r.Header.Get(encoding.ContentTypeHeader)
-			if respHeader != "" {
-				w.Header().Set(encoding.ContentTypeHeader, respHeader)
+			hdr := r.Header.Get(encoding.AcceptEncodingHeader)
+			selectedEncoding, err := parseAcceptEncoding(hdr)
+			if err != nil {
+				log.Debugf("encoding %q is not supported in compression middleware, "+
+					"and client doesn't accept anything else", hdr)
+				http.Error(w, http.StatusText(http.StatusNotAcceptable), http.StatusNotAcceptable)
+				return
 			}
 
-			var cw io.WriteCloser
-			var err error
-			switch hdr {
+			var writer io.WriteCloser
+			switch selectedEncoding {
 			case gzipHeader:
-				cw = gzip.NewWriter(w)
+				writer = gzip.NewWriter(w)
+				w.Header().Set(encoding.ContentEncodingHeader, gzipHeader)
 			case deflateHeader:
-				cw, err = flate.NewWriter(w, deflateLevel)
+				writer, err = flate.NewWriter(w, deflateLevel)
 				if err != nil {
 					next.ServeHTTP(w, r)
 					return
 				}
+				w.Header().Set(encoding.ContentEncodingHeader, deflateHeader)
+			case identityHeader, "":
+				w.Header().Set(encoding.ContentEncodingHeader, identityHeader)
+				fallthrough
+			// `*`, `identity` and others must fall through here to be served without compression
 			default:
 				next.ServeHTTP(w, r)
 				return
@@ -196,9 +281,9 @@ func NewCompressionMiddleware(deflateLevel int, ignoreRoutes ...string) Middlewa
 				if err != nil {
 					log.Errorf("error in deferred call to Close() method on %v compression middleware : %v", hdr, err.Error())
 				}
-			}(cw)
+			}(writer)
 
-			crw := compressionResponseWriter{Writer: cw, ResponseWriter: w}
+			crw := compressionResponseWriter{Writer: writer, ResponseWriter: w}
 			next.ServeHTTP(crw, r)
 			log.Debugf("url %s used with %s compression method", r.URL.String(), hdr)
 		})
@@ -237,10 +322,6 @@ func MiddlewareChain(f http.Handler, mm ...MiddlewareFunc) http.Handler {
 	return f
 }
 
-func isCompressionHeader(h string) bool {
-	return strings.Contains(h, "gzip") || strings.Contains(h, "deflate")
-}
-
 func logRequestResponse(corID string, w *responseWriter, r *http.Request) {
 	if !log.Enabled(log.DebugLevel) {
 		return


Test patch:
diff --git a/component/http/middleware_test.go b/component/http/middleware_test.go
index ccedabf3a..70654e81c 100644
--- a/component/http/middleware_test.go
+++ b/component/http/middleware_test.go
@@ -2,6 +2,7 @@ package http
 
 import (
 	"errors"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -273,7 +274,7 @@ func TestNewCompressionMiddleware(t *testing.T) {
 }
 
 func TestNewCompressionMiddleware_Ignore(t *testing.T) {
-	var ceh, cth string // accept-encoding, content type
+	var ceh string // accept-encoding, content type
 
 	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(202) })
 	middleware := NewCompressionMiddleware(8, "/metrics")
@@ -293,15 +294,10 @@ func TestNewCompressionMiddleware_Ignore(t *testing.T) {
 	assert.NotNil(t, ceh)
 	assert.Equal(t, ceh, "")
 
-	cth = rc1.Header().Get("Content-Type")
-	assert.NotNil(t, cth)
-	assert.Equal(t, cth, "")
-
 	// check if other routes remains untouched
 	req2, err := http.NewRequest("GET", "/alive", nil)
 	assert.NoError(t, err)
 	req2.Header.Set("Accept-Encoding", "gzip")
-	req2.Header.Set("Content-Type", "application/json")
 
 	rc2 := httptest.NewRecorder()
 	middleware(handler).ServeHTTP(rc2, req2)
@@ -309,62 +305,247 @@ func TestNewCompressionMiddleware_Ignore(t *testing.T) {
 	ceh = rc2.Header().Get("Content-Encoding")
 	assert.NotNil(t, ceh)
 	assert.Equal(t, "gzip", ceh)
-
-	cth = rc2.Header().Get("Content-Type")
-	assert.NotNil(t, cth)
-	assert.Equal(t, "application/json", cth)
 }
 
 func TestNewCompressionMiddleware_Headers(t *testing.T) {
-	var ceh, cth string // accept-encoding, content type
-
-	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(202) })
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) })
+	middleware := NewCompressionMiddleware(8, "/metrics")
 
 	tests := map[string]struct {
-		cm MiddlewareFunc
+		cm               MiddlewareFunc
+		statusCode       int
+		encodingExpected string
 	}{
-		"gzip":    {cm: NewCompressionMiddleware(8, "/metrics")},
-		"deflate": {cm: NewCompressionMiddleware(8, "/metrics")},
+		"gzip":                {cm: middleware, statusCode: http.StatusOK, encodingExpected: gzipHeader},
+		"deflate":             {cm: middleware, statusCode: http.StatusOK, encodingExpected: deflateHeader},
+		"gzip, *":             {cm: middleware, statusCode: http.StatusOK, encodingExpected: gzipHeader},
+		"deflate, *":          {cm: middleware, statusCode: http.StatusOK, encodingExpected: deflateHeader},
+		"invalid, gzip, *":    {cm: middleware, statusCode: http.StatusOK, encodingExpected: gzipHeader},
+		"invalid, deflate, *": {cm: middleware, statusCode: http.StatusOK, encodingExpected: deflateHeader},
+		"invalid":             {cm: middleware, statusCode: http.StatusNotAcceptable, encodingExpected: ""},
+		"invalid, *":          {cm: middleware, statusCode: http.StatusOK, encodingExpected: ""},
+		"*":                   {cm: middleware, statusCode: http.StatusOK, encodingExpected: ""},
+		"":                    {cm: middleware, statusCode: http.StatusOK, encodingExpected: identityHeader},
+		"not present":         {cm: middleware, statusCode: http.StatusOK, encodingExpected: identityHeader},
 	}
 
-	for name, tc := range tests {
-		t.Run(name, func(t *testing.T) {
-			middleware := tc.cm
-			assert.NotNil(t, middleware)
-
-			// check if the route actually ignored
+	for encodingName, tc := range tests {
+		t.Run(fmt.Sprintf("%q: compression middleware acts according the Accept-Encoding header", encodingName), func(t *testing.T) {
+			require.NotNil(t, tc.cm)
+			// given
 			req1, err := http.NewRequest("GET", "/alive", nil)
-			assert.NoError(t, err)
-			req1.Header.Set("Accept-Encoding", name)
-			req1.Header.Set("Content-Type", "text/plain")
+			require.NoError(t, err)
+			if encodingName != "not present" {
+				req1.Header.Set("Accept-Encoding", encodingName)
+			}
 
+			// when
 			rc1 := httptest.NewRecorder()
-			middleware(handler).ServeHTTP(rc1, req1)
+			tc.cm(handler).ServeHTTP(rc1, req1)
 
-			ceh = rc1.Header().Get("Content-Encoding")
-			assert.NotNil(t, ceh)
-			assert.Equal(t, name, ceh)
+			// then
+			assert.Equal(t, tc.statusCode, rc1.Code)
 
-			cth = rc1.Header().Get("Content-Type")
-			assert.NotNil(t, cth)
-			assert.Equal(t, "text/plain", cth)
+			contentEncodingHeader := rc1.Header().Get("Content-Encoding")
+			assert.NotNil(t, contentEncodingHeader)
+			assert.Equal(t, tc.encodingExpected, contentEncodingHeader)
+		})
+	}
+}
 
-			// check if other routes remains untouched
-			req2, err := http.NewRequest("GET", "/alive", nil)
-			assert.NoError(t, err)
-			req2.Header.Set("Accept-Encoding", name)
-			req2.Header.Set("Content-Type", "application/json")
+func TestSelectEncoding(t *testing.T) {
+	tests := []struct {
+		optionalName string
+		given        string
+		expected     string
+		isErr        bool
+	}{
+		{given: "", expected: "identity", optionalName: "is empty but present, only identity"},
+
+		{given: "*", expected: "*"},
+		{given: "gzip", expected: "gzip"},
+		{given: "deflate", expected: "deflate"},
+
+		{given: "whatever", expected: "", isErr: true, optionalName: "whatever, not supported"},
+		{given: "whatever, *", expected: "*", optionalName: "whatever, but also a star"},
+
+		{given: "gzip, deflate", expected: "gzip"},
+		{given: "whatever, gzip, deflate", expected: "gzip"},
+		{given: "gzip, whatever, deflate", expected: "gzip"},
+		{given: "gzip, deflate, whatever", expected: "gzip"},
+
+		{given: "gzip,deflate", expected: "gzip"},
+		{given: "gzip,whatever,deflate", expected: "gzip"},
+		{given: "whatever,gzip,deflate", expected: "gzip"},
+		{given: "gzip,deflate,whatever", expected: "gzip"},
+
+		{given: "deflate, gzip", expected: "deflate"},
+		{given: "whatever, deflate, gzip", expected: "deflate"},
+		{given: "deflate, whatever, gzip", expected: "deflate"},
+		{given: "deflate, gzip, whatever", expected: "deflate"},
+
+		{given: "deflate, gzip", expected: "deflate"},
+		{given: "whatever,deflate,gzip", expected: "deflate"},
+		{given: "deflate,whatever,gzip", expected: "deflate"},
+		{given: "deflate,gzip,whatever", expected: "deflate"},
+
+		{given: "gzip;q=1.0, deflate;q=1.0", expected: "gzip", optionalName: "equal weights"},
+		{given: "deflate;q=1.0, gzip;q=1.0", expected: "deflate", optionalName: "equal weights 2"},
+
+		{given: "gzip;q=1.0, deflate;q=0.5", expected: "gzip"},
+		{given: "gzip;q=1.0, deflate;q=0.5, *;q=0.2", expected: "gzip"},
+		{given: "deflate;q=1.0, gzip;q=0.5", expected: "deflate"},
+		{given: "deflate;q=1.0, gzip;q=0.5, *;q=0.2", expected: "deflate"},
+
+		{given: "gzip;q=0.5, deflate;q=1.0", expected: "deflate"},
+		{given: "gzip;q=0.5, deflate;q=1.0, *;q=0.2", expected: "deflate"},
+		{given: "deflate;q=0.5, gzip;q=1.0", expected: "gzip"},
+		{given: "deflate;q=0.5, gzip;q=1.0, *;q=0.2", expected: "gzip"},
+
+		{given: "whatever;q=1.0, *;q=0.2", expected: "*"},
+
+		{given: "deflate, gzip;q=1.0", expected: "deflate"},
+		{given: "deflate, gzip;q=0.5", expected: "deflate"},
+
+		{given: "deflate;q=0.5, gzip", expected: "gzip"},
+
+		{given: "deflate;q=0.5, gzip;q=-0.5", expected: "deflate"},
+		{given: "deflate;q=0.5, gzip;q=1.5", expected: "gzip"},
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("encoding %q is parsed as %s ; error is expected: %t ; %s", tc.given, tc.expected, tc.isErr, tc.optionalName), func(t *testing.T) {
+			// when
+			result, err := parseAcceptEncoding(tc.given)
+
+			// then
+			assert.Equal(t, tc.isErr, err != nil)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
 
-			rc2 := httptest.NewRecorder()
-			middleware(handler).ServeHTTP(rc2, req2)
+func TestSupported(t *testing.T) {
+	tests := []struct {
+		algorithm   string
+		isSupported bool
+	}{
+		{algorithm: "gzip", isSupported: true},
+		{algorithm: "deflate", isSupported: true},
+		{algorithm: "*", isSupported: true},
+		{algorithm: "something else", isSupported: false},
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("%q check results in %t", tc.algorithm, tc.isSupported), func(t *testing.T) {
+			// when
+			result := !notSupportedCompression(tc.algorithm)
+
+			// then
+			assert.Equal(t, result, tc.isSupported)
+		})
+	}
+}
+
+func TestParseWeights(t *testing.T) {
+	tests := []struct {
+		priorityStr string
+		expected    float64
+	}{
+		{priorityStr: "q=1.0", expected: 1.0},
+		{priorityStr: "q=0.5", expected: 0.5},
+		{priorityStr: "q=-0.5", expected: 0.0},
+		{priorityStr: "q=1.5", expected: 1.0},
+		{priorityStr: "q=", expected: 1.0},
+		{priorityStr: "", expected: 1.0},
+	}
+
+	for _, tc := range tests {
+		t.Run(fmt.Sprintf("for given priority: %q, expect %f", tc.priorityStr, tc.expected), func(t *testing.T) {
+			// when
+			result := parseWeight(tc.priorityStr)
+
+			// then
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}
+
+func TestSelectByWeight(t *testing.T) {
+	tests := []struct {
+		name     string
+		given    map[float64]string
+		expected string
+		isErr    bool
+	}{
+		{
+			name:     "sorted map",
+			given:    map[float64]string{1.0: "gzip", 0.5: "deflate"},
+			expected: "gzip",
+		},
+		{
+			name:     "not sorted map",
+			given:    map[float64]string{0.5: "gzip", 1.0: "deflate"},
+			expected: "deflate",
+		},
+		{
+			name:     "empty weights map",
+			given:    map[float64]string{},
+			expected: "",
+			isErr:    true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// when
+			selected, err := selectByWeight(tc.given)
+
+			// then
+			assert.Equal(t, tc.isErr, err != nil)
+			assert.Equal(t, tc.expected, selected)
+		})
+	}
+}
+
+func TestAddWithWeight(t *testing.T) {
+	tests := []struct {
+		name        string
+		weightedMap map[float64]string
+		weight      float64
+		algorithm   string
+		expected    map[float64]string
+	}{
+		{
+			name:        "empty",
+			weightedMap: map[float64]string{},
+			weight:      1.0,
+			algorithm:   "gzip",
+			expected:    map[float64]string{1.0: "gzip"},
+		},
+		{
+			name:        "new",
+			weightedMap: map[float64]string{1.0: "gzip"},
+			weight:      0.5,
+			algorithm:   "deflate",
+			expected:    map[float64]string{1.0: "gzip", 0.5: "deflate"},
+		},
+		{
+			name:        "already exists",
+			weightedMap: map[float64]string{1.0: "gzip"},
+			weight:      1.0,
+			algorithm:   "deflate",
+			expected:    map[float64]string{1.0: "gzip"},
+		},
+	}
 
-			ceh = rc2.Header().Get("Content-Encoding")
-			assert.NotNil(t, ceh)
-			assert.Equal(t, name, ceh)
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// when
+			addWithWeight(tc.weightedMap, tc.weight, tc.algorithm)
 
-			cth = rc2.Header().Get("Content-Type")
-			assert.NotNil(t, cth)
-			assert.Equal(t, "application/json", cth)
+			// then
+			assert.Equal(t, tc.expected, tc.weightedMap)
 		})
 	}
 }
diff --git a/component/http/route_cache_test.go b/component/http/route_cache_test.go
index 447a8d39c..91426568a 100644
--- a/component/http/route_cache_test.go
+++ b/component/http/route_cache_test.go
@@ -445,7 +445,7 @@ func assertResponse(ctx context.Context, t *testing.T, expected []http.Response,
 	cl, err := httpclient.New()
 	assert.NoError(t, err)
 	req, err := http.NewRequest("GET", fmt.Sprintf("http://localhost:%d/path", port), nil)
-	req.Header.Set(encoding.AcceptEncodingHeader, "Non-GZIP") // bypass default HTTP client GZIP-ing our response
+	req.Header.Set(encoding.AcceptEncodingHeader, "*") // bypass default HTTP client GZIP-ing our response
 	assert.NoError(t, err)
 
 	for _, expectedResponse := range expected {