HybridCache - rename RemoveKeyAsync and RemoveTagAsync to "By"

[mgravell]

This is a .NET 9 feature that is in preview 4 but is not yet GA.

It was raised in post-merge feedback that the "remove" naming is a little unclear/ambiguous/inconsistent - in particular, for RemoveTagAsync, we're not "removing a tag", we're removing other things (cache entries) by their tag. Cross-reference: #53255

proposed:

  namespace Microsoft.Extensions.Caching.Hybrid;

  public abstract class HybridCache
  {
-     public abstract ValueTask RemoveKeyAsync(string key, CancellationToken token = default);
-     public virtual ValueTask RemoveKeysAsync(IEnumerable<string> keys, CancellationToken token = default)
-     public abstract ValueTask RemoveTagAsync(string tag, CancellationToken token = default);
-     public virtual ValueTask RemoveTagsAsync(IEnumerable<string> tags, CancellationToken token = default)

+     public abstract ValueTask RemoveByKeyAsync(string key, CancellationToken token = default);
+     public virtual ValueTask RemoveByKeyAsync(IEnumerable<string> keys, CancellationToken token = default)
+     public abstract ValueTask RemoveByTagAsync(string tag, CancellationToken token = default);
+     public virtual ValueTask RemoveByTagAsync(IEnumerable<string> tags, CancellationToken token = default)
  }

This adds the By nomenclature, and removes the plurality (RemoveByTag seems fine when taking multiple tags, and RemoveByTags prompts the question as to whether a cache-entry needs to have all the tags). The ambiguity of null (implicitly castable to string and IEnumerable<string>) is not important; this is invalid both in terms of the API (via NRT) and implementation (null is not really valid for either scanario; we have advised implementors to silently no-op null in the "tag" case, but we don't need to encourage it, and explicit null would already have been a compiler warning because of NRT).

Additionally, this makes the API more consistent with IOutputCacheStore, which uses the name EvictByTagAsync, i.e. uses the By naming. The Evict vs Remove is a separate discussion - happy to have that "now", but for comparison: IMemoryCache uses Remove; IDistributedCache uses Remove/RemoveAsync; our main objective here is to clarify the "what by?".

[dotnet-policy-service]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[jodydonetti]

This adds the By nomenclature

Good! I like the "By" usage for the tags.

For the RemoveByKey though I see you pointed out the consistency with the memory/distributed caches where it's simply Remove so by following the same approach here it should be simply Remove (since the "by key" is the main assumption).
But, again for consistency, there's the "by tag(s)", so there are 2 different point of views about consistency here contraddicting each other.
Considering that in the future even the memory cache will get a RemoveByTag feature (but it will keep the Remove as-is), maybe it makes sense to use simply Remove for the "by key" here, too.
Just throwing some thoughts here to reason about.
Anyway yeah, naming is hard, and this is a tough one 😅

and removes the plurality (RemoveByTag seems fine when taking multiple tags, and RemoveByTags prompts the question as to whether a cache-entry needs to have all the tags).

Here I'm not so sure. I got the simplification approach, and I kinda like it, but still not 100% sure... I went back and forth about maybe using RemoveByAnyTag(...) (to be explicit about the fact the match is not for all tags but any of them), but.. I don't know.
Again throwing an additional idea here, thoughts?

NOTE: not being a native speaker I'm not 100% sure about if it should be RemoveByAnyTag(...) or RemoveByAnyTags(...) (probably the first, singular, but again not 100% sure).

The ambiguity of null (implicitly castable to string and IEnumerable<string>) is not important; this is invalid both in terms of the API (via NRT) and implementation (null is not really valid for either scanario; we have advised implementors to silently no-op null in the "tag" case, but we don't need to encourage it, and explicit null would already have been a compiler warning because of NRT).

I totally agree: worth to point out the rationale here, but it's not an issue.
Also, if someone wants to explicitly call with null they can simply cast with (string?)null and call it a day, right?

Additionally, this makes the API more consistent with IOutputCacheStore, which uses the name EvictByTagAsync, i.e. uses the By naming.

Agree, even though there is limited only to 1 tag, so the doubts about going plural still stands.
Again, a tough one.

The Evict vs Remove is a separate discussion - happy to have that "now", but for comparison: IMemoryCache uses Remove; IDistributedCache uses Remove/RemoveAsync; our main objective here is to clarify the "what by?".

Agree. Also, even the new RCache (or whatever name will have) will have a Remove method.

Just as a side note, in FusionCache I have both Remove and Expire, but that's because with fail-safe it has the opportunity to either remove it for good (for when you actually remove something from the db and want the cached copy to disappear from the cache) or to expire it logically (meaning it will keep the cached value, marked as expired, as a fallback for future fail-safe activation in case of problems).
But this does not apply here.

My 2 cents: I would go with Remove instead of Evict without issues.

To recap, an idea to play with:

namespace Microsoft.Extensions.Caching.Hybrid;

public abstract class HybridCache
{
  public abstract ValueTask RemoveAsync(string key, CancellationToken token = default);
  public virtual ValueTask RemoveAsync(IEnumerable<string> keys, CancellationToken token = default)


  public abstract ValueTask RemoveByTagAsync(string tag, CancellationToken token = default);
  public virtual ValueTask RemoveByAnyTagAsync(IEnumerable<string> tags, CancellationToken token = default)
  // or
  public abstract ValueTask RemoveByTagAsync(string tag, CancellationToken token = default);
  public virtual ValueTask RemoveByTagAsync(IEnumerable<string> tags, CancellationToken token = default)
}

Just something for you all to think about, marinate a little bit, and see if it sticks.

[amcasey]

[API Review]

We like @jodydonetti's point about RemoveByKey.

  namespace Microsoft.Extensions.Caching.Hybrid;

  public abstract class HybridCache
  {
-     public abstract ValueTask RemoveKeyAsync(string key, CancellationToken token = default);
-     public virtual ValueTask RemoveKeysAsync(IEnumerable<string> keys, CancellationToken token = default)
-     public abstract ValueTask RemoveTagAsync(string tag, CancellationToken token = default);
-     public virtual ValueTask RemoveTagsAsync(IEnumerable<string> tags, CancellationToken token = default)

+     public abstract ValueTask RemoveAsync(string key, CancellationToken token = default);
+     public virtual ValueTask RemoveAsync(IEnumerable<string> keys, CancellationToken token = default)
+     public abstract ValueTask RemoveByTagAsync(string tag, CancellationToken token = default);
+     public virtual ValueTask RemoveByTagAsync(IEnumerable<string> tags, CancellationToken token = default)
  }

Slight preference for no "Any" but approved either way.

[BrennanConroy]

Looks like this was done in #55732