Skip to content

Implement IProvideEndpointMetadata & IProvideEndpointParameterMetadata #40926

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

Merged
merged 20 commits into from
Apr 7, 2022

Conversation

DamianEdwards
Copy link
Member

Implement IProvideEndpointMetadata & IProvideEndpointParameterMetadata

Implements the IProvideEndpointMetadata and IProvideEndpointParameterMetadata interfaces in Microsoft.AspNetCore.Http and updates RequestDelegateFactory to gather metadata for endpoints when these interfaces are implemented by endpoint request handler delegate parameters, return types, and method attributes.

Interfaces were added to the Microsoft.AspNetCore.Http.Extensions project. Unit tests added to the RequestDelegateFactoryTests class.

Fixes #40646

@DamianEdwards DamianEdwards requested a review from a team March 28, 2022 22:00
@ghost ghost added the area-runtime label Mar 28, 2022
Comment on lines 239 to 242
if (typeof(IProvideEndpointParameterMetadata).IsAssignableFrom(parameter.ParameterType))
{
// Parameter type implements IProvideEndpointParameterMetadata
var metadata = GetMetadataForParameterMethod.MakeGenericMethod(parameter.ParameterType).Invoke(null, new object?[] { parameter, factoryContext.ServiceProvider });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephentoub and @davidwrighton, this is the first example of us calling into an interface with a static abstract virtual method from late bound code. Ideally, this would be a helper like:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this would be a helper like:

like... :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I brain farted when I wrote this. Originally I had something like:

public class RuntimeHelpers
{
    public T StaticCast<T>(Type type);
}

But I think this method would need to be an intrinsic (or something like it) since today a generic constraint is required to make this work normally.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda annoying that we have to define RequestDelegateFacotry.GetMetadataForParameter<T>(...) in order to call T.GetMetadata(...) via reflection in the first place. It's defined lower down in the file, but I'll copy it here for reference.

private static IEnumerable<object> GetMetadataForParameter<T>(ParameterInfo parameter, IServiceProvider services)
    where T : IProvideEndpointParameterMetadata
{
     return T.GetMetadata(parameter, services);
}

In most cases, we could just look for a public GetMetadata method on T, but what about explicit interface implementations? AFAIK, you are then forced to get the MethodInfo from the interface in this case. The problem is that a static abstract MethodInfo isn't invocable via reflection since there is no this parameter and no overload of MethodInfo.Invoke takes the derived type to call the static method on.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is technically possible to invoke the static abstract completely through reflection. We added support for static abstract methods in the Type.GetInterfaceMap api. However, that api is rather awkward and slow to use, so if late bound usage is critical, I would welcome proposals for updated reflection apis.

@DamianEdwards DamianEdwards marked this pull request as ready for review March 29, 2022 17:05
@DamianEdwards DamianEdwards requested a review from javiercn as a code owner April 5, 2022 01:31
@DamianEdwards DamianEdwards requested a review from halter73 April 5, 2022 17:34
@DamianEdwards DamianEdwards added the api-approved API was approved in API review, it can be implemented label Apr 6, 2022
@DamianEdwards DamianEdwards requested a review from halter73 April 7, 2022 16:39
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we're happy with the added InitialEndpointMetadata API?

@DamianEdwards DamianEdwards merged commit e0549fc into main Apr 7, 2022
@DamianEdwards DamianEdwards deleted the damianedwards/iprovideendpointmetadata branch April 7, 2022 23:33
@ghost ghost added this to the 7.0-preview4 milestone Apr 7, 2022
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow parameter and return types of route handler delegates (Minimal APIs) to contribute to endpoint metadata
7 participants