Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
fix Cookies(url) #16
Conversation
kat-co
approved these changes
Oct 4, 2016
Needs a reference to the bug in the commit message, and QA steps in the PR.
| + cookies[i] = &http.Cookie{ | ||
| + Name: e.Name, | ||
| + Value: e.Value, | ||
| + Path: e.Path, |
cmars
Oct 4, 2016
Owner
Interesting. Looks like we were dropping everything other than Name and Value before. And, it looks like net/http does the same here: https://golang.org/src/net/http/cookie.go?s=439:952#L219
What is the reason for preserving them?
natefinch
Oct 5, 2016
Contributor
Good question. So, the Domain is needed to properly reference the cookie with Remove cookie. The Domain is part of the id. I don't know why the stdlib is doing that, it seems like a bug, but I might just be missing something. I posted about it here: https://groups.google.com/forum/#!topic/golang-nuts/GPijgXvBYuY
mhilton
suggested changes
Oct 5, 2016
I think to be compatible we need to leave Cookies at it is. Perhaps add a new function to get the full details out.
| @@ -292,10 +292,22 @@ func (j *Jar) cookies(u *url.URL, now time.Time) (cookies []*http.Cookie) { | ||
| } | ||
| sort.Sort(byPathLength(selected)) | ||
| - for _, e := range selected { | ||
| - cookies = append(cookies, &http.Cookie{Name: e.Name, Value: e.Value}) |
mhilton
Oct 5, 2016
Member
This worries me. The cookie jar has always been a little obtuse, but I have a feeling that it is important that only these fields get filled in. As these will be serialised for adding to HTTP requests and I don't know the implications of sending data that is only expected in the response in the request. At best it will use unnecessary bandwidth. If you need visibility into the Cookie jar other than for doing an HTTP request it might be better to add a different interface.
rogpeppe
reviewed
Oct 5, 2016
Thanks for the suggestion, but as discussed online, I believe that this isn't right, as it will break the contract of the http.CookieJar interface.
|
Yep, I'll abandon it. The changes to cookies returned by Cookies(u) are incompatible with the way the CookieJar is expected to be used. This is an unfortunate design decision, but I guess there's nothing that can be done about it for now. I'll put up a different PR with a RemoveCookies(url) method. |
natefinch commentedOct 4, 2016
The cookies returned from Cookies(url) were invalid for use
in calling RemoveCookie.