Skip to content

Add SafeRetrievingSessionRepository #646

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

Closed
wants to merge 1 commit into from

Conversation

vpavic
Copy link
Contributor

@vpavic vpavic commented Oct 8, 2016

This resolves #529.

I've originally looked into the possibility of having SessionRepository implementations throw consistent exceptions during deserialization, but this proved to be quite a tricky business with implementations providing users the options to define custom serialization of Sessionss.

Considering above said, and the fact that this decorator is opt-in, I've implemented it to require the users to explicitly define the set of exceptions they'd like to ignore. That made the implementation not explicitly tied to deserialization concerns so I've named it SafeRetrievingSessionRepository (vs originally suggested SafeDeserializingSessionRepository).

Some further notes:

  • the SafeRetrievingSessionRepository itself is an FindByIndexNameSessionRepository, however it's capable of wrapping SessionRepository instances too
  • solution proposal outlined in Add a SafeDeserializingSessionRepository #529 always deleted the session it was unable to deserialize, but I've made it possible to disable such behavior
  • perhaps it would be a good idea to provide an option to log the occurrence of ignored exception?

TODOs:

  • documentation
  • sample app


public Map<String, S> findByIndexNameAndIndexValue(String indexName, String indexValue) {
try {
return this.delegate.findByIndexNameAndIndexValue(indexName, indexValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible some of the sessions are deserializable while some are not? It would seem nice to be able to return just the ones that are deserializable and delete the ones that are not. Otherwise, repeated attempts to find will always result in an empty result (until any configured timeout of the sessions that cannot be deserialized). I'm not sure how this can be accomplished in a way that supports any FindByIndexNameSessionRepository implementation, though.
At least documenting the behavior in the JavaDoc for this method would be nice, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for raising this concern @shakuzen - I've planned to comment on findByIndexNameAndIndexValue behavior but forgot. I'm not sure what would be the best way to address this due to exact reasons you've described. Delegates will fail even if only one of the results is not deserializable and I don't see a way around it.

@vpavic vpavic modified the milestone: 1.4.0 M1 Dec 16, 2016
@vpavic vpavic changed the base branch from master to 1.4.x December 20, 2016 20:51
@vpavic
Copy link
Contributor Author

vpavic commented Dec 20, 2016

Rebased the PR against 1.4.x.

}
}

public void delete(String id) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I have just tested the above using Redis and calling delegate.delete results in a serialization issue as RedisOperationsSessionRepository goes away and tries to get the session again
RedisSession session = getSession(sessionId, true);

Not sure if this is an issue with this code or with RedisOperationsSessionRepository. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for giving this a test run @davidmelia. It's a problem with this code since it doesn't take into account that delegate's delete operation (or any other for that matter) could internally call an operation that can result in a deserialization error.

I guess this is a case in point for implementing some sort of deserialization strategy vs the session repository decorator pattern.

@vpavic
Copy link
Contributor Author

vpavic commented Mar 17, 2017

I'm closing this since decorator approach doesn't seem suitable for addressing the original problem as there are no guarantees any non-decorated repository operation won't internally call getSession thus causing deserialization error, as demonstrated by RedisOperationsSessionRepository where delete operation internally calls getSession (see comment).

Another problematic bit is the configuration as we would need to register both delegating implementation and SafeRetrievingSessionRepository decorator as beans, and mark decorator as @Primary.

@vpavic vpavic closed this Mar 17, 2017
@vpavic vpavic removed this from the 1.4.0 M1 milestone Mar 17, 2017
@YSavitski
Copy link

Hi @vpavic . I'm using spring-session 2.1.6 and also have such Serialization issue
org.springframework.data.redis.serializer.SerializationException: Cannot deserialize; nested exception is org.springframework.core.serializer.support.SerializationFailedException: Failed to deserialize payload. Is the byte array a result of corresponding serialization for DefaultDeserializer?; nested exception is java.io.InvalidClassException: de.conrad.oci.sessionManagementService.Session; local class incompatible: stream classdesc serialVersionUID = -5426516310069563679, local class serialVersionUID = -3942004278227141456

i found out the article https://sdqali.in/blog/2016/11/02/handling-deserialization-errors-in-spring-redis-sessions/ but i'm not if this solution still is relevant for current spring-session version.

@vpavic Could you advise something for closing my issue?

@vpavic
Copy link
Contributor Author

vpavic commented May 22, 2019

@YSavitski, could you comment on #529 instead so we don't lose track of your comments?

This PR was closed as it didn't address the problem from #529 adequately.

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.

4 participants