code review 7069054: exp/cookiejar: implement defaul-path of cookie (issue 7069054)

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

Reviewers: nigeltao,

Message:
Hello nige...@golang.org (cc: golan...@googlegroups.com),

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


Description:
exp/cookiejar: implement defaul-path of cookie

The Path of a cookie is set according to sections 5.1.4 and
5.2.4 of RFC 6265.

Update issue 1960.

Please review this at https://codereview.appspot.com/7069054/

Affected files:
M src/pkg/exp/cookiejar/jar.go
A src/pkg/exp/cookiejar/jar_test.go


Index: src/pkg/exp/cookiejar/jar.go
===================================================================
--- a/src/pkg/exp/cookiejar/jar.go
+++ b/src/pkg/exp/cookiejar/jar.go
@@ -11,6 +11,7 @@
import (
"net/http"
"net/url"
+ "strings"
)

// PublicSuffixList provides the public suffix of a domain. For example:
@@ -85,5 +86,31 @@
//
// It does nothing if the URL's scheme is not HTTP or HTTPS.
func (j *Jar) SetCookies(u *url.URL, cookies []*http.Cookie) {
- // TODO.
+ if u.Scheme != "http" && u.Scheme != "https" {
+ return
+ }
+
+ defPath := defaultPath(u.Path)
+ for _, cookie := range cookies {
+ if cookie.Path == "" || cookie.Path[0] != '/' {
+ cookie.Path = defPath
+ }
+ // TODO: handle domain
+ // TODO: handle expiration/deletion
+ // TODO: update storage
+ }
}
+
+// defaultPath returns the "directory" part of an URL's path according to
+// RFC 6265 section 5.1.4.
+func defaultPath(path string) string {
+ if len(path) == 0 || path[0] != '/' {
+ return "/" // empty or malformed
+ }
+
+ i := strings.LastIndex(path, "/") // path starts with "/", so i != -1
+ if i == 0 {
+ return "/" // the "/abc" case
+ }
+ return path[:i] // the "/abc/xyz" and "/abc/xyz/" case
+}
Index: src/pkg/exp/cookiejar/jar_test.go
===================================================================
new file mode 100644
--- /dev/null
+++ b/src/pkg/exp/cookiejar/jar_test.go
@@ -0,0 +1,27 @@
+// Copyright 2013 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package cookiejar
+
+import (
+ "testing"
+)
+
+var defaultPathTests = map[string]string{
+ "/": "/",
+ "/abc": "/",
+ "/abc/": "/abc",
+ "/abc/xyz": "/abc",
+ "/abc/xyz/": "/abc/xyz",
+ "": "/",
+ "strange": "/",
+}
+
+func TestDefaultPath(t *testing.T) {
+ for path, want := range defaultPathTests {
+ if got := defaultPath(path); got != want {
+ t.Errorf("%q: got %q, want %q", path, got, want)
+ }
+ }
+}

[nige...@golang.org]

Can this wait until after 6996044 "dumbify the storage interface" gets
resolved? That would change what Jar.SetCookies looks like.

I know that 6996044 is still waiting for my response; I've been busy
with other things since coming back from vacation.

Finally, there's a typo in the change description: s/defaul/default/.


https://codereview.appspot.com/7069054/diff/7001/src/pkg/exp/cookiejar/jar.go
File src/pkg/exp/cookiejar/jar.go (right):

https://codereview.appspot.com/7069054/diff/7001/src/pkg/exp/cookiejar/jar.go#newcode96
src/pkg/exp/cookiejar/jar.go:96: cookie.Path = defPath
I'm not sure if SetCookies should modify the cookies passed to it.

https://codereview.appspot.com/7069054/

[Volker Dobler]

On Wed, Jan 9, 2013 at 12:25 PM, <nige...@golang.org> wrote:

Can this wait until after 6996044 "dumbify the storage interface" gets
resolved? That would change what Jar.SetCookies looks like.

Sure.

 

src/pkg/exp/cookiejar/jar.go:96: cookie.Path = defPath
I'm not sure if SetCookies should modify the cookies passed to it.

Actually this is a TODO in http's cookie handling:

http://golang.org/src/pkg/net/http/cookie.go?s=2884:2910#L120

but I don't think it should be fixed there as http.Cookie is

more the "cookie on the wire" and not so much a jar'ed cookie.

The path parsing could be fixed in readSetCookie but the

domain parsing TODO 25 lines above not.

V.

[Volker Dobler]

I composed a rough overview of the Cookiejar/Storage

design goals, proposed APIs, my personal impression,

some suggestions and still open question in the document

https://docs.google.com/document/d/1w8VHqIGBmud9huUQ_nBNqOSqufqoxuSs93ZrXVvut_Q/edit

Maybe it will be easier and quicker to discuss the upcoming

API in this form than several CLs and counter CLs.

Any comments welcome.

V.

On Wed, Jan 9, 2013 at 12:25 PM, <nige...@golang.org> wrote:

--
Dr. Volker Dobler