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

GetEndpointAttribute<T> in EndpointHttpContextExtensions.cs #37425

Open
danleonard-nj opened this issue Oct 9, 2021 · 8 comments
Open

GetEndpointAttribute<T> in EndpointHttpContextExtensions.cs #37425

danleonard-nj opened this issue Oct 9, 2021 · 8 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Milestone

Comments

@danleonard-nj
Copy link

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;
}
@captainsafia captainsafia added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 21, 2021
@captainsafia captainsafia added this to the .NET 7 Planning milestone Oct 21, 2021
@captainsafia captainsafia 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 Oct 21, 2021
@ghost
Copy link

ghost commented Oct 21, 2021

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.

@captainsafia
Copy link
Member

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

@JamesNK
Copy link
Member

JamesNK commented Oct 24, 2021

@pranavkm
Copy link
Contributor

pranavkm commented Oct 25, 2021

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 pranavkm added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Oct 25, 2021
@danleonard-nj
Copy link
Author

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.

@ghost
Copy link

ghost commented Oct 11, 2022

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.

@captainsafia
Copy link
Member

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

@halter73
Copy link
Member

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.

@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-web-frameworks labels Jun 20, 2023
@captainsafia captainsafia modified the milestones: .NET 8 Planning, Backlog Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc
Projects
No open projects
Status: No status
Development

No branches or pull requests

6 participants