-
Notifications
You must be signed in to change notification settings - Fork 145
[WIP] Skip writing session for all API GET requests #1292
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
base: master
Are you sure you want to change the base?
Conversation
8e377b4
to
7326d60
Compare
7326d60
to
4ba6c6a
Compare
after_action :skip_session_write | ||
|
||
def skip_session_write | ||
request.session_options[:skip] = true if %w[GET HEAD].include?(request.request_method) |
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.
Thought we used OPTIONS?
Probably not too many times but may be a minor addition
request.session_options[:skip] = true if %w[GET HEAD].include?(request.request_method) | |
request.session_options[:skip] = true if %w[GET HEAD OPTIONS].include?(request.request_method) |
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'm fine with adding it. Do you have any idea why this isn't the default? Why would you want contention on session writes for read only options like GET/HEAD/OPTIONS? Maybe to update ttl to timeout cookies?
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.
yea. great question.
I guess everything can have side effects?
Whether changing a cached value or a counter or who knows what.
But I like where you are heading here.
In our main app, it is not rest. GET explorer or GET children for an explorer does modify the session and the request is sticky.
polling on whether a task is complete or fetching a pdf uses the session (possibly modifies 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.
ttl is actually an important one - every GET should technically extend the session time - but we can also do that differently - perhaps with a dedicated memcached d that isn't part of the session content
4ba6c6a
to
5a2b75b
Compare
after_action :skip_session_write | ||
|
||
def skip_session_write | ||
request.session_options[:skip] = true if %w[GET HEAD OPTIONS].include?(request.request_method) |
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.
In discussion, Jason thinks we might not need to gate this for GET as all requests should be able to bypass writing session. 🙏 ❤️
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.
We should make sure some pages using react work after trying to bypass session writes for all request... also make sure API requests outside of the UI still work when reusing a token.
TODO: Determine if we can filter this to just GET requests and if we need to watch for the CSRF protections which generally require saving cookies.
Before
Note the /api/notifications write_session below:
After
Including the UI classic change in ManageIQ/manageiq-ui-classic#9533
Note, there is no /api/notifications GET write_session line: