From 08b2a9c1f67831b8860235714525fc99061425ce Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Wed, 23 Jul 2025 10:56:11 +0200 Subject: [PATCH 1/5] Add a fallback for null cache client in Remote.cs Add a fallback for null cache clients in case a null implementation is passed through DI by external consumers. This ensures that the applicationcan still function without a cache client, preventing potential null reference exceptions. --- src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index 6ba32349b7..70ff066a46 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -53,7 +53,7 @@ public Remote( _remoteFactory = remoteFactory; _locationResolver = locationResolver; _fileManager = new DependencyFileManager(remoteGitClient, _versionDetailsParser, _logger); - _cache = cacheClient; + _cache = cacheClient ?? new NoOpRedisClient(); // caching is disabled by default, only used in by specific consumers } public async Task CreateNewBranchAsync(string repoUri, string baseBranch, string newBranch) From 7015283d8f73e0aefa95b8b6de5c3dfea49089e4 Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Wed, 23 Jul 2025 11:32:23 +0200 Subject: [PATCH 2/5] throw if client is null, modify docstrings --- .../Darc/Options/CommandLineOptions.cs | 2 +- .../DarcLib/IRedisCacheClient.cs | 32 ------------------- src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | 9 ++++-- .../ProductConstructionServiceExtension.cs | 2 +- .../RedisCacheClient.cs | 11 ++++--- .../RemoteTests.cs | 2 +- .../PullRequestPolicyFailureNotifierTests.cs | 2 +- 7 files changed, 16 insertions(+), 44 deletions(-) delete mode 100644 src/Microsoft.DotNet.Darc/DarcLib/IRedisCacheClient.cs diff --git a/src/Microsoft.DotNet.Darc/Darc/Options/CommandLineOptions.cs b/src/Microsoft.DotNet.Darc/Darc/Options/CommandLineOptions.cs index b539536a5b..220735786e 100644 --- a/src/Microsoft.DotNet.Darc/Darc/Options/CommandLineOptions.cs +++ b/src/Microsoft.DotNet.Darc/Darc/Options/CommandLineOptions.cs @@ -182,7 +182,7 @@ public virtual IServiceCollection RegisterServices(IServiceCollection services) { return new VmrInfo(string.Empty, string.Empty); }); - services.TryAddSingleton(); + services.TryAddSingleton(); return services; } diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IRedisCacheClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/IRedisCacheClient.cs deleted file mode 100644 index fdc5871458..0000000000 --- a/src/Microsoft.DotNet.Darc/DarcLib/IRedisCacheClient.cs +++ /dev/null @@ -1,32 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System; -using System.Threading.Tasks; - -#nullable enable -namespace Microsoft.DotNet.DarcLib; - -public interface IRedisCacheClient -{ - Task TrySetAsync(string key, T value, TimeSpan? expiration = null) where T : class; - Task TryGetAsync(string key) where T : class; - Task DeleteAsync(string key); -} - -// This no-op redis client is used when DarcLib is invoked through CLI operations where redis is not available. -public class NoOpRedisClient : IRedisCacheClient -{ - public Task TrySetAsync(string key, T value, TimeSpan? expiration = null) where T : class - { - return Task.FromResult(false); - } - public Task TryGetAsync(string key) where T : class - { - return Task.FromResult(null); - } - public Task DeleteAsync(string key) - { - return Task.FromResult(false); - } -} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index 70ff066a46..e86fcd5c3d 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -20,13 +20,14 @@ namespace Microsoft.DotNet.DarcLib; public sealed class Remote : IRemote { + private readonly ICache _cacheClient; private readonly IVersionDetailsParser _versionDetailsParser; private readonly DependencyFileManager _fileManager; private readonly IRemoteGitRepo _remoteGitClient; private readonly ISourceMappingParser _sourceMappingParser; private readonly IRemoteFactory _remoteFactory; private readonly IAssetLocationResolver _locationResolver; - private readonly IRedisCacheClient _cache; + private readonly IDistributedCacheClient _cache; private readonly ILogger _logger; //[DependencyUpdate]: <> (Begin) @@ -43,7 +44,7 @@ public Remote( ISourceMappingParser sourceMappingParser, IRemoteFactory remoteFactory, IAssetLocationResolver locationResolver, - IRedisCacheClient cacheClient, + IDistributedCacheClient cacheClient, ILogger logger) { _logger = logger; @@ -53,7 +54,9 @@ public Remote( _remoteFactory = remoteFactory; _locationResolver = locationResolver; _fileManager = new DependencyFileManager(remoteGitClient, _versionDetailsParser, _logger); - _cache = cacheClient ?? new NoOpRedisClient(); // caching is disabled by default, only used in by specific consumers + _cache = cacheClient ?? throw new ArgumentNullException( + nameof(cacheClient), + "The cache client must not be null. Please use the no-op implementation of IDistributedCacheClient provided in this package."); } public async Task CreateNewBranchAsync(string repoUri, string baseBranch, string newBranch) diff --git a/src/ProductConstructionService/ProductConstructionService.Common/ProductConstructionServiceExtension.cs b/src/ProductConstructionService/ProductConstructionService.Common/ProductConstructionServiceExtension.cs index fdce6ef3cb..3217073b4a 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/ProductConstructionServiceExtension.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/ProductConstructionServiceExtension.cs @@ -77,7 +77,7 @@ public static async Task AddRedisCache( builder.Services.AddSingleton(redisConfig); builder.Services.AddSingleton(); - builder.Services.AddSingleton(); + builder.Services.AddSingleton(); } public static void AddMetricRecorder(this IHostApplicationBuilder builder) diff --git a/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs b/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs index 4c8aaa82b2..5a31a8eb05 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs @@ -7,11 +7,12 @@ namespace ProductConstructionService.Common; /// -/// This class acts as a delegate for RedisCache and RedisCacheFactory. -/// It is needed because DarcLib does not depend on ProductConstructionService and can only access caching through a delegate. +/// This class acts as a delegate for RedisCache and RedisCacheFactory. It is needed because DarcLib +/// does not depend on ProductConstructionService and can only implement caching there through a delegate. /// -internal class RedisCacheClient : IRedisCacheClient +internal class RedisCacheClient : IDistributedCacheClient { + private static readonly TimeSpan DefaultExpiration = TimeSpan.FromDays(15); private readonly IRedisCacheFactory _factory; private readonly ILogger _logger; @@ -26,11 +27,11 @@ public RedisCacheClient(IRedisCacheFactory factory, ILogger lo return await _factory.Create(key).TryGetStateAsync(); } - public async Task TrySetAsync(string key, T value, TimeSpan? expiration = null) where T : class + public async Task TrySetAsync(string key, T value, TimeSpan? expiration ) where T : class { try { - await _factory.Create(key).SetAsync(value, expiration); + await _factory.Create(key).SetAsync(value, expiration ?? DefaultExpiration); } catch (Exception ex) { diff --git a/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs b/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs index 559774f3db..5226e730e0 100644 --- a/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs +++ b/test/Microsoft.DotNet.DarcLib.Tests/RemoteTests.cs @@ -101,7 +101,7 @@ See [Dependency Description Format](https://github.com/dotnet/arcade/blob/main/D sourceMappingParser.Object, Mock.Of(), new AssetLocationResolver(barClient.Object), - new NoOpRedisClient(), + new NoOpCacheClient(), logger); await remote.MergeDependencyPullRequestAsync("https://github.com/test/test2", mergePullRequest); diff --git a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs index e38170c890..524cb9769d 100644 --- a/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs +++ b/test/ProductConstructionService.DependencyFlow.Tests/PullRequestPolicyFailureNotifierTests.cs @@ -101,7 +101,7 @@ where subscription.Id.Equals(subscriptionToFind) SourceMappingParser.Object, RemoteFactory.Object, new AssetLocationResolver(BarClient.Object), - new NoOpRedisClient(), + new NoOpCacheClient(), NullLogger.Instance); RemoteFactory.Setup(m => m.CreateRemoteAsync(It.IsAny())).ReturnsAsync(MockRemote); From 9c5805b1b83aa0793c74265c2a98dd9c19bc60ad Mon Sep 17 00:00:00 2001 From: Adam Date: Wed, 23 Jul 2025 11:38:04 +0200 Subject: [PATCH 3/5] Update src/Microsoft.DotNet.Darc/DarcLib/Remote.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index e86fcd5c3d..007e5c365f 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -20,7 +20,6 @@ namespace Microsoft.DotNet.DarcLib; public sealed class Remote : IRemote { - private readonly ICache _cacheClient; private readonly IVersionDetailsParser _versionDetailsParser; private readonly DependencyFileManager _fileManager; private readonly IRemoteGitRepo _remoteGitClient; From cd68b742bd3d607867a0dc70a1dc0f123c118217 Mon Sep 17 00:00:00 2001 From: Adam Date: Wed, 23 Jul 2025 11:38:10 +0200 Subject: [PATCH 4/5] Update src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../ProductConstructionService.Common/RedisCacheClient.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs b/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs index 5a31a8eb05..f578a1eb44 100644 --- a/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs +++ b/src/ProductConstructionService/ProductConstructionService.Common/RedisCacheClient.cs @@ -27,7 +27,7 @@ public RedisCacheClient(IRedisCacheFactory factory, ILogger lo return await _factory.Create(key).TryGetStateAsync(); } - public async Task TrySetAsync(string key, T value, TimeSpan? expiration ) where T : class + public async Task TrySetAsync(string key, T value, TimeSpan? expiration) where T : class { try { From 24ff71eb624ff07927fd0221e250891a2fa049df Mon Sep 17 00:00:00 2001 From: Adam Zippor Date: Fri, 25 Jul 2025 04:02:50 +0200 Subject: [PATCH 5/5] add accidentally removed file --- .../DarcLib/IDistributedCacheClient.cs | 39 +++++++++++++++++++ src/Microsoft.DotNet.Darc/DarcLib/Remote.cs | 1 - 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 src/Microsoft.DotNet.Darc/DarcLib/IDistributedCacheClient.cs diff --git a/src/Microsoft.DotNet.Darc/DarcLib/IDistributedCacheClient.cs b/src/Microsoft.DotNet.Darc/DarcLib/IDistributedCacheClient.cs new file mode 100644 index 0000000000..470186bcbe --- /dev/null +++ b/src/Microsoft.DotNet.Darc/DarcLib/IDistributedCacheClient.cs @@ -0,0 +1,39 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Threading.Tasks; + +#nullable enable +namespace Microsoft.DotNet.DarcLib; + +/// +/// Generic interface for caching POCOs. Implementations should create keys based on the provided key and +/// object type to avoid collisions. +/// +public interface IDistributedCacheClient +{ + Task TrySetAsync(string key, T value, TimeSpan? expiration = null) where T : class; + Task TryGetAsync(string key) where T : class; + Task DeleteAsync(string key); +} + +/// +/// This no-op cache client the default implementation of IDistributedCacheClient in DarcLib. Caching is not mandatory +/// and requires explicit implemnetation if used. +/// +public class NoOpCacheClient : IDistributedCacheClient +{ + public Task TrySetAsync(string key, T value, TimeSpan? expiration = null) where T : class + { + return Task.FromResult(false); + } + public Task TryGetAsync(string key) where T : class + { + return Task.FromResult(null); + } + public Task DeleteAsync(string key) + { + return Task.FromResult(false); + } +} diff --git a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs index e86fcd5c3d..007e5c365f 100644 --- a/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs +++ b/src/Microsoft.DotNet.Darc/DarcLib/Remote.cs @@ -20,7 +20,6 @@ namespace Microsoft.DotNet.DarcLib; public sealed class Remote : IRemote { - private readonly ICache _cacheClient; private readonly IVersionDetailsParser _versionDetailsParser; private readonly DependencyFileManager _fileManager; private readonly IRemoteGitRepo _remoteGitClient;