Improve OutputCaching vary-by API

[sebastienros]

Background and Motivation

StringValues VaryByPrefix can contain multiple values which are not ordered deterministically. I suggest to change it to string VaryByKeyPrefix like in Response Caching, which will make its purpose obvious.

VaryByValue() supports "single" values and key/pair values. I suggest to only have key/pair values and have users always set a key to be able to order the values in the cache key.

Proposed API

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCachePolicyBuilder
{
    public OutputCachePolicyBuilder();
    public OutputCachePolicyBuilder AddPolicy(Type policyType);
    public OutputCachePolicyBuilder AddPolicy<T>() where T : IOutputCachePolicy;
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, bool> predicate);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, CancellationToken, ValueTask<bool>> predicate);
    public OutputCachePolicyBuilder Tag(params string[] tags);
    public OutputCachePolicyBuilder Expire(TimeSpan expiration);
    public OutputCachePolicyBuilder Cache();
    public OutputCachePolicyBuilder AllowLocking(bool lockResponse = true);
    public OutputCachePolicyBuilder Clear();
    public OutputCachePolicyBuilder NoCache();
    public OutputCachePolicyBuilder VaryByRouteValue(params string[] routeValueNames);
    public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headerNames);
-   public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
-   public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<string>> varyBy);
+   public OutputCachePolicyBuilder VaryByValue(string key, string value);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyBy);
+   public OutputCachePolicyBuilder VaryByKeyPrefix(string keyPrefix);
+   public OutputCachePolicyBuilder VaryByKeyPrefix(Func<HttpContext, string> keyPrefix);
+   public OutputCachePolicyBuilder VaryByKeyPrefix(Func<HttpContext, CancellationToken, ValueTask<string>> keyPrefix);
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; set }
    public StringValues RouteValueNames { get; set; }
    public StringValues HeaderNames { get; set; }
    public StringValues QueryKeys { get; set; }
-   public StringValues VaryByPrefix { get; set; }
+   public string? VaryByKeyPrefix { get; set; }
}

Usage Examples

options.AddBasePolicy(b => b.VaryByKeyPrefix(tenantId));

app.MapGet("/", endpoint.OutputCachebuilder.VaryByValue(c => "customerId", c.GetCurrentCustomerId());

-->

Alternative Designs

If having a prefix is only helpful for diagnostics (seeing groups of keys in the cache) then we could remove VaryByKeyPrefix and only use VaryByValues(string, string).

Risks

n/a

[sebastienros]

Existing PR showing the changes: #43257
This PR changes VaryByCustom { get; } to VaryByValues { get; } which I have not suggested in the api review as I don't think it's necessary.

[davidfowl]

Your usage sample has a syntax error

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.

[halter73]

API Review Notes:

• Is it obvious that VaryByKeyPrefix overrides previous calls?
  • Docs will make this clear
  • Can we come up with a better name?
    • SetCacheKeyPrefix? Sold!
  • Can we make it a property.

API Approved!

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCachePolicyBuilder
{
    public OutputCachePolicyBuilder();
    public OutputCachePolicyBuilder AddPolicy(Type policyType);
    public OutputCachePolicyBuilder AddPolicy<T>() where T : IOutputCachePolicy;
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, bool> predicate);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, CancellationToken, ValueTask<bool>> predicate);
    public OutputCachePolicyBuilder Tag(params string[] tags);
    public OutputCachePolicyBuilder Expire(TimeSpan expiration);
    public OutputCachePolicyBuilder Cache();
    public OutputCachePolicyBuilder AllowLocking(bool lockResponse = true);
    public OutputCachePolicyBuilder Clear();
    public OutputCachePolicyBuilder NoCache();
    public OutputCachePolicyBuilder VaryByRouteValue(params string[] routeValueNames);
    public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headerNames);
-   public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
-   public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<string>> varyBy);
+   public OutputCachePolicyBuilder VaryByValue(string key, string value);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyBy);
+   public OutputCachePolicyBuilder SetCacheKeyPrefix(string keyPrefix);
+   public OutputCachePolicyBuilder SetCacheKeyPrefix(Func<HttpContext, string> keyPrefix);
+   public OutputCachePolicyBuilder SetCacheKeyPrefix(Func<HttpContext, CancellationToken, ValueTask<string>> keyPrefix);
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; set }
    public StringValues RouteValueNames { get; set; }
    public StringValues HeaderNames { get; set; }
    public StringValues QueryKeys { get; set; }
-   public StringValues VaryByPrefix { get; set; }
+   public string? CacheKeyPrefix { get; set; }
}

[sebastienros]

Forgot to suggest changing VaryByCustom to VaryByValues:

public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
-    public IDictionary<string, string> VaryByCustom { get; set }
+    public IDictionary<string, string> VaryByValues { get; set }
    public StringValues RouteValueNames { get; set; }
    public StringValues HeaderNames { get; set; }
    public StringValues QueryKeys { get; set; }
-   public StringValues VaryByPrefix { get; set; }
+   public string? CacheKeyPrefix { get; set; }
}