net/http/cookiejar: A cookie jar implementation. (issue 5544082)

[dr.volke...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
This got rather lengthy even in its unfinished state.

Any comments welcome.

The non-http stuff could be simplified (just ignoring
any non-http/https protocol) but I wanted to try all
the testcases first.

V.

Description:
net/http/cookiejar: A cookie jar implementation.

This implementation is a very simple one: Cookies are
stored in a unsorted, flat slice. All operations
(insert, update, delete, retrieve) are painfully slow
at the moment but the test cases should cover all
aspects of cookie handling.

Still missing are:
- host/domain name canonicalization (this will need
additional tables)
- an exported method to persist the persistent cookies

Please review this at http://codereview.appspot.com/5544082/

Affected files:
A src/pkg/net/http/cookiejar/Makefile
A src/pkg/net/http/cookiejar/cookie.go
A src/pkg/net/http/cookiejar/jar.go
A src/pkg/net/http/cookiejar/jar_test.go
A src/pkg/net/http/cookiejar/publicsuffixes.go
A src/pkg/net/http/cookiejar/publicsuffixes_test.go
A src/pkg/net/http/cookiejar/table.go
A src/pkg/net/http/cookiejar/url.go

[je...@somethingsimilar.com]

Could you uncomment the tests in TestRedirectCookiesOnRequest in
net/http/client_test.go and make sure they pass? They seem to be failing
when I run them with your code.

http://codereview.appspot.com/5544082/

[Volker Dobler]

I think the testcase is broken: If the second c..Do uses the same
client as the first, it will use the same cookie jar. And this jar
contains cookies from the redirect chain triggered by the first
c.Do.

func TestRedirectCookiesOnRequest(t *testing.T) {
var ts *httptest.Server
ts = httptest.NewServer(echoCookiesRedirectHandler)
defer ts.Close()
c := &Client{}
c.Jar = cookiejar.New(100,100,100)
req, _ := NewRequest("GET", ts.URL, nil)
req.AddCookie(expectedCookies[0])
_ = c
resp, _ := c.Do(req)
matchReturnedCookies(t, expectedCookies, resp.Cookies())

req, _ = NewRequest("GET", ts.URL, nil)
resp, _ = c.Do(req)
matchReturnedCookies(t, expectedCookies[1:], resp.Cookies()) // BROKEN

/*
CC: ChocolateChips; F: First; S: Second

jar is empty
req1.AddCookie(CC) // manualy added CC to request
c.Do(req1):
--> GET / (CC) // CC cookie from to requets
<-- 302 to /second (Set CC; F) // CC echoed back,
additional F
store CC and F in jar
--> GET /second (CC; F) // both CC and F from jar
<-- 200 (Set CC; F; S) // echo CC and F, add S
update CC and F, store S
3 cookies recieved

jar contains CC; F; S

c.Do(fresh_req): // will use same jar!
--> GET / (CC; F; S) // CC, F and S from jar!
<-- 302 to /second (Set CC; F; S; F) // echo all and add F
update jar // no
real change, still contains CC,F,S
--> GET /second (CC; F; S) // all from jar
<-- 200 (Set CC; F; S; S) // echo CC,F,S add S
update CC,F,S and S a second time
here 4 cookies are recieved: CC, F, S and S
*/
}

I am not sure if the whole second (fresh) request can test anything
useful if it uses the same, filled jar without further modification (cookie
restricted to path, expired, etc.)

Correct me if I am wrong!

Volker

--
Dr. Volker Dobler

[Jeff Hodges]

Volker and I accidentally went offline. We agreed that this test was
wrong and that a nil Jar should mean "don't pass cookies". It should
probably be removed.

[r...@golang.org]

R=bradfitz?

http://codereview.appspot.com/5544082/

[je...@somethingsimilar.com]

I haven't had the chance to review more beyond the first few files.
Sending this before I forget to.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/cookie.go
File src/pkg/net/http/cookiejar/cookie.go (right):

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/cookie.go#newcode38
src/pkg/net/http/cookiejar/cookie.go:38: // shouldSendCookie determines
whether to send cookie via a secure/nonHttp request
s/shouldSendCookie/shouldSend/

The other way may be more clear.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/cookie.go#newcode45
src/pkg/net/http/cookiejar/cookie.go:45: httpProtocol(c.HttpOnly,
!nonHttp)
This double negative is troublesome.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/cookie.go#newcode75
src/pkg/net/http/cookiejar/cookie.go:75: return !c.HostOnly &&
strings.HasSuffix(host, "."+c.Domain)
What happens if c.Domain has a dot at the front? (I think it's expected
to?)

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/cookie.go#newcode97
src/pkg/net/http/cookiejar/cookie.go:97: } else if
requestPath[len(c.Path)] == '/' {
When requestPath and c.Path are the same size, this will error.

A better way might be to use strings.LastIndex and reuse that for both
of these conditionals.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/jar.go
File src/pkg/net/http/cookiejar/jar.go (right):

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/jar.go#newcode109
src/pkg/net/http/cookiejar/jar.go:109: for _, cookie := range
jar.cookies {
This seems a little much just to get access to the cookies. Why not do
this inline and return an array?

Also, I don't see a jar.lock.Lock() here. Are you sure we don't need it?

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/jar.go#newcode369
src/pkg/net/http/cookiejar/jar.go:369: runtime.GC()
Remove.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/jar.go#newcode425
src/pkg/net/http/cookiejar/jar.go:425: nonHttp := isNonHttp(u)
Double negatives are a pain to deal with.

Also, Go conventions are to capitalize all the letters in an acronym.

http://codereview.appspot.com/5544082/diff/2001/src/pkg/net/http/cookiejar/jar.go#newcode426
src/pkg/net/http/cookiejar/jar.go:426: /*
Remove.

http://codereview.appspot.com/5544082/

[Brad Fitzpatrick]

I'll review this also.

On Jan 19, 2012 1:10 PM, <r...@golang.org> wrote:

R=bradfitz?


http://codereview.appspot.com/5544082/