code review 6653049: net/http/client.go: fix cookie handling on (*Client) Do()

net/http/client.go: fix cookie handling on (*Client) Do()

Fix the problem with no cookie handling when sending
other than GET or HEAD request through
(*Client) Do(*Request) (*Resposne, error).
https://code.google.com/p/go/issues/detail?id=3985

Adds a function (*Client) send(*Request) (*Reponse, error):
- sets cookies from CookieJar to request,
- sends request
- parses a reply cookies and updates CookieJar

[pawelszczur, 2012-10-12 15:36:09]

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

I'd like you to review this change to
https://code.google.com/p/go

[pawelszczur, 2012-10-18 08:47:20]

Ping,

https://codereview.appspot.com/6653049/

[gobot, 2012-10-22 21:41:54]

R=bradfitz (assigned by bradfitz)

[bradfitz, 2012-10-29 11:31:51]

Sorry for the slow review.  Kick me in the future if you want my attention.

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:90: func handleCookiesAndSend(jar CookieJar, req
*Request, transport RoundTripper) (resp *Response, err error) {
since CookieJar and RoundTripper both always come from the Client anyway, maybe
just make this a private method on *Client?

func (c *Client) cookieSend(req *Request) (*Response, error)

(and I have a slight preference for not naming the result parameters, for
clarity in the return statements below)

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:97: if err == nil && jar != nil {
more idiomatic would be to return early on error.

if err != nil {
    return nil, err
}

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:98: jar.SetCookies(req.URL, resp.Cookies())
most the time this jar will be a blackHoleJar, which means we waste time parsing
cookies in resp.Cookies() just to throw them away, so maybe we should ditch
blackHoleJar, once this is a method on Client and we have the jar == nil checks
anyway

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:233: jar = blackHoleJar{}
perhaps we should get rid of this now

[pawelszczur, 2012-10-29 14:38:36]

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

Please take another look.

[pawelszczur, 2012-10-29 14:41:23]

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

Please take another look.

[bradfitz, 2012-10-29 14:46:57]

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (left):

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:220: jar = blackHoleJar{}
this was the only use of blackHoleJar, so you can delete it from jar.go now.

be sure to add jar.go to this CL, with "hg change nnnnnn" and then add it to the
file list and save before you hg mail again.

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:90: func (c *Client) handleCookiesAndSend(req
*Request) (resp *Response, err error) {
let's just call this method "send".  it's internal.  no need to have the callers
list out everything it does.

also, drop the named result parameters.  just (*Response, error)

[pawelszczur, 2012-10-29 16:38:26]

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

Please take another look.

[pawelszczur, 2012-10-29 16:39:00]

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:90: func handleCookiesAndSend(jar CookieJar, req
*Request, transport RoundTripper) (resp *Response, err error) {
On 2012/10/29 11:31:51, bradfitz wrote:
> since CookieJar and RoundTripper both always come from the Client anyway,
maybe
> just make this a private method on *Client?
> 
> func (c *Client) cookieSend(req *Request) (*Response, error)
> 
> (and I have a slight preference for not naming the result parameters, for
> clarity in the return statements below)

Thanks for pointing that.

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:97: if err == nil && jar != nil {
On 2012/10/29 11:31:51, bradfitz wrote:
> more idiomatic would be to return early on error.
> 
> if err != nil {
>     return nil, err
> }

Done.

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:98: jar.SetCookies(req.URL, resp.Cookies())
On 2012/10/29 11:31:51, bradfitz wrote:
> most the time this jar will be a blackHoleJar, which means we waste time
parsing
> cookies in resp.Cookies() just to throw them away, so maybe we should ditch
> blackHoleJar, once this is a method on Client and we have the jar == nil
checks
> anyway

Done.

https://codereview.appspot.com/6653049/diff/2001/src/pkg/net/http/client.go#n...
src/pkg/net/http/client.go:233: jar = blackHoleJar{}
On 2012/10/29 11:31:51, bradfitz wrote:
> perhaps we should get rid of this now

Done.

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (left):

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:220: jar = blackHoleJar{}
On 2012/10/29 14:46:57, bradfitz wrote:
> this was the only use of blackHoleJar, so you can delete it from jar.go now.
> 
> be sure to add jar.go to this CL, with "hg change nnnnnn" and then add it to
the
> file list and save before you hg mail again.

Done.

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/6653049/diff/13001/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:90: func (c *Client) handleCookiesAndSend(req
*Request) (resp *Response, err error) {
So there will be 2 methods called "send". One on *Client and a stand alone one.
That can be quite misleading?

Done.
 
On 2012/10/29 14:46:57, bradfitz wrote:
> let's just call this method "send".  it's internal.  no need to have the
callers
> list out everything it does.
> 
> also, drop the named result parameters.  just (*Response, error)

[pawelszczur, 2012-10-29 16:39:41]

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

Please take another look.

[pawelszczur, 2012-10-29 16:39:58]

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

Please take another look.

[bradfitz, 2012-10-29 16:41:40]

LGTM

Thanks!

On Mon, Oct 29, 2012 at 5:39 PM, <filemon@google.com> wrote:

> Hello bradfitz@golang.org (cc: gobot@golang.org,
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6653049/<http://codereview.appspot.com/6653049/>
>

[bradfitz, 2012-10-29 16:56:35]

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

net/http/client.go: fix cookie handling on (*Client) Do()

Fix the problem with no cookie handling when sending
other than GET or HEAD request through
(*Client) Do(*Request) (*Resposne, error).
https://code.google.com/p/go/issues/detail?id=3985

Adds a function (*Client) send(*Request) (*Reponse, error):
- sets cookies from CookieJar to request,
- sends request
- parses a reply cookies and updates CookieJar

Fixes issue 3985

R=bradfitz
CC=gobot, golang-dev
http://codereview.appspot.com/6653049

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[serbaut, 2012-12-19 16:45:19]

https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:101: c.Jar.SetCookies(req.URL, resp.Cookies())
SetCookies is always called here, even if there are no cookies in resp.
Shouldn't it be (as before):

if c.Jar != nil {
    if rc := resp.Cookies(); len(rc) > 0 {
        c.Jar.SetCookies(req.URL, rc)
    }
}

?

[bradfitz, 2012-12-19 17:04:02]

https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go
File src/pkg/net/http/client.go (right):

https://codereview.appspot.com/6653049/diff/12002/src/pkg/net/http/client.go#...
src/pkg/net/http/client.go:101: c.Jar.SetCookies(req.URL, resp.Cookies())
On 2012/12/19 16:45:19, serbaut wrote:
> SetCookies is always called here, even if there are no cookies in resp.
> Shouldn't it be (as before):
> 
> if c.Jar != nil {
>     if rc := resp.Cookies(); len(rc) > 0 {
>         c.Jar.SetCookies(req.URL, rc)
>     }
> }
> 
> ?

That would also be fine.  Feel free to send a CL.