Polly.Caching: HybridCache-based caching strategy (net9.0) #2709
Conversation
|
@dotnet-policy-service agree |
|
@martincostello All feedback addressed; please re-review |
There was a problem hiding this comment.
Some initial comments.
This is also missing changes to add the project to the solution.
Mutation tests and documentation would need adding later if this PR progresses to the point of being a candidate to be merged, but those can wait until the API/design is determined.
|
I've moved this back to draft until the API shape is agreed. |
|
Thanks for the review. Given the open API direction (IMemoryCache vs IDistributedCache/HybridCache), I’ll park this PR as draft for now and move on to other issues. If a concrete direction is decided, I’m happy to pick it back up. |
|
I'm saying we should pursue HybridCache as it gives us more flexibility than IMemoryCache. |
|
Thanks for clarifying the direction toward HybridCache. Before I implement, could you confirm the scope so I don’t go off-track? |
Yep.
Let's start with just
Yep. Start with the minimum amount of options to get things working, and we can add things if needed.
|
mohammed-saalim
force-pushed
the
feature/caching-strategy-v8
branch
from
76d01e4 to
b7c110f
Compare
|
I’ve pivoted this PR to HybridCache-only per feedback:
Build and tests are green locally. Ready for review @martincostello |
mohammed-saalim
changed the title
…sions, public API, and tests. Refs App-vNext#1127
…pendency; green tests for caching.
…s, docs, static factory, non-empty key, tests updated
mohammed-saalim
force-pushed
the
feature/caching-strategy-v8
branch
from
cb41f65 to
7a16bc7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2709 +/- ##
==========================================
+ Coverage 96.12% 96.18% +0.05%
==========================================
Files 309 313 +4
Lines 7118 7174 +56
Branches 1008 1015 +7
==========================================
+ Hits 6842 6900 +58
+ Misses 222 221 -1
+ Partials 54 53 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Rebased and addressed all feedback:
Build and tests are green. Ready for review @martincostello |
…ception caching, cleanup
…age (no behavior change)
…e coverage excludes; fix using order
|
@martincostello All feedback addressed! Key changes:
|
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
…xtensionsTests.cs Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
Co-authored-by: Martin Costello <[email protected]>
test/Polly.Caching.Tests/HybridCacheResiliencePipelineBuilderExtensionsTests.cs
Show resolved
Hide resolved
Co-authored-by: Martin Costello <[email protected]>
…mmed-saalim/Polly into feature/caching-strategy-v8
|
@martincostello The build is failing due to code coverage in |
|
@martincostello All CI checks are now passing! The previous coverage failure appears to have been a transient CI issue - the fresh run shows all tests passing with full coverage maintained. Summary of changes addressed from review:
|
| var key = _keyGenerator(context) ?? string.Empty; | ||
| if (key.Length == 0) |
There was a problem hiding this comment.
This hasn't directly answered my question. I can see that if the key is null or empty a specific key is used instead, but why?
What is the behaviour of HybridCache here? Why are null and "" equivalent?
| var result = await _cache.GetOrCreateAsync( | ||
| key, | ||
| (callback, context, state), | ||
| static async (s, _) => | ||
| { | ||
| var outcome = await s.callback(s.context, s.state).ConfigureAwait(s.context.ContinueOnCapturedContext); | ||
| outcome.ThrowIfException(); | ||
| return outcome.Result!; | ||
| }, | ||
| cancellationToken: context.CancellationToken).ConfigureAwait(context.ContinueOnCapturedContext); | ||
|
|
||
| // Handle non-generic (object) pipelines where serializer may return JsonElement. | ||
| return Outcome.FromResult(ConvertUntypedIfJsonElement(result)); | ||
| } | ||
|
|
||
| private static TResult ConvertUntypedIfJsonElement(TResult value) | ||
| { | ||
| if (typeof(TResult) == typeof(object) && value is System.Text.Json.JsonElement json) | ||
| { | ||
| if (json.ValueKind == System.Text.Json.JsonValueKind.Null) | ||
| { | ||
| return (TResult)(object?)null!; | ||
| } | ||
|
|
||
| return (TResult)(object?)json.ToString()!; | ||
| } | ||
|
|
||
| return value; | ||
| } |
There was a problem hiding this comment.
I'm going to mull this over further and play with the code locally when I get the time - this still feels a little odd to me, so I'd like to avoid this special-casing code if possible.
| return (TResult)(object?)null!; | ||
| } | ||
|
|
||
| return (TResult)(object?)json.ToString()!; |
There was a problem hiding this comment.
Doing some investigation, this behaviour is just wrong. While I'm not sure what we should do instead, but the following test illustrates that this is broken:
[Fact]
public async Task NonGeneric_AddHybridCache_BuildsAndCaches_ValueType()
{
var services = new ServiceCollection().AddHybridCache();
using var provider = services.Services.BuildServiceProvider();
var cache = provider.GetRequiredService<HybridCache>();
var options = new HybridCacheStrategyOptions
{
Cache = cache,
CacheKeyGenerator = _ => "builder-key",
};
var pipeline = new ResiliencePipelineBuilder()
.AddHybridCache(options)
.Build();
var r1 = await pipeline.ExecuteAsync(static _ => ValueTask.FromResult(1));
var r2 = await pipeline.ExecuteAsync(static _ => ValueTask.FromResult(2));
r1.ShouldBe(1);
r2.ShouldBe(1);
}This fails with the following exception:
System.InvalidCastException : Unable to cast object of type 'System.String' to type 'System.Int32'.
We can't just keep special-casing types to coerce the type to make the object behaviour magically "just work" for the user.
Maybe the fix is something like swapping out object for some internal surrogate type before doing the cache operation to bypass the JsonElement behaviour, but there's definitely something off with the non-generic/object behaviour here.
Another option might just be to prevent the non-generic pipeline being used with the cache, but that would be a last resort.
|
Updated per feedback: |
|
|
||
| // wrapper moved to top-level public type | ||
|
|
||
| private static object? NormalizeValue(object? value) |
There was a problem hiding this comment.
Unfortunately despite the wrapping type, object is still used, so this ultimately falls down for the same reason as before as it tries to manually convert the JsonDocument back to a specific type.
While this might work for primitive values, it doesn't work when complex types are used:
[Fact]
public async Task NonGeneric_AddHybridCache_For_Custom_Object()
{
var services = new ServiceCollection().AddHybridCache();
using var provider = services.Services.BuildServiceProvider();
var cache = provider.GetRequiredService<HybridCache>();
var options = new HybridCacheStrategyOptions
{
Cache = cache,
CacheKeyGenerator = _ => "json-key"
};
var bandit = new Dog { Name = "Bandit" };
var chilli = new Dog { Name = "Chilli" };
var bluey = new Dog { Name = "Bluey", Father = bandit, Mother = chilli };
var bingo = new Dog { Name = "Bingo", Father = bandit, Mother = chilli };
var family = new List<Dog>
{
bandit,
bingo,
bluey,
chilli,
};
var pipeline = new ResiliencePipelineBuilder()
.AddHybridCache(options)
.Build();
var r1 = await pipeline.ExecuteAsync(_ => ValueTask.FromResult(family));
var r2 = await pipeline.ExecuteAsync(_ => ValueTask.FromResult(family));
r1.ShouldBe(family);
r2.ShouldBe(family);
}
private sealed class Dog
{
public required string Name { get; init; }
public Dog? Father { get; set; }
public Dog? Mother { get; set; }
} Message:
System.InvalidCastException : Unable to cast object of type 'System.String' to type 'System.Collections.Generic.List`1[Polly.Caching.Tests.HybridCacheResiliencePipelineBuilderExtensionsTests+Dog]'.
Stack Trace:
BridgeComponentBase.ConvertOutcome[TFrom,TTo](Outcome`1 outcome) line 30
BridgeComponent`1.ExecuteCore[TResult,TState](Func`3 callback, ResilienceContext context, TState state) line 40
CompositeComponent.ExecuteCoreWithoutTelemetry[TResult,TState](Func`3 callback, ResilienceContext context, TState state) line 95
CompositeComponent.ExecuteCore[TResult,TState](Func`3 callback, ResilienceContext context, TState state) line 78
ResiliencePipeline.ExecuteAsync[TResult](Func`2 callback, CancellationToken cancellationToken)
HybridCacheResiliencePipelineBuilderExtensionsTests.NonGeneric_AddHybridCache_For_Custom_Object() line 114
--- End of stack trace from previous location ---
Summary
Implements a HybridCache-based caching resilience strategy for Polly v8 in a new
Polly.Cachingpackage. This follows the requested direction to useHybridCache(async-first) instead ofIMemoryCache.Scope
Microsoft.Extensions.Caching.Hybrid9.3.0 via VersionOverride; central package versions remain unchanged.Implementation
src/Polly.CachingPolly.Caching.HybridCacheStrategyOptions<TResult>(minimal options: HybridCache instance, key generator; TTL kept for future use but not applied yet to keep scope small).Polly.Caching.HybridCacheResilienceStrategy<TResult>.Polly.HybridCacheResiliencePipelineBuilderExtensionswith:AddHybridCache(this ResiliencePipelineBuilder, HybridCacheStrategyOptions)AddHybridCache<TResult>(this ResiliencePipelineBuilder<TResult>, HybridCacheStrategyOptions<TResult>)PublicAPI.Unshipped.txtupdated.EnablePackageValidation=falsewith a TODO to enable after the first NuGet release baseline.Tests
test/Polly.Caching.Tests:Out-of-scope / Follow-ups
Checklist