Abstract out session store from SessionManager #4567
Conversation
Added abstractions and a default implementation for a session store to be used with the Blazor state SessionManager. The default implementation continues to use in-memory persistence in a ConcurrentDictionary.
joshhanson314
changed the title
There was a problem hiding this comment.
I think @StefanOssendorf would be much happier if this had async methods instead of sync methods 😁
Seriously though, if we're going to improve the code like this, I do think we should expect that people will use async stores for the data.
There was a problem hiding this comment.
Yes please :)
You can always do sync with a Task/ValueTask method but not async with a void method :)
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public void DeleteSessions(SessionsFilter filter) |
There was a problem hiding this comment.
I don't like this method name. Perhaps it should be more explicit that this is expiring sessions?
| /// </summary> | ||
| /// <param name="key"></param> | ||
| /// <param name="session"></param> | ||
| void CreateSession(string key, Session session); |
There was a problem hiding this comment.
Could the create/update be combined to a TryUpdate or something similar?
There was a problem hiding this comment.
AddOrUpdate like the ConcurrentDictionary methods?
| { | ||
| if (!Expiration.HasValue) | ||
| { | ||
| throw new ArgumentNullException("Expiration is required."); |
There was a problem hiding this comment.
No hard-coded use of English is allowed in the framework. This needs to be from a resource string or be ArgumentException(nameof(HasValue))
There was a problem hiding this comment.
My suggestion to get rid of this exception would be: Don't allow the creation of an invalid object state.
- Make
Expirationnon nullable. - Either add a constructor expecting the value and/or make the property
required
With those changes the whole Validate() method becomes obsolete.
There was a problem hiding this comment.
This is basically internal to the default implementation right?
As in, if someone were to use a different session store they'd probably use their own approach for expiring old data.
There was a problem hiding this comment.
No. Currently this class is part of the public API.
ISessionStore.DeleteSession(SessionsFilter)
There was a problem hiding this comment.
I really appreciate this refactoring - absolutely on the right track. While we're doing this, might as well also embrace async.
Perhaps also consider using file scoped namespaces so Simon doesn't need to come back later and clean that up?
| @@ -44,9 +50,19 @@ public void UpdateSession(Session newSession) | |||
| { | |||
| ArgumentNullException.ThrowIfNull(newSession); | |||
| var key = _sessionIdManager.GetSessionId(); | |||
| var existingSession = _sessions[key]; | |||
| var existingSession = _sessionStore.GetSession(key)!; | |||
There was a problem hiding this comment.
Why do you suppress with !? You should check for it and handle the default case.
|
@rockfordlhotka This is a change for v10. It's a breaking one ;) |
#4565 Added abstractions and a default implementation for a session store to be used with the Blazor state SessionManager. The default implementation continues to use in-memory persistence in a ConcurrentDictionary.