aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

Add tag trough OutputCacheAttribute

Open kellberg opened this issue 2 years ago • 6 comments
trafficstars

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?

  • [X] I have searched the existing issues

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")]

kellberg avatar Jan 02 '23 22:01 kellberg

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

mitchdenny avatar Jan 03 '23 01:01 mitchdenny

@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.

mitchdenny avatar Jan 03 '23 02:01 mitchdenny

@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.

captainsafia avatar Jan 03 '23 04:01 captainsafia

@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?

halter73 avatar Jan 04 '23 00:01 halter73

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.

ghost avatar Jan 04 '23 04:01 ghost

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

mitchdenny avatar Jan 04 '23 04:01 mitchdenny

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!

halter73 avatar Jan 05 '23 23:01 halter73

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

mitchdenny avatar Jan 11 '23 23:01 mitchdenny