[FEATURE] Update cachemanager and cacherepository implementations to async#715
[FEATURE] Update cachemanager and cacherepository implementations to async#715HarryKambo wants to merge 12 commits intoguibranco:mainfrom
cachemanager and cacherepository implementations to async#715Conversation
Reviewer's GuideThis PR overhauls the caching layer by migrating all CacheManager methods, repository interfaces, and implementations to asynchronous APIs with CancellationToken support, built-in default timeouts, enhanced logging and error handling, and automatic promotion of fetched entries into the in-memory cache. Tests have been updated to exercise the new async methods. Sequence diagram for async GetAsync with automatic memory cache promotionsequenceDiagram
participant Client
participant CacheManager
participant MemoryCacheRepository
participant RedisCacheRepository
participant CouchDBCacheRepository
Client->>CacheManager: GetAsync<T>(key, cancellationToken)
CacheManager->>MemoryCacheRepository: GetAsync<T>(key, cancellationToken)
alt Found in memory
MemoryCacheRepository-->>CacheManager: value
CacheManager-->>Client: value
else Not found in memory
CacheManager->>RedisCacheRepository: GetAsync<T>(key, cancellationToken)
alt Found in Redis
RedisCacheRepository-->>CacheManager: value
CacheManager->>MemoryCacheRepository: SetAsync<T>(value, key, CancellationToken.None)
CacheManager-->>Client: value
else Not found in Redis
CacheManager->>CouchDBCacheRepository: GetAsync<T>(key, cancellationToken)
alt Found in CouchDB
CouchDBCacheRepository-->>CacheManager: value
CacheManager->>MemoryCacheRepository: SetAsync<T>(value, key, CancellationToken.None)
CacheManager-->>Client: value
else Not found in any
CouchDBCacheRepository-->>CacheManager: (throws)
CacheManager-->>Client: (throws InvalidOperationException)
end
end
end
Class diagram for updated ICacheRepository interface and implementationsclassDiagram
class ICacheRepository {
<<interface>>
+ValueTask SetAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken cancellationToken = default)
+ValueTask SetAsync<T>(T value, string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, string subKey, CancellationToken cancellationToken = default)
+Task<TimeSpan> TTLAsync(string key, CancellationToken cancellationToken = default)
+Task Clear()
}
class MemoryCacheRepository {
+ValueTask SetAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken cancellationToken = default)
+ValueTask SetAsync<T>(T value, string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, string subKey, CancellationToken cancellationToken = default)
+Task<TimeSpan> TTLAsync(string key, CancellationToken cancellationToken = default)
+Task Clear()
}
class RedisCacheRepository {
+ValueTask SetAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken cancellationToken = default)
+ValueTask SetAsync<T>(T value, string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, string subKey, CancellationToken cancellationToken = default)
+Task<TimeSpan> TTLAsync(string key, CancellationToken cancellationToken = default)
+Task Clear()
}
class CouchDBCacheRepository {
+ValueTask SetAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken cancellationToken = default)
+ValueTask SetAsync<T>(T value, string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, CancellationToken cancellationToken = default)
+Task RemoveAsync(string key, string subKey, CancellationToken cancellationToken = default)
+Task<TimeSpan> TTLAsync(string key, CancellationToken cancellationToken = default)
+Task Clear()
}
ICacheRepository <|.. MemoryCacheRepository
ICacheRepository <|.. RedisCacheRepository
ICacheRepository <|.. CouchDBCacheRepository
Class diagram for updated CacheManager async APIclassDiagram
class CacheManager {
<<static>>
+ValueTask SetAsync<T>(T value, string key, CancellationToken cancellationToken = default)
+ValueTask SetAsync<T>(T value, string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask SetAsync<T>(T value, string key, TimeSpan ttl, CancellationToken cancellationToken = default)
+ValueTask SetToAsync<TCacheRepository, TValue>(TValue value, string key, CancellationToken cancellationToken = default)
+ValueTask SetToAsync<TCacheRepository, TValue>(TValue value, string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask SetToAsync<TCacheRepository, TValue>(TValue value, string key, TimeSpan ttl, CancellationToken cancellationToken = default)
+ValueTask SetToAsync<TCacheRepository, TValue>(TValue value, string key, string subKey, TimeSpan ttl, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<T> GetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<TValue> GetFrom<TCacheRepository, TValue>(string key, CancellationToken cancellationToken = default)
+ValueTask<TValue> GetFromAsync<TCacheRepository, TValue>(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask<(bool Success, T Value)> TryGetAsync<T>(string key, CancellationToken cancellationToken = default)
+ValueTask<bool> TryGetAsync<T>(string key, string subKey, CancellationToken cancellationToken = default)
+Task<TimeSpan> TTLAsync(string key)
+ValueTask Remove(string key, CancellationToken cancellationToken = default)
+ValueTask Remove(string key, string subKey, CancellationToken cancellationToken = default)
+ValueTask RemoveFrom<TCacheRepository>(string key, CancellationToken cancellationToken = default)
+ValueTask RemoveFrom<TCacheRepository>(string key, string subKey, CancellationToken cancellationToken = default)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
gstraccini
Bot
added
the
🚦 awaiting triage
WalkthroughThis change set refactors the entire caching infrastructure from synchronous to asynchronous patterns across all repositories, the cache manager, and related utilities. All cache operations now use async/await, return Task or ValueTask, and support cancellation tokens. Corresponding tests and interface definitions are updated for asynchronous execution and cancellation support. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CacheManager
participant Repo1 as ICacheRepository (async)
participant Repo2 as ICacheRepository (async)
Note over CacheManager: Example: SetAsync flow
Caller->>CacheManager: SetAsync(value, key, token)
CacheManager->>Repo1: SetAsync(value, key, token)
CacheManager->>Repo2: SetAsync(value, key, token)
Repo1-->>CacheManager: (await) Success/Failure
Repo2-->>CacheManager: (await) Success/Failure
CacheManager-->>Caller: (await) Complete
sequenceDiagram
participant Caller
participant CacheManager
participant Repo1 as ICacheRepository (async)
participant Repo2 as ICacheRepository (async)
Note over CacheManager: Example: GetAsync flow
Caller->>CacheManager: GetAsync(key, token)
CacheManager->>Repo1: GetAsync(key, token)
alt Value found
Repo1-->>CacheManager: (await) Value
CacheManager-->>Caller: (await) Value
else Value not found
Repo1-->>CacheManager: (await) null
CacheManager->>Repo2: GetAsync(key, token)
Repo2-->>CacheManager: (await) Value/null
CacheManager-->>Caller: (await) Value/null
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi I request an early feedback before I update Test cases. |
|
❌ Build CrispyWaffle 10.0.352 failed (commit 3e9349bd33 by @HarryKambo) |
…ispyWaffle into feature/cacheasync hanges from master branch merged
|
✅ Build CrispyWaffle 10.0.354 completed (commit 1f9fc7b543 by @code-factor) |
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sonarcsharp (reported by Codacy) found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
Hi @HarryKambo 👋, Thank you so much for your pull request! 🙌 I appreciate the time and effort you put into this contribution. Thanks again for your valuable contribution! 🚀 |
|
Hi @guibranco |
|
✅ Build CrispyWaffle 10.0.370 completed (commit 8985d38231 by @guibranco) |
guibranco
left a comment
guibranco
left a comment
There was a problem hiding this comment.
Hi @HarryKambo 👋, this PR looks good to me, do you plan to do more work over it? Or it's ready to approve and merge?
guibranco
added
📝 documentation
|
@gstraccini csharpier |
|
Running CSharpier on this branch! 🔧 |
|
❌ CSharpier failed! |
|
✅ Build CrispyWaffle 10.0.383 completed (commit 7d8f05382f by @guibranco) |
Please merge |
penify-dev
Bot
added
Review effort [1-5]: 4
and removed
tests
PR Review 🔍
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @HarryKambo - I've reviewed your changes - here's some feedback:
- There’s a lot of repeated async/cancellation/timeout boilerplate in CacheManager and repository implementations—consider extracting the common patterns (cancellation/token linking, logging, error handling, Task.WhenAll) into shared helper methods to reduce duplication and improve maintainability.
- The
DefaultTimeoutof 10 seconds is currently hard‐coded—make this configurable (e.g. via DI or settings) so callers can adjust timeouts per scenario instead of relying on a single static value. - The fire‐and‐forget promotion to memory cache uses the original cancellation token and ignores exceptions—switch to
CancellationToken.Nonefor background tasks, await or explicitly log any failures, and applyConfigureAwait(false)to avoid unobserved exceptions or deadlocks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There’s a lot of repeated async/cancellation/timeout boilerplate in CacheManager and repository implementations—consider extracting the common patterns (cancellation/token linking, logging, error handling, Task.WhenAll) into shared helper methods to reduce duplication and improve maintainability.
- The `DefaultTimeout` of 10 seconds is currently hard‐coded—make this configurable (e.g. via DI or settings) so callers can adjust timeouts per scenario instead of relying on a single static value.
- The fire‐and‐forget promotion to memory cache uses the original cancellation token and ignores exceptions—switch to `CancellationToken.None` for background tasks, await or explicitly log any failures, and apply `ConfigureAwait(false)` to avoid unobserved exceptions or deadlocks.
## Individual Comments
### Comment 1
<location> `Src/CrispyWaffle/Cache/CacheManager.cs:138` </location>
<code_context>
+ }
+ catch (OperationCanceledException)
+ {
+ Console.WriteLine("Cache operation was cancelled");
+ return false;
+ }
</code_context>
<issue_to_address>
Console.WriteLine for cancellation is inconsistent with logging strategy.
Use LogConsumer.Info or LogConsumer.Warning for cancellation events to align with the project's logging approach.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
Console.WriteLine("Cache operation was cancelled");
=======
LogConsumer.Info("Cache operation was cancelled");
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `Src/CrispyWaffle/Cache/CacheManager.cs:148` </location>
<code_context>
+ }
+ });
+
+ var results = await Task.WhenAll(tasks);
+ var successCount = results.Count(r => r);
+
+ LogConsumer.Info("Successfully set {0} in {1} out of {2} repositories", key, successCount, _repositories.Count);
}
</code_context>
<issue_to_address>
Success count may be misleading if all repositories fail.
Log a warning or error when successCount is zero or less than the total repository count to highlight partial or total failures.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
var results = await Task.WhenAll(tasks);
var successCount = results.Count(r => r);
LogConsumer.Info("Successfully set {0} in {1} out of {2} repositories", key, successCount, _repositories.Count);
}
=======
var results = await Task.WhenAll(tasks);
var successCount = results.Count(r => r);
if (successCount == 0)
{
LogConsumer.Error("Failed to set {0} in all {1} repositories", key, _repositories.Count);
}
else if (successCount < _repositories.Count)
{
LogConsumer.Warning("Successfully set {0} in only {1} out of {2} repositories", key, successCount, _repositories.Count);
}
else
{
LogConsumer.Info("Successfully set {0} in all {1} repositories", key, _repositories.Count);
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `Src/CrispyWaffle/Cache/CacheManager.cs:463` </location>
<code_context>
+ );
+
+ // Create timeout for the entire operation
+ using var timeoutCts = new CancellationTokenSource(DefaultTimeout);
+ using var combinedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
- if (_isMemoryRepositoryInList && repository.GetType() != typeof(MemoryCacheRepository))
</code_context>
<issue_to_address>
Timeout is hardcoded and not configurable.
Allow the timeout value to be set by the caller or made configurable to better accommodate different use cases and environments.
</issue_to_address>
### Comment 4
<location> `Src/CrispyWaffle/Cache/CacheManager.cs:503` </location>
<code_context>
+ }, cancellationToken);
+ }
+
+ LogConsumer.Debug("Key {0} not found in repository {1} ({2}ms)", key, repositoryName, repositoryStopwatch.ElapsedMilliseconds);
+ return result;
+ }
</code_context>
<issue_to_address>
Debug log message may be misleading for successful gets.
The log statement should only be executed when the key is not found to avoid confusion.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
LogConsumer.Debug("Key {0} not found in repository {1} ({2}ms)", key, repositoryName, repositoryStopwatch.ElapsedMilliseconds);
return result;
}
=======
if (result == null)
{
LogConsumer.Debug("Key {0} not found in repository {1} ({2}ms)", key, repositoryName, repositoryStopwatch.ElapsedMilliseconds);
}
return result;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `Src/CrispyWaffle/Cache/MemoryCacheRepository.cs:49` </location>
<code_context>
+ {
+ cancellationToken.ThrowIfCancellationRequested();
+
+ if (_data.Count >= int.MaxValue)
+ {
+ throw new OverflowException("The dictionary already contains the maximum number of elements.");
</code_context>
<issue_to_address>
Check for int.MaxValue is unnecessary for most practical scenarios.
System memory will be exhausted before reaching int.MaxValue. Consider removing this check or using a more practical resource constraint.
</issue_to_address>
### Comment 6
<location> `Src/CrispyWaffle/Cache/MemoryCacheRepository.cs:227` </location>
<code_context>
{
+ cancellationToken.ThrowIfCancellationRequested();
var finalKey = $"{key}-{subKey}";
if (_data.ContainsKey(finalKey))
{
_hash.TryRemove(finalKey, out _);
</code_context>
<issue_to_address>
Inconsistent use of _data and _hash for subKey removal.
The method checks _data but removes from _hash. Please verify that both the check and removal target the correct dictionary for subKey operations.
</issue_to_address>
### Comment 7
<location> `Src/CrispyWaffle.Redis/Cache/RedisCacheRepository.cs:248` </location>
<code_context>
+ throw new ArgumentException("SubKey cannot be null or empty", nameof(subKey));
+ }
+
+ if (EqualityComparer<T>.Default.Equals(value, default(T)) && !typeof(T).IsValueType)
+ {
+ throw new ArgumentNullException(nameof(value));
</code_context>
<issue_to_address>
Null value check for reference types may not be sufficient.
This approach may miss cases like nullable value types. Consider using 'value == null' for reference types or clarify your intent.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (EqualityComparer<T>.Default.Equals(value, default(T)) && !typeof(T).IsValueType)
{
throw new ArgumentNullException(nameof(value));
}
=======
if (!typeof(T).IsValueType || Nullable.GetUnderlyingType(typeof(T)) != null)
{
if (value == null)
{
throw new ArgumentNullException(nameof(value));
}
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 8
<location> `Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs:50` </location>
<code_context>
+ public async Task<int> GetDocCount<T>()
+ where T : CouchDBCacheDocument
+ {
+ var db = await ResolveDatabase<T>().ConfigureAwait(false);
+ return db.Where(x => x.Id != null).ToList().Count;
+ }
</code_context>
<issue_to_address>
ToList().Count is inefficient for counting documents.
Consider using a database-level count operation instead to avoid loading all documents into memory, which is inefficient for large datasets.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
return db.Where(x => x.Id != null).ToList().Count;
=======
return db.Count(x => x.Id != null);
>>>>>>> REPLACE
</suggested_fix>
### Comment 9
<location> `Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs:203` </location>
<code_context>
+ cancellationToken.ThrowIfCancellationRequested();
+
+ var client = await ResolveDatabase<T>().ConfigureAwait(false);
+ var doc = client
.Where(x => x.Key == key && x.SubKey == subKey)
.FirstOrDefault();
</code_context>
<issue_to_address>
Synchronous LINQ query on async database may block.
Consider replacing the synchronous query with an async alternative to prevent potential thread blocking.
Suggested implementation:
```csharp
var doc = await client
.Where(x => x.Key == key && x.SubKey == subKey)
.FirstOrDefaultAsync(cancellationToken)
.ConfigureAwait(false);
```
- Ensure that `using System.Linq;` is replaced or supplemented with `using System.Linq.Async;` or the correct namespace for async LINQ extensions, depending on your database client and async support.
- If your database client does not support async LINQ, you may need to use its own async query API.
</issue_to_address>
### Comment 10
<location> `Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs:81` </location>
<code_context>
[Fact]
- public void GetShouldThrowWhenItemNotFound()
+ public async Task SetAsyncShouldSetValueWithSubKey()
{
// Arrange
var key = "testKey";
- _mockCacheRepository.TryGet(key, out Arg.Any<object>()).Returns(false);
+ var subKey = "testSubKey";
+ var value = new { Name = "Test" };
+ CacheManager.AddRepository(_mockCacheRepository);
// Act
- Action act = () => CacheManager.Get<dynamic>(key);
+ await CacheManager.SetAsync(value, key, subKey);
// Assert
- act.Should()
- .Throw<InvalidOperationException>()
- .WithMessage("Unable to get the item with key testKey");
+ await _mockCacheRepository.Received(1).SetAsync(value, key, subKey, Arg.Any<CancellationToken>());
}
</code_context>
<issue_to_address>
Missing test for SetAsync with TTL and subKey together.
Please add a test for SetAsync that uses both subKey and TTL to confirm the method handles both parameters together correctly.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
[Fact]
public async Task SetAsyncShouldSetValueInAllRepositories()
=======
[Fact]
public async Task SetAsyncShouldSetValueWithSubKeyAndTtl()
{
// Arrange
var key = "testKey";
var subKey = "testSubKey";
var value = new { Name = "Test" };
var ttl = TimeSpan.FromMinutes(5);
CacheManager.AddRepository(_mockCacheRepository);
// Act
await CacheManager.SetAsync(value, key, subKey, ttl);
// Assert
await _mockCacheRepository.Received(1).SetAsync(value, key, subKey, ttl, Arg.Any<CancellationToken>());
}
[Fact]
public async Task SetAsyncShouldSetValueInAllRepositories()
>>>>>>> REPLACE
</suggested_fix>
### Comment 11
<location> `Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs:97` </location>
<code_context>
[Fact]
- public void SetToShouldThrowWhenRepositoryNotFound()
+ public async Task GetAsyncShouldThrowWhenItemNotFound()
{
// Arrange
var key = "testKey";
- var value = new { Name = "Test" };
CacheManager.AddRepository(_mockCacheRepository);
-
// Act
- Action act = () => CacheManager.SetTo<ICacheRepository, object>(value, key);
+ Func<Task> act = async () => await CacheManager.GetAsync<dynamic>(key);
// Assert
- act.Should()
- .Throw<InvalidOperationException>()
- .WithMessage(
- "The repository of type CrispyWaffle.Cache.ICacheRepository isn't available in the repositories providers list"
- );
+ await act.Should()
+ .ThrowAsync<InvalidOperationException>()
+ .WithMessage("Unable to get the item with key testKey");
}
</code_context>
<issue_to_address>
No test for GetAsync with subKey when item is not found.
Add a test for GetAsync with both key and subKey when the item is not found to verify the correct exception and message are thrown.
</issue_to_address>
### Comment 12
<location> `Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs:131` </location>
<code_context>
[Fact]
- public void TTLShouldReturnCorrectTTLFromRepositories()
+ public async Task TTLShouldReturnCorrectTTLFromRepositories()
{
// Arrange
var key = "testKey";
var expectedTTL = TimeSpan.FromMinutes(10);
- CacheManager.AddRepository(_mockCacheRepository);
- _mockCacheRepository.TTL(key).Returns(expectedTTL);
+ CacheManager.AddRepository(_mockCacheRepository);
+ await CacheManager.SetAsync(new { Name = "Test" }, key, expectedTTL, CancellationToken.None); // Set value with TTL
// Act
- var result = CacheManager.TTL(key);
+ var result = await CacheManager.TTLAsync(key);
// Assert
result.Should().Be(expectedTTL);
</code_context>
<issue_to_address>
No test for TTLAsync when key does not exist.
Please add a test to verify that TTLAsync returns TimeSpan.Zero when the key is missing from all repositories.
</issue_to_address>
### Comment 13
<location> `Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs:112` </location>
<code_context>
[Fact]
- public void RemoveShouldRemoveKeyFromAllRepositories()
+ public async Task SetToShouldThrowWhenRepositoryNotFound()
{
// Arrange
var key = "testKey";
+ var value = new { Name = "Test" };
CacheManager.AddRepository(_mockCacheRepository);
// Act
- CacheManager.Remove(key);
+ Func<Task> act = async () => await CacheManager.SetToAsync<ICacheRepository, object>(value, key);
// Assert
- _mockCacheRepository.Received().Remove(key);
- }
+ await act.Should()
+ .ThrowAsync<InvalidOperationException>()
+ .WithMessage(
+ "The repository of type CrispyWaffle.Cache.ICacheRepository isn't available in the repositories providers list"
+ );
+ }
</code_context>
<issue_to_address>
No test for RemoveFromAsync with subKey when repository is not found.
Add a test for RemoveFromAsync with both key and subKey when the repository is missing to verify the correct exception is thrown.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
await act.Should()
.ThrowAsync<InvalidOperationException>()
.WithMessage("Unable to get the item with key testKey");
}
[Fact]
=======
await act.Should()
.ThrowAsync<InvalidOperationException>()
.WithMessage("Unable to get the item with key testKey");
}
[Fact]
public async Task RemoveFromAsyncShouldThrowWhenRepositoryNotFound()
{
// Arrange
var key = "testKey";
var subKey = "testSubKey";
// Do not add any repository to CacheManager
// Act
Func<Task> act = async () => await CacheManager.RemoveFromAsync<ICacheRepository>(key, subKey);
// Assert
await act.Should()
.ThrowAsync<InvalidOperationException>()
.WithMessage(
"The repository of type CrispyWaffle.Cache.ICacheRepository isn't available in the repositories providers list"
);
}
[Fact]
>>>>>>> REPLACE
</suggested_fix>
### Comment 14
<location> `Tests/CrispyWaffle.IntegrationTests/Cache/CouchDBCacheRepositoryTests.cs:27` </location>
<code_context>
[Fact]
- public void GetAndSetCouchDocTest()
+ public async Task GetAndSetAsyncCouchDocTest()
{
var doc = new CouchDBCacheDocument();
- _repository.Set(doc, Guid.NewGuid().ToString());
+ await _repository.SetAsync(doc, Guid.NewGuid().ToString());
- var docDB = _repository.Get<CouchDBCacheDocument>(doc.Key);
+ var docDB = await _repository.GetAsync<CouchDBCacheDocument>(doc.Key);
Assert.True(doc.Key == docDB.Key);
- _repository.Remove(doc.Key);
+ await _repository.RemoveAsync(doc.Key);
}
</code_context>
<issue_to_address>
Integration tests do not cover cancellation or error propagation.
Please add integration tests for cancellation scenarios and exception propagation to ensure these cases are handled correctly.
</issue_to_address>
### Comment 15
<location> `Tests/CrispyWaffle.Tests/Cache/MemoryCacheRepositoryTests.cs:47` </location>
<code_context>
[Fact]
- public void RemoveShouldRemoveStoredValue()
+ public async Task RemoveShouldRemoveStoredValue()
{
// Arrange
var key = "test-key";
- _repository.Set("test-value", key);
- _repository.Remove(key);
+ var Task1 = _repository.SetAsync("test-value", key).AsTask();
+ var Task2 = _repository.RemoveAsync(key);
+ await Task.WhenAll(Task1, Task2);
// Act
- var exception = Assert.Throws<InvalidOperationException>(() =>
- _repository.Get<string>(key)
+ var exception = Assert.ThrowsAsync<InvalidOperationException>(async () => await _repository.GetAsync<string>(key)
);
</code_context>
<issue_to_address>
No test for MemoryCacheRepository GetAsync with subKey.
Add tests for GetAsync with both key and subKey, including scenarios where the subKey is missing to verify exception handling.
</issue_to_address>
### Comment 16
<location> `Tests/CrispyWaffle.Tests/Cache/CouchDBCacheRepositoryTests.cs:54` </location>
<code_context>
[Fact]
- public void RemoveFromDatabaseShouldRemoveValue()
+ public async Task RemoveFromDatabaseShouldRemoveValue()
{
// Arrange
var key = "test-key";
+ var value = "test-value";
+
+ // Act
+ await _repository.SetAsync(value, key);
// Act
- _repository.Remove(key);
+ await _repository.RemoveAsync(key);
+ (bool success, _)= await _repository.TryGetAsync<object>(key);
// Assert
+ success.Should().Be(false);
}
}
</code_context>
<issue_to_address>
No test for CouchDBCacheRepository TryGetAsync with subKey.
Add a test for TryGetAsync using both key and subKey, including a scenario where the subKey is missing, to verify correct handling.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
[Fact]
public async Task SetToDatabaseShouldStoreValue()
{
=======
[Fact]
public async Task TryGetAsync_WithSubKey_ShouldReturnCorrectly()
{
// Arrange
var key = "test-key";
var subKey = "test-subkey";
var value = "test-value";
var missingSubKey = "missing-subkey";
// Store value with subKey
await _repository.SetAsync(value, key, subKey);
// Act & Assert: Should retrieve successfully with correct subKey
(bool found, object? retrieved) = await _repository.TryGetAsync<string>(key, subKey);
found.Should().BeTrue();
retrieved.Should().Be(value);
// Act & Assert: Should not retrieve with missing subKey
(bool foundMissing, object? retrievedMissing) = await _repository.TryGetAsync<string>(key, missingSubKey);
foundMissing.Should().BeFalse();
retrievedMissing.Should().BeNull();
}
[Fact]
public async Task SetToDatabaseShouldStoreValue()
{
>>>>>>> REPLACE
</suggested_fix>
### Comment 17
<location> `Src/CrispyWaffle/Cache/CacheManager.cs:123` </location>
<code_context>
+ /// <param name="cancellationToken">Cancel.</param>
+ /// <exception cref="OperationCanceledException">If cancelled.</exception>
+ /// <returns>void.</returns>
+ public static async ValueTask SetAsync<T>(T value, [Localizable(false)] string key, CancellationToken cancellationToken = default)
{
+ cancellationToken.ThrowIfCancellationRequested();
</code_context>
<issue_to_address>
Consider refactoring the repeated repository iteration logic into two generic helper methods to greatly simplify and reduce duplication in the public API methods.
Here’s one way to collapse all of these almost‐identical methods into two small helpers—one for “run on all repos in parallel” (e.g. SetAsync/RemoveAsync) and one for “query until first success” (e.g. GetAsync/TryGetAsync). Then each public API method becomes a one–liner.
```csharp
// 1) helper for doing something in parallel on all repositories
private static async ValueTask RunOnAllAsync(
Func<ICacheRepository, CancellationToken, Task> action,
string startMessage,
string summaryMessage,
CancellationToken cancellationToken = default
)
{
cancellationToken.ThrowIfCancellationRequested();
LogConsumer.Trace(startMessage, _repositories.Count);
using var timeoutCts = new CancellationTokenSource(DefaultTimeout);
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
var results = await Task.WhenAll(_repositories.Values.Select(async repo =>
{
try
{
await action(repo, linkedCts.Token);
return true;
}
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
LogConsumer.Info("Operation canceled by user");
return false;
}
catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested)
{
LogConsumer.Error("Operation timed out");
throw new TimeoutException("Cache operation timed out");
}
catch (Exception ex)
{
LogConsumer.Error("Cache error in {0}: {1}", repo.GetType().Name, ex.Message);
return false;
}
}));
LogConsumer.Info(summaryMessage,
results.Count(r => r),
_repositories.Count);
}
// 2) helper for querying each repo in turn until one succeeds
private static async ValueTask<T> QueryFirstAsync<T>(
Func<ICacheRepository, CancellationToken, Task<T>> query,
string startMessage,
string notFoundMessage,
CancellationToken cancellationToken = default
)
{
cancellationToken.ThrowIfCancellationRequested();
LogConsumer.Trace(startMessage, _repositories.Count);
using var timeoutCts = new CancellationTokenSource(DefaultTimeout);
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
foreach (var repo in _repositories.Values)
{
try
{
var result = await query(repo, linkedCts.Token);
LogConsumer.Info("Found in {0}", repo.GetType().Name);
return result;
}
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
LogConsumer.Info("Operation canceled by user");
throw;
}
catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested)
{
LogConsumer.Error("Operation timed out");
throw new TimeoutException("Cache operation timed out");
}
catch (Exception ex)
{
LogConsumer.Error("Cache error in {0}: {1}",
repo.GetType().Name,
ex.Message);
}
}
throw new InvalidOperationException(notFoundMessage);
}
```
Now your public methods shrink to:
```csharp
public static ValueTask SetAsync<T>(T value, string key, CancellationToken ct = default)
=> RunOnAllAsync(
(repo, c) => repo.SetAsync(value, key, cancellationToken: c),
"Adding {0} to {1} cache repositories",
"Successfully set {0} in {1} out of {2} repositories",
ct
);
public static ValueTask RemoveAsync(string key, CancellationToken ct = default)
=> RunOnAllAsync(
(repo, c) => repo.RemoveAsync(key, cancellationToken: c),
"Removing key {0} from {1} repositories",
"Removed key {0} from all repositories",
ct
);
public static ValueTask<T> GetAsync<T>(string key, CancellationToken ct = default)
=> QueryFirstAsync(
(repo, c) => repo.GetAsync<T>(key, cancellationToken: c),
"Getting {0} from {1} cache repositories",
$"Unable to get the item with key {key}",
ct
);
```
You can parameterize those lambdas to handle sub-keys and TTLs, and similarly replace all of your `SetToAsync`, `GetFromAsync`, `TryGetAsync`, etc. This collapses hundreds of lines into one helper + one small invocation per overload.
</issue_to_address>
### Comment 18
<location> `Src/CrispyWaffle.Redis/Cache/RedisCacheRepository.cs:160` </location>
<code_context>
/// <inheritdoc />
- public void Set<T>(T value, string key, TimeSpan? ttl = null)
+ public async ValueTask SetAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken cancellationToken = default)
{
if (!typeof(CouchDBCacheDocument).IsAssignableFrom(typeof(T)))
</code_context>
<issue_to_address>
Consider extracting common try/catch and cancellation logic into reusable helper methods to dramatically simplify each public API method into concise one-liners.
Here’s one way to collapse almost all of the per‐method boilerplate into two small helpers and turn your 30–50 line methods into one‐ or two‐liners. All functionality (cancellation, exception‐suppression, TTL, exists‐checks, etc.) remains intact, but you no longer repeat the same try/catch/ThrowIfCancelled logic everywhere.
```csharp
// inside RedisCacheRepository
private async ValueTask ExecuteAsync(
Func<CancellationToken, Task> op,
CancellationToken ct)
{
ct.ThrowIfCancellationRequested();
try
{
await op(ct).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
throw;
}
catch (Exception e) when (!ShouldPropagateExceptions)
{
HandleException(e);
}
}
// for return‐value methods
private async ValueTask<T> ExecuteAsync<T>(
Func<CancellationToken, Task<T>> op,
CancellationToken ct,
string errorMsg)
{
ct.ThrowIfCancellationRequested();
try
{
return await op(ct).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
throw;
}
catch (Exception e) when (!ShouldPropagateExceptions)
{
HandleException(e);
throw new InvalidOperationException(errorMsg, e);
}
}
```
Now each public API is a one‐liner:
```csharp
public ValueTask SetAsync<T>(
T value,
string key,
TimeSpan? ttl = null,
CancellationToken ct = default)
=> ExecuteAsync(
token => ttl.HasValue
? _cacheClient.Db0.AddAsync(key, value, ttl.Value)
: _cacheClient.Db0.AddAsync(key, value),
ct);
public ValueTask<T> GetAsync<T>(
string key,
CancellationToken ct = default)
=> ExecuteAsync(async token =>
{
if (!await _cacheClient.Db0.ExistsAsync(key).ConfigureAwait(false))
throw new InvalidOperationException($"Unable to get the item with key {key}");
return await _cacheClient.Db0.GetAsync<T>(key, CommandFlags.PreferReplica)
.ConfigureAwait(false);
}, ct, $"Unable to get the item with key {key}");
public ValueTask<(bool Success, T Value)> TryGetAsync<T>(
string key,
CancellationToken ct = default)
=> ExecuteAsync(async token =>
{
if (!await _cacheClient.Db0.ExistsAsync(key).ConfigureAwait(false))
return (false, default(T));
var val = await _cacheClient.Db0.GetAsync<T>(key, CommandFlags.PreferReplica)
.ConfigureAwait(false);
return (true, val);
}, ct, null);
```
You can apply the same pattern to hash‐sets, TTL, RemoveAsync, Clear, etc. This removes all the duplicated try/catch, cancellation checks, `Wait()/.Result`, and huge XML comments from each method.
</issue_to_address>
### Comment 19
<location> `Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs:147` </location>
<code_context>
+ /// <exception cref="OperationCanceledException">Thrown if the operation is cancelled.</exception>
/// <exception cref="InvalidOperationException">Thrown in case the operation fails.</exception>
- public T GetSpecific<T>(string key)
+ public async Task<T> GetSpecificAsync<T>(string key, CancellationToken cancellationToken = default)
where T : CouchDBCacheDocument
{
</code_context>
<issue_to_address>
Consider introducing a single generic async executor method to centralize error handling, cancellation, and database resolution logic for all async entry-points.
Here’s one way to dramatically DRY out almost all of your async entry‐points by introducing a single generic executor. All of your methods then become one‐liners that pass to this executor instead of re‐implementing the same try/catch/ResolveDatabase/ThrowIfCancellationRequested logic everywhere.
```csharp
// 1) Put this in your class as a private helper
private async Task<TResult> ExecuteAsync<TDoc, TResult>(
Func<CouchDatabase<TDoc>, CancellationToken, Task<TResult>> work,
string operationDescription,
CancellationToken ct = default
)
where TDoc : CouchDBCacheDocument
{
try
{
ct.ThrowIfCancellationRequested();
var db = await ResolveDatabase<TDoc>().ConfigureAwait(false);
return await work(db, ct).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
LogConsumer.Warning($"Operation cancelled: {operationDescription}");
throw;
}
catch (Exception ex)
{
if (ShouldPropagateExceptions) throw;
LogConsumer.Handle(ex);
throw new InvalidOperationException(operationDescription, ex);
}
}
```
```csharp
// 2) Refactor GetSpecificAsync
public Task<T> GetSpecificAsync<T>(string key, CancellationToken ct = default)
where T : CouchDBCacheDocument
=> ExecuteAsync<T, T>(
async (db, token) =>
{
var doc = await db.FindAsync(key).ConfigureAwait(false);
if (doc != null && doc.ExpiresAt <= DateTime.UtcNow)
{
await RemoveSpecificAsync<T>(key, token).ConfigureAwait(false);
return default;
}
return doc;
},
$"Unable to get item with key: {key}",
ct
);
```
```csharp
// 3) Refactor RemoveSpecificAsync
public Task RemoveSpecificAsync<T>(string key, CancellationToken ct = default)
where T : CouchDBCacheDocument
=> ExecuteAsync<T, object>(
async (db, token) =>
{
var doc = await db.FindAsync(key).ConfigureAwait(false);
if (doc != null)
{
await db.DeleteAsync(doc).ConfigureAwait(false);
LogConsumer.Info($"Removed document {key}");
}
else
{
LogConsumer.Info($"Document {key} not found");
}
return null;
},
$"Failed removing document with key: {key}",
ct
);
```
```csharp
// 4) Refactor SetSpecificAsync
public Task SetSpecificAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken ct = default)
where T : CouchDBCacheDocument
=> ExecuteAsync<T, object>(
async (db, token) =>
{
value.Key = key;
if (ttl != null)
{
value.TTL = ttl.Value;
value.ExpiresAt = DateTime.UtcNow.Add(ttl.Value);
}
await db.CreateAsync(value).ConfigureAwait(false);
return null;
},
$"Failed setting document with key: {key}",
ct
);
```
You can then delete all of the duplicated try/catch/cancellation/ResolveDatabase boilerplate in every method. The sync counterparts can either call `.GetAwaiter().GetResult()` on these or be slim wrappers around them as well.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| Console.WriteLine("Cache operation was cancelled"); |
There was a problem hiding this comment.
suggestion: Console.WriteLine for cancellation is inconsistent with logging strategy.
Use LogConsumer.Info or LogConsumer.Warning for cancellation events to align with the project's logging approach.
| Console.WriteLine("Cache operation was cancelled"); | |
| LogConsumer.Info("Cache operation was cancelled"); |
| var successCount = results.Count(r => r); | ||
|
|
||
| LogConsumer.Info("Successfully set {0} in {1} out of {2} repositories", key, successCount, _repositories.Count); | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): Success count may be misleading if all repositories fail.
Log a warning or error when successCount is zero or less than the total repository count to highlight partial or total failures.
| var results = await Task.WhenAll(tasks); | |
| var successCount = results.Count(r => r); | |
| LogConsumer.Info("Successfully set {0} in {1} out of {2} repositories", key, successCount, _repositories.Count); | |
| } | |
| var results = await Task.WhenAll(tasks); | |
| var successCount = results.Count(r => r); | |
| if (successCount == 0) | |
| { | |
| LogConsumer.Error("Failed to set {0} in all {1} repositories", key, _repositories.Count); | |
| } | |
| else if (successCount < _repositories.Count) | |
| { | |
| LogConsumer.Warning("Successfully set {0} in only {1} out of {2} repositories", key, successCount, _repositories.Count); | |
| } | |
| else | |
| { | |
| LogConsumer.Info("Successfully set {0} in all {1} repositories", key, _repositories.Count); | |
| } | |
| } |
| using var combinedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token); | ||
|
|
There was a problem hiding this comment.
suggestion: Timeout is hardcoded and not configurable.
Allow the timeout value to be set by the caller or made configurable to better accommodate different use cases and environments.
| return result; | ||
| } | ||
| catch (InvalidOperationException) |
There was a problem hiding this comment.
suggestion: Debug log message may be misleading for successful gets.
The log statement should only be executed when the key is not found to avoid confusion.
| LogConsumer.Debug("Key {0} not found in repository {1} ({2}ms)", key, repositoryName, repositoryStopwatch.ElapsedMilliseconds); | |
| return result; | |
| } | |
| if (result == null) | |
| { | |
| LogConsumer.Debug("Key {0} not found in repository {1} ({2}ms)", key, repositoryName, repositoryStopwatch.ElapsedMilliseconds); | |
| } | |
| return result; | |
| } |
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| if (_data.Count >= int.MaxValue) |
There was a problem hiding this comment.
nitpick: Check for int.MaxValue is unnecessary for most practical scenarios.
System memory will be exhausted before reaching int.MaxValue. Consider removing this check or using a more practical resource constraint.
| public async Task RemoveShouldRemoveStoredValue() | ||
| { | ||
| // Arrange | ||
| var key = "test-key"; | ||
| _repository.Set("test-value", key); | ||
| _repository.Remove(key); | ||
| var Task1 = _repository.SetAsync("test-value", key).AsTask(); | ||
| var Task2 = _repository.RemoveAsync(key); | ||
|
|
||
| await Task.WhenAll(Task1, Task2); | ||
| // Act | ||
| var exception = Assert.Throws<InvalidOperationException>(() => | ||
| _repository.Get<string>(key) | ||
| var exception = Assert.ThrowsAsync<InvalidOperationException>(async () => await _repository.GetAsync<string>(key) |
There was a problem hiding this comment.
suggestion (testing): No test for MemoryCacheRepository GetAsync with subKey.
Add tests for GetAsync with both key and subKey, including scenarios where the subKey is missing to verify exception handling.
|
|
||
| [Fact] | ||
| public void SetToDatabaseShouldStoreValue() | ||
| public async Task SetToDatabaseShouldStoreValue() | ||
| { |
There was a problem hiding this comment.
suggestion (testing): No test for CouchDBCacheRepository TryGetAsync with subKey.
Add a test for TryGetAsync using both key and subKey, including a scenario where the subKey is missing, to verify correct handling.
| [Fact] | |
| public void SetToDatabaseShouldStoreValue() | |
| public async Task SetToDatabaseShouldStoreValue() | |
| { | |
| [Fact] | |
| public async Task TryGetAsync_WithSubKey_ShouldReturnCorrectly() | |
| { | |
| // Arrange | |
| var key = "test-key"; | |
| var subKey = "test-subkey"; | |
| var value = "test-value"; | |
| var missingSubKey = "missing-subkey"; | |
| // Store value with subKey | |
| await _repository.SetAsync(value, key, subKey); | |
| // Act & Assert: Should retrieve successfully with correct subKey | |
| (bool found, object? retrieved) = await _repository.TryGetAsync<string>(key, subKey); | |
| found.Should().BeTrue(); | |
| retrieved.Should().Be(value); | |
| // Act & Assert: Should not retrieve with missing subKey | |
| (bool foundMissing, object? retrievedMissing) = await _repository.TryGetAsync<string>(key, missingSubKey); | |
| foundMissing.Should().BeFalse(); | |
| retrievedMissing.Should().BeNull(); | |
| } | |
| [Fact] | |
| public async Task SetToDatabaseShouldStoreValue() | |
| { |
| /// <exception cref="OperationCanceledException">If cancelled.</exception> | ||
| /// <returns>void.</returns> | ||
| public static async ValueTask SetAsync<T>(T value, [Localizable(false)] string key, CancellationToken cancellationToken = default) | ||
| { |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the repeated repository iteration logic into two generic helper methods to greatly simplify and reduce duplication in the public API methods.
Here’s one way to collapse all of these almost‐identical methods into two small helpers—one for “run on all repos in parallel” (e.g. SetAsync/RemoveAsync) and one for “query until first success” (e.g. GetAsync/TryGetAsync). Then each public API method becomes a one–liner.
// 1) helper for doing something in parallel on all repositories
private static async ValueTask RunOnAllAsync(
Func<ICacheRepository, CancellationToken, Task> action,
string startMessage,
string summaryMessage,
CancellationToken cancellationToken = default
)
{
cancellationToken.ThrowIfCancellationRequested();
LogConsumer.Trace(startMessage, _repositories.Count);
using var timeoutCts = new CancellationTokenSource(DefaultTimeout);
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
var results = await Task.WhenAll(_repositories.Values.Select(async repo =>
{
try
{
await action(repo, linkedCts.Token);
return true;
}
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
LogConsumer.Info("Operation canceled by user");
return false;
}
catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested)
{
LogConsumer.Error("Operation timed out");
throw new TimeoutException("Cache operation timed out");
}
catch (Exception ex)
{
LogConsumer.Error("Cache error in {0}: {1}", repo.GetType().Name, ex.Message);
return false;
}
}));
LogConsumer.Info(summaryMessage,
results.Count(r => r),
_repositories.Count);
}
// 2) helper for querying each repo in turn until one succeeds
private static async ValueTask<T> QueryFirstAsync<T>(
Func<ICacheRepository, CancellationToken, Task<T>> query,
string startMessage,
string notFoundMessage,
CancellationToken cancellationToken = default
)
{
cancellationToken.ThrowIfCancellationRequested();
LogConsumer.Trace(startMessage, _repositories.Count);
using var timeoutCts = new CancellationTokenSource(DefaultTimeout);
using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, timeoutCts.Token);
foreach (var repo in _repositories.Values)
{
try
{
var result = await query(repo, linkedCts.Token);
LogConsumer.Info("Found in {0}", repo.GetType().Name);
return result;
}
catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested)
{
LogConsumer.Info("Operation canceled by user");
throw;
}
catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested)
{
LogConsumer.Error("Operation timed out");
throw new TimeoutException("Cache operation timed out");
}
catch (Exception ex)
{
LogConsumer.Error("Cache error in {0}: {1}",
repo.GetType().Name,
ex.Message);
}
}
throw new InvalidOperationException(notFoundMessage);
}Now your public methods shrink to:
public static ValueTask SetAsync<T>(T value, string key, CancellationToken ct = default)
=> RunOnAllAsync(
(repo, c) => repo.SetAsync(value, key, cancellationToken: c),
"Adding {0} to {1} cache repositories",
"Successfully set {0} in {1} out of {2} repositories",
ct
);
public static ValueTask RemoveAsync(string key, CancellationToken ct = default)
=> RunOnAllAsync(
(repo, c) => repo.RemoveAsync(key, cancellationToken: c),
"Removing key {0} from {1} repositories",
"Removed key {0} from all repositories",
ct
);
public static ValueTask<T> GetAsync<T>(string key, CancellationToken ct = default)
=> QueryFirstAsync(
(repo, c) => repo.GetAsync<T>(key, cancellationToken: c),
"Getting {0} from {1} cache repositories",
$"Unable to get the item with key {key}",
ct
);You can parameterize those lambdas to handle sub-keys and TTLs, and similarly replace all of your SetToAsync, GetFromAsync, TryGetAsync, etc. This collapses hundreds of lines into one helper + one small invocation per overload.
| public void Set<T>(T value, string key, TimeSpan? ttl = null) | ||
| /// <param name="cancellationToken">To cancel operation.</param> | ||
| /// <returns>A valuetask representing the asynchronous operation.</returns> | ||
| public async ValueTask SetAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
issue (complexity): Consider extracting common try/catch and cancellation logic into reusable helper methods to dramatically simplify each public API method into concise one-liners.
Here’s one way to collapse almost all of the per‐method boilerplate into two small helpers and turn your 30–50 line methods into one‐ or two‐liners. All functionality (cancellation, exception‐suppression, TTL, exists‐checks, etc.) remains intact, but you no longer repeat the same try/catch/ThrowIfCancelled logic everywhere.
// inside RedisCacheRepository
private async ValueTask ExecuteAsync(
Func<CancellationToken, Task> op,
CancellationToken ct)
{
ct.ThrowIfCancellationRequested();
try
{
await op(ct).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
throw;
}
catch (Exception e) when (!ShouldPropagateExceptions)
{
HandleException(e);
}
}
// for return‐value methods
private async ValueTask<T> ExecuteAsync<T>(
Func<CancellationToken, Task<T>> op,
CancellationToken ct,
string errorMsg)
{
ct.ThrowIfCancellationRequested();
try
{
return await op(ct).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
throw;
}
catch (Exception e) when (!ShouldPropagateExceptions)
{
HandleException(e);
throw new InvalidOperationException(errorMsg, e);
}
}Now each public API is a one‐liner:
public ValueTask SetAsync<T>(
T value,
string key,
TimeSpan? ttl = null,
CancellationToken ct = default)
=> ExecuteAsync(
token => ttl.HasValue
? _cacheClient.Db0.AddAsync(key, value, ttl.Value)
: _cacheClient.Db0.AddAsync(key, value),
ct);
public ValueTask<T> GetAsync<T>(
string key,
CancellationToken ct = default)
=> ExecuteAsync(async token =>
{
if (!await _cacheClient.Db0.ExistsAsync(key).ConfigureAwait(false))
throw new InvalidOperationException($"Unable to get the item with key {key}");
return await _cacheClient.Db0.GetAsync<T>(key, CommandFlags.PreferReplica)
.ConfigureAwait(false);
}, ct, $"Unable to get the item with key {key}");
public ValueTask<(bool Success, T Value)> TryGetAsync<T>(
string key,
CancellationToken ct = default)
=> ExecuteAsync(async token =>
{
if (!await _cacheClient.Db0.ExistsAsync(key).ConfigureAwait(false))
return (false, default(T));
var val = await _cacheClient.Db0.GetAsync<T>(key, CommandFlags.PreferReplica)
.ConfigureAwait(false);
return (true, val);
}, ct, null);You can apply the same pattern to hash‐sets, TTL, RemoveAsync, Clear, etc. This removes all the duplicated try/catch, cancellation checks, Wait()/.Result, and huge XML comments from each method.
| /// <exception cref="InvalidOperationException">Thrown in case the operation fails.</exception> | ||
| public T GetSpecific<T>(string key) | ||
| public async Task<T> GetSpecificAsync<T>(string key, CancellationToken cancellationToken = default) | ||
| where T : CouchDBCacheDocument |
There was a problem hiding this comment.
issue (complexity): Consider introducing a single generic async executor method to centralize error handling, cancellation, and database resolution logic for all async entry-points.
Here’s one way to dramatically DRY out almost all of your async entry‐points by introducing a single generic executor. All of your methods then become one‐liners that pass to this executor instead of re‐implementing the same try/catch/ResolveDatabase/ThrowIfCancellationRequested logic everywhere.
// 1) Put this in your class as a private helper
private async Task<TResult> ExecuteAsync<TDoc, TResult>(
Func<CouchDatabase<TDoc>, CancellationToken, Task<TResult>> work,
string operationDescription,
CancellationToken ct = default
)
where TDoc : CouchDBCacheDocument
{
try
{
ct.ThrowIfCancellationRequested();
var db = await ResolveDatabase<TDoc>().ConfigureAwait(false);
return await work(db, ct).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
LogConsumer.Warning($"Operation cancelled: {operationDescription}");
throw;
}
catch (Exception ex)
{
if (ShouldPropagateExceptions) throw;
LogConsumer.Handle(ex);
throw new InvalidOperationException(operationDescription, ex);
}
}// 2) Refactor GetSpecificAsync
public Task<T> GetSpecificAsync<T>(string key, CancellationToken ct = default)
where T : CouchDBCacheDocument
=> ExecuteAsync<T, T>(
async (db, token) =>
{
var doc = await db.FindAsync(key).ConfigureAwait(false);
if (doc != null && doc.ExpiresAt <= DateTime.UtcNow)
{
await RemoveSpecificAsync<T>(key, token).ConfigureAwait(false);
return default;
}
return doc;
},
$"Unable to get item with key: {key}",
ct
);// 3) Refactor RemoveSpecificAsync
public Task RemoveSpecificAsync<T>(string key, CancellationToken ct = default)
where T : CouchDBCacheDocument
=> ExecuteAsync<T, object>(
async (db, token) =>
{
var doc = await db.FindAsync(key).ConfigureAwait(false);
if (doc != null)
{
await db.DeleteAsync(doc).ConfigureAwait(false);
LogConsumer.Info($"Removed document {key}");
}
else
{
LogConsumer.Info($"Document {key} not found");
}
return null;
},
$"Failed removing document with key: {key}",
ct
);// 4) Refactor SetSpecificAsync
public Task SetSpecificAsync<T>(T value, string key, TimeSpan? ttl = null, CancellationToken ct = default)
where T : CouchDBCacheDocument
=> ExecuteAsync<T, object>(
async (db, token) =>
{
value.Key = key;
if (ttl != null)
{
value.TTL = ttl.Value;
value.ExpiresAt = DateTime.UtcNow.Add(ttl.Value);
}
await db.CreateAsync(value).ConfigureAwait(false);
return null;
},
$"Failed setting document with key: {key}",
ct
);You can then delete all of the duplicated try/catch/cancellation/ResolveDatabase boilerplate in every method. The sync counterparts can either call .GetAwaiter().GetResult() on these or be slim wrappers around them as well.
PR Code Suggestions ✨
|
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Implicit whitespace handling ▹ view | |
 |
Inefficient Document Lookup ▹ view | |
|
Inefficient Document Counting ▹ view | |
 |
Misleading Cache Hit Log Message ▹ view | |
 |
Inconsistent logging mechanism ▹ view | |
|
Unnecessary Error List Allocation ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| Src/CrispyWaffle/Cache/ICacheRepository.cs | ✅ |
| Src/CrispyWaffle.Utils/Communications/SmtpMailer.cs | ✅ |
| Src/CrispyWaffle/Cache/MemoryCacheRepository.cs | ✅ |
| Src/CrispyWaffle.Redis/Cache/RedisCacheRepository.cs | ✅ |
| Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs | ✅ |
| Src/CrispyWaffle/Cache/CacheManager.cs | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🔭 Outside diff range comments (2)
Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs (1)
147-162: Fix incorrect async test patternThe test method is not async but uses async assertions. This will cause the test to complete before the async operation finishes, potentially leading to false positives.
-public void RemoveFromShouldThrowWhenRepositoryNotFound() +public async Task RemoveFromAsyncShouldThrowWhenRepositoryNotFound() { // Arrange var key = "testKey"; CacheManager.AddRepository(_mockCacheRepository); // Act Func<Task> act = async () => await CacheManager.RemoveFrom<ICacheRepository>(key); // Assert - act.Should() - .ThrowAsync<InvalidOperationException>() + await act.Should() + .ThrowAsync<InvalidOperationException>() .WithMessage( "The repository of type CrispyWaffle.Cache.ICacheRepository isn't available in the repositories providers list" ); }Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs (1)
81-81: Replace Task.WaitAll with await Task.WhenAllUsing
Task.WaitAllin an async method blocks the thread and defeats the purpose of async/await. This should useawait Task.WhenAllinstead.-Task.WaitAll(tasks.ToArray()); +await Task.WhenAll(tasks);
🧹 Nitpick comments (4)
Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs (2)
112-112: Update test method name to reflect async operationThe test method name should be updated from
SetToShouldThrowWhenRepositoryNotFoundtoSetToAsyncShouldThrowWhenRepositoryNotFoundto accurately reflect that it's testing the async method.-public async Task SetToShouldThrowWhenRepositoryNotFound() +public async Task SetToAsyncShouldThrowWhenRepositoryNotFound()
131-131: Update test method name to reflect async operationThe test method name should be updated to
TTLAsyncShouldReturnCorrectTTLFromRepositoriesto accurately reflect that it's testing the async method.-public async Task TTLShouldReturnCorrectTTLFromRepositories() +public async Task TTLAsyncShouldReturnCorrectTTLFromRepositories()Src/CrispyWaffle/Cache/MemoryCacheRepository.cs (2)
87-97: Remove unnecessary async keywordThe method doesn't use
awaitso theasynckeyword is unnecessary. ValueTask can be created directly.-public async ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default) +public ValueTask<T> GetAsync<T>(string key, CancellationToken cancellationToken = default) { cancellationToken.ThrowIfCancellationRequested(); if (!_data.TryGetValue(key, out var value)) { throw new InvalidOperationException($"Unable to get the item with key {key}"); } - return (T)value; + return new ValueTask<T>((T)value); }
208-211: Remove redundant ContainsKey checkThe
ContainsKeycheck is redundant becauseTryRemovesafely handles non-existent keys by returning false.-if (_data.ContainsKey(key)) -{ - _data.TryRemove(key, out _); -} +_data.TryRemove(key, out _);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs(16 hunks)Src/CrispyWaffle.Redis/Cache/RedisCacheRepository.cs(11 hunks)Src/CrispyWaffle.Utils/Communications/SmtpMailer.cs(6 hunks)Src/CrispyWaffle/Cache/CacheManager.cs(22 hunks)Src/CrispyWaffle/Cache/ICacheRepository.cs(3 hunks)Src/CrispyWaffle/Cache/MemoryCacheRepository.cs(4 hunks)Tests/CrispyWaffle.IntegrationTests/Cache/CouchDBCacheRepositoryTests.cs(2 hunks)Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs(4 hunks)Tests/CrispyWaffle.Tests/Cache/CouchDBCacheRepositoryTests.cs(2 hunks)Tests/CrispyWaffle.Tests/Cache/MemoryCacheRepositoryTests.cs(2 hunks)
🧰 Additional context used
🪛 GitHub Check: linter-check
Src/CrispyWaffle.Utils/Communications/SmtpMailer.cs
[failure] 350-350:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Tests/CrispyWaffle.Tests/Cache/MemoryCacheRepositoryTests.cs
[failure] 56-56:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Tests/CrispyWaffle.Tests/Cache/CouchDBCacheRepositoryTests.cs
[failure] 65-65:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Tests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs
[failure] 55-55:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Src/CrispyWaffle.Redis/Cache/RedisCacheRepository.cs
[failure] 150-150:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Src/CrispyWaffle/Cache/ICacheRepository.cs
[failure] 34-34:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Tests/CrispyWaffle.IntegrationTests/Cache/CouchDBCacheRepositoryTests.cs
[failure] 45-45:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Src/CrispyWaffle/Cache/MemoryCacheRepository.cs
[failure] 45-45:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Src/CrispyWaffle/Cache/CacheManager.cs
[failure] 123-123:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
Src/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs
[failure] 124-124:
File is not formatted correctly. Run 'dotnet csharpier .' to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (csharp)
- GitHub Check: Deep Source Coverage report
- GitHub Check: Codacy Security Scan
🔇 Additional comments (19)
Src/CrispyWaffle.Utils/Communications/SmtpMailer.cs (4)
11-11: Good addition of Threading namespace for cancellation support.The addition of
System.Threadingnamespace is appropriate for the async cache operations that now support cancellation tokens.
333-333: Proper async handling of exception extension.The
HandleExtensionmethod call is correctly awaited, maintaining the asynchronous flow.
375-375: Proper async method signature conversion.The
HandleExtensionmethod signature is correctly updated to returnTask<bool>for async operation.
384-384: Correct async cache operation.The
CacheManager.SetAsynccall is properly awaited with appropriate timeout configuration.Tests/CrispyWaffle.Tests/Cache/MemoryCacheRepositoryTests.cs (3)
2-2: Good addition of Task namespace for async tests.Adding
System.Threading.Tasksis appropriate for the async test methods.
16-29: LGTM - Proper async test conversion.The test method is correctly converted to async with proper await usage on repository operations.
32-44: LGTM - Consistent async pattern.The test follows the same proper async pattern as the previous test.
Tests/CrispyWaffle.Tests/Cache/CouchDBCacheRepositoryTests.cs (3)
1-1: Good addition of Task namespace.Adding
System.Threading.Tasksis appropriate for async test methods.
28-38: LGTM - Proper async test conversion.The test method correctly uses async/await pattern with the repository operations.
41-51: LGTM - Consistent async testing.The test follows proper async patterns and correctly awaits repository operations.
Tests/CrispyWaffle.IntegrationTests/Cache/CouchDBCacheRepositoryTests.cs (4)
27-38: LGTM - Proper async integration test.The integration test correctly uses async/await patterns for CouchDB repository operations.
41-63: LGTM - Comprehensive async testing with specific operations.The test properly exercises both regular and specific async operations with correct await usage.
94-108: Excellent concurrent execution pattern.The concurrent execution of multiple
SetAsyncoperations usingTask.WhenAllis a good testing approach for database operations.
111-125: LGTM - Proper TTL testing with async operations.The TTL test correctly uses async operations for both setting and getting cached values with timeout verification.
Src/CrispyWaffle/Cache/ICacheRepository.cs (4)
2-3: Good namespace additions for async support.Adding
System.ThreadingandSystem.Threading.Tasksis appropriate for the async interface methods.
90-90: Excellent improvement: tuple return instead of out parameters.The change from
outparameters to tuple return(bool Success, T Value)is a significant improvement that makes the API more modern and async-friendly.
108-108: Proper async method signature for Remove operations.Using
Taskreturn type forRemoveAsyncis appropriate since removal operations are typically always asynchronous.
125-131: LGTM - Consistent async method signatures.Both
TTLAsyncandClearmethods have appropriate return types and cancellation support.Src/CrispyWaffle/Cache/CacheManager.cs (1)
829-830: Fix incorrect return type documentationThe XML documentation says the method returns
<c>true</c> if the value was found; otherwise, <c>false</c>, but the actual method signature returns a tuple(bool Success, T Value).-/// <returns><c>true</c> if the value was found; otherwise, <c>false</c>.</returns> +/// <returns>A tuple containing Success (true if found) and Value (the cached item or default(T)).</returns>Likely an incorrect or invalid review comment.
|
✅ Build CrispyWaffle 10.0.414 completed (commit 03292c8b19 by @guibranco) |
|
✅ Build CrispyWaffle 10.0.426 completed (commit 97017ccdba by @guibranco) |
|
✅ Build CrispyWaffle 10.0.1036 completed (commit 4c1ae48590 by @guibranco) |
guibranco
changed the title
guibranco
changed the title
cachemanager and cacherepository implementations to async
|
✅ Build CrispyWaffle 10.0.1545 completed (commit 5bb80ee8c5 by @guibranco) |
5 participants
User description
Closes #88
📑 Description
✅ Checks
☢️ Does this introduce a breaking change?
ℹ Additional Information
Summary by Sourcery
Convert cache management APIs and repository implementations to asynchronous patterns, adding cancellation support, timeouts, and enhanced logging throughout the caching layers, and update dependent tests and SmtpMailer to use the new async methods.
New Features:
Enhancements:
Tests:
Note
I'm currently writing a description for your pull request. I should be done shortly (<1 minute). Please don't edit the description field until I'm finished, or we may overwrite each other. If I find nothing to write about, I'll delete this message.
Description
Changes walkthrough 📝
CacheManager.cs
Migrate CacheManager to Asynchronous OperationsSrc/CrispyWaffle/Cache/CacheManager.cs
CouchDBCacheRepository.cs
Enhance CouchDBCacheRepository with Async SupportSrc/CrispyWaffle.CouchDB/Cache/CouchDBCacheRepository.cs
RedisCacheRepository.cs
Refactor RedisCacheRepository for Asynchronous OperationsSrc/CrispyWaffle.Redis/Cache/RedisCacheRepository.cs
MemoryCacheRepository.cs
Update MemoryCacheRepository for Async MethodsSrc/CrispyWaffle/Cache/MemoryCacheRepository.cs
ICacheRepository.cs
Modify ICacheRepository for Async Method SupportSrc/CrispyWaffle/Cache/ICacheRepository.cs
CacheManagerTests.cs
Refactor CacheManager Tests for Asynchronous MethodsTests/CrispyWaffle.Tests/Cache/CacheManagerTests.cs
MemoryCacheRepositoryTests.cs
Update MemoryCacheRepository Tests for Async SupportTests/CrispyWaffle.Tests/Cache/MemoryCacheRepositoryTests.cs
Summary by CodeRabbit
New Features
Bug Fixes
Tests