code review 4239076: http: change ResponseWriter.SetHeader(k,v) to Header() ...

http: change ResponseWriter.SetHeader(k,v) to Header() accessor

Caller code needs to change:

rw.SetHeader("Content-Type", "text/plain")
to:
rw.Header().Set("Content-Type", "text/plain")

This now permits returning multiple headers
with the same name using Add:

rw.Header().Add("Set-Cookie", "..")
rw.Header().Add("Set-Cookie", "..")

This patch also fixes serialization of headers, removing newline characters.

Fixes issue 488
Fixes Issue 914

[bradfitz, 2011-03-09 16:50:02]

Hello rsc (cc: gburd, golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-03-09 16:57:34]

Hello rsc (cc: gburd, golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-03-09 17:09:27]

LGTM

Assuming that it is responsewrite_test.go that is wrong.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/host.go
File src/pkg/http/cgi/host.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/host.go#newc...
src/pkg/http/cgi/host.go:171: case header == "Status":
not for this CL but i am a little surprised that this doesn't have
a special case for Location:.  i thought that cgi programs could
print location without printing a status.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/matryoshka_t...
File src/pkg/http/cgi/matryoshka_test.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/matryoshka_t...
src/pkg/http/cgi/matryoshka_test.go:1: // Copyright 2011 The Go Authors. All
rights reserved.
I only just noticed the file name.

:-)

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/httptest/recorde...
File src/pkg/http/httptest/recorder.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/httptest/recorde...
src/pkg/http/httptest/recorder.go:21: header         http.Header
it would be nice to keep this all exported fields.
HeaderMap maybe?

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/responsewrite_te...
File src/pkg/http/responsewrite_test.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/responsewrite_te...
src/pkg/http/responsewrite_test.go:88: "Foo: Bar\n" +
why is the \n still here?

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go#newcode57
src/pkg/http/server.go:57: // Modifications to Headers after WriteHeader (or
Write)
// Header returns the header map that will be sent by WriteHeader.
// Changing the header after a call to WriteHeader (or Write) has
// no effect.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:188: // Header returns the response headers to be set.
Modifications
// Header implements the ResponseWriter.Header method.

godoc doesn't show this comment so there's no point
in saying much more.  i wouldn't mind if you deleted
all the comments of that form but it's okay to leave them too.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:307: v = strings.Replace(v, "\n", " ", -1)
I like this (no wrapped lines).  But it does not appear to be what the test
tests.

[bradfitz, 2011-03-09 17:36:02]

Hello rsc (cc: gburd, golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-03-09 17:37:53]

I also removed some duplicate code and now call writeSortedHeader() which is
where I actually needed the header clean-up code.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/host.go
File src/pkg/http/cgi/host.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/host.go#newc...
src/pkg/http/cgi/host.go:171: case header == "Status":
On 2011/03/09 17:09:27, rsc wrote:
> not for this CL but i am a little surprised that this doesn't have
> a special case for Location:.  i thought that cgi programs could
> print location without printing a status.

filed http://code.google.com/p/go/issues/detail?id=1597

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/matryoshka_t...
File src/pkg/http/cgi/matryoshka_test.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/cgi/matryoshka_t...
src/pkg/http/cgi/matryoshka_test.go:1: // Copyright 2011 The Go Authors. All
rights reserved.
On 2011/03/09 17:09:27, rsc wrote:
> I only just noticed the file name.
> 

I wasn't sure if my first choice, yo_dawg_test.go would fly.  I also debated
turducken_test.go

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/httptest/recorde...
File src/pkg/http/httptest/recorder.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/httptest/recorde...
src/pkg/http/httptest/recorder.go:21: header         http.Header
On 2011/03/09 17:09:27, rsc wrote:
> it would be nice to keep this all exported fields.
> HeaderMap maybe?

Yeah, wasn't sure what to do there.  HeaderMap SGTM

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/responsewrite_te...
File src/pkg/http/responsewrite_test.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/responsewrite_te...
src/pkg/http/responsewrite_test.go:88: "Foo: Bar\n" +
On 2011/03/09 17:09:27, rsc wrote:
> why is the \n still here?

Whoops, all.bash was still running when I got off the shuttle.

I'd moved the patch over but hadn't updated it to the new logic yet.  Will fix
the test.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go
File src/pkg/http/server.go (right):

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go#newcode57
src/pkg/http/server.go:57: // Modifications to Headers after WriteHeader (or
Write)
On 2011/03/09 17:09:27, rsc wrote:
> // Header returns the header map that will be sent by WriteHeader.
> // Changing the header after a call to WriteHeader (or Write) has
> // no effect.

Done.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:188: // Header returns the response headers to be set.
Modifications
On 2011/03/09 17:09:27, rsc wrote:
> // Header implements the ResponseWriter.Header method.
> 
> godoc doesn't show this comment so there's no point
> in saying much more.  i wouldn't mind if you deleted
> all the comments of that form but it's okay to leave them too.

Deleted all but the Hijack one, which actually had value in explaining that we
implemented both interfaces.

http://codereview.appspot.com/4239076/diff/6001/src/pkg/http/server.go#newcod...
src/pkg/http/server.go:307: v = strings.Replace(v, "\n", " ", -1)
On 2011/03/09 17:09:27, rsc wrote:
> I like this (no wrapped lines).  But it does not appear to be what the test
> tests.

Fixed the test.

[bradfitz, 2011-03-09 17:41:10]

*** Submitted as http://code.google.com/p/go/source/detail?r=9da4e71e8f1b ***

http: change ResponseWriter.SetHeader(k,v) to Header() accessor

Caller code needs to change:

rw.SetHeader("Content-Type", "text/plain")
to:
rw.Header().Set("Content-Type", "text/plain")

This now permits returning multiple headers
with the same name using Add:

rw.Header().Add("Set-Cookie", "..")
rw.Header().Add("Set-Cookie", "..")

This patch also fixes serialization of headers, removing newline characters.

Fixes issue 488
Fixes Issue 914

R=rsc
CC=gburd, golang-dev
http://codereview.appspot.com/4239076