aspnetcore icon indicating copy to clipboard operation
aspnetcore copied to clipboard

GetEndpointAttribute<T> in EndpointHttpContextExtensions.cs

Open danleonard-nj opened this issue 4 years ago • 8 comments

Is your feature request related to a problem?

Not a problem, just a nice-to-have. It'd be convenient to be able to fetch route attributes directly from HttpContext. I've found it comes up pretty often when working with custom middleware, it's not a long path to get to the metadata from the context itself as it is now, but as there's already extensions to fetch the endpoint itself from HttpContext, it may also be useful to include something like this and maybe another extension to fetch a collection of route attributes.

Describe the solution you'd like

Perhaps something like GetEndpointAttribute<T> and GetEndpointAttributes<T> in EndpointHttpContextExtensions.cs alongside the other endpoint-related extensions.

Additional context

This is the basic idea (minus handling nulls, etc - that'd be handled the same way it is right now in GetEndpoint<T>)

public static T GetEndpointAttribute<T>(this HttpContext httpContext) where T : Attribute
{
  var attribute = httpContext
    .Features
    .Get<IEndpointFeature>()
    .Endpoint
    .Metadata.GetMetadata<T>();
  
  return attribute;
}

danleonard-nj avatar Oct 09 '21 21:10 danleonard-nj

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 Oct 21 '21 18:10 ghost

Triage: this seems like a fair API suggestion. Marking it for review.

captainsafia avatar Oct 21 '21 18:10 captainsafia

  • What happens if there is no endpoint? Throw an error or return default(T)?
  • The name should be GetEndpointMetadata.
  • What about getting multiple metadata for a type, e.g. calling into https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.http.endpointmetadatacollection.getorderedmetadata?view=aspnetcore-6.0#Microsoft_AspNetCore_Http_EndpointMetadataCollection_GetOrderedMetadata__1. GetEndpointOrderedMetadata?
  • Concern that this will fill up intellisense for HttpContext?

JamesNK avatar Oct 24 '21 04:10 JamesNK

API review:

👍🏽 all the points @JamesNK raises. It's a bit difficult to reason about what to do when GetEndpoint() returns null. A alternative we might consider is to bump the GetMetadata and GetOrderedMetadata to hang off of Endpoint:

+ public static class EndpointExtensions
+ {
+    public T? GetMetadata<T>(this Endpoint endpoint) where T : class
+    public IReadOnlyList<T> GetOrderedMetadata<T>(this Endpoint endpoint) where T : class
+ }

pranavkm avatar Oct 25 '21 17:10 pranavkm

Thanks for the great feedback :)

I originally figured a null return would probably be appropriate since that's the behaivor now when you fetch Endpoint from HttpContext.

public static Endpoint? GetEndpoint(this HttpContext context)
{
    if (context == null)
    {
        throw new ArgumentNullException(nameof(context));
    }

    return context.Features.Get<IEndpointFeature>()?.Endpoint;
}

Good question on the multiple metadata, hadn't fully considered. Does it muddy things up to return IOrderedEnumerable<T>? That partially addresses the returns - trying to fetch attributes that don't exist on the endpoint would just hand back an empty collection.

If the endpoint doesn't exist at all, I'd think it make sense to follow GetEndpoint and return a null.

Also considering GetEndpoint, it does most of the job, in case adding anything further downstream seems like more trouble than the payoff, that one is definitely handy.

danleonard-nj avatar Oct 26 '21 01:10 danleonard-nj

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost avatar Oct 11 '22 19:10 ghost

@halter73 Thoughts on closing this out? We were pretty lukewarm about this feature when we last discussed it and I don't think the scenario is super motivating given the existing shorthands that are available.

captainsafia avatar Oct 13 '22 17:10 captainsafia

The Endpoint extension idea is tempting. context.GetEndpoint()?.Metadata.GetMetadata<Foo>() is pretty ugly. context.GetEndpoint()?.GetMetadata<Foo>() looks less repetitive and is just as clear, but I'm not sure if it's worth adding another GetMetadata method.

halter73 avatar Oct 13 '22 20:10 halter73