Skip to content

Add Output Caching middleware #41955

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sebastienros opened this issue May 31, 2022 · 33 comments
Closed

Add Output Caching middleware #41955

sebastienros opened this issue May 31, 2022 · 33 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares feature-output-caching
Milestone

Comments

@sebastienros
Copy link
Member

sebastienros commented May 31, 2022

Background and Motivation

Implement Output Caching middleware.

Proposed API

Setup

public static class OutputCacheServicesExtensions
{
    public static IServiceCollection AddOutputCaching(this IServiceCollection services);
    public static IServiceCollection AddOutputCaching(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}

public static class OutputCacheExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}

public class OutputCacheOptions
{
    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public PoliciesCollection BasePolicies { get; }
    public IServiceProvider ApplicationServices { get; set; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);
}

Configuration

public interface IOutputCachePolicy
{
    Task OnRequestAsync(OutputCacheContext context);
    Task OnServeFromCacheAsync(OutputCacheContext context);
    Task OnServeResponseAsync(OutputCacheContext context);
}

public sealed class OutputCachePolicyBuilder : IOutputCachePolicy
{
    public OutputCachePolicyBuilder();
    public OutputCachePolicyBuilder AddPolicy(IOutputCachePolicy policy);
    public OutputCachePolicyBuilder AddPolicy(Type policyType);
    public OutputCachePolicyBuilder AddPolicy<T>() where T : IOutputCachePolicy;
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, bool> predicate);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, Task<bool>> predicate);
    public OutputCachePolicyBuilder Policy(string profileName);
    public OutputCachePolicyBuilder Tag(params string[] tags);
    public OutputCachePolicyBuilder Expire(TimeSpan expiration);
    public OutputCachePolicyBuilder AllowLocking(bool lockResponse = true);
    public OutputCachePolicyBuilder Clear();
    public OutputCachePolicyBuilder NoCache();
    public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headers);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, Task<string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, (string, string)> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, Task<(string, string)>> varyBy);
}

public sealed class OutputCacheContext
{
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
    public HttpContext HttpContext { get; }
    public DateTimeOffset? ResponseTime { get; }
    public CachedVaryByRules CachedVaryByRules { get; set; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}

public class CachedVaryByRules
{
    public void SetVaryByCustom(string key, string value);
    public StringValues Headers { get; set; }
    public StringValues QueryKeys { get; set; }
    public StringValues VaryByPrefix { get; set; }
}


public class OutputCacheAttribute : Attribute, IOutputCachePolicy
{
    public int Duration { get; set; }
    public bool NoStore { get; set; }
    public string[]? VaryByQueryKeys { get; set; }
    public string[]? VaryByHeaders { get; set; }
    public string? PolicyName { get; set; }
}

public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
    PoliciesCollection Policies { get; }
}

Output Cache Store

Kept for now since it's necessary to evict by tags. Suggested to be dropped for first preview.

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken token);
    ValueTask<OutputCacheEntry?> GetAsync(string key, CancellationToken token);
    ValueTask SetAsync(string key, OutputCacheEntry entry, TimeSpan validFor, CancellationToken token);
}

public sealed class OutputCacheEntry
{
    public DateTimeOffset Created { get; set; }
    public int StatusCode { get; set; }
    public IHeaderDictionary Headers { get; set; }
    public CachedResponseBody Body { get; set; }
    public string[]? Tags { get; set; }
}

public class CachedResponseBody
{
    public List<byte[]> Segments { get; }
    public long Length { get; }
    public CachedResponseBody(List<byte[]> segments, long length);
    public async Task CopyToAsync(PipeWriter destination, CancellationToken cancellationToken);
}

Sample

using System.Globalization;
using Microsoft.AspNetCore.OutputCaching;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddOutputCaching(options =>
{
    // Define policies for all requests which are not configured per endpoint or per request
    options.BasePolicies.AddPolicy(b => b.With(c => c.HttpContext.Request.Path.StartsWithSegments("/js")).Expire(TimeSpan.FromDays(1)).AddPolicy<EnableCachePolicy>());
    options.BasePolicies.AddPolicy(b => b.With(c => c.HttpContext.Request.Path.StartsWithSegments("/js")).NoCache());

    options.AddPolicy("NoCache", b => b.NoCache());
});

var app = builder.Build();

app.UseOutputCache();

app.MapGet("/", Gravatar.WriteGravatar);

app.MapGet("/cached", Gravatar.WriteGravatar).CacheOutput();

app.MapGet("/nocache", Gravatar.WriteGravatar).CacheOutput(x => x.NoCache());

app.MapGet("/profile", Gravatar.WriteGravatar).CacheOutput(x => x.Policy("NoCache"));

app.MapGet("/attribute", [OutputCache(PolicyName = "NoCache")] () => Gravatar.WriteGravatar);

var blog = app.MapGroup("blog").CacheOutput(x => x.Tag("blog"));
blog.MapGet("/", Gravatar.WriteGravatar);
blog.MapGet("/post/{id}", Gravatar.WriteGravatar).CacheOutput(x => x.Tag("byid")); // check we get the two tags. Or if we need to get the last one

app.MapPost("/purge/{tag}", async (IOutputCacheStore cache, string tag) =>
{
    // POST such that the endpoint is not cached itself

    await cache.EvictByTagAsync(tag, default);
});

// Cached entries will vary by culture, but any other additional query is ignored and returns the same cached content
app.MapGet("/query", Gravatar.WriteGravatar).CacheOutput(p => p.VaryByQuery("culture"));

app.MapGet("/vary", Gravatar.WriteGravatar).CacheOutput(c => c.VaryByValue((context) => ("time", (DateTime.Now.Second % 2).ToString(CultureInfo.InvariantCulture))));

long requests = 0;

// Locking is enabled by default
app.MapGet("/lock", async (context) =>
{
    await Task.Delay(1000);
    await context.Response.WriteAsync($"<pre>{requests++}</pre>");
}).CacheOutput(p => p.AllowLocking(false).Expire(TimeSpan.FromMilliseconds(1)));

// Etag
app.MapGet("/etag", async (context) =>
{
    // If the client sends an If-None-Match header with the etag value, the server
    // returns 304 if the cache entry is fresh instead of the full response

    var etag = $"\"{Guid.NewGuid():n}\"";
    context.Response.Headers.ETag = etag;

    await Gravatar.WriteGravatar(context);

    var cacheContext = context.Features.Get<IOutputCacheFeature>()?.Context;

}).CacheOutput();

// When the request header If-Modified-Since is provided, return 304 if the cached entry is older
app.MapGet("/ims", Gravatar.WriteGravatar).CacheOutput();

await app.RunAsync();
@sebastienros sebastienros added api-suggestion Early API idea and discussion, it is NOT ready for implementation feature-output-caching labels May 31, 2022
@martincostello
Copy link
Member

Should the methods on IOutputCacheStore have a CancellationToken parameter?

@adityamandaleeka adityamandaleeka added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 2, 2022
@ghost
Copy link

ghost commented Jun 2, 2022

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
Copy link
Member

halter73 commented Jun 2, 2022

I think all the async methods should probably have a CancellationToken parameter.

What's the difference between IOutputCachingPolicy and IOutputCachingPolicyProvider? Are they different just because one is on metadata and one is from DI?

@Tratcher Tratcher added this to the 7.0-preview6 milestone Jun 3, 2022
@sebastienros
Copy link
Member Author

#41037

@halter73
Copy link
Member

halter73 commented Jun 7, 2022

API Review Notes:

  • Can we have an overload that takes just the policy name as a "string"
    • We like builder.Policy("policyName"). over builder.Profile("profileName")
    • Everywhere we reference Profile we should use Policy instead to align with auth.
  • Wait. Is a policy different from a profile?
    • No. We're going with a single policy per endpoint.
    • Should a policy be able to reference another policy to merge with? But couldn't the following be infinitely recursive? Do we just guard against this at runtime?
options.Policies["NoCache"] = new OutputCachePolicyBuilder().Policy("NoCache").Build();
  • What's OutputCachePolicyBuilder().Enabled() for?
    • We need a disabled state.
    • Review: Policies should be enabled by default if the given endpoint has output caching metadata pointing to that policy.
  • Can we copy auth more?
    • Caching should be opt-in by default.
      • Auth is also opt-in by default. Auth also doesn't activate unless it has metadata defined on the given endpoint
    • The default policy should be the policy used by endpoints with .CacheOutput() with no parameters. It should not be applied to all endpoints.
  • Should CacheOutput(Action<OutputCachePolicyBuilder>) use a default OutputCachePolicyBuilder?
    • To take requirements from DefaultPolicy that may be overridden, sure but each OutputCachePolicyBuilder should be fresh instance indistinguishable from new OutputCachePolicyBuilder().
  • Can this support nested groups?
    • Not yet. It uses the most local metadata and then chains to the default policy.
    • We should be able to chain to arbitrary parent polices to support nested groups of arbitrary depths.
  • What policy is used why I call app.MapGet("/uncached", Gravatar.WriteGravatar).CacheOutput(myPolicy); where var myPolicy = new OutputCachePolicyBuilder().NoCache().Build();?
    • It will use the OutputCachingOptions.DefaultPolicy with behaviors merged with myPolicy. Where there are conflicts, the more local myPolicy is preferred.
  • How do I use caching from custom middleware?
    • Can we add a feature to the HttpContext that says "cache this response"? HttpContext.CacheOutput() This is somewhat similar to being able to call HttpContext.ChallengeAsync() for auth?
    • Keep the caching as static as possible. We can have things like OutputCachePolicyBuilder.VaryByQuery, but we'd only check it if the policy built by that OutputCachePolicyBuilder was applied to the given endpoint.
    • This probably means that you cannot share any caches across endpoints.
    • We should have a service that makes it easy to store and responses and replay them based on a user-specified key. Like a higher level IMemoryCache.
    • Let's not redo routing either even if it leverages common logic.
  • Can we use IDistributedCache instead of IOutputCache?
  • We should have zero extra allocations when an endpoint that doesn't have a policy defined is hit.
  • Should Endpoint middleware warn if an endpoint has output caching metadata with not output caching middleware?
    • Yes.
  • IOutputCachingContext and anything else people are not expected to implement should just be a public sealed class with public constructors.
  • Can we use ActivatorUtilities for adding a policy based on Type that injects singleton services.
  • We shouldn't expose options.Policies. Instead, we should expose options.AddPolicy(string name, IOutputCachingPolicy) and options.AddPolicy<TPolicy>(string name) where TPolicy : IOutputCachingPolicy.
  • Make allocation pay per play
    • As stated earlier "The default policy should be the policy used by endpoints with .CacheOutput() with no parameters. It should not be applied to all endpoints." At the very least, it needs to be opted in by something more than just defining a DefaultPolicy.
  • Should WebApplicationBuilder auto-add output caching middleware?
    • Didn't get to this.

@halter73 halter73 added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jun 7, 2022
@halter73
Copy link
Member

API Review Notes:

  • Can we put all of output caching in a single assembly instead of splitting out abstraction? This helps avoid InternalsVisibleTo

  • Let's add OutputCachingOptions.ApplicationServices for ActivatorUtilites usage. See KestrelServerOptionsSetup.

  • Can we have bool OutputCachingOptions.EnableBasePolicyForAllRequests { get; set; }.

  • Do we still need Enabled()/Disabled()?

    • If we need to be able to build a policy using OutputPolicyBuilder that acts the same as no policy when applied to an endpoint, we should call it OutputPolicyBuilder.NoCache() (similar to NoStore() but doesn't serve from cacher either)
    • NoCacheMetadata on the endpoint should disable whatever policy would have otherwise configured.
  • How do we make this work for non-endpoints? Like static file middleware.

    • Do we attempt to serve every request from cache even if there is no endpoint?
    • Do we need a slow mode that attempts to serve every request from the cache even if there is no endpint?
      • Yes this should be an opt-in global option.
  • How do we do "Composite" global policies that define different policies for different types of requests (e.g. cache requests with /js prefixed paths for 1 day and /css prefixed paths for 2 days.

  • We like OutputCachingPolicy.AddPolicy(IOutputCachingPolicy policy) is a global policy. And OutputCachingPolicy.AddPolicy(string policy, IOutputCachingPolicy policy) could only be referenced by name.

  • Do we need to make CompositePolicy public for named composite policies?

    • Seems useful. Maybe it can be exposed via a helper method somewhere.
  • Endpoint-specific policy should not use BasePolicy at all. It should be a complete policy.

  • We should not support adding "policy" to an ongoing request. Just set state on the OutputCachingContext

  • Do policies need to be able to reference other named policy?

    • It doesn't seem that useful. You can just reference the IOutputCachingPolicy directly instead of by name.
  • Can we throw an InvalidOperationException when using the feature to manually store a response to the cache when we know there is no global policy or endpoint-specific policy for this request?

    • This seems useful.

@sebastienros
Copy link
Member Author

Updated

@halter73
Copy link
Member

halter73 commented Jun 15, 2022

API Review Notes:

  • What's a PoliciesCollection? Why isn't it part of this API? Can't this just be exposed as a IList<IOutputCachePolicy> instead?
  • What about CacheOutput() methods? That's used a lot in the sample. Where is that defined?
    • I'm adding it below based on what's in the PR and my feedback.
    • Why isn't there a string policyName overload?
    • Shouldn't the namespace be in Microsoft.AspNetCore.Builder instead of Microsoft.AspNetCore.OutputCaching?
    • If so, can we use a less generic name than PolicyExtensions? Maybe OutputCacheConventionBuilderExtensions.
  • UseOutputCache and OutputCacheExtensions should also be in Microsoft.AspNetCore.Builder instead of Microsoft.AspNetCore.OutputCaching.
    • This means OutputCacheExtensions is again too generic. How about OutputCacheApplicationBuilderExtensions?
    • Would we use a common static class for all OutputCaching extension methods? ``OutputCacheBuilderExtensions`?
  • AddOutputCaching should be in Microsoft.Extensions.DependencyInjection instead of Microsoft.AspNetCore.OutputCaching
    • OutputCacheServicesExtensions should be OutputCacheServiceCollectionExtensions.
  • Why UseOutputCache and then AddOutputCaching?
    • Let's change AddOutputCaching to AddOutputCache.
  • Do we really need to add other polices to the OutputCachePolicyBuilder.AddPolicy(IOutputCachePolicy policy)?
    • What if it's recursive? Do we have a test for that?
    • Let's just leave the type based overloads for now.
  • What about OutputCachePolicyBuilder.Policy(string profileName)
    • This one sems more usable than AddPolicy(IOutputCachePolicy policy) but still has the recursion problem.
    • Add a test for recursive OutputCachePolicyBuilder.Policy(string profileName) since we're going to keep it!
    • All string profileName should be string policyName instead! No "policy" should be anywhere in public API!
    • Shouldn't it be AddPolicy like all the other similar methods instead of just Policy?
  • Is new OutputCachePolicyBuilder().NoCache().Build() different than new OutputCachePolicyBuilder().Build()?
    • If so, how? If not, when does it behave differently.
    • I'll assume we need it and keep it in the following API I'm ready to approve.
  • Can we seal OutputCacheOptions, CachedVaryByRules and OutputCacheAttribute?
    • Can we rename CachedVaryByRules to CacheVaryByRules?
    • Is the past tense significant?
  • Can we make OutputCacheOptions.ApplicationServices get only (to external assemblies?
  • Can we make CachedVaryByRules and OutputCacheAttribute setters init-only?
  • Why is CacheVaryByRules.SetVaryByCustom(string key, string value) a think rather than just making it public IDictionary<string, string> CachedVaryByRules.VaryByCustom { get; }?
  • (string, string) doesn't communicate what the elements are.
    • If it's just a key and a value, can we use KeyValuePair<string, string> instead? It's more widely used.
  • OutputCachePolicyBuilder implements IOutputCachePolicy by rebuilding itself on every call to one if it's methods!?
    • Let's not do that.
    • And if we must, let's still not implement IOutputCachePolicy publically. We can create a wrapper if we need to.
  • OutputCacheAttribute does the same thing!?
    • Yeah. No.
  • Everything that returns a Task, including Funcs should take a CancellationToken.

I don't think we should ship the Output Cache Store APIs until we have at least one non-in-memory implementation that shows an advantage over IDistributedCache

Is there any public API added by #41037 that I didn't catch? I caught that PoliciesCollection and the CacheOutput() were missed. Is there anything else? It's very possible I missed stuff. We shouldn't add any public API that's not listed exactly as it is in the final API once it's approved.

Here's the diff from the most recent proposal.

+namespace Microsoft.AspNetCore.Builder;
+
-public static class OutputCacheServicesExtensions
+public static class OutputCacheServiceCollectionExtensions
 {
-    public static IServiceCollection AddOutputCaching(this IServiceCollection services);
-    public static IServiceCollection AddOutputCaching(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
+    public static IServiceCollection AddOutputCache(this IServiceCollection services);
+    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
 }
 
-public static class OutputCacheExtensions
+public static class OutputCacheApplicationBuilderExtensions
 {
     public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
 }
 
+namespace Microsoft.Extensions.DependencyInjection;
+
+public static class OutputCacheConventionBuilderExtensions
+{
+    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
+    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
+    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
+    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
+}
+
+namespace Microsoft.AspNetCore.OutputCaching;
+
-public class OutputCacheOptions
+public sealed class OutputCacheOptions
 {
     public long SizeLimit { get; set; }
     public long MaximumBodySize { get; set; }
     public TimeSpan DefaultExpirationTimeSpan { get; set; }
-    public PoliciesCollection BasePolicies { get; }
-    public IServiceProvider ApplicationServices { get; set; }
+    public IList<IOutputCachePolicy> BasePolicies { get; }
+    public IServiceProvider ApplicationServices { get; }
     public void AddPolicy(string name, IOutputCachePolicy policy);
     public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
-    Task OnRequestAsync(OutputCacheContext context);
-    Task OnServeFromCacheAsync(OutputCacheContext context);
-    Task OnServeResponseAsync(OutputCacheContext context);
+    Task OnRequestAsync(OutputCacheContext context, CancellationToken token);
+    Task OnServeFromCacheAsync(OutputCacheContext context, CancellationToken token);
+    Task OnServeResponseAsync(OutputCacheContext context, CancellationToken token);
 }
 
-public sealed class OutputCachePolicyBuilder : IOutputCachePolicy
+public sealed class OutputCachePolicyBuilder
 {
     public OutputCachePolicyBuilder();
-    public OutputCachePolicyBuilder AddPolicy(IOutputCachePolicy policy);
     public OutputCachePolicyBuilder AddPolicy(Type policyType);
     public OutputCachePolicyBuilder AddPolicy<T>() where T : IOutputCachePolicy;
+    public OutputCachePolicyBuilder AddPolicy(string policyName);
     public OutputCachePolicyBuilder With(Func<OutputCacheContext, bool> predicate);
-    public OutputCachePolicyBuilder With(Func<OutputCacheContext, Task<bool>> predicate);
+    public OutputCachePolicyBuilder With(Func<OutputCacheContext, CancellationToken, Task<bool>> asyncPredicate);
-    public OutputCachePolicyBuilder Policy(string profileName);
     public OutputCachePolicyBuilder Tag(params string[] tags);
     public OutputCachePolicyBuilder Expire(TimeSpan expiration);
     public OutputCachePolicyBuilder AllowLocking(bool lockResponse = true);
     public OutputCachePolicyBuilder Clear();
     public OutputCachePolicyBuilder NoCache();
     public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
     public OutputCachePolicyBuilder VaryByHeader(params string[] headers);
     public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
-    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, Task<string>> varyBy);
-    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, (string, string)> varyBy);
-    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, Task<(string, string)>> varyBy);
+    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, Task<string>> varyByAsync);
+    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
+    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, Task<KeyValuePair<string, string>>> varyByAsnc);
 }
 
 public sealed class OutputCacheContext
 {
+    public OutputCacheContext();
     public bool EnableOutputCaching { get; set; }
     public bool AllowCacheLookup { get; set; }
     public bool AllowCacheStorage { get; set; }
     public bool AllowLocking { get; set; }
     public HttpContext HttpContext { get; }
     public DateTimeOffset? ResponseTime { get; }
     public CachedVaryByRules CachedVaryByRules { get; set; }
     public HashSet<string> Tags { get; }
     public TimeSpan? ResponseExpirationTimeSpan { get; set; }
 }
 
-public class CachedVaryByRules
+public sealed class CacheVaryByRules
 {
-    public void SetVaryByCustom(string key, string value);
-    public StringValues Headers { get; set; }
-    public StringValues QueryKeys { get; set; }
-    public StringValues VaryByPrefix { get; set; }
+    public CacheVaryByRules();
+    public IDictionary<string, string> VaryByCustom { get; }
+    public StringValues Headers { get; init; }
+    public StringValues QueryKeys { get; init; }
+    public StringValues VaryByPrefix { get; init; }
 }
 
-
-public class OutputCacheAttribute : Attribute, IOutputCachePolicy
+public sealed class OutputCacheAttribute : Attribute
 {
-    public int Duration { get; set; }
-    public bool NoStore { get; set; }
-    public string[]? VaryByQueryKeys { get; set; }
-    public string[]? VaryByHeaders { get; set; }
-    public string? PolicyName { get; set; }
+    public OutputCacheAttribute();
+    public int Duration { get; init; }
+    public bool NoStore { get; init; }
+    public string[]? VaryByQueryKeys { get; init; }
+    public string[]? VaryByHeaders { get; init; }
+    public string? PolicyName { get; init; }
 }
 
 public interface IOutputCacheFeature
 {
     OutputCacheContext Context { get; }
-    PoliciesCollection Policies { get; }
+    IList<IOutputCachePolicy> Policies { get; }
}
-
-public interface IOutputCacheStore
-{
-    ValueTask EvictByTagAsync(string tag, CancellationToken token);
-    ValueTask<OutputCacheEntry?> GetAsync(string key, CancellationToken token);
-    ValueTask SetAsync(string key, OutputCacheEntry entry, TimeSpan validFor, CancellationToken token);
-}
-
-public sealed class OutputCacheEntry
-{
-    public DateTimeOffset Created { get; set; }
-    public int StatusCode { get; set; }
-    public IHeaderDictionary Headers { get; set; }
-    public CachedResponseBody Body { get; set; }
-    public string[]? Tags { get; set; }
-}
-
-public class CachedResponseBody
-{
-    public List<byte[]> Segments { get; }
-    public long Length { get; }
-    public CachedResponseBody(List<byte[]> segments, long length);
-    public async Task CopyToAsync(PipeWriter destination, CancellationToken cancellationToken);
-}

@sebastienros @dotnet/aspnet-api-review Does this seem reasonable to all of you?

Edit: Updated IReadOnlyList<IOutputCachePolicy> to IList<IOutputCachePolicy>.
Edit 2: Add CancellationToken to all Task-returning APIs.
Edit 3: Use diff

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Jun 16, 2022
@ghost
Copy link

ghost commented Jun 16, 2022

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 halter73 removed the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Jun 16, 2022
@halter73
Copy link
Member

halter73 commented Jun 16, 2022

@sebastienros Can you explain OutputCachePolicyBuilder.AllowLocking() and OutputCacheContext.AllowLocking a little bit more? It looks like setting this to false disables serving from the response cache. Is that it? Will most users of this API understand that?

Also, if the default to true, wouldn't we want AllowLocking() to change the default? So shouldn't it be AllowLocking(bool lockResponse = false) instead of AllowLocking(bool lockResponse = true)?

@davidfowl
Copy link
Member

davidfowl commented Jun 16, 2022

  • Why does the feature have a mutable list of policies?
  • Do we need these With methods?
  • The things that return Task<T> should be ValueTask<T>
  • The On* naming for the IOutputCachePolicy is strange, can we remove that prefix?

@sebastienros
Copy link
Member Author

Mvc sample:

using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.OutputCaching;

namespace MvcSandbox.Controllers;

public class HomeController : Controller
{
    [ModelBinder]
    public string Id { get; set; }

    [OutputCache(Duration = 10, VaryByQueryKeys = new[] { "culture" })]
    public IActionResult Index()
    {
        return View();
    }
}

@sebastienros
Copy link
Member Author

What's a PoliciesCollection? Why isn't it part of this API? Can't this just be exposed as a IList instead?

During the meetings it was suggested to use BasePolicies.AddPolicy instead of Add (and the existing List<IOutputCachePolicy>) for consistency, hence the dedicated collection for that purpose. What's the decision then?

What about CacheOutput() methods? That's used a lot in the sample. Where is that defined? I'm adding it below based on what's in the PR and my feedback.

Missed them, thanks.

Why isn't there a string policyName overload?

To make the API lighter. The option is already available with the builder CacheOutput(x => x.Policy("myPolicy"). Do you prefer an additional overload on CacheOutput too?

Shouldn't the namespace be in Microsoft.AspNetCore.Builder instead of Microsoft.AspNetCore.OutputCaching?
If so, can we use a less generic name than PolicyExtensions? Maybe OutputCacheConventionBuilderExtensions.
UseOutputCache and OutputCacheExtensions should also be in Microsoft.AspNetCore.Builder instead of Microsoft.AspNetCore.OutputCaching.

They already are. Wrongly referenced in the previous issue version maybe?

This means OutputCacheExtensions is again too generic. How about OutputCacheApplicationBuilderExtensions?

I followed the naming from UseResponseCaching, UseStaticFiles, UseDefaultFiles. Do you still want to change it?

Would we use a common static class for all OutputCaching extension methods? ``OutputCacheBuilderExtensions`?

AddOutputCaching should be in Microsoft.Extensions.DependencyInjection instead of Microsoft.AspNetCore.OutputCaching

It's already there.

OutputCacheServicesExtensions should be OutputCacheServiceCollectionExtensions.

I followed AddResponseCaching naming. Now I see there is a mix, will change.

Why UseOutputCache and then AddOutputCaching?

Let's change AddOutputCaching to AddOutputCache.

OK

Do we really need to add other polices to the OutputCachePolicyBuilder.AddPolicy(IOutputCachePolicy policy)?
Let's just leave the type based overloads for now.

OK

What if it's recursive? Do we have a test for that?

Can you explain where would the recursivity be?

What about OutputCachePolicyBuilder.Policy(string profileName)
This one sems more usable than AddPolicy(IOutputCachePolicy policy) but still has the recursion problem.
Add a test for recursive OutputCachePolicyBuilder.Policy(string profileName) since we're going to keep it!

Recursivity in this context as in named policies could invoke each others?

All string profileName should be string policyName instead! No "policy" should be anywhere in public API!

I assume you meant "no profile in public api".

Shouldn't it be AddPolicy like all the other similar methods instead of just Policy?

Is new OutputCachePolicyBuilder().NoCache().Build() different than new OutputCachePolicyBuilder().Build()?
If so, how? If not, when does it behave differently.
I'll assume we need it and keep it in the following API I'm ready to approve.

Yes, since we decided in meetings to have new OutputCachePolicyBuilder() be "enabled" by default. But the absence of a BasePolicy or one on the endpoint be the default "disabled" state. Was not the case before the meetings.

Can we seal OutputCacheOptions, CachedVaryByRules and OutputCacheAttribute?
Can we rename CachedVaryByRules to CacheVaryByRules?

Is the past tense significant?

Copied from Response Caching.

Can we make OutputCacheOptions.ApplicationServices get only (to external assemblies?
Can we make CachedVaryByRules and OutputCacheAttribute setters init-only?

OK

Why is CacheVaryByRules.SetVaryByCustom(string key, string value) a think rather than just making it public IDictionary<string, string> CachedVaryByRules.VaryByCustom { get; }?

It was a dictionary but during the meetings the feedback was to have SetVaryByCustom instead.

(string, string) doesn't communicate what the elements are.
If it's just a key and a value, can we use KeyValuePair<string, string> instead? It's more widely used.

OK

OutputCachePolicyBuilder implements IOutputCachePolicy by rebuilding itself on every call to one if it's methods!?
Let's not do that.
And if we must, let's still not implement IOutputCachePolicy publically. We can create a wrapper if we need to.

It was suggested that OutputCachePolicyBuilder should implement it so we can pass a builder as the policy. And it's building it only once, lazily.

OutputCacheAttribute does the same thing!?
Yeah. No.

It was implementing IOutputPolicyMetadata but was removed based on meetings feedback. What is the suggestion to have it as a metadata then? Check for the attribute specifically in addition to IOutputCachePolicy? Or just have IOutputPolicyMetadata back and use it on endpoints and the attribute (like it was before).

Everything that returns a Task, including Funcs should take a CancellationToken.

OK

I don't think we should ship the Output Cache Store APIs until we have at least one non-in-memory implementation that shows an advantage over IDistributedCache

  • I think users need to be able to store their output cache in a store that is different than the default IDistributedCache service. Would it be ok to have an interface that is exactly like IDistributedCache without the OutputCacheEntry, the same we did with IDistributedCacheTagHelperStorage. We can then have two default implementations (memory and distributed cache).
  • How do we handle tags eviction? Currently EvictByTagAsync. Should we just implement it using IDistributedCache (suggested above) "dumbly", but it will hurt perf a lot. I'd prefer to have a service for that which can be customized. And then it would be better on the same "serialization" interface that is used by output caching.

Is there any public API added by #41037 that I didn't catch? I caught that PoliciesCollection and the CacheOutput() were missed. Is there anything else? It's very possible I missed stuff. We shouldn't add any public API that's not listed exactly as it is in the final API once it's approved.

Did another pass and found

 public sealed class OutputCacheOptions
 {
+    public bool UseCaseSensitivePaths{ get; set; }
 }

@sebastienros
Copy link
Member Author

New proposal:

  • Kept IOutputCacheStore but with byte[] to be as close as possible to IDistributedCache, and with EvictByTagAsync. Removing other POCOs from cache store.
  • Removed the On prefixes and used CacheRequestAsync to keep the 'verb' based method.
  • Removed list of policies from the feature, since now we don't have useful usages in middleware.

Diff from proposal:

public interface IOutputCachePolicy
{
-   Task OnRequestAsync(OutputCacheContext context, CancellationToken token);
-   Task OnServeFromCacheAsync(OutputCacheContext context, CancellationToken token);
-   Task OnServeResponseAsync(OutputCacheContext context, CancellationToken token);
+   Task CacheRequestAsync(OutputCacheContext context, CancellationToken token);
+   Task ServeFromCacheAsync(OutputCacheContext context, CancellationToken token);
+   Task ServeResponseAsync(OutputCacheContext context, CancellationToken token);
}

public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
-   IList<IOutputCachePolicy> Policies { get; }
}

+public interface IOutputCacheStore
+{
+    ValueTask EvictByTagAsync(string tag, CancellationToken token);
+    ValueTask<byte[]> GetAsync(string key, CancellationToken token);
+    ValueTask SetAsync(string key, byte[] value, TimeSpan validFor, CancellationToken token);
+}

Full proposal:

namespace Microsoft.AspNetCore.Builder;

public static class OutputCacheServiceCollectionExtensions
{
    public static IServiceCollection AddOutputCache(this IServiceCollection services);
    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}
 
public static class OutputCacheApplicationBuilderExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}
 
namespace Microsoft.Extensions.DependencyInjection;

public static class OutputCacheConventionBuilderExtensions
{
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheOptions
{
    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public bool UseCaseSensitivePaths{ get; set; }
    public IList<IOutputCachePolicy> BasePolicies { get; }
    public IServiceProvider ApplicationServices { get; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
    Task CacheRequestAsync(OutputCacheContext context, CancellationToken token);
    Task ServeFromCacheAsync(OutputCacheContext context, CancellationToken token);
    Task ServeResponseAsync(OutputCacheContext context, CancellationToken token);
 }
 
public sealed class OutputCachePolicyBuilder
{
    public OutputCachePolicyBuilder();
    public OutputCachePolicyBuilder AddPolicy(Type policyType);
    public OutputCachePolicyBuilder AddPolicy<T>() where T : IOutputCachePolicy;
    public OutputCachePolicyBuilder AddPolicy(string policyName);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, bool> predicate);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, CancellationToken, Task<bool>> asyncPredicate);
    public OutputCachePolicyBuilder Tag(params string[] tags);
    public OutputCachePolicyBuilder Expire(TimeSpan expiration);
    public OutputCachePolicyBuilder AllowLocking(bool lockResponse = true);
    public OutputCachePolicyBuilder Clear();
    public OutputCachePolicyBuilder NoCache();
    public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headers);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, Task<string>> varyByAsync);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, Task<KeyValuePair<string, string>>> varyByAsnc);
}
 
public sealed class OutputCacheContext
{
    public OutputCacheContext();
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
    public HttpContext HttpContext { get; }
    public DateTimeOffset? ResponseTime { get; }
    public CachedVaryByRules CachedVaryByRules { get; set; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; }
    public StringValues Headers { get; init; }
    public StringValues QueryKeys { get; init; }
    public StringValues VaryByPrefix { get; init; }
}
 
public sealed class OutputCacheAttribute : Attribute
{
    public OutputCacheAttribute();
    public int Duration { get; init; }
    public bool NoStore { get; init; }
    public string[]? VaryByQueryKeys { get; init; }
    public string[]? VaryByHeaders { get; init; }
    public string? PolicyName { get; init; }
}
 
public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
}

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken token);
    ValueTask<byte[]> GetAsync(string key, CancellationToken token);
    ValueTask SetAsync(string key, byte[] value, TimeSpan validFor, CancellationToken token);
}

@halter73
Copy link
Member

Is there any reason for not using ValueTask for IOutputCachePolicy or was that just an oversight?

Same goes for With(Func<OutputCacheContext, CancellationToken, Task<bool>> asyncPredicate). Shouldn't that be ValueTask<bool> instead?

I kinda agree with @davidfowl's comments about it not being obvious what the predicates are doing based on the method name "With". You said, if the predicate is true, the policy stays enabled, right? Would WhenTrue be a better name then? I'm fine with approving this as-is in the interest of getting it out there.

