Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
+31 −4
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion httpbin/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for k, vs := range args {
for _, v := range vs {
w.Header().Add(k, v)
}
if len(vs) == 1 {
body[k] = vs[0]
} else {
body[k] = vs
}
Comment on lines +322 to +326
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if contentType := w.Header().Get("Content-Type"); contentType == "" {
w.Header().Set("Content-Type", jsonContentType)
}
mustMarshalJSON(w, args)
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)
Comment on lines +331 to +336
Copy link
Owner

@mccutchen mccutchen Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func redirectLocation(r *http.Request, relative bool, n int) string {
Expand Down
22 changes: 19 additions & 3 deletions httpbin/handlers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,16 +1163,32 @@ func TestResponseHeaders(t *testing.T) {

req, _ := http.NewRequest("GET", fmt.Sprintf("%s/response-headers?%s", srv.URL, wantHeaders.Encode()), nil)
resp := must.DoReq(t, client, req)
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")
Comment on lines -1166 to +1177
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


for k, expectedValues := range wantHeaders {
// expected headers should be present in the HTTP response itself
respValues := resp.Header[k]
assert.DeepEqual(t, respValues, expectedValues, "HTTP response headers mismatch")

// they should also be reflected in the decoded JSON resposne
resultValues := result[k]
assert.DeepEqual(t, resultValues, expectedValues, "JSON response headers mismatch")
if len(expectedValues) == 1 {
value := reflect.ValueOf(result).FieldByName(k).String()
assert.Equal[string](t, value, expectedValues[0], "JSON response headers mismatch")
} else {
value := reflect.ValueOf(result).FieldByName(k).Interface().([]string)
assert.DeepEqual(t, value, expectedValues, "JSON response headers mismatch")
}
}
})

Expand Down
Loading