Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(747)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 1 month ago by pawelszczur
Modified:
9 years, 11 months ago
Reviewers:
serbaut, bradfitz
CC:
gobot, golang-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r 189cd011c4f3 https://code.google.com/p/go #

Total comments: 8

Patch Set 3 : diff -r bb4ee132b967 https://code.google.com/p/go #

Patch Set 4 : diff -r bb4ee132b967 https://code.google.com/p/go #

Total comments: 4

Patch Set 5 : diff -r bb4ee132b967 https://code.google.com/p/go #

Patch Set 6 : diff -r bb4ee132b967 https://code.google.com/p/go #

Patch Set 7 : diff -r bb4ee132b967 https://code.google.com/p/go #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -38 lines) Patch
M src/pkg/net/http/client.go View 1 2 3 4 5 8 chunks +23 lines, -27 lines 2 comments Download
M src/pkg/net/http/client_test.go View 1 1 chunk +4 lines, -0 lines 0 comments Download
M src/pkg/net/http/jar.go View 1 2 3 4 5 6 1 chunk +6 lines, -11 lines 0 comments Download

Messages

Total messages: 15
pawelszczur
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
10 years, 1 month ago (2012-10-12 15:36:09 UTC) #1
pawelszczur
Ping, https://codereview.appspot.com/6653049/
10 years, 1 month ago (2012-10-18 08:47:20 UTC) #2
gobot
R=bradfitz (assigned by bradfitz)
10 years, 1 month ago (2012-10-22 21:41:54 UTC) #3
bradfitz
Sorry for the slow review. Kick me in the future if you want my attention. ...
10 years, 1 month ago (2012-10-29 11:31:51 UTC) #4
pawelszczur
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
10 years, 1 month ago (2012-10-29 14:38:36 UTC) #5
pawelszczur
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
10 years, 1 month ago (2012-10-29 14:41:23 UTC) #6
bradfitz
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#oldcode220 src/pkg/net/http/client.go:220: jar = blackHoleJar{} this was the only use of ...
10 years, 1 month ago (2012-10-29 14:46:57 UTC) #7
pawelszczur
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
10 years, 1 month ago (2012-10-29 16:38:26 UTC) #8
pawelszczur
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#newcode90 src/pkg/net/http/client.go:90: func handleCookiesAndSend(jar CookieJar, req *Request, transport RoundTripper) (resp *Response, ...
10 years, 1 month ago (2012-10-29 16:39:00 UTC) #9
pawelszczur
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
10 years, 1 month ago (2012-10-29 16:39:41 UTC) #10
pawelszczur
Hello bradfitz@golang.org (cc: gobot@golang.org, golang-dev@googlegroups.com), Please take another look.
10 years, 1 month ago (2012-10-29 16:39:58 UTC) #11
bradfitz
LGTM Thanks! On Mon, Oct 29, 2012 at 5:39 PM, <filemon@google.com> wrote: > Hello bradfitz@golang.org ...
10 years, 1 month ago (2012-10-29 16:41:40 UTC) #12
bradfitz
*** 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 ...
10 years, 1 month ago (2012-10-29 16:56:35 UTC) #13
serbaut
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#newcode101 src/pkg/net/http/client.go:101: c.Jar.SetCookies(req.URL, resp.Cookies()) SetCookies is always called here, even if ...
9 years, 11 months ago (2012-12-19 16:45:19 UTC) #14
bradfitz
9 years, 11 months ago (2012-12-19 17:04:02 UTC) #15
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b