net/http/httptest: Explicitly discourage using ResponseRecorder.HeaderMap

[cespare]

(This is basically #16717 again.)

Quick background: if you write a header after the body has been written and status has been set in an HTTP handler, that's a bug because it has no effect. If you're testing using an httptest.ResponseRecorder, its HeaderMap will record the header and might make you think that your code is working when it's not. This is #8857. In response to that bug the behavior was changed to snapshot the headers instead, but that caused another problem (#15560) so it was reverted.

Instead, you're now supposed to use Result to get an http.Response and then look at resp.Header.

The documentation is not clear enough about this. I previously filed #16717 and 8e4ea2f added the following documentation to ResponseRecorder.HeaderMap:

// HeaderMap contains the headers explicitly set by the Handler.
//
// To get the implicit headers set by the server (such as
// automatic Content-Type), use the Result method.
HeaderMap http.Header

I think we need to be clearer and explicitly discourage looking at HeaderMap.

I've recently fixed a production bug where headers weren't being set and the tests specifically looked for them using HeaderMap. Then I found a whole bunch more places across many code bases where tests looked for headers to be set in HeaderMap. None of those would have caught headers written after Write.

/cc @bradfitz

[bradfitz]

What do you propose?

[cespare]

I'd like to just deprecate any usage of that field and say that it shouldn't be accessed. But the previous discussions make me think that there's some still-reasonable usage of HeaderMap that I don't understand?

[cespare]

I also filed #25764 for changing the API for Go 2.

[bradfitz]

Okay, sounds like this isn't concrete enough for Go 1.11 at least.

[cespare]

The way I see it, ResponseRecorder has a footgun first reported 4 years ago via #8857, and although an alternative to the footgun was provided 2 years ago (Result) there is still nothing in the documentation explicitly stating that it's a footgun. (It's obliquely alluded to in the Result doc comment but why would you even look at that once you've found the headers you need in HeaderMap?)

To be concrete, I propose replacing the HeaderMap field documentation with:

// Deprecated: HeaderMap exists for historical compatibility and should not be used.

[bradfitz]

I'd be fine with:

// HeaderMap is the map returned by the Header method. It is an internal detail.
//
// Deprecated: HeaderMap exists for historical compatibility and should not be used.
// To access the headers returned by a handler, use the Response.Header map as returned
// by the Result method.
HeaderMap http.Header

Want to send a change?

[cespare]

Yeah, I'll do that. Thanks.

[gopherbot]

Change https://golang.org/cl/117675 mentions this issue: net/http/httptest: deprecate ResponseRecorder.HeaderMap