-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Mutable rich session objects don't work with session's saveDelta() #177
Comments
@brentonr storing mutable data in a session is considered bad practice and leads to hard to track issues. Implementing some sort of dirty tracking via proxies would be possible but wouldn`t solve all inherent problems that mutable state in a session causes. There is a nice read why mutable state in a session is an issue: |
@brentonr This is expected behavior and is the same when using a container's distributed sessions (i.e. Tomcat can persist sessions to a database. If you want this functionality, you could implement SessionRepository yourself using something that is returning proxied objects. For example, you might implement the store using JPA. |
@AndreasKl I agree totally. @rwinch I was hoping there was something simpler that was available where I could use spring-session with spring-security-oauth2 "out of the box". Thanks to both of you for your feedback. |
@brentonr Thanks for your response. If there is something that is preventing spring-security-oauth2 from working we can and should certainly make that happen out of the box. I just don't think that this is the correct way of doing it. Please make a more specific issue stating the problem (perhaps providing a sample too). |
I haven't looked at the 1.0.1 release yet to see if it somehow changes things, but I was using spring-security-oauth2 with spring-session-1.0.0. The problem is in Because the RequestMapping controllers in Thus, it seems all modifications get lost between requests. I've worked around it by re-setting all present attributes in the session in a filter, but I'd be happy to try any suggestions. |
FWIW, I didn't explicitly call this out in previous comments, but this seems to only be a problem with
|
Thanks for bringing this to my attention. This doesn't align with the way I recall things working in the past. Perhaps things have changed or (more likely) my memory is poor. The problem with saving every attribute on every request can have a pretty significant impact on performance. I suppose we could look into providing a flag to save every attribute every time. Thanks again for the feedback. I'll reopen this to look into other options and further investigate how other implementations work. |
Not every library would :
I don't think this is "spring-session"'s fault. But provide some fix in "spring-session" would be most convenient (We could not wait every 3rd library to fix it)。What about this suggestion : Modify RedisOperationsSessionRepository to provide a switch (system property? or add some APIs), this switch enable different delta saving method:
I think the second way would fix most situation. |
Did version 1.2.0-Release fix this problem, I'm also wating for a new version to solve the problem |
Any update on this? I have taken a look at the implementation of RedisOperationsSessionRepository and only explicit calls to setAttribute will trigger an attribute to be saved again. |
When a mutable object is referenced in a fetched session, and that object is modified, the attribute is never put into the delta list for saving to redis.
This was observed using
spring-session
withspring-security-oauth2
, specifically withorg.springframework.security.oauth2.provider.endpoint.AuthorizationEndpoint
and it's@SessionAttrtibutes("authorizationRequest")
annotation. While the authorizationRequest object (map in this case) is modified, the attribute "authorizationRequest" is not entered into the delta. On a subsequent POST, the request is unauthorized because the authorizationRequest map is not up to date in the session.The text was updated successfully, but these errors were encountered: