-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
Abstract out session store from SessionManager #4567
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,6 @@ | |
// <summary>Manages all user session data</summary> | ||
//----------------------------------------------------------------------- | ||
|
||
using System.Collections.Concurrent; | ||
using Csla.State; | ||
|
||
namespace Csla.Blazor.State | ||
|
@@ -16,23 +15,30 @@ namespace Csla.Blazor.State | |
/// root DI container. | ||
/// </summary> | ||
/// <param name="sessionIdManager"></param> | ||
public class SessionManager(ISessionIdManager sessionIdManager) : ISessionManager | ||
/// <param name="sessionStore"></param> | ||
public class SessionManager(ISessionIdManager sessionIdManager, ISessionStore sessionStore) : ISessionManager | ||
{ | ||
private readonly ConcurrentDictionary<string, Session> _sessions = []; | ||
private readonly ISessionIdManager _sessionIdManager = sessionIdManager; | ||
private readonly ISessionStore _sessionStore = sessionStore; | ||
|
||
/// <summary> | ||
/// Gets the session data for the current user. | ||
/// </summary> | ||
public Session GetSession() | ||
{ | ||
Session result; | ||
var key = _sessionIdManager.GetSessionId(); | ||
if (!_sessions.ContainsKey(key)) | ||
_sessions.TryAdd(key, []); | ||
result = _sessions[key]; | ||
result.Touch(); | ||
return result; | ||
var session = _sessionStore.GetSession(key); | ||
if (session == null) | ||
{ | ||
session = []; | ||
session.Touch(); | ||
_sessionStore.CreateSession(key, session); | ||
return session; | ||
} | ||
|
||
session.Touch(); | ||
_sessionStore.UpdateSession(key, session); | ||
return session; | ||
} | ||
|
||
/// <summary> | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you suppress with |
||
Replace(newSession, existingSession); | ||
existingSession.Touch(); | ||
_sessionStore.UpdateSession(key, existingSession); | ||
} | ||
|
||
/// <summary> | ||
/// Remove all expired session data. | ||
/// </summary> | ||
/// <param name="expiration">Expiration duration</param> | ||
public void PurgeSessions(TimeSpan expiration) | ||
{ | ||
_sessionStore.DeleteSessions(new SessionsFilter { Expiration = expiration }); | ||
} | ||
|
||
/// <summary> | ||
|
@@ -62,21 +78,6 @@ private static void Replace(Session newSession, Session oldSession) | |
oldSession.Add(key, newSession[key]); | ||
} | ||
|
||
/// <summary> | ||
/// Remove all expired session data. | ||
/// </summary> | ||
/// <param name="expiration">Expiration duration</param> | ||
public void PurgeSessions(TimeSpan expiration) | ||
{ | ||
var expirationTime = DateTimeOffset.UtcNow.ToUnixTimeSeconds() - expiration.TotalSeconds; | ||
List<string> toRemove = []; | ||
foreach (var session in _sessions) | ||
if (session.Value.LastTouched < expirationTime) | ||
toRemove.Add(session.Key); | ||
foreach (var key in toRemove) | ||
_sessions.TryRemove(key, out _); | ||
} | ||
|
||
// wasm client-side methods | ||
Task<Session> ISessionManager.RetrieveSession(TimeSpan timeout) => throw new NotImplementedException(); | ||
Session ISessionManager.GetCachedSession() => throw new NotImplementedException(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
//----------------------------------------------------------------------- | ||
// <copyright file="StateManager.cs" company="Marimer LLC"> | ||
// Copyright (c) Marimer LLC. All rights reserved. | ||
// Website: https://cslanet.com | ||
// </copyright> | ||
// <summary>Default implementation of session storage.</summary> | ||
//----------------------------------------------------------------------- | ||
#nullable enable | ||
|
||
using System.Collections.Concurrent; | ||
using Csla.State; | ||
|
||
namespace Csla.Blazor.State | ||
{ | ||
/// <summary> | ||
/// Default implementation of <see cref="ISessionStore"/> | ||
/// </summary> | ||
public class DefaultSessionStore : ISessionStore | ||
{ | ||
private readonly ConcurrentDictionary<string, Session> _store = new(); | ||
|
||
/// <inheritdoc /> | ||
public Session? GetSession(string key) | ||
{ | ||
_store.TryGetValue(key, out var item); | ||
return item; | ||
} | ||
|
||
/// <inheritdoc /> | ||
public void CreateSession(string key, Session session) | ||
{ | ||
if (!_store.TryAdd(key, session)) | ||
{ | ||
throw new Exception("Key already exists"); | ||
} | ||
} | ||
|
||
/// <inheritdoc /> | ||
public void UpdateSession(string key, Session session) | ||
{ | ||
ArgumentNullException.ThrowIfNull(session); | ||
_store[key] = session; | ||
} | ||
|
||
/// <inheritdoc /> | ||
public void DeleteSession(string key) | ||
{ | ||
_store.TryRemove(key, out _); | ||
} | ||
|
||
/// <inheritdoc /> | ||
public void DeleteSessions(SessionsFilter filter) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this method name. Perhaps it should be more explicit that this is expiring sessions? |
||
{ | ||
filter.Validate(); | ||
|
||
var query = _store.AsQueryable(); | ||
if (filter.Expiration.HasValue) | ||
{ | ||
var expirationTime = DateTimeOffset.UtcNow.ToUnixTimeSeconds() - filter.Expiration.Value.TotalSeconds; | ||
query = query.Where(x => x.Value.LastTouched < expirationTime); | ||
} | ||
|
||
var keys = query.Select(x => x.Key).ToArray(); | ||
|
||
foreach (var key in keys) | ||
{ | ||
_store.TryRemove(key, out _); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes please :) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
//----------------------------------------------------------------------- | ||
// <copyright file="ISessionStore.cs" company="Marimer LLC"> | ||
// Copyright (c) Marimer LLC. All rights reserved. | ||
// Website: https://cslanet.com | ||
// </copyright> | ||
// <summary>Manages session storage</summary> | ||
//----------------------------------------------------------------------- | ||
#nullable enable | ||
|
||
namespace Csla.State | ||
{ | ||
/// <summary> | ||
/// Session store | ||
/// </summary> | ||
public interface ISessionStore | ||
{ | ||
/// <summary> | ||
/// Retrieves a session | ||
/// </summary> | ||
/// <param name="key"></param> | ||
/// <returns>The session for the given key, or default if not found</returns> | ||
Session? GetSession(string key); | ||
|
||
/// <summary> | ||
/// Creates a session | ||
/// </summary> | ||
/// <param name="key"></param> | ||
/// <param name="session"></param> | ||
void CreateSession(string key, Session session); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could the create/update be combined to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
/// <summary> | ||
/// Updates a session | ||
/// </summary> | ||
/// <param name="key"></param> | ||
/// <param name="session"></param> | ||
void UpdateSession(string key, Session session); | ||
|
||
/// <summary> | ||
/// Deletes a session | ||
/// </summary> | ||
/// <param name="key"></param> | ||
void DeleteSession(string key); | ||
|
||
/// <summary> | ||
/// Deletes sessions based on the filter. | ||
/// </summary> | ||
/// <param name="filter"></param> | ||
void DeleteSessions(SessionsFilter filter); | ||
} | ||
} |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. Currently this class is part of the public API. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
//----------------------------------------------------------------------- | ||
// <copyright file="SessionsFilter.cs" company="Marimer LLC"> | ||
// Copyright (c) Marimer LLC. All rights reserved. | ||
// Website: https://cslanet.com | ||
// </copyright> | ||
// <summary>Filter for querying session storage</summary> | ||
//----------------------------------------------------------------------- | ||
#nullable enable | ||
|
||
namespace Csla.State | ||
{ | ||
/// <summary> | ||
/// Filter to query sessions | ||
/// </summary> | ||
public class SessionsFilter | ||
{ | ||
/// <summary> | ||
/// A timespan to filter sessions last touched after the expiration | ||
/// </summary> | ||
public TimeSpan? Expiration { get; set; } | ||
|
||
/// <summary> | ||
/// Validates | ||
/// </summary> | ||
public void Validate() | ||
{ | ||
if (!Expiration.HasValue) | ||
{ | ||
throw new ArgumentNullException("Expiration is required."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No hard-coded use of English is allowed in the framework. This needs to be from a resource string or be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion to get rid of this exception would be: Don't allow the creation of an invalid object state.
With those changes the whole |
||
} | ||
} | ||
} | ||
} |
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 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?