code review 4620049: http: make Headers be source of truth

http: make Headers be source of truth

Previously Request and Response had redundant fields for
Referer, UserAgent, and cookies which caused confusion and
bugs.  It also didn't allow us to expand the package over
time, since the way to access fields would be in the Headers
one day and promoted to a field the next day.  That would be
hard to gofix, especially with code ranging over Headers.

After a discussion on the mail package's design with a similar
problem, we've designed to make the Headers be the source of
truth and add accessors instead.

Request:
change: Referer -> Referer()
change: UserAgent -> UserAgent()
change: Cookie -> Cookies()
new: Cookie(name) *Cookie
new: AddCookie(*Cookie)

Response:
change: Cookie -> Cookies()

Cookie:
new: String() string

[bradfitz, 2011-06-15 20:14:47]

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

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

[bradfitz, 2011-06-15 20:17:49]

I'll add a gofix to this CL if/once the rest is cool.

On Wed, Jun 15, 2011 at 1:14 PM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> http: make Headers be source of truth
>
> Previously Request and Response had redundant fields for
> Referer, UserAgent, and cookies which caused confusion and
> bugs.  It also didn't allow us to expand the package over
> time, since the way to access fields would be in the Headers
> one day and promoted to a field the next day.  That would be
> hard to gofix, especially with code ranging over Headers.
>
> After a discussion on the mail package's design with a similar
> problem, we've designed to make the Headers be the source of
> truth and add accessors instead.
>
> Request:
> change: Referer -> Referer()
> change: UserAgent -> UserAgent()
> change: Cookie -> Cookies()
> new: Cookie(name) *Cookie
>
> Response:
> change: SetCookie -> SetCookies()
>
> Cookie:
> new: SetCookieValue() string
>
> Please review this at
http://codereview.appspot.com/**4620049/<http://codereview.appspot.com/4620049/>
>
> Affected files:
>  M src/pkg/http/cgi/child.go
>  M src/pkg/http/cgi/child_test.go
>  M src/pkg/http/cgi/host.go
>  M src/pkg/http/client.go
>  M src/pkg/http/client_test.go
>  M src/pkg/http/cookie.go
>  M src/pkg/http/cookie_test.go
>  M src/pkg/http/readrequest_test.**go
>  M src/pkg/http/request.go
>  M src/pkg/http/requestwrite_**test.go
>  M src/pkg/http/response.go
>  M src/pkg/http/reverseproxy.go
>  M src/pkg/http/reverseproxy_**test.go
>  M src/pkg/http/server.go
>
>
>

[gburd, 2011-06-15 20:47:28]

http://codereview.appspot.com/4620049/diff/2001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4620049/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:168: return readCookies(r.Header)
Because readCookies removes the parsed cookies from r.Header, there's only one
chance to fetch the cookies.  I recommend memoizing the result of readCookies or
changing to readCookies to not modify the header.

[bradfitz, 2011-06-15 20:53:40]

http://codereview.appspot.com/4620049/diff/2001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4620049/diff/2001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:168: return readCookies(r.Header)
On 2011/06/15 20:47:28, gburd wrote:
> Because readCookies removes the parsed cookies from r.Header, there's only one
> chance to fetch the cookies.  I recommend memoizing the result of readCookies
or
> changing to readCookies to not modify the header.

Good catch.  It shouldn't modify the header.

Memoization is a separate issue we can do later.  Currently Request and Response
have no private fields.  We'd also need to drop or ignore the memoized results
if the underlying map changed, but people use the underlying map directly too,
so there's nothing to hook into.  So let's ignore memoization for now.

[rsc, 2011-06-15 21:00:10]

There's no need for memoization.
It's okay for a method to do real work.

Russ

[bradfitz, 2011-06-15 21:12:26]

Sure, but there's a line.  Cheap-looking accesssors shouldn't be
surprisingly expensive.

I'm happy punting on it for now but at least we could memoize it if needed
in the future.


On Wed, Jun 15, 2011 at 2:00 PM, Russ Cox <rsc@golang.org> wrote:

> There's no need for memoization.
> It's okay for a method to do real work.
>
> Russ
>

[bradfitz, 2011-06-15 22:25:50]

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

Please take another look.

[rsc, 2011-06-15 22:34:29]

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go
File src/pkg/http/cookie.go (right):

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go#newcod...
src/pkg/http/cookie.go:135: // TODO(bradfitz): do anything with unparsedLines?
Delete.

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go#newcod...
src/pkg/http/cookie.go:144: // SetCookieValue returns the serialization of the
cookie for use in a
This sounds like a setter (same problem as SetCookie but let's not
let them proliferate).  Can this just be String() ?

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go#newcod...
src/pkg/http/cookie.go:215: // TODO(bradfitz): do anything with unparsedLines?
Nah.  Delete.

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go#newcod...
src/pkg/http/cookie.go:224: // TODO(bradfitz): this code is no longer used.
export? delete?
Delete whole function.

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:176: // TODO: memoize result of Cookies()?
I wouldn't bother with that, but it's probably worth scanning the string slice
for the name= before invoking the parser.

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/response.go
File src/pkg/http/response.go (right):

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/response.go#newc...
src/pkg/http/response.go:71: // SetCookies parses and returns the cookies set in
the Set-Cookie headers.
Should be SetCookie, like the field was.

