Repository: beatlabs/patron
Pull request #207 (2020-05-12 17:41:40)

Add support for determining encoding from multi value headers


## Which problem is this PR solving?
Some applications (kubernetes apiserver in my case) might send headers like ``application/json, */*`` which are legit according to RFC 26116 section 4.2 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2).

The current version doesn't take that into account and is returning an error in the` determineEncoding` [function](https://github.com/beatlabs/patron/blob/master/component/http/handler.go#L53) instead of the json which would be expected in the above example.

## Short description of the changes
Change `determineEncoding` [function](https://github.com/beatlabs/patron/blob/master/component/http/handler.go#L53) to parse headers and compare all available values of the header to the needed values.
<!-- REQUIRED -->

Addresses https://github.com/beatlabs/patron/issues/215

codecov[bot]: # [Codecov](https://codecov.io/gh/beatlabs/patron/pull/207?src=pr&el=h1) Report
> Merging [#207](https://codecov.io/gh/beatlabs/patron/pull/207?src=pr&el=desc) into [master](https://codecov.io/gh/beatlabs/patron/commit/375084b0e6deed91f444975c248bcbefaad2c2e0&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `91.91%`.

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

```diff
@@            Coverage Diff             @@
##           master     #207      +/-   ##
==========================================
+ Coverage   84.48%   84.53%   +0.04%     
==========================================
  Files          52       52              
  Lines        3074     3116      +42     
==========================================
+ Hits         2597     2634      +37     
- Misses        398      401       +3     
- Partials       79       81       +2     
```


| [Impacted Files](https://codecov.io/gh/beatlabs/patron/pull/207?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [client/amqp/amqp.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L2FtcXAvYW1xcC5nbw==) | `40.98% <ø> (ø)` | |
| [client/amqp/option.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L2FtcXAvb3B0aW9uLmdv) | `66.66% <ø> (ø)` | |
| [client/amqp/user.pb.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L2FtcXAvdXNlci5wYi5nbw==) | `26.92% <ø> (ø)` | |
| [client/http/option.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L2h0dHAvb3B0aW9uLmdv) | `100.00% <ø> (ø)` | |
| [client/redis/redis.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L3JlZGlzL3JlZGlzLmdv) | `44.44% <ø> (ø)` | |
| [client/sns/message.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L3Nucy9tZXNzYWdlLmdv) | `97.53% <ø> (ø)` | |
| [client/sns/publisher.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L3Nucy9wdWJsaXNoZXIuZ28=) | `93.33% <ø> (ø)` | |
| [client/sql/sql.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L3NxbC9zcWwuZ28=) | `86.25% <ø> (ø)` | |
| [client/sqs/message.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L3Nxcy9tZXNzYWdlLmdv) | `92.50% <ø> (ø)` | |
| [client/sqs/publisher.go](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree#diff-Y2xpZW50L3Nxcy9wdWJsaXNoZXIuZ28=) | `93.33% <ø> (ø)` | |
| ... and [62 more](https://codecov.io/gh/beatlabs/patron/pull/207/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/beatlabs/patron/pull/207?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/207?src=pr&el=footer). Last update [3e49af8...665e93f](https://codecov.io/gh/beatlabs/patron/pull/207?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

astrikos: >I am just thinking if we should fail in the case where we have contradicting headers amongst the multi values

Yeah maybe for content headers would make sense since I guess it can send either xml or json.
But for the accept header it could be both, since that would mean that the server can accept both, which is valid, right?

I could write more logic for the content header but I don't know if it's worth spending so much time on checking the headers so thoroughly.
It's better to be liberal in what you accept according to [Postel's law](https://en.wikipedia.org/wiki/Robustness_principle) :) but I am okay adding more logic if you guys think that's the best here.
astrikos: I see tests now are broken on kafka tests, which are unrelated to my change :thinking: , I'll try to get the latest master and see if that fixes it.

Also new issue created and linked in here https://github.com/beatlabs/patron/issues/215
mantzas: > I see tests now are broken on kafka tests, which are unrelated to my change thinking , I'll try to get the latest master and see if that fixes it.
> 
> Also new issue created and linked in here #215

Yes, it is already known and I am already working on it in #212 so no need for a new issue.
astrikos: @mantzas @drakos74 rebased from master, tests looks good locally and on ci, let me know if you need anything more before we can merge this one.

Patch:
diff --git a/component/http/handler.go b/component/http/handler.go
index a6d62b95a..fcffe67f9 100644
--- a/component/http/handler.go
+++ b/component/http/handler.go
@@ -83,27 +83,63 @@ func determineEncoding(h http.Header) (string, encoding.DecodeFunc, encoding.Enc
 	}
 
 	if aok {
-		switch ach[0] {
-		case "*/*", json.Type, json.TypeCharset:
-			enc = json.Encode
-			if dec == nil {
-				dec = json.Decode
+		var err error
+		encFound := false
+
+		ah := getMultiValueHeaders(ach[0])
+		for _, v := range ah {
+			ct, dec, enc, err = getSingleHeaderEncoding(v)
+			if err == nil {
+				encFound = true
+				break
 			}
-			ct = json.TypeCharset
-		case protobuf.Type, protobuf.TypeGoogle:
-			enc = protobuf.Encode
-			if dec == nil {
-				dec = protobuf.Decode
-			}
-			ct = protobuf.Type
-		default:
-			return "", nil, nil, errors.New("accept Header not supported")
 		}
+		// No valid headers found after going through all headers
+		if !encFound {
+			return "", nil, nil, errors.New("accept header not supported")
+		}
+	}
+
+	return ct, dec, enc, nil
+}
+
+func getSingleHeaderEncoding(header string) (string, encoding.DecodeFunc, encoding.EncodeFunc, error) {
+
+	var enc encoding.EncodeFunc
+	var dec encoding.DecodeFunc
+	var ct string
+
+	switch header {
+	case "*/*", json.Type, json.TypeCharset:
+		enc = json.Encode
+		dec = json.Decode
+		ct = json.TypeCharset
+	case protobuf.Type, protobuf.TypeGoogle:
+		enc = protobuf.Encode
+		dec = protobuf.Decode
+		ct = protobuf.Type
+	default:
+		return "", nil, nil, errors.New("accept header not supported")
 	}
 
 	return ct, dec, enc, nil
 }
 
+func getMultiValueHeaders(header string) []string {
+	if !strings.Contains(header, ",") {
+		return []string{header}
+	}
+
+	splitHeaders := strings.Split(header, ",")
+
+	trimmedHeaders := []string{}
+	for _, v := range splitHeaders {
+		trimmedHeaders = append(trimmedHeaders, strings.TrimSpace(v))
+	}
+
+	return trimmedHeaders
+}
+
 func extractFields(r *http.Request) map[string]string {
 	f := make(map[string]string)
 


Test patch:
diff --git a/component/http/handler_test.go b/component/http/handler_test.go
index 39f4bc7c6..0580fa238 100644
--- a/component/http/handler_test.go
+++ b/component/http/handler_test.go
@@ -64,6 +64,7 @@ func Test_determineEncoding(t *testing.T) {
 		{"missing headers, defaults json", args{req: request(t, "", "")}, json.Decode, json.Encode, json.TypeCharset, false},
 		{"accept */*, defaults to json", args{req: request(t, json.TypeCharset, "*/*")}, json.Decode, json.Encode, json.TypeCharset, false},
 		{"wrong content", args{req: request(t, "application/xml", json.TypeCharset)}, nil, nil, json.TypeCharset, true},
+		{"multi-value accept", args{req: request(t, json.TypeCharset, "application/json, */*")}, json.Decode, json.Encode, json.TypeCharset, false},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -83,6 +84,28 @@ func Test_determineEncoding(t *testing.T) {
 	}
 }
 
+func Test_getMultiValueHeaders(t *testing.T) {
+	tests := []struct {
+		name            string
+		headers         string
+		expectedHeaders []string
+	}{
+		{"empty string", "", []string{""}},
+		{"single header", "*/*", []string{"*/*"}},
+		{"comma separated multi(2) header with space", "application/json, */*", []string{"application/json", "*/*"}},
+		{"comma separated multi(2) header with multiple spaces", " application/json, */* ", []string{"application/json", "*/*"}},
+		{"comma separated multi(2) header with no space", "application/json,*/*", []string{"application/json", "*/*"}},
+		{"comma separated multi(2) header", "application/json,*/*", []string{"application/json", "*/*"}},
+		{"comma separated multi(3) header", "application/json,*/*,application/xml", []string{"application/json", "*/*", "application/xml"}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			newHeaders := getMultiValueHeaders(tt.headers)
+			assert.Equal(t, tt.expectedHeaders, newHeaders)
+		})
+	}
+}
+
 func request(t *testing.T, contentType, accept string) *http.Request {
 	req, err := http.NewRequest(http.MethodGet, "/", nil)
 	require.NoError(t, err)