dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.43k stars 10.01k forks source link

Add tag trough OutputCacheAttribute #45842

Closed kellberg closed 1 year ago

kellberg commented 1 year ago

NOTE: Commandeering @kellberg's original issue to turn it into an API proposal since folks are already engaged here.

Background and Motivation

Output Caching can be controlled via various APIs including via policy setup in DI, extension methods on the endpoint builder, and attributes on MVC actions and endpoint handler delegates. Cache entries can also be "tagged" which makes it possible to evict sets of cache entries. Unfortunately, the tagging mechanism is not exposed when configuring output caching via attributes.

The proposal is that we fix this short coming with the [OutputCache] attritribute by allowing the following usage:

app.MapGet("/api/points-of-interest", [OutputCache(Tags = new [] { "geospatial" })]() => {});

Proposed API

namespace Microsoft.AspNetCore.OutputCaching;

public sealed class OutputCacheAttribute : Attribute
{
+    public string[]? Tags { get; init; }
}

Usage Examples

Minimal APIs

app.MapGet("/api/points-of-interest", [OutputCache(Tags = new [] { "geospatial" })]() => { ... });

MVC

[OutputCache(Tags = new [] { "geospatial" })]
public PointOfInterest[] GetPointsOfInterest() { }

Alternative Designs

We already have multiple ways of setting output cache values, this just adds a missing one to the [OutputCache(...)] attribute. However I did consider whether we should use the singular Tag as the property, however other properties on the attribute are already pluralized.

Risks

None that I can think of.

Original issue content from OP.

Is there an existing issue for this?

Is your feature request related to a problem? Please describe the problem.

I would like to add an output cache tag for cache eviction but the OutputCacheAttribute doesn't have that option.

Describe the solution you'd like

Add a Tag parameter to OutputCacheAttribute and update BuildPolicy to add the tag to the IOutputCachePolicy.

Then you can add the tag through the attribute: [OutputCache(Tags = "test")]

mitchdenny commented 1 year ago

This sounds like a good idea for the sake of consistency.

mitchdenny commented 1 year ago

@captainsafia do you know if small changes like this need to use the formal API review template?

Note to @kellberg in my implementation I used the plural Tags and it tags an array like many of the other properties on the attribute.

captainsafia commented 1 year ago

@captainsafia do you know if small changes like this need to use the formal API review template?

@halter73 can provide the conclusive decision but I believe that adding a parameter to an existing API does require API review.

halter73 commented 1 year ago

@halter73 can provide the conclusive decision but I believe that adding a parameter to an existing API does require API review.

It sure does. We do formal API reviews for anything that touches PublicAPI.Unshipped.txt unless it's just fixing a nullability annotation to more accurately match existing behavior like in #45751. There are even things that don't touch PublicAPI.Unshipped.txt like analyzers and explicitly implemented interfaces that we cover in API review.

@mitchdenny Can you copy the template from https://github.com/dotnet/aspnetcore/issues/new?assignees=&labels=api-suggestion&template=30_api_proposal.md&title= into a new comment on this issue, fill it out, and add the api-ready-for-review label?

ghost commented 1 year ago

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:

mitchdenny commented 1 year ago

@halter73 missed your comment, I updated the initial issue content in this instance ;)

halter73 commented 1 year ago

API Review Notes:

  1. This follows a very similar pattern to other string[]? properties on the OutputCacheAttribute.
  2. @sebastienros omitted this originally because tag data is often dynamic, but even in our samples, we sometimes use static sets of tags for a given endpoint.

API approved as proposed!

mitchdenny commented 1 year ago

Closing this issue now since the code has been added to main, it will appear in .NET 8.0.