Skip to content

Mark session as written when updating options#142

Merged
appleboy merged 1 commit intogin-contrib:masterfrom
andrewfrench:options-written
Dec 21, 2021
Merged

Mark session as written when updating options#142
appleboy merged 1 commit intogin-contrib:masterfrom
andrewfrench:options-written

Conversation

@andrewfrench
Copy link
Copy Markdown
Contributor

@andrewfrench andrewfrench commented Nov 1, 2021

This PR changes the behavior of the Options() method.

Currently, updating session options does set the written flag to true. This causes an issue when expiring/destroying a session, for example, because changes to session options will not be updated unless a key/value update has been made. If a developer wants to expire a session, they must set MaxAge to -1 and then make a trivial call to a method like Set() to ensure the options change will persist:

// Destroy session
session := sessions.Default(c)
session.Options(sessions.Options{
    MaxAge: -1,
})
session.Set("deleteMe", true)
_ = session.Save()

Documentation from the session stores, like this snippet from the memstore source, indicate that setting MaxAge to -1 should immediately expire a session:

https://github.com/quasoft/memstore/blob/2bce066d2b0b885eab067015d658dfd8ec1880e0/memstore.go#L103

To make this library more usable, I've updated the Options() method to mark the session as written when session options have been updated, saving the additional call:

// Destroy session
session := sessions.Default(c)
session.Option(sessions.Options{
    MaxAge: -1,
})
_ = session.Save()

Passing tests, including a case to test this behavior, can be found here.

@kpritam
Copy link
Copy Markdown

kpritam commented Dec 21, 2021

@appleboy do you think this can be merged? Any suggestion from your side on this?

@appleboy appleboy merged commit 202bf0f into gin-contrib:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants