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
Adds support for max-age cookie value. Fixes #184 #412
Conversation
3cdc245
to
614ea5d
Compare
614ea5d
to
cefc5a8
Compare
cefc5a8
to
e4c472a
Compare
@erikdubbelboer good for merge? |
cookie.go
Outdated
@@ -52,6 +53,7 @@ type Cookie struct { | |||
key []byte | |||
value []byte | |||
expire time.Time | |||
maxAge time.Time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maxAge
should be kept as int
.
cookie.go
Outdated
@@ -203,6 +213,10 @@ func (c *Cookie) AppendBytes(dst []byte) []byte { | |||
} | |||
dst = append(dst, c.value...) | |||
|
|||
if !c.maxAge.IsZero() { | |||
ts := strconv.Itoa(int(c.maxAge.Unix())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use c.bufKV.value = AppendUint(...
so we don't have a heap allocation here.
cookie.go
Outdated
if !c.maxAge.IsZero() { | ||
ts := strconv.Itoa(int(c.maxAge.Unix())) | ||
dst = appendCookiePart(dst, strCookieMaxAge, []byte(ts)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this the line below should be else if
so we can't set both.
cookie.go
Outdated
@@ -319,6 +342,19 @@ func (c *Cookie) ParseBytes(src []byte) error { | |||
return nil | |||
} | |||
|
|||
var errInvalidMaxAge = errors.New("max-age must be non-zero digit") | |||
|
|||
func parseTimestamp(b []byte) (time.Time, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max-age
is not a timestamp, max-age
is the number of seconds the cookie should live from the time you receive it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix this first and then we'll look at the tests.
Addressed feedback @erikdubbelboer, made the API simple and only removes expires if max-age is set. |
1b80424
to
05b5485
Compare
05b5485
to
c948f6f
Compare
Thanks! |
Issue: #184
Note, because
max-age
takes precedence over expires, it only supportsmax-age
when encoded/decoded. However, it does not allow the client to setmax-age
at the API and prefersSetExpire
instead.