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.19% +0.06%
==========================================
Files 309 313 +4
Lines 7118 7196 +78
Branches 1008 1018 +10
==========================================
+ Hits 6842 6922 +80
+ 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)
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
…xtensionsTests.cs Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
Co-authored-by: Martin Costello <martin@martincostello.com>
test/Polly.Caching.Tests/HybridCacheResiliencePipelineBuilderExtensionsTests.cs
Show resolved
Hide resolved
Co-authored-by: Martin Costello <martin@martincostello.com>
…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 ---
There was a problem hiding this comment.
@martincostello We’ve kept existing non-generic semantics for complex values (object/array → string) to avoid breaking established tests and behaviors. Typed pipelines are fully type-preserving and unaffected.
If you’d like full type preservation for complex objects in non-generic pipelines, I’ll follow up with an opt-in option on HybridCacheStrategyOptions (default false) so we don’t change existing behavior by default.
Current state: null/empty keys bypass caching, untyped wrapper + primitive JsonElement normalization, typed unchanged; tests updated; 100% coverage.
…efaults - Add PreserveComplexUntypedValues option (default false) - Untyped path: if enabled, restore complex types via recorded type name; otherwise keep existing normalization - Added tests for custom object + opt-in paths - All tests passing; 100% coverage
|
@martincostello Implemented an opt-in option to preserve complex types in non-generic pipelines without changing defaults:
This keeps backward compatibility while enabling the desired behavior for users who need it. If you prefer a different default or approach, happy to adjust. |
|
I'm still not convinced by the latest changes. Untyped pipelines seem to basically be a pit-of-failure that require a workaround internally that I don't think we should really need to be dealing with. The new setting feels weird to me. Given the caching strategy would be new, having to turn a setting on by default to get it to work would look weird to users, but having a "make it work" option itself is weirder. I think either it needs to be refactored in a way that it Just Works™️, or all the support for untyped pipelines should just be removed. |
|
@martincostello I understand your concern about the complexity. You're right that the opt-in setting adds friction. I see three paths forward: 1. Make it "Just Work"™️ by default
2. Remove untyped pipeline support
3. Keep primitives working, block complex types
Which direction do you prefer? I'm happy to implement any of these approaches. |
|
I'm not sure 1 is possible using the second step proposed for it to work. I'm not sure we need to throw an exception for untyped pipelines, as we could just make it unusable by not adding any factory/build methods that allow you to create one for an untyped pipeline? Option 3 just sounds like a pit of failure - you try to use it, then find it doesn't actually work. 1 is preferred if you can find a way to make it work somehow, otherwise we can take a look at 2, but if the implementation is only partially working and incomplete, then I'm not sure it's something we should build into Polly.Core. For option 2 that makes me gravitate more towards this being an add-on the community can provide with another package, rather than it being something we support and include in-box here. |
2 participants
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