try get nullable

[SimonCropp]

@martincostello

i put together a minimal example to show how i came to use the NotNull for TResult in IAsyncCacheProvider<TResult>

this PR enables nullable for AsyncGenericCacheProvider.cs and IAsyncCacheProvider

you will notice we get the warning

  AsyncGenericCacheProvider.cs(22, 48): [CS8604] Possible null reference argument for parameter 'value' in 'Task IAsyncCacheProvider.PutAsync(string key, object value, Ttl ttl, CancellationToken cancellationToken, bool continueOnCapturedContext)'.

so in

internal class AsyncGenericCacheProvider<TCacheFormat> : IAsyncCacheProvider<TCacheFormat>
{
    private readonly IAsyncCacheProvider _wrappedCacheProvider;

    internal AsyncGenericCacheProvider(IAsyncCacheProvider nonGenericCacheProvider)
        => _wrappedCacheProvider = nonGenericCacheProvider ?? throw new ArgumentNullException(nameof(nonGenericCacheProvider));

    async Task<(bool, TCacheFormat?)> IAsyncCacheProvider<TCacheFormat>.TryGetAsync(string key, CancellationToken cancellationToken, bool continueOnCapturedContext)
    {
        (bool cacheHit, object? result) = await _wrappedCacheProvider.TryGetAsync(key, cancellationToken, continueOnCapturedContext).ConfigureAwait(continueOnCapturedContext);
        return (cacheHit, (TCacheFormat?)(result ?? default(TCacheFormat)));
    }

    Task IAsyncCacheProvider<TCacheFormat>.PutAsync(string key, TCacheFormat value, Ttl ttl, CancellationToken cancellationToken, bool continueOnCapturedContext)
        => _wrappedCacheProvider.PutAsync(key, value, ttl, cancellationToken, continueOnCapturedContext);
}

since someone could define AsyncGenericCacheProvider<MyType?> it means value could be null in

_wrappedCacheProvider.PutAsync(key, value, ttl, cancellationToken, continueOnCapturedContext);

so we could allow null for value through the stack. so IAsyncCacheProvider would need to accept null for PutAsync. Do we want that?

if we notnull on TCacheFormat it means people cant pass a null value to PutAsync.

TBH i think the fix here is to add a Clear method that takes a key??

thoughts?

[martincostello]

I think it depends on what the original intention of the cache policy is - it currently allows null values, so I would have thought the nullable annotations would therefore continue to allow that, otherwise that's a change in behaviour.

Looking at IMemoryCache in dotnet/runtime for some prior art, it allows null for cached values, just not keys:

• object? ICacheEntry.Value { get; set; }
• public static object? Get(this IMemoryCache cache, object key)

[SimonCropp]

@martincostello i made some changes based on your feedback. can u take another look.

[codecov-commenter]

Codecov Report

Merging #1015 (9343ee4) into main (ab957f2) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1015      +/-   ##
==========================================
+ Coverage   71.19%   71.22%   +0.03%     
==========================================
  Files         137      137              
  Lines        3760     3764       +4     
  Branches      764      764              
==========================================
+ Hits         2677     2681       +4     
  Misses        872      872              
  Partials      211      211              

Flag	Coverage Δ
linux	71.22% <100.00%> (+0.03%)	⬆️
macos	71.22% <100.00%> (+0.03%)	⬆️
windows	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/Polly/Caching/AsyncCacheEngine.cs	100.00% <100.00%> (ø)
src/Polly/Caching/AsyncGenericCacheProvider.cs	83.33% <100.00%> (ø)
src/Polly/Caching/AsyncSerializingCacheProvider.cs	100.00% <100.00%> (ø)
src/Polly/Caching/CacheEngine.cs	100.00% <100.00%> (ø)
src/Polly/Caching/GenericCacheProvider.cs	80.00% <100.00%> (ø)
src/Polly/Caching/SerializingCacheProvider.cs	100.00% <100.00%> (ø)
src/Polly/Fallback/AsyncFallbackPolicy.cs	88.57% <100.00%> (+1.47%)	⬆️
src/Polly/Retry/AsyncRetryPolicy.cs	94.73% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[martincostello]

Should this method return TResult? so that this isn't needed?

[SimonCropp]

i dont think so. if we have a cacheHit then we know valueFromCache is not null. so we can be sure ImplementationAsync will return a not null

[martincostello]

I thought we changed things based on my earlier comment to allow you to set null into the cache, so in that case you'd get a cache hit on a null.

[martincostello]

+1