I'm fine with shipping the simplified IOutputCacheStore too. I'd love to see other implementations soon though.

@dotnet/aspnet-api-review I think we should approve what @sebastienros proposed above with all the Tasks changed to ValueTask? This is slotted for preview6, so we're running out of time for big changes. Are there any objections? @davidfowl @DamianEdwards @adityamandaleeka

@davidfowl
Copy link
Member

davidfowl commented Jun 17, 2022

Last piece of feedback:

  • IOutputCacheStore is a bit weird. It's not clear how the entries get tagged..

Lets approve for preview6 and get some feedback and make more breaking changes in preview7

@halter73
Copy link
Member

Final preview6 API review notes:

  • Use ValueTask everywhere.
  • IOutputCacheStore is a bit weird. It's not clear how the entries get tagged.
  • Lets approve for preview6 and get some feedback and make more breaking changes in preview7
namespace Microsoft.AspNetCore.Builder;

public static class OutputCacheServiceCollectionExtensions
{
    public static IServiceCollection AddOutputCache(this IServiceCollection services);
    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}
 
public static class OutputCacheApplicationBuilderExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}
 
namespace Microsoft.Extensions.DependencyInjection;

public static class OutputCacheConventionBuilderExtensions
{
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheOptions
{
    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public bool UseCaseSensitivePaths{ get; set; }
    public IList<IOutputCachePolicy> BasePolicies { get; }
    public IServiceProvider ApplicationServices { get; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
    ValueTask CacheRequestAsync(OutputCacheContext context, CancellationToken token);
    ValueTask ServeFromCacheAsync(OutputCacheContext context, CancellationToken token);
    ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken token);
 }
 
public sealed class OutputCachePolicyBuilder
{
    public OutputCachePolicyBuilder();
    public OutputCachePolicyBuilder AddPolicy(Type policyType);
    public OutputCachePolicyBuilder AddPolicy<T>() where T : IOutputCachePolicy;
    public OutputCachePolicyBuilder AddPolicy(string policyName);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, bool> predicate);
    public OutputCachePolicyBuilder With(Func<OutputCacheContext, CancellationToken, ValueTask<bool>> asyncPredicate);
    public OutputCachePolicyBuilder Tag(params string[] tags);
    public OutputCachePolicyBuilder Expire(TimeSpan expiration);
    public OutputCachePolicyBuilder AllowLocking(bool lockResponse = true);
    public OutputCachePolicyBuilder Clear();
    public OutputCachePolicyBuilder NoCache();
    public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headers);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<string>> varyByAsync);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyByAsnc);
}
 
public sealed class OutputCacheContext
{
    public OutputCacheContext();
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
    public HttpContext HttpContext { get; }
    public DateTimeOffset? ResponseTime { get; }
    public CachedVaryByRules CachedVaryByRules { get; set; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; }
    public StringValues Headers { get; init; }
    public StringValues QueryKeys { get; init; }
    public StringValues VaryByPrefix { get; init; }
}
 
public sealed class OutputCacheAttribute : Attribute
{
    public OutputCacheAttribute();
    public int Duration { get; init; }
    public bool NoStore { get; init; }
    public string[]? VaryByQueryKeys { get; init; }
    public string[]? VaryByHeaders { get; init; }
    public string? PolicyName { get; init; }
}
 
public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
}

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken token);
    ValueTask<byte[]> GetAsync(string key, CancellationToken token);
    ValueTask SetAsync(string key, byte[] value, TimeSpan validFor, CancellationToken token);
}

API Approved!

@mkArtakMSFT mkArtakMSFT modified the milestones: 7.0-preview6, 7.0-rc1 Jul 17, 2022
@mkArtakMSFT
Copy link
Contributor

@adityamandaleeka, I've moved this to RC1 milestone. Please readjust as necessary.

@halter73
Copy link
Member

API Review Notes:

  • We're fine with removing "Async" from predicate names.

  • Should OutputCacheContext.CacheVaryByValues still be settable? How are developers expected to set the properties on CacheVaryByValues?

  • GetAsync and SetAsync changes to change cancellationToken param name and add tags parameters makes senes.

  • Other API additions and removals make sense.

  • Cange all token parameters to cancellationToken.

  • OutputCachePolicyBuilder.With(Func<OutputCacheContext, CancellationToken, Task<bool>> predicate); should use ValueTask.

  • Can you no longer construct a OutputCacheContext for tests and stuff now that there's no public default constructor?

Approved as follows. Feel free to tell us what we messed up when you get back @sebastienros! 😆

namespace Microsoft.AspNetCore.Builder;

public static class OutputCacheApplicationBuilderExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}
 
namespace Microsoft.Extensions.DependencyInjection;

public static class OutputCacheServiceCollectionExtensions
{
    public static IServiceCollection AddOutputCache(this IServiceCollection services);
    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}

public static class OutputCacheConventionBuilderExtensions
{
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheOptions
{
    public OutputCacheOptions();

    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public bool UseCaseSensitivePaths{ get; set; }
    public IServiceProvider ApplicationServices { get; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);

    public void AddBasePolicy(IOutputCachePolicy policy);
    public void AddBasePolicy(Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
    ValueTask CacheRequestAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeFromCacheAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken cancellationToken);
 }
 
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 AllowLocking(bool lockResponse = true);
    public OutputCachePolicyBuilder Clear();
    public OutputCachePolicyBuilder NoCache();
    public OutputCachePolicyBuilder VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headers);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyBy);
}
 
public sealed class OutputCacheContext
{
    public OutputCacheContext();
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
    public HttpContext HttpContext { get; }
    public DateTimeOffset? ResponseTime { get; }
    public CacheVaryByRules CacheVaryByRules { get; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; set }
    public StringValues Headers { get; set; }
    public StringValues QueryKeys { get; set; }
    public StringValues VaryByPrefix { get; set; }
}
 
public sealed class OutputCacheAttribute : Attribute
{
    public OutputCacheAttribute();
    public int Duration { get; init; }
    public bool NoStore { get; init; }
    public string[]? VaryByQueryKeys { get; init; }
    public string[]? VaryByHeaders { get; init; }
    public string? PolicyName { get; init; }
}
 
public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
}

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken);
    ValueTask<byte[]?> GetAsync(string key, CancellationToken cancellationToken);
    ValueTask SetAsync(string key, byte[] value, string[]? tags, TimeSpan validFor, CancellationToken cancellationToken);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Jul 18, 2022
@davidfowl
Copy link
Member

We missed VaryByRouteValue

@adityamandaleeka
Copy link
Member

@sebastienros Can we make sure the currently merged API and the one here are in sync (and reconcile any deltas)?

@sebastienros
Copy link
Member Author

OK on all the recommendations, with these comments:

Can you no longer construct a OutputCacheContext for tests and stuff now that there's no public default constructor?

I don't think we can/should have a default constructor. There is an internal one, visible from Tests, which requires a bunch of mandatory arguments (HttpContext, IOutputCacheStore, OutputCacheOptions, ILogger). So I am suggesting to not add it

I want to add a new method to the builder to be able to instantiate a default policy (the one that accepts 200s, anonymous, GET/HEAD). Right now any time a policy is built using the builder, it will use the DefaultPolicy as its base and add more constraints. Calling .OutputCache on an endpoint also create a new DefaultPolicy when the builder is used. The issue is the only way to use the default policy as-is with the builder pattern is to pass an empty body in the action. Example if you want to enable caching on the whole site without customization on the policy itself:

services.AddOutputCache(options =>
{
    // Create a new policy that will contain the DefaultPolicy and enable all endpoints (GET, un-authenticated)
    options.AddBasePolicy(build => { });
});

I suggest we add a method on the builder to be explicit about getting a policty that is the default:

options.AddBasePolicy(build => build.Enable());

This would be no-op, but it would reflect what it return, a behavior that enables the cache. I could also suggest Cache() as opposed to the existing NoCache().

API after suggestions

namespace Microsoft.AspNetCore.Builder;

public static class OutputCacheApplicationBuilderExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}
 
namespace Microsoft.Extensions.DependencyInjection;

public static class OutputCacheServiceCollectionExtensions
{
    public static IServiceCollection AddOutputCache(this IServiceCollection services);
    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}

public static class OutputCacheConventionBuilderExtensions
{
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheOptions
{
    public OutputCacheOptions();

    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public bool UseCaseSensitivePaths{ get; set; }
    public IServiceProvider ApplicationServices { get; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);

    public void AddBasePolicy(IOutputCachePolicy policy);
    public void AddBasePolicy(Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
    ValueTask CacheRequestAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeFromCacheAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken cancellationToken);
 }
 
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 VaryByQuery(params string[] queryKeys);
    public OutputCachePolicyBuilder VaryByHeader(params string[] headers);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, string> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyBy);
}
 
public sealed class OutputCacheContext
{
-   public OutputCacheContext();
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
    public HttpContext HttpContext { get; }
    public DateTimeOffset? ResponseTime { get; }
    public CacheVaryByRules CacheVaryByRules { get; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; set }
    public StringValues Headers { get; set; }
    public StringValues QueryKeys { get; set; }
    public StringValues VaryByPrefix { get; set; }
}
 
public sealed class OutputCacheAttribute : Attribute
{
    public OutputCacheAttribute();
    public int Duration { get; init; }
    public bool NoStore { get; init; }
    public string[]? VaryByQueryKeys { get; init; }
    public string[]? VaryByHeaders { get; init; }
    public string? PolicyName { get; init; }
}
 
public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
}

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken);
    ValueTask<byte[]?> GetAsync(string key, CancellationToken cancellationToken);
    ValueTask SetAsync(string key, byte[] value, string[]? tags, TimeSpan validFor, CancellationToken cancellationToken);
}

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels Aug 5, 2022
@ghost
Copy link

ghost commented Aug 5, 2022

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
Copy link
Member

halter73 commented Aug 8, 2022

API Review Notes:

  1. Cache() being overridden without an explicit call to NoCache() is weird. Let's make sure that doesn't happen.
  2. Make sure unit tests can construct contexts.
  3. Add VaryByRouteValue
    1. Are we worried that CacheVaryByRules.RouteValues would imply the StringValues are the actual values and not the keys like they are supposed to be? We use CacheVaryByRules.QueryKeys not CacheVaryByRules.Query or CacheVaryByRules.QueryValues.
      1. Maybe, but let's not make this overly verbose. The "query" or querystring is a thing but we cannot say RouteKeys because the concept is called "RouteValues". It's on HttpRequest.RouteValues. The querystring is not on HttpRequest.QueryValues and headers are not HttpRequest.HeaderValues. It's just HttpRequest.Query and HttpRequest.Headers, so these are different.
      2. This feedback applies to all the APIs that refer just to "RouteValues" to mean "RouteValueNames".
      3. Let's be very clear in the doc comments about this.
    2. Then should we rename VaryByQueryKeys -> VaryByQuery?
      1. No, because then it's not plural, and VaryByQueries would be needlessly confusing.

API Approved!

namespace Microsoft.AspNetCore.Builder;

public static class OutputCacheApplicationBuilderExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}
 
namespace Microsoft.Extensions.DependencyInjection;

public static class OutputCacheServiceCollectionExtensions
{
    public static IServiceCollection AddOutputCache(this IServiceCollection services);
    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}

public static class OutputCacheConventionBuilderExtensions
{
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheOptions
{
    public OutputCacheOptions();

    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public bool UseCaseSensitivePaths{ get; set; }
    public IServiceProvider ApplicationServices { get; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);

    public void AddBasePolicy(IOutputCachePolicy policy);
    public void AddBasePolicy(Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
    ValueTask CacheRequestAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeFromCacheAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken cancellationToken);
 }
 
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[] headers);
+    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(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyBy);
}
 
public sealed class OutputCacheContext
{
    public OutputCacheContext();
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
-    public required HttpContext HttpContext { get; }
+    public required HttpContext HttpContext { get; init; }
-    public DateTimeOffset? ResponseTime { get; }
+    public DateTimeOffset? ResponseTime { get; set; }
    public CacheVaryByRules CacheVaryByRules { get; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; set }
+    public StringValues RouteValues { get; set; }
    public StringValues Headers { get; set; }
    public StringValues QueryKeys { get; set; }
    public StringValues VaryByPrefix { get; set; }
}
 
public sealed class OutputCacheAttribute : Attribute
{
    public OutputCacheAttribute();
    public int Duration { get; init; }
    public bool NoStore { get; init; }
+    public string[]? VaryByRouteValues { get; init; }
    public string[]? VaryByQueryKeys { get; init; }
    public string[]? VaryByHeaders { get; init; }
    public string? PolicyName { get; init; }
}
 
public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
}

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken);
    ValueTask<byte[]?> GetAsync(string key, CancellationToken cancellationToken);
    ValueTask SetAsync(string key, byte[] value, string[]? tags, TimeSpan validFor, CancellationToken cancellationToken);
}

Edit: Let's not do the OutputCacheAttribute.VaryByQueryKeys to VaryByQuery rename because then it's not plural, and VaryByQueries would be needlessly confusing.
Edit 2: Rename headers param to headerNames.

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 8, 2022
@halter73
Copy link
Member

halter73 commented Aug 8, 2022

I made two minor adjustments to this API after the meeting.

Edit: Let's not do the OutputCacheAttribute.VaryByQueryKeys to VaryByQuery rename because then it's not plural, and VaryByQueries would be needlessly confusing.
Edit 2: Rename headers param to headerNames.

@sebastienros
Copy link
Member Author

Edit 2: Rename headers param to headerNames.

There are other things called headers like CachedVaryByRules.Headers, OutputCacheAttribute.Headers. Should these be renamed too?

@sebastienros
Copy link
Member Author

New update from PR feedback, using Names and Keys suffixes where meaningful in properties and arguments.

namespace Microsoft.AspNetCore.Builder;

public static class OutputCacheApplicationBuilderExtensions
{
    public static IApplicationBuilder UseOutputCache(this IApplicationBuilder app);
}
 
namespace Microsoft.Extensions.DependencyInjection;

public static class OutputCacheServiceCollectionExtensions
{
    public static IServiceCollection AddOutputCache(this IServiceCollection services);
    public static IServiceCollection AddOutputCache(this IServiceCollection services, Action<OutputCacheOptions> configureOptions);
}

public static class OutputCacheConventionBuilderExtensions
{
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, IOutputCachePolicy policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, Action<OutputCachePolicyBuilder> policy) where TBuilder : IEndpointConventionBuilder;
    public static TBuilder CacheOutput<TBuilder>(this TBuilder builder, string policyName) where TBuilder : IEndpointConventionBuilder;
}

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheOptions
{
    public OutputCacheOptions();

    public long SizeLimit { get; set; }
    public long MaximumBodySize { get; set; }
    public TimeSpan DefaultExpirationTimeSpan { get; set; }
    public bool UseCaseSensitivePaths{ get; set; }
    public IServiceProvider ApplicationServices { get; }
    public void AddPolicy(string name, IOutputCachePolicy policy);
    public void AddPolicy(string name, Action<OutputCachePolicyBuilder> build);

    public void AddBasePolicy(IOutputCachePolicy policy);
    public void AddBasePolicy(Action<OutputCachePolicyBuilder> build);
 }
 
 public interface IOutputCachePolicy
 {
    ValueTask CacheRequestAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeFromCacheAsync(OutputCacheContext context, CancellationToken cancellationToken);
    ValueTask ServeResponseAsync(OutputCacheContext context, CancellationToken cancellationToken);
 }
 
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(Func<HttpContext, KeyValuePair<string, string>> varyBy);
    public OutputCachePolicyBuilder VaryByValue(Func<HttpContext, CancellationToken, ValueTask<KeyValuePair<string, string>>> varyBy);
}
 
public sealed class OutputCacheContext
{
    public OutputCacheContext();
    public bool EnableOutputCaching { get; set; }
    public bool AllowCacheLookup { get; set; }
    public bool AllowCacheStorage { get; set; }
    public bool AllowLocking { get; set; }
    public required HttpContext HttpContext { get; init; }
    public DateTimeOffset? ResponseTime { get; set; }
    public CacheVaryByRules CacheVaryByRules { get; }
    public HashSet<string> Tags { get; }
    public TimeSpan? ResponseExpirationTimeSpan { get; set; }
}
 
public sealed class CacheVaryByRules
{
    public CacheVaryByRules();
    public IDictionary<string, string> VaryByCustom { get; set }
-   public StringValues RouteValues { get; set; }
+   public StringValues RouteValueNames { get; set; }
-   public StringValues Headers { get; set; }
+   public StringValues HeaderNames { get; set; }
    public StringValues QueryKeys { get; set; }
    public StringValues VaryByPrefix { get; set; }
}
 
public sealed class OutputCacheAttribute : Attribute
{
    public OutputCacheAttribute();
    public int Duration { get; init; }
    public bool NoStore { get; init; }
-   public string[]? VaryByRouteValues { get; init; }
+   public string[]? VaryByRouteValueNames { get; init; }
    public string[]? VaryByQueryKeys { get; init; }
-   public string[]? VaryByHeaders { get; init; }
+   public string[]? VaryByHeaderNames { get; init; }
    public string? PolicyName { get; init; }
}
 
public interface IOutputCacheFeature
{
    OutputCacheContext Context { get; }
}

public interface IOutputCacheStore
{
    ValueTask EvictByTagAsync(string tag, CancellationToken cancellationToken);
    ValueTask<byte[]?> GetAsync(string key, CancellationToken cancellationToken);
    ValueTask SetAsync(string key, byte[] value, string[]? tags, TimeSpan validFor, CancellationToken cancellationToken);
}

@halter73 halter73 added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-approved API was approved in API review, it can be implemented labels Aug 13, 2022
@ghost
Copy link

ghost commented Aug 13, 2022

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
Copy link
Member

The above renames all look good to me. I like the improved clarity of adding the "Names" suffix to these properties. Does anyone else agree? @dotnet/aspnet-api-review @JamesNK @adityamandaleeka? You can use 👍 on my comment to show approval.

@halter73
Copy link
Member

API review notes

API Approved!

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 15, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 19, 2022
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares feature-output-caching
Projects
None yet
Development

No branches or pull requests

10 participants