-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Session creation with pre-specified id. #1545
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
Conversation
While i'm aware of MR: #204 . Original author is inactive for last 5 years, and there are merge conflicts - which i don't think he will resolve any time soon. This feature is on general backlog since 30 Jan. And while this MR does things differently (not-strategy approach) it provides sollution that does not collide with future strategy-way of handling things. |
I've added second MR : #1547 if this one is not an acceptable way of handling things. |
Are there any plans to have this kind of feature in the release version? Integration with existing session management systems where id creation is delegated to external system is a very useful feature. |
|
||
@Override | ||
public RedisSession createSession(final String id) { | ||
MapSession cached = Optional.ofNullable(id).map(MapSession::new).orElse(new MapSession()); |
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.
It's better to use .orElseGet(MapSession::new)
to avoid creating an unnecessary object if id
is provided.
} | ||
|
||
@Override | ||
public RedisSession createSession(final String id) { |
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.
final
is here redundant, it's not used in other methods.
@zapp88 hi! |
I'd also like to have this feature place. When is it going to be merged? |
} | ||
|
||
@Override | ||
public HazelcastSession createSession(final String id) { |
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.
please make the HazelcastSession
public
, so that can override
Closing in favor of #2286 |
This pull request address specific use-case of creating session with pre-specified id.
While standard
SessionRepository
interfaces provides means of creating session viacreateSession()
- it does noting to let you control the 'id' creation process. All known to me implementation ofSessionRepository
(Redis,Hazelcast,Jdbc
) fallback to callingMapSession
and what is peculiarMapSession
supports creating session object with externally defined id - via its secondary construtor -new MapSession(id)
.However all implementations (
Redis,Hazelcast,Jdbc
) fallback to using the default construtor which in its default bahaviour callsUUID.randomUUID().toString()
. While this provides necessary uniqueness to id (provied low probability of UUID ever repeating) it leaves us without controll over this process.There are quite a few business casses when internaly created id would not be desired :
We don't want to break any existing functionality (keeping
SessionRepository
as clean as possible) - here comesCreateWithIdSessionRepository
- it expands default implementation with ability to create session with pre-specified id viaS createSession(@Nullable String id);
the same way
FindByIndexNameSessionRepository
expands session repository with ability to find session with principal/indexName.I took upon myself implementing this interfaces in 3 most common used implementations
(Redis,Hazelcast,Jdbc)
since it does not break any existing functionality while still providing benefit on controlling this process by adhering to proper interface.There are no reasons it could not be supported in other available implementations.
PS. Sorry for my english. English is not my primary language.
If you have any suggestions on how i could improve my approach or would like to ask me a question - feel free to contact me. My email - [email protected]