Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cookiejar implementation. #526

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Cookiejar implementation. #526

wants to merge 18 commits into from

Conversation

dgrr
Copy link
Contributor

@dgrr dgrr commented Feb 3, 2019

Hello,

This commit tries to implement a basic CookieJar structure to handle Cookies easily without using external packages.

…d CookieJar structure helping user to collect cookies easily without using external packages.
@erikdubbelboer
Copy link
Collaborator

I think if we're going to add a CookieJar we should do it right and follow the spec. So:

  1. Which cookies are returned should be based on the path as well.
  2. Cookies should expire based on their expire time
  3. You can't set cookies for TLDs (for this you need a public suffix list, I suggest making this optional)

cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
client.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@erikdubbelboer erikdubbelboer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to handle cookies expiring and paths.

cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Show resolved Hide resolved
cookiejar.go Show resolved Hide resolved
cookiejar.go Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Show resolved Hide resolved
cookiejar.go Show resolved Hide resolved
cookiejar.go Show resolved Hide resolved
cookiejar.go Outdated Show resolved Hide resolved
cookiejar.go Outdated
c := searchCookieByKeyAndPath(key, path, cookies)
if c == nil {
c = AcquireCookie()
created = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better do the expired check here first so you don't acquire and release a cookie immediately if it didn't exist already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any other way to do this.
If I do the expiration checking at cookiejar.go:197 first I need to call ParseBytes and the code will be repeated in the case c == nil and c != nil. I prefer to keep it like this unless you see any other way to do that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, I forgot that you have to somehow parse the cookie first to get the expiration date. Doing that in a stack allocate cookie value would be possible for example but it would result in messier code.

cookiejar.go Outdated
cj.m.Lock()
{
if cj.hostCookies == nil {
cj.hostCookies = make(map[string][]*Cookie)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can just do return. No need to allocate anything.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make sure you do defer cj.m.Unlock() instead of doing this at the end of the function.

I myself always prefer to defer Unlocks as its much easier to follow the code. Also defer is basically free these days so no worry about performance.

cookiejar.go Outdated
c := searchCookieByKeyAndPath(key, path, cookies)
if c == nil {
c = AcquireCookie()
created = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you are right, I forgot that you have to somehow parse the cookie first to get the expiration date. Doing that in a stack allocate cookie value would be possible for example but it would result in messier code.

@efectn
Copy link
Contributor

efectn commented May 10, 2022

Any progress?

@erg43hergeg
Copy link

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants