Skip to content
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

OutputCache configuration - policy validation #52419

Open
witskeeper opened this issue Nov 28, 2023 · 7 comments
Open

OutputCache configuration - policy validation #52419

witskeeper opened this issue Nov 28, 2023 · 7 comments
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-caching Includes: StackExchangeRedis and SqlServer distributed caches

Comments

@witskeeper
Copy link
Contributor

witskeeper commented Nov 28, 2023

Background and Motivation

The ASP.NET Core output caching middleware is great, but "limited" in terms of policy validation. Let's start with some code that you can write today in .NET 7:

builder.Services.AddOutputCache(options =>
{
    options.AddPolicy("customPolicy", builder =>  builder.Expire(TimeSpan.FromSeconds(20)));
});

There is no way to validate that customPolicy actually exists. This is useful when configuring multiple routes from configuration such as is the case for YARP. See microsoft/reverse-proxy#2328

Proposed API

It would be preferred to something similar to IAuthorizationPolicyProvider implemented via DefaultAuthorizationPolicyProvider and ICorsPolicyProvider implemented via DefaultCorsPolicyProvider

namespace Microsoft.AspNetCore.OutputCaching;

+ public interface IOutputCachePolicyProvider
+ {
+     ValueTask<IOutputCachePolicy?> GetPolicyAsync(string policyName);
+ }
+
+ public class DefaultOutputCachePolicyProvider : IOutputCachePolicyProvider
+ {
+     private readonly OutputCacheOptions _options;
+     
+     public DefaultOutputCachePolicyProvider(IOptions<OutputCacheOptions> options)
+     {
+     
+     }
+     
+     public ValueTask<IOutputCachePolicy?> GetPolicyAsync(string policyName)
+     {
+         options.NamedPolicies[policyName];
+     }
+ }

OutputCacheOptions.NamedPolicies is internal hence this feature cannot be added in another library or the final application.

Usage Examples

See YARP: https://github.com/microsoft/reverse-proxy/blob/02435e0e2eb3e757fc928d9157cfcc7f2859910b/src/ReverseProxy/Configuration/RouteValidators/OutputCachePolicyValidator.cs#L27-L33

Alternative Designs

None

Risks

None

@witskeeper witskeeper added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 28, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares label Nov 28, 2023
@martincostello martincostello added the feature-caching Includes: StackExchangeRedis and SqlServer distributed caches label Nov 28, 2023
@witskeeper
Copy link
Contributor Author

like #45684

@Tratcher Tratcher added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 28, 2023
@ghost
Copy link

ghost commented Nov 28, 2023

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 Dec 7, 2023

@sebastienros Have you seen this yet? Do you think it's a good idea?

@sebastienros
Copy link
Member

sebastienros commented Dec 7, 2023

My naive response would be to add a GetPolicy(string name) to the options, but the suggested design would offer more flexibility and allow new patterns to provide custom policies.

Note that the API suggestion seems to be a copy-paste of the RateLimiting one since it's referring to DefaultKeyType which is not part of OutputCache. The returned value should be IOutputCachePolicy.

Update: API suggestion has been fixed

@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 Dec 7, 2023
@halter73
Copy link
Member

halter73 commented Dec 7, 2023

API Review Notes:

  • We removed references to DefaultKey in the proposal that appears to be copy-pasted from the rate limiter proposal in Rate Limiting configuration - policy validation #45684.
  • We don't think the implementation needs to be public.
  • Given that this doesn't need to expose a new key type publicly like the rate limiter proposal, we think we can accept this.
  • The current middleware should use this interface without accessing the options directly to make replacing the implementation useful.

API Approved!

namespace Microsoft.AspNetCore.OutputCaching;

+ public interface IOutputCachePolicyProvider
+ {
+     ValueTask<IOutputCachePolicy?> GetPolicyAsync(string policyName);
+ }

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@onurkanbakirci
Copy link
Contributor

I'm working on it

@onurkanbakirci
Copy link
Contributor

onurkanbakirci commented Aug 16, 2024

Done #57362

@witskeeper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-caching Includes: StackExchangeRedis and SqlServer distributed caches
Projects
None yet
Development

No branches or pull requests

8 participants
@halter73 @sebastienros @martincostello @Tratcher @witskeeper @wtgodbe @onurkanbakirci and others