Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Allow disable of Session saver #1885

Open
AtomicNibble opened this issue Jan 25, 2020 · 4 comments
Open

Allow disable of Session saver #1885

AtomicNibble opened this issue Jan 25, 2020 · 4 comments
Labels
breaking change This feature / fix introduces breaking changes enhancement New feature or request s: triage Some tests need to be run to confirm the issue
Milestone

Comments

@AtomicNibble
Copy link

Hello, the session save was made none optional in 3c8d9cb.

But this means with the default session store you send a new cookie with every response even if you change nothing, bloating responses and expire keeps moving forward.

This also makes Redis backed session stores very chatty.

I would like to request this middleware is made optional again.
Thanks.

@stanislas-m stanislas-m added the enhancement New feature or request label Jan 29, 2020
@AtomicNibble
Copy link
Author

AtomicNibble commented Feb 2, 2020

I have this working in a fork I made, required a few additional changes to only save flash state if changed.
But has resulted in a lot less writes to my redis backed session store.

It dose mean you have to explicitly call c.Session().Save() after making changes in a action, but I prefer that.

I will open a PR for review / feedback.

@markbates
Copy link
Member

This is a big breaking change, a PR probably wouldn’t be accepted, sorry.

An option would be to open a PR that is smarter about saving sessions, and only writing them when necessary. Another would be to expose the session saving middleware, that people, such as yourself, can remove it from the middleware stack and write their own.

@markbates markbates added the breaking change This feature / fix introduces breaking changes label Feb 2, 2020
@AtomicNibble
Copy link
Author

Sorry if not clear, my PR would be to add a option to disable the middleware, I was not planning to disable it totally. So behaviour would be unchanged by default.

Simular to how if you don't pass a logger instance one is created for you.

I had thought about a change tracking aproach, but decided to explore explicit saving first.

@markbates
Copy link
Member

We are trying to move away from adding more options, in favor of a plugin design pattern, so we wouldn't accept a PR that adds a new option at this time.

My recommendation is a simple bool dirty check in session.go if someone calls Set then set the bool and check for it in Save. It doesn't have to be more complicated than that, and it would solve the problem. I would definitely accept a PR for that, as long as there are passing tests of course. :)

@sio4 sio4 modified the milestone: Backlog Sep 26, 2022
@sio4 sio4 added the s: triage Some tests need to be run to confirm the issue label Sep 26, 2022
@sio4 sio4 added this to the Proposal milestone Sep 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking change This feature / fix introduces breaking changes enhancement New feature or request s: triage Some tests need to be run to confirm the issue
Projects
None yet
Development

No branches or pull requests

4 participants