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

Add generic versions of attributes in ASP.NET Core #37767

Closed
DamianEdwards opened this issue Oct 21, 2021 · 12 comments · Fixed by #47075
Closed

Add generic versions of attributes in ASP.NET Core #37767

DamianEdwards opened this issue Oct 21, 2021 · 12 comments · Fixed by #47075
Labels
api-approved API was approved in API review, it can be implemented area-web-frameworks Priority:2 Work that is important, but not critical for the release triage-focus Add this label to flag the issue for focus at triage

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Oct 21, 2021

Background and Motivation

Now that C# supports generic attributes as of version 10/.NET 6, we should look through ASP.NET Core and consider adding generic versions of any attributes that accept type parameters (e.g. ConsumesAttribute).

Proposed API

namespace Microsoft.AspNetCore.Mvc;

//Original: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ModelMetadataTypeAttribute.cs
+[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
+public class ModelMetadataTypeAttribute<TType> : ModelMetadataTypeAttribute
+{
+    public ModelMetadataTypeAttribute() : base(typeof(TType))
+    { }
+}

// Original: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/Filters/MiddlewareFilterAttribute.cs
+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+public class MiddlewareFilterAttribute<TConfigurationType> : MiddlewareFilterAttribute
+{
+    public MiddlewareFilterAttribute() : base(typeof(TConfigurationType))
+    { }
+}

// Original: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ServiceFilterAttribute.cs
+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+[System.Diagnostics.DebuggerDisplay("ServiceFilter: Type={ServiceType} Order={Order}")]
+public class ServiceFilterAttribute<TFilterType> : ServiceFilterAttribute
+ where TFilterType : IFilterMetadata
+{
+    public ServiceFilterAttribute() : base(typeof(TFilterType))
+    { }
+}

// Original: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ModelBinderAttribute.cs
+[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Struct, AllowMultiple = false, Inherited = true)]
+public class ModelBinderAttribute<TBinderType> : ModelBinderAttribute
+ where TBinderType : IModelBinder
+{
+    public ModelBinderAttribute() : base(typeof(TBinderType))
+    { }
+}

// Original: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ProducesAttribute.cs
+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+public class ProducesAttribute<TType> : ProducesAttribute
+{
+    public ProducesAttribute() : base(typeof(TType))
+    { }
+}

// Original: https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/src/ProducesResponseTypeAttribute.cs
+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+public class ProducesResponseTypeAttribute<TType> : ProducesResponseTypeAttribute
+{
+    public ProducesResponseTypeAttribute(int statusCode) : base(typeof(TType), statusCode)
+    { }
+    public ProducesResponseTypeAttribute(int statusCode, string contentType, params string[] additionalContentTypes)
+             : base(typeof(TType), statusCode, contentType, additionalContentTypes)
+    { }
+}

