code review 6968049: net/http: only call client SetCookie when needed

net/http: only call client SetCookie when needed

[serbaut, 2012-12-19 23:27:48]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-12-19 23:29:28]

I think that test is a little large.  It only needs to test the new
behavior you introduced, not the entire cookie system.

On Wed, Dec 19, 2012 at 3:27 PM, <serbaut@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: only call client SetCookie when needed
>
> Also: a test of a typical cookie based session that verifies
> the client cookie jar.
>
> Please review this at
https://codereview.appspot.**com/6968049/<https://codereview.appspot.com/6968...
>
> Affected files:
>   M src/pkg/net/http/client.go
>   M src/pkg/net/http/client_test.**go
>
>
> Index: src/pkg/net/http/client.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/client.go
> +++ b/src/pkg/net/http/client.go
> @@ -98,7 +98,9 @@
>                 return nil, err
>         }
>         if c.Jar != nil {
> -               c.Jar.SetCookies(req.URL, resp.Cookies())
> +               if rc := resp.Cookies(); len(rc) > 0 {
> +                       c.Jar.SetCookies(req.URL, rc)
> +               }
>         }
>         return resp, err
>  }
> Index: src/pkg/net/http/client_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/client_**test.go
> +++ b/src/pkg/net/http/client_**test.go
> @@ -415,6 +415,68 @@
>         }
>  }
>
> +// TestCookieSession verifies that a typical cookie based session works
> +func TestCookieSession(t *testing.T) {
> +       ts := httptest.NewServer(**HandlerFunc(func(w ResponseWriter, r
> *Request) {
> +               switch r.RequestURI {
> +               case "/login":
> +                       SetCookie(w, &Cookie{Name: "ssid", Value: "hash"})
> +                       Redirect(w, r, "/", 302)
> +               case "/logout":
> +                       SetCookie(w, &Cookie{Name: "ssid"})
> +               default:
> +                       if c := r.Cookies(); len(c) == 1 && c[0].Name ==
> "ssid" && c[0].Value == "hash" {
> +                               w.Write([]byte("ok"))
> +                       } else {
> +                               Error(w, "401 unauthorized",
> StatusUnauthorized)
> +                       }
> +               }
> +       }))
> +       defer ts.Close()
> +
> +       c := &Client{Jar: &TestJar{}}
> +
> +       if r, err := c.Get(ts.URL); err != nil {
> +               t.Fatal(err)
> +       } else {
> +               if got, want := r.StatusCode, StatusUnauthorized; got !=
> want {
> +                       t.Errorf("get: got %d, expected %d", got, want)
> +               }
> +       }
> +
> +       if r, err := c.PostForm(ts.URL+"/login", nil); err != nil {
> +               t.Fatal(err)
> +       } else {
> +               if got, want := r.StatusCode, StatusOK; got != want {
> +                       t.Errorf("/login: got %d, expected %d", got, want)
> +               }
> +               if sc := c.Jar.Cookies(r.Request.URL); len(sc) == 1 {
> +                       got, want := sc[0], Cookie{Name: "ssid", Value:
> "hash"}
> +                       if got.Name != want.Name || got.Value !=
> want.Value {
> +                               t.Errorf("client jar: got %#v, expected
> %#v", got, want)
> +                       }
> +               } else {
> +                       t.Errorf("client jar: got %d cookies, expected 1",
> len(sc))
> +               }
> +       }
> +
> +       if r, err := c.Get(ts.URL + "/logout"); err != nil {
> +               t.Fatal(err)
> +       } else {
> +               if got, want := r.StatusCode, StatusOK; got != want {
> +                       t.Errorf("/logout: got %d, exptected %d", got,
> want)
> +               }
> +               if sc := c.Jar.Cookies(r.Request.URL); len(sc) == 1 {
> +                       got, want := sc[0], Cookie{Name: "ssid", Value: ""}
> +                       if got.Name != want.Name || got.Value !=
> want.Value {
> +                               t.Errorf("client jar: got %#v, expected
> %#v", got, want)
> +                       }
> +               } else {
> +                       t.Errorf("client jar: got %d cookies, expected 1",
> len(sc))
> +               }
> +       }
> +}
> +
>  func TestJarCalls(t *testing.T) {
>         ts := httptest.NewServer(**HandlerFunc(func(w ResponseWriter, r
> *Request) {
>                 pathSuffix := r.RequestURI[1:]
>
>
>

[serbaut, 2012-12-19 23:50:12]

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

Please take another look.

[bradfitz, 2012-12-20 00:04:19]

LGTM

Much nicer test!

[bradfitz, 2012-12-20 00:24:45]

*** Submitted as https://code.google.com/p/go/source/detail?r=5acb449b2a67 ***

net/http: only call client SetCookie when needed

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/6968049

Committer: Brad Fitzpatrick <bradfitz@golang.org>