[bradfitz, 2011-06-15 22:40:37]

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go
File src/pkg/http/cookie.go (right):

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/cookie.go#newcod...
src/pkg/http/cookie.go:144: // SetCookieValue returns the serialization of the
cookie for use in a
On 2011/06/15 22:34:29, rsc wrote:
> This sounds like a setter (same problem as SetCookie but let's not
> let them proliferate).  Can this just be String() ?

Cookies have two types of serialization:

Cookie: foo=bar
Set-Cookie: (all this junk)

Promoting either of them as the String() value seems wrong.

Perhaps StringSetCookie() so it doesn't start with "Set" and look like a setter?

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/response.go
File src/pkg/http/response.go (right):

http://codereview.appspot.com/4620049/diff/9001/src/pkg/http/response.go#newc...
src/pkg/http/response.go:71: // SetCookies parses and returns the cookies set in
the Set-Cookie headers.
On 2011/06/15 22:34:29, rsc wrote:
> Should be SetCookie, like the field was.

Intentional change.  I wanted to mirror Cookie(string)/Cookies().  Accessing a
cookie by exact name is much more common than asking for all cookies.

It would seem weird to have:

Cookies() []*Cookie
SetCookie() []*Cookie

Unless you want to keep Cookie() as-is and come up with a new name for the
singular Cookie accessor, but I couldn't think of anything.

[bradfitz, 2011-06-16 01:06:11]

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

Please take another look.

[bradfitz, 2011-06-16 17:24:26]

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

Please take another look.

[rsc, 2011-06-16 17:36:51]

LGTM

http://codereview.appspot.com/4620049/diff/16001/src/pkg/http/cookie.go
File src/pkg/http/cookie.go (right):

http://codereview.appspot.com/4620049/diff/16001/src/pkg/http/cookie.go#newco...
src/pkg/http/cookie.go:146: func writeSetCookieToBuffer(buf *bytes.Buffer, c
*Cookie) {
It looks like this is used only in Cookie.String.  If so, inline there.

[bradfitz, 2011-06-16 18:59:14]

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

Please take another look.

[rsc, 2011-06-16 19:22:40]

The gofix looks good but I am slightly worried about
other people's structs having those fields.  Did you try
calling typecheck?  If it's easy to add it's worth doing.

http://codereview.appspot.com/4620049/diff/21001/src/cmd/gofix/httpheaders.go
File src/cmd/gofix/httpheaders.go (right):

http://codereview.appspot.com/4620049/diff/21001/src/cmd/gofix/httpheaders.go...
src/cmd/gofix/httpheaders.go:14: `Rename http Referer, UserAgent, Cookie,
SetCookie accesses which are now methods.
s/accesses/&,/

http://codereview.appspot.com/4620049/diff/21001/src/cmd/gofix/httpheaders.go...
src/cmd/gofix/httpheaders.go:39: wasCalled := false
can delete this stuff

http://codereview.appspot.com/4620049/diff/21001/src/cmd/gofix/httpheaders.go...
src/cmd/gofix/httpheaders.go:45: if !wasCalled {
if !called[n] {

[rsc, 2011-06-16 19:22:41]

The gofix looks good but I am slightly worried about
other people's structs having those fields.  Did you try
calling typecheck?  If it's easy to add it's worth doing.

[bradfitz, 2011-06-16 19:41:09]

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

Please take another look.

[rsc, 2011-06-16 19:43:22]

very close

http://codereview.appspot.com/4620049/diff/26001/src/cmd/gofix/httpheaders.go
File src/cmd/gofix/httpheaders.go (right):

http://codereview.appspot.com/4620049/diff/26001/src/cmd/gofix/httpheaders.go...
src/cmd/gofix/httpheaders.go:45: if typeof[n] == "string" {
I don't think this is what you want.  If someone else defines a struct with
Referer string, we don't want to stomp on it.  I think what we want is

case *ast.SelectorExpr:
    if called[n] || typeof[n] != "*http.Request" && typeof[n] !=
"*http.Response" {
        break
    }
    switch n.Sel.Name {
    ...

Because we don't care about field types I think you can then pass 
&TypeConfig{} to typecheck.

[bradfitz, 2011-06-16 19:52:03]

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

Please take another look.

[bradfitz, 2011-06-16 19:54:48]

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

Please take another look.

[rsc, 2011-06-16 19:56:39]

while you're there, in gofix/main.go:

		if allowed != nil && !allowed[fix.desc] {

s/desc/name/

then the -r flag will work better

[bradfitz, 2011-06-16 19:58:55]

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

Please take another look.

[rsc, 2011-06-16 20:01:25]

LGTM

[bradfitz, 2011-06-16 20:02:36]

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

http: make Headers be source of truth

Previously Request and Response had redundant fields for
Referer, UserAgent, and cookies which caused confusion and
bugs.  It also didn't allow us to expand the package over
time, since the way to access fields would be in the Headers
one day and promoted to a field the next day.  That would be
hard to gofix, especially with code ranging over Headers.

After a discussion on the mail package's design with a similar
problem, we've designed to make the Headers be the source of
truth and add accessors instead.

Request:
change: Referer -> Referer()
change: UserAgent -> UserAgent()
change: Cookie -> Cookies()
new: Cookie(name) *Cookie
new: AddCookie(*Cookie)

Response:
change: Cookie -> Cookies()

Cookie:
new: String() string

R=rsc
CC=golang-dev
http://codereview.appspot.com/4620049