In addition to what listed above the original list (#37767 (comment)) contains:

src/Mvc/Mvc.Razor/src/Compilation/RazorViewAttribute.cs:
  13:     public class RazorViewAttribute : Attribute

src/Razor/Microsoft.AspNetCore.Razor.Language/src/ProvideRazorExtensionInitializerAttribute.cs:
  9:     public class ProvideRazorExtensionInitializerAttribute : Attribute

RazorViewAttribute is marked as obsolete, so, I don't think we need to do anything (cc @mkArtakMSFT in case you want to have it added).
Also, ProvideRazorExtensionInitializerAttribute is not part of the ASPNET.Core repo anymore.

Usage Examples

[ProducesResponseType<ValidationProblemDetails>(StatusCodes.Status400BadRequest)]
[ProducesResponseType<Todo>(StatusCodes.Status201Created)]
[EndpointName(nameof(AddTodo))]
[Tags("TodoApi")]
async Task<IResult> AddTodo(Todo todo, TodoDb db)
{
    if (!MiniValidator.TryValidate(todo, out var errors))
        return Results.ValidationProblem(errors);

    db.Todos.Add(todo);
    await db.SaveChangesAsync();

    return Results.Created($"/todos/{todo.Id}", todo);
}

Alternative Designs

Risks

Very small, this will introduce new types derived from the non-generic ones.

@DamianEdwards DamianEdwards added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Oct 22, 2021
@javiercn
Copy link
Member

WHAT!! how could I have missed this!!

This is great!

@DamianEdwards
Copy link
Member Author

There may still be some issues with the implementation it turns out: dotnet/csharplang#124 (comment)

@DamianEdwards
Copy link
Member Author

Confirmed, this is still behind the preview language version in .NET 6:
image

@ShreyasJejurkar
Copy link
Contributor

ShreyasJejurkar commented Oct 23, 2021

Did a regex search with public class [a-zA-Z]+ : Attribute throughout the repo for attribute taking Type as the parameter.

9 results - 9 files

src/Mvc/Mvc.Core/src/ModelBinderAttribute.cs:
  28:     public class ModelBinderAttribute : Attribute, IModelNameProvider, IBinderTypeProviderMetadata

src/Mvc/Mvc.Core/src/ModelMetadataTypeAttribute.cs:
  12:     public class ModelMetadataTypeAttribute : Attribute

src/Mvc/Mvc.Core/src/ProducesAttribute.cs:
  21:     public class ProducesAttribute : Attribute, IResultFilter, IOrderedFilter, IApiResponseMetadataProvider

src/Mvc/Mvc.Core/src/ProducesResponseTypeAttribute.cs:
  16:     public class ProducesResponseTypeAttribute : Attribute, IApiResponseMetadataProvider

src/Mvc/Mvc.Core/src/ServiceFilterAttribute.cs:
  25:     public class ServiceFilterAttribute : Attribute, IFilterFactory, IOrderedFilter

src/Mvc/Mvc.Core/src/TypeFilterAttribute.cs:
  27:     public class TypeFilterAttribute : Attribute, IFilterFactory, IOrderedFilter

src/Mvc/Mvc.Core/src/Filters/MiddlewareFilterAttribute.cs:
  15:     public class MiddlewareFilterAttribute : Attribute, IFilterFactory, IOrderedFilter

src/Mvc/Mvc.Razor/src/Compilation/RazorViewAttribute.cs:
  13:     public class RazorViewAttribute : Attribute

src/Razor/Microsoft.AspNetCore.Razor.Language/src/ProvideRazorExtensionInitializerAttribute.cs:
  9:     public class ProvideRazorExtensionInitializerAttribute : Attribute

I have excluded the attributes that are part of the tests project.

@DamianEdwards DamianEdwards added this to the .NET 7 Planning milestone Oct 23, 2021
@mkArtakMSFT mkArtakMSFT added Priority:1 Work that is critical for the release, but we could probably ship without triaged labels Nov 11, 2021
@mkArtakMSFT mkArtakMSFT added area-web-frameworks and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Nov 30, 2021
@mkArtakMSFT
Copy link
Member

@rafikiassumani-msft moving this over to your area path. FYI

@rafikiassumani-msft rafikiassumani-msft added triage-focus Add this label to flag the issue for focus at triage Cost:M and removed triaged labels Jan 11, 2022
@yecril71pl
Copy link
Contributor

This could prevent wrong code from being compiled. Currently you get an exception if the parameter binderType does not implement the interface IModelBinder 👎
Of course, you do not exactly need generic attributes for this, the work-around would be to use a generic type carrier as a parameter rather than a bare Type.

@rafikiassumani-msft rafikiassumani-msft added Priority:2 Work that is important, but not critical for the release and removed Priority:1 Work that is critical for the release, but we could probably ship without labels Jun 14, 2022
@captainsafia
Copy link
Member

Triage: @brunolins16 will take the content from this comment and craft an API proposal template for it.

@captainsafia captainsafia added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 7, 2023
@brunolins16 brunolins16 removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Feb 8, 2023
@brunolins16 brunolins16 added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Feb 8, 2023
@ghost
Copy link

ghost commented Feb 8, 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.

@brunolins16 brunolins16 removed their assignment Feb 24, 2023
@BrennanConroy
Copy link
Member

API Review Notes:

  • We prefer T over TType for generic arguments, unless they are constrained, then we add a suffix to T.

API Approved!

namespace Microsoft.AspNetCore.Mvc;

+[AttributeUsage(AttributeTargets.Class, AllowMultiple = false, Inherited = true)]
+public class ModelMetadataTypeAttribute<T> : ModelMetadataTypeAttribute
+{
+    public ModelMetadataTypeAttribute() : base(typeof(T))
+    { }
+}

+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+public class MiddlewareFilterAttribute<T> : MiddlewareFilterAttribute
+{
+    public MiddlewareFilterAttribute() : base(typeof(T))
+    { }
+}

+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+[System.Diagnostics.DebuggerDisplay("ServiceFilter: Type={ServiceType} Order={Order}")]
+public class ServiceFilterAttribute<TFilter> : ServiceFilterAttribute
+ where TFilter : IFilterMetadata
+{
+    public ServiceFilterAttribute() : base(typeof(TFilter))
+    { }
+}

+[AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.Class | AttributeTargets.Enum | AttributeTargets.Struct, AllowMultiple = false, Inherited = true)]
+public class ModelBinderAttribute<TBinder> : ModelBinderAttribute
+ where TBinder : IModelBinder
+{
+    public ModelBinderAttribute() : base(typeof(TBinder))
+    { }
+}

+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = false, Inherited = true)]
+public class ProducesAttribute<T> : ProducesAttribute
+{
+    public ProducesAttribute() : base(typeof(T))
+    { }
+}

+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+public class ProducesResponseTypeAttribute<T> : ProducesResponseTypeAttribute
+{
+    public ProducesResponseTypeAttribute(int statusCode) : base(typeof(T), statusCode)
+    { }

+    public ProducesResponseTypeAttribute(int statusCode, string contentType, params string[] additionalContentTypes)
+             : base(typeof(T), statusCode, contentType, additionalContentTypes)
+    { }
+}

@BrennanConroy BrennanConroy 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 Mar 6, 2023
@captainsafia
Copy link
Member

captainsafia commented Mar 7, 2023

While working on implementing this, I found one more type that needed generic attribute overloads.

+[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method, AllowMultiple = true, Inherited = true)]
+public class TypeFilterAttribute<TFilter> : TypeFilterAttribute where TFilter : IFilterMetadata
+{
+    public TypeFilterAttribute() : base(typeof(T)) { }
+}

@BrennanConroy
Copy link
Member

Looks like it needs to be constrained to IFilterMetadata as well.

@captainsafia
Copy link
Member

Looks like it needs to be constrained to IFilterMetadata as well.

Indeed! I updated the comment to reflect this.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 15, 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-web-frameworks Priority:2 Work that is important, but not critical for the release triage-focus Add this label to flag the issue for focus at triage
Projects
No open projects
Status: Committed
Development

Successfully merging a pull request may close this issue.

9 participants