fix: API response-headers compatibility with origin #131 #142

sonbui00 · 2023-08-07T04:43:29Z

API response-headers compatibility with the origin
Fix: #141

curl  "http://127.0.0.1:8080/response-headers?Bar=bar1&Bar=bar2&Foo=foo"                   
{
  "Bar": [
    "bar1",
    "bar2"
  ],
  "Content-Length": 104,
  "Content-Type": "application/json; charset=utf-8",
  "Foo": "foo"
}

curl "https://httpbin.org/response-headers?Bar=bar1&Bar=bar2&Foo=foo"
{
  "Bar": [
    "bar1", 
    "bar2"
  ], 
  "Content-Length": "127", 
  "Content-Type": "application/json", 
  "Foo": "foo"
}

Signed-off-by: Son Bui <sonbv00@gmail.com>

mccutchen

Thanks for the contribution! I've left a few comments below that will help simplify these changes, please let me know if they make sense to you.

Also, have you explored what the original httpbin's behavior is in weird circumstances? Like, what happens with a request like /response-headers?Content-Length=999999999? I think we need some test coverage for at least that edge case, to codify whatever behavior we decide on.

mccutchen · 2023-08-07T22:29:15Z

httpbin/handlers.go

+		if len(vs) == 1 {
+			body[k] = vs[0]
+		} else {
+			body[k] = vs
+		}


Please drop this logic. Per my comment here

But, to be clear, go-httpbin will continue to serialize header values (and form values and any other “zero or more” values inherent to HTTP) as JSON arrays for consistency, where original httpbin serializes them as single scalar values, arrays, or comma-separated strings, depending on the endpoint and incoming request parameters.

That might not be very clear. To restate it, what I mean is that go-httpbin returns all header values as JSON arrays, even if only one header is given.

I know that is not actually backwards-compatible with the original httpbin, but for now I've decided that I prefer the consistency (and implementation simplicity) of this approach and I'm okay with the incompatibility. (See #37 and #15 for related discussion.)

mccutchen · 2023-08-07T22:29:55Z

httpbin/handlers.go

@@ -314,15 +314,26 @@ func (h *HTTPBin) Unstable(w http.ResponseWriter, r *http.Request) {
 // ResponseHeaders responds with a map of header values
 func (h *HTTPBin) ResponseHeaders(w http.ResponseWriter, r *http.Request) {
 	args := r.URL.Query()
+	body := make(map[string]interface{})


With my comment below, I don't think we need anything more than args here.

mccutchen · 2023-08-07T22:32:16Z

httpbin/handlers.go

+	body["Content-Type"] = w.Header().Get("Content-Type")
+	body["Content-Length"] = 0
+	buf := &bytes.Buffer{}
+	json.NewEncoder(buf).Encode(body)
+	body["Content-Length"] = buf.Len()
+	mustMarshalJSON(w, body)


This is a bit of a strange approach, double-encoding the body just to get the content length. Please rewrite it to do the encoding once. The existing mustMarshalJSON() and writeResponse() helper funcs might be useful here.

mccutchen · 2023-08-07T22:33:14Z

httpbin/handlers_test.go

-		result := mustParseResponse[http.Header](t, resp)
+
+		type bodyResponse struct {
+			Foo           string   `json:"Foo"`
+			Bar           []string `json:"Bar"`
+			ContentType   string   `json:"Content-Type"`
+			ContentLength int      `json:"Content-Length"`
+		}
+
+		result := mustParseResponse[bodyResponse](t, resp)
+
+		assert.ContentType(t, resp, result.ContentType)
+		assert.Equal[bool](t, result.ContentLength > 0, true, "JSON response Content-Length mismatch")


I think we can do away with most of this specialization if we simplify the response as requested above.

mccutchen · 2023-08-07T22:37:17Z

(Also, I apologize in advance for slow replies on my part, I am traveling right now.)

codecov · 2023-08-07T22:38:25Z

Codecov Report

Merging #142 (eae4786) into main (62ec555) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   98.04%   98.05%   +0.01%     
==========================================
  Files           8        8              
  Lines        1585     1596      +11     
==========================================
+ Hits         1554     1565      +11     
  Misses         22       22              
  Partials        9        9

Files Changed	Coverage Δ
httpbin/handlers.go	`99.46% <100.00%> (+<0.01%)`	⬆️

mccutchen · 2023-08-07T22:44:03Z

Also, have you explored what the original httpbin's behavior is in weird circumstances? Like, what happens with a request like /response-headers?Content-Length=999999999? I think we need some test coverage for at least that edge case, to codify whatever behavior we decide on.

The behavior is interesting (TLS-related noise elided below):

$ curl -v --http1.1 'https://httpbin.org/response-headers?Content-Type=text/plain&Content-Length=99999'
*   Trying 54.210.149.139:443...
* Connected to httpbin.org (54.210.149.139) port 443 (#0)
> GET /response-headers?Content-Type=text/plain&Content-Length=99999 HTTP/1.1
> Host: httpbin.org
> User-Agent: curl/8.1.2
> Accept: */*
> 
< HTTP/1.1 200 OK
< Date: Mon, 07 Aug 2023 22:40:24 GMT
< Content-Type: text/plain
< Content-Length: 99999
< Connection: keep-alive
< Server: gunicorn/19.9.0
< Access-Control-Allow-Origin: *
< Access-Control-Allow-Credentials: true
< 
{
  "Content-Length": [
    "122", 
    "99999"
  ], 
  "Content-Type": [
    "application/json", 
    "text/plain"
  ]
}
* transfer closed with 99877 bytes remaining to read
* Closing connection 0
curl: (18) transfer closed with 99877 bytes remaining to read

I can convince myself that letting the incoming request to this specific endpoint dictate the actual Content-Length of the response could be useful for testing weird behavior in HTTP client or proxy code, so I'd be in favor of matching the Python implementation there.

sonbui00 · 2023-08-08T13:04:11Z

@mccutchen How can I get just key - value?
Another way for this, use [] after key for list
Ex:

/response-headers?Bar[]=bar1&Bar[]=bar2&Foo=foo
{
  "Bar": [
    "bar1",
    "bar2"
  ],
  "Foo": "foo"
}

I know many frameworks use this way.

mccutchen · 2023-08-09T22:09:00Z

How can I get just key - value?

I'm sorry, but I don't understand what you mean. Can you provide a bit more context, or specific examples of what you'd like to see?

At the risk of over-explaining: The HTTP protocol allows for both multiple headers and multiple URL query parameters with the same name. This may be relatively rare, but it is entirely normal and compliant. For go-httpbin — unlike the original httpbin — we do not special-case the way we serialize single headers vs multiple headers or single query params vs multiple query params with a given name. They are always serialized in go-httpbin's responses as arrays (single values as a 1-element array, multiple values as multiple-element arrays).

Another way for this, use [] after key for list Ex:

...

I know many frameworks use this way.

That is not part of the HTTP spec. It is framework specific and entirely unnecessary, given a spec-compliant HTTP implementations. I've never researched the history here, but I have always assumed that some frameworks made this implementation decision out of ignorance of the HTTP spec, but maybe there are historical reasons that necessitated this that I'm not aware of.

Regardless, go-httpbin is not going to implement any special cases like this.

(See #37 (comment) for prior discussion of this key[]=1&key[]=2 idea.)

fix: API response-headers compatibility with origin mccutchen#131

eae4786

Signed-off-by: Son Bui <sonbv00@gmail.com>

mccutchen reviewed Aug 7, 2023

View reviewed changes

sonbui00 closed this Aug 11, 2023

mccutchen mentioned this pull request Aug 15, 2023

compat: /response-headers endpoint missing headers #141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: API response-headers compatibility with origin #131 #142

fix: API response-headers compatibility with origin #131 #142

sonbui00 commented Aug 7, 2023 •

edited

mccutchen left a comment

mccutchen Aug 7, 2023

mccutchen Aug 7, 2023

mccutchen Aug 7, 2023 •

edited

mccutchen Aug 7, 2023

mccutchen commented Aug 7, 2023

codecov bot commented Aug 7, 2023

mccutchen commented Aug 7, 2023

sonbui00 commented Aug 8, 2023

mccutchen commented Aug 9, 2023

fix: API response-headers compatibility with origin #131 #142

fix: API response-headers compatibility with origin #131 #142

Conversation

sonbui00 commented Aug 7, 2023 • edited

mccutchen left a comment

Choose a reason for hiding this comment

mccutchen Aug 7, 2023

Choose a reason for hiding this comment

mccutchen Aug 7, 2023

Choose a reason for hiding this comment

mccutchen Aug 7, 2023 • edited

Choose a reason for hiding this comment

mccutchen Aug 7, 2023

Choose a reason for hiding this comment

mccutchen commented Aug 7, 2023

codecov bot commented Aug 7, 2023

Codecov Report

mccutchen commented Aug 7, 2023

sonbui00 commented Aug 8, 2023

mccutchen commented Aug 9, 2023

sonbui00 commented Aug 7, 2023 •

edited

mccutchen Aug 7, 2023 •

edited