Remove last ! from Caching.Abstractions
#85918
Conversation
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-extensions-caching Issue DetailsClose #64147
|
| @@ -174,7 +174,8 @@ public static ICacheEntry SetOptions(this ICacheEntry entry, MemoryCacheEntryOpt | |||
|
|
|||
| foreach (PostEvictionCallbackRegistration postEvictionCallback in options.PostEvictionCallbacks) | |||
| { | |||
| entry.RegisterPostEvictionCallback(postEvictionCallback.EvictionCallback!, postEvictionCallback.State); | |||
| ArgumentNullException.ThrowIfNull(postEvictionCallback.EvictionCallback); | |||
There was a problem hiding this comment.
Why is this a beneficial change? It seems like it's adding another check-and-throw that's just duplicating what RegisterPostEvictionCallback is about to do, anyway.
There was a problem hiding this comment.
I'm not opposite to simply remove !
There was a problem hiding this comment.
Here is a small discussion #64147 (comment)
There was a problem hiding this comment.
I'm not opposite to simply remove !
EvictionCallback is typed as PostEvictionDelegate?. RegisterPostEvictionCallback is typed to accept PostEvictionDelegate. So we can't just remove the !. I'm just not clear on why we need to change anything here, if all we're going to do is throw the same exception.
There was a problem hiding this comment.
If it's fine to just pass null to the next check in RegisterPostEvictionCallback, I'll update it
There was a problem hiding this comment.
My original thought was to throw a more appropriate exception here if postEvictionCallback.EvictionCallback is null. ArgumentNullException isn't appropriate for this situation because there wasn't a null argument to this method.
There was a problem hiding this comment.
Nevermind, I wrote this together with I'm not opposite... before your message, just get some internet lag :)
Yep, feel free to close PR and Issue, I just randomly found it and it looked easy to fix 😄
There was a problem hiding this comment.
Here is the code that will get into this situation:
ICacheEntry entry = ...;
MemoryCacheEntryOptions options = new MemoryCacheEntryOptions();
options.PostEvictionCallbacks.Add(new PostEvictionCallbackRegistration());
entry.SetOptions(options);Getting an ArgumentNullException from the above code doesn't make much sense IMO.
There was a problem hiding this comment.
Does something like throw new ArgumentException("Eviction callbacks are not allowed to be null here, but one of the PostEvictionCallbacks was", nameof(options)) looks better?
There was a problem hiding this comment.
I would clean the message up a bit, but yes I think an ArgumentException on options is a better experience. Maybe something like:
"MemoryCacheEntryOptions.PostEvictionCallbacks contains a PostEvictionCallbackRegistration with a null EvictionCallback at index {0}."
We should also add a test.
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/CacheEntryExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Abstractions/src/CacheEntryExtensions.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Eric Erhardt <[email protected]>
src/libraries/Microsoft.Extensions.Caching.Memory/tests/MemoryCacheSetAndRemoveTests.cs
Outdated
Show resolved
Hide resolved
|
@eerhardt is there anything that blocks us from merging this PR? |
I don't believe so. The nit/whitespace comment above could be addressed, but it isn't a major concern. |
…CacheSetAndRemoveTests.cs Co-authored-by: Christopher Watford <[email protected]>
Close #64147