From 7eb5d9849c3ea60ee953f3dd51627b78b8ffd6f7 Mon Sep 17 00:00:00 2001 From: Mike Alhayek Date: Fri, 1 Nov 2024 13:05:14 -0700 Subject: [PATCH 1/3] Fix the User TimeZoneService #16932 --- .../Drivers/UserTimeZoneDisplayDriver.cs | 2 +- .../TimeZone/Services/IUserTimeZoneService.cs | 16 +++++ .../TimeZone/Services/UserTimeZoneSelector.cs | 18 +++--- .../TimeZone/Services/UserTimeZoneService.cs | 61 +++++++++++-------- 4 files changed, 63 insertions(+), 34 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs index 49091f6def9..40f6c45dfa2 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs @@ -33,7 +33,7 @@ public override async Task UpdateAsync(User user, UserTimeZone u userTimeZone.TimeZoneId = model.TimeZoneId; // Remove the cache entry, don't update it, as the form might still fail validation for other reasons. - await _userTimeZoneService.UpdateAsync(user); + await _userTimeZoneService.ForgetCacheAsync(user.UserName); return await EditAsync(user, userTimeZone, context); } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs index e7fcec8ccdb..e25ccaa79b2 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs @@ -11,11 +11,27 @@ public interface IUserTimeZoneService /// Gets the time zone for the given user. /// /// The . + [Obsolete("This method is obsolete and will be removed in future release. Instead use GetAsync(userName).")] ValueTask GetAsync(IUser user); /// /// Updates the time zone for the given user. /// /// The . + [Obsolete("This method is obsolete and will be removed in future release. Instead use ForgetCacheAsync(userName).")] ValueTask UpdateAsync(IUser user); + + /// + /// Gets the time zone for the given user. + /// + /// The user name. + ValueTask GetAsync(string userName) + => ValueTask.FromResult(null); + + /// + /// Forget the time zone for the given user. + /// + /// The user name. + ValueTask ForgetCacheAsync(string userName) + => ValueTask.CompletedTask; } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs index f2b25c2ef28..20b4edc8bf2 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs @@ -10,29 +10,31 @@ namespace OrchardCore.Users.TimeZone.Services; public class UserTimeZoneSelector : ITimeZoneSelector { private readonly IUserTimeZoneService _userTimeZoneService; - private readonly UserManager _userManager; private readonly IHttpContextAccessor _httpContextAccessor; public UserTimeZoneSelector( IUserTimeZoneService userTimeZoneService, - UserManager userManager, IHttpContextAccessor httpContextAccessor) { _userTimeZoneService = userTimeZoneService; - _userManager = userManager; _httpContextAccessor = httpContextAccessor; } - public async Task GetTimeZoneAsync() + public Task GetTimeZoneAsync() { - var currentUser = await _userManager.GetUserAsync(_httpContextAccessor.HttpContext.User); - - return currentUser == null + var result = !_httpContextAccessor.HttpContext.User.Identity.IsAuthenticated ? null : new TimeZoneSelectorResult { Priority = 100, - TimeZoneId = async () => (await _userTimeZoneService.GetAsync(currentUser))?.TimeZoneId + TimeZoneId = async () => + { + var timeZone = await _userTimeZoneService.GetAsync(_httpContextAccessor.HttpContext.User.Identity.Name); + + return timeZone.TimeZoneId; + }, }; + + return Task.FromResult(result); } } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs index cd4511d20ab..3891b64e361 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs @@ -1,3 +1,4 @@ +using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Caching.Distributed; using OrchardCore.Entities; using OrchardCore.Modules; @@ -11,25 +12,31 @@ public class UserTimeZoneService : IUserTimeZoneService private const string CacheKey = "UserTimeZone/"; private const string EmptyTimeZone = "empty"; - private static readonly DistributedCacheEntryOptions _slidingExpiration = new() { SlidingExpiration = TimeSpan.FromHours(1) }; + private static readonly DistributedCacheEntryOptions _slidingExpiration = new() + { + SlidingExpiration = TimeSpan.FromHours(1), + }; private readonly IClock _clock; private readonly IDistributedCache _distributedCache; + private readonly UserManager _userManager; public UserTimeZoneService( IClock clock, - IDistributedCache distributedCache) + IDistributedCache distributedCache, + UserManager userManager) { _clock = clock; _distributedCache = distributedCache; + _userManager = userManager; } /// - public async ValueTask GetAsync(IUser user) + public async ValueTask GetAsync(string userName) { - ArgumentNullException.ThrowIfNull(user); + ArgumentException.ThrowIfNullOrEmpty(userName); - var currentTimeZoneId = await GetTimeZoneIdAsync(user); + var currentTimeZoneId = await GetTimeZoneIdAsync(userName); if (string.IsNullOrEmpty(currentTimeZoneId)) { @@ -40,40 +47,43 @@ public async ValueTask GetAsync(IUser user) } /// - public async ValueTask UpdateAsync(IUser user) + public async ValueTask ForgetCacheAsync(string userName) { - ArgumentNullException.ThrowIfNull(user); - - if (string.IsNullOrEmpty(user.UserName)) - { - return; - } + ArgumentException.ThrowIfNullOrEmpty(userName); - await _distributedCache.RemoveAsync(GetCacheKey(user.UserName)); + await _distributedCache.RemoveAsync(GetCacheKey(userName)); } /// - private async ValueTask GetTimeZoneIdAsync(IUser user) - { - if (string.IsNullOrEmpty(user.UserName)) - { - return null; - } + public ValueTask GetAsync(IUser user) + => GetAsync(user?.UserName); + + /// + public ValueTask UpdateAsync(IUser user) + => ForgetCacheAsync(user?.UserName); - var key = GetCacheKey(user.UserName); + /// + private async ValueTask GetTimeZoneIdAsync(string userName) + { + var key = GetCacheKey(userName); var timeZoneId = await _distributedCache.GetStringAsync(key); - // The timezone is not cached yet, resolve it and store the value + // The timeZone is not cached yet, resolve it and store the value. if (string.IsNullOrEmpty(timeZoneId)) { + // At this point, we know the timeZoneId is not cached for the given userName. + // Retrieve the user and cache the timeZoneId. + var user = await _userManager.FindByNameAsync(userName); + if (user is User u) { timeZoneId = u.As()?.TimeZoneId; } - // We store a special string to remember there is no specific value for this user. - // And actual distributed cache implementation might not be able to store null values. + // We store a placeholder string to indicate that there is no specific value for this user. + // This approach ensures compatibility with distributed cache implementations that may not support null values. + // Caching this placeholder helps avoid redundant queries for this user on each request when no time zone is set. if (string.IsNullOrEmpty(timeZoneId)) { timeZoneId = EmptyTimeZone; @@ -82,7 +92,7 @@ private async ValueTask GetTimeZoneIdAsync(IUser user) await _distributedCache.SetStringAsync(key, timeZoneId, _slidingExpiration); } - // Do we know this user doesn't have a configured value? + // If TimeZoneId matches the placeholder value, we return null instead of the placeholder itself. if (timeZoneId == EmptyTimeZone) { return null; @@ -91,5 +101,6 @@ private async ValueTask GetTimeZoneIdAsync(IUser user) return timeZoneId; } - private static string GetCacheKey(string userName) => CacheKey + userName; + private static string GetCacheKey(string userName) + => CacheKey + userName; } From c7c065c53aaa599151c7147af4e823cd8020cb1f Mon Sep 17 00:00:00 2001 From: Mike Alhayek Date: Sat, 2 Nov 2024 10:15:32 -0700 Subject: [PATCH 2/3] Simplify the service --- .../Drivers/UserTimeZoneDisplayDriver.cs | 12 +------ .../TimeZone/Services/IUserTimeZoneService.cs | 11 ++----- .../TimeZone/Services/UserTimeZoneService.cs | 32 ++++++++++++------- .../OrchardCore.Users/TimeZone/Startup.cs | 5 ++- .../Handlers/UserEventHandlerBase.cs | 27 ++++++++++------ 5 files changed, 46 insertions(+), 41 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs index 40f6c45dfa2..cc010425b0b 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Drivers/UserTimeZoneDisplayDriver.cs @@ -3,20 +3,12 @@ using OrchardCore.DisplayManagement.Views; using OrchardCore.Users.Models; using OrchardCore.Users.TimeZone.Models; -using OrchardCore.Users.TimeZone.Services; using OrchardCore.Users.TimeZone.ViewModels; namespace OrchardCore.Users.TimeZone.Drivers; public sealed class UserTimeZoneDisplayDriver : SectionDisplayDriver { - private readonly IUserTimeZoneService _userTimeZoneService; - - public UserTimeZoneDisplayDriver(IUserTimeZoneService userTimeZoneService) - { - _userTimeZoneService = userTimeZoneService; - } - public override IDisplayResult Edit(User user, UserTimeZone userTimeZone, BuildEditorContext context) { return Initialize("UserTimeZone_Edit", model => @@ -30,10 +22,8 @@ public override async Task UpdateAsync(User user, UserTimeZone u var model = new UserTimeZoneViewModel(); await context.Updater.TryUpdateModelAsync(model, Prefix); - userTimeZone.TimeZoneId = model.TimeZoneId; - // Remove the cache entry, don't update it, as the form might still fail validation for other reasons. - await _userTimeZoneService.ForgetCacheAsync(user.UserName); + userTimeZone.TimeZoneId = model.TimeZoneId; return await EditAsync(user, userTimeZone, context); } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs index e25ccaa79b2..8ac2ed17c7f 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/IUserTimeZoneService.cs @@ -11,14 +11,14 @@ public interface IUserTimeZoneService /// Gets the time zone for the given user. /// /// The . - [Obsolete("This method is obsolete and will be removed in future release. Instead use GetAsync(userName).")] + [Obsolete("This method is deprecated and will be removed in a future release. Please use GetAsync(userName) instead..")] ValueTask GetAsync(IUser user); /// /// Updates the time zone for the given user. /// /// The . - [Obsolete("This method is obsolete and will be removed in future release. Instead use ForgetCacheAsync(userName).")] + [Obsolete("This method is obsolete and will be removed in a future release. Cache invalidation is now handled automatically.")] ValueTask UpdateAsync(IUser user); /// @@ -27,11 +27,4 @@ public interface IUserTimeZoneService /// The user name. ValueTask GetAsync(string userName) => ValueTask.FromResult(null); - - /// - /// Forget the time zone for the given user. - /// - /// The user name. - ValueTask ForgetCacheAsync(string userName) - => ValueTask.CompletedTask; } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs index 3891b64e361..4077e66d1db 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneService.cs @@ -2,15 +2,16 @@ using Microsoft.Extensions.Caching.Distributed; using OrchardCore.Entities; using OrchardCore.Modules; +using OrchardCore.Users.Handlers; using OrchardCore.Users.Models; using OrchardCore.Users.TimeZone.Models; namespace OrchardCore.Users.TimeZone.Services; -public class UserTimeZoneService : IUserTimeZoneService +public class UserTimeZoneService : UserEventHandlerBase, IUserTimeZoneService { private const string CacheKey = "UserTimeZone/"; - private const string EmptyTimeZone = "empty"; + private const string EmptyTimeZone = "NoTimeZoneFound"; private static readonly DistributedCacheEntryOptions _slidingExpiration = new() { @@ -46,21 +47,14 @@ public async ValueTask GetAsync(string userName) return _clock.GetTimeZone(currentTimeZoneId); } - /// - public async ValueTask ForgetCacheAsync(string userName) - { - ArgumentException.ThrowIfNullOrEmpty(userName); - - await _distributedCache.RemoveAsync(GetCacheKey(userName)); - } /// public ValueTask GetAsync(IUser user) => GetAsync(user?.UserName); /// - public ValueTask UpdateAsync(IUser user) - => ForgetCacheAsync(user?.UserName); + public async ValueTask UpdateAsync(IUser user) + => await ForgetCacheAsync(user?.UserName); /// private async ValueTask GetTimeZoneIdAsync(string userName) @@ -101,6 +95,22 @@ private async ValueTask GetTimeZoneIdAsync(string userName) return timeZoneId; } + public override Task DeletedAsync(UserDeleteContext context) + => ForgetCacheAsync(context.User.UserName); + + public override Task UpdatedAsync(UserUpdateContext context) + => ForgetCacheAsync(context.User.UserName); + + public override Task DisabledAsync(UserContext context) + => ForgetCacheAsync(context.User.UserName); + + private Task ForgetCacheAsync(string userName) + { + var key = GetCacheKey(userName); + + return _distributedCache.RemoveAsync(key); + } + private static string GetCacheKey(string userName) => CacheKey + userName; } diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Startup.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Startup.cs index a390d22ef15..493b03bf048 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Startup.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Startup.cs @@ -1,6 +1,7 @@ using Microsoft.Extensions.DependencyInjection; using OrchardCore.DisplayManagement.Handlers; using OrchardCore.Modules; +using OrchardCore.Users.Handlers; using OrchardCore.Users.Models; using OrchardCore.Users.TimeZone.Drivers; using OrchardCore.Users.TimeZone.Services; @@ -13,7 +14,9 @@ public sealed class Startup : StartupBase public override void ConfigureServices(IServiceCollection services) { services.AddScoped(); - services.AddScoped(); + services.AddScoped(); + services.AddScoped(sp => sp.GetRequiredService()); + services.AddScoped(sp => sp.GetRequiredService()); services.AddScoped, UserTimeZoneDisplayDriver>(); } } diff --git a/src/OrchardCore/OrchardCore.Users.Abstractions/Handlers/UserEventHandlerBase.cs b/src/OrchardCore/OrchardCore.Users.Abstractions/Handlers/UserEventHandlerBase.cs index 6cca02c3dfc..45c7ac9f5b0 100644 --- a/src/OrchardCore/OrchardCore.Users.Abstractions/Handlers/UserEventHandlerBase.cs +++ b/src/OrchardCore/OrchardCore.Users.Abstractions/Handlers/UserEventHandlerBase.cs @@ -3,29 +3,38 @@ namespace OrchardCore.Users.Handlers; public abstract class UserEventHandlerBase : IUserEventHandler { /// - public virtual Task CreatingAsync(UserCreateContext context) => Task.CompletedTask; + public virtual Task CreatingAsync(UserCreateContext context) + => Task.CompletedTask; /// - public virtual Task CreatedAsync(UserCreateContext context) => Task.CompletedTask; + public virtual Task CreatedAsync(UserCreateContext context) + => Task.CompletedTask; /// - public virtual Task DeletingAsync(UserDeleteContext context) => Task.CompletedTask; + public virtual Task DeletingAsync(UserDeleteContext context) + => Task.CompletedTask; /// - public virtual Task DeletedAsync(UserDeleteContext context) => Task.CompletedTask; + public virtual Task DeletedAsync(UserDeleteContext context) + => Task.CompletedTask; /// - public virtual Task UpdatingAsync(UserUpdateContext context) => Task.CompletedTask; + public virtual Task UpdatingAsync(UserUpdateContext context) + => Task.CompletedTask; /// - public virtual Task UpdatedAsync(UserUpdateContext context) => Task.CompletedTask; + public virtual Task UpdatedAsync(UserUpdateContext context) + => Task.CompletedTask; /// - public virtual Task DisabledAsync(UserContext context) => Task.CompletedTask; + public virtual Task DisabledAsync(UserContext context) + => Task.CompletedTask; /// - public virtual Task EnabledAsync(UserContext context) => Task.CompletedTask; + public virtual Task EnabledAsync(UserContext context) + => Task.CompletedTask; /// - public virtual Task ConfirmedAsync(UserConfirmContext context) => Task.CompletedTask; + public virtual Task ConfirmedAsync(UserConfirmContext context) + => Task.CompletedTask; } From 1018f71bf706af10e9c357b518a6da427496d65c Mon Sep 17 00:00:00 2001 From: Mike Alhayek Date: Tue, 5 Nov 2024 15:57:56 -0800 Subject: [PATCH 3/3] null check --- .../TimeZone/Services/UserTimeZoneSelector.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs index 20b4edc8bf2..b0523e7e639 100644 --- a/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs +++ b/src/OrchardCore.Modules/OrchardCore.Users/TimeZone/Services/UserTimeZoneSelector.cs @@ -1,5 +1,4 @@ using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Identity; using OrchardCore.Modules; namespace OrchardCore.Users.TimeZone.Services; @@ -31,7 +30,7 @@ public Task GetTimeZoneAsync() { var timeZone = await _userTimeZoneService.GetAsync(_httpContextAccessor.HttpContext.User.Identity.Name); - return timeZone.TimeZoneId; + return timeZone?.TimeZoneId; }, };