fix: API response-headers compatibility with origin #131

[sonbui00]

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"
}

[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]

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]

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

[mccutchen]

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]

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

[mccutchen]

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

[codecov]

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]

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]

@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]

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.)