Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
add RemoveAll(url) method #17
Conversation
mhilton
approved these changes
Oct 5, 2016
| @@ -396,6 +396,34 @@ func (j *Jar) deleteExpired(now time.Time) { | ||
| } | ||
| } | ||
| +// RemoveAll removes any cookies from the jar that were set for the given URL. |
mhilton
Oct 5, 2016
Member
This does not seem to take the URL path into account when deciding what to delete. That isn't unreasonable behavior, but let's document it. Perhaps change the parameter to host to make it clear what will be deleted.
rogpeppe
requested changes
Oct 5, 2016
The API looks reasonable, thanks, but the tests could use some improvement (and I suspect the implementation will need some changes as a result)
| + jar := newTestJar("") | ||
| + google := mustParseURL("https://www.google.com") | ||
| + apple := mustParseURL("https://www.apple.com") | ||
| + cookies := []*http.Cookie{ |
rogpeppe
Oct 5, 2016
Owner
Let's have some tests with cookies set for sub-paths, please, and some
that remove cookies with a URL with a path set.
For example, RemoveAll("https://foo.com/bar/") should not remove cookies
set for https://foo.com/.
| + t.Fatalf("Expected 4 cookies, got %d", len(withApple)) | ||
| + } | ||
| + jar.RemoveAll(apple) | ||
| + after := jar.AllCookies() |
rogpeppe
Oct 5, 2016
Owner
ISTM that this isn't actually testing that the correct cookies have been removed.
natefinch
Oct 5, 2016
•
Edited 1 time
-
natefinch
Oct 5, 2016
Contributor
-
Oct 5, 2016
Isn't it? The "original" is from before apple is added, so we're testing that the only ones left are the google ones.
mitechie
Oct 5, 2016
Owner
Right, this is just testing the number but not that they're the right ones or not. A better test is that after, no cookies with apple in the url are found, regardless of what else is there.
The test that there are still two not-apple is legit in making sure you don't accidentally remove the wrong ones though.
natefinch
Oct 5, 2016
Contributor
It is testing if they're the right ones. This specific line 2009 above is really not even necessary, I considered leaving it out. It's a checkpoint to make sure we're still in a sane state. At the end of the function, after removing the apple cookies, it checks that there are exactly 2 cookies left, and that they're the same google cookies we set.
I've refactored things a bit to make it a more reusable test for different scenarios.... but the actual test code is mostly the same.
add 2 cookies
record current list of cookies
add 2 more cookies w/ HOST.
ensure we have 4 cookies
remove cookies w/ HOST
ensure we have 2 cookies and that they're the initial 2.
natefinch
added some commits
Oct 5, 2016
rogpeppe
requested changes
Oct 6, 2016
I'm not entirely sure about the semantics we've got going here. Shouldn't removing a host just remove entries for that host, not all hosts with the same public suffix. I'm open to arguments either way.
Plus a few other suggestions.
| @@ -396,6 +396,34 @@ func (j *Jar) deleteExpired(now time.Time) { | ||
| } | ||
| } | ||
| +// RemoveAll removes any cookies from the jar that were set for the given host. |
rogpeppe
Oct 6, 2016
Owner
This comment isn't technically correct. It removes all cookies that were set
for any host with the same public suffix as host, which is quite
a different thing.
Is this really the behaviour we want?
| @@ -396,6 +396,34 @@ func (j *Jar) deleteExpired(now time.Time) { | ||
| } | ||
| } | ||
| +// RemoveAll removes any cookies from the jar that were set for the given host. | ||
| +func (j *Jar) RemoveAll(host string) { |
rogpeppe
Oct 6, 2016
Owner
Maybe name this RemoveAllHost so that we can still provide RemoveAll(url) later?
| @@ -396,6 +396,34 @@ func (j *Jar) deleteExpired(now time.Time) { | ||
| } | ||
| } | ||
| +// RemoveAll removes any cookies from the jar that were set for the given host. | ||
| +func (j *Jar) RemoveAll(host string) { | ||
| + now := time.Now().Add(-1 * time.Second) |
rogpeppe
Oct 6, 2016
Owner
"now" isn't a great name for something that's actually a second in the past.
I'd suggest naming it "expiry" and declaring it closer to where it's used
(just before the for loop)
natefinch
Oct 6, 2016
Contributor
Good point. I was going to do that and then never got around to it (another remnant of copy and paste).
| + defer j.mu.Unlock() | ||
| + | ||
| + submap := j.entries[key] | ||
| + if submap == nil { |
| +} | ||
| + | ||
| +func TestRemoveAllRoot(t *testing.T) { | ||
| + testRemoveAll(t, mustParseURL("https://www.apple.com"), "apple.com") |
rogpeppe
Oct 6, 2016
Owner
I'm not convinced about this behaviour. Or the (currently untested) behaviour that removing cookies from foo.apple.com will also remove certs from bar.apple.com.
natefinch
Oct 6, 2016
Contributor
Yep.. fixed and added a test for different subdomains not removing each other.
|
not using this anymore |
natefinch commentedOct 5, 2016
•
Edited 1 time
-
natefinch
Oct 5, 2016
This is to support the logout method in juju. Also it removes some confusion about whether or not to do this:
(which doesn't work)