Skip to content

Populate ActionDescriptor EndpointMetadata #34065

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

Conversation

martincostello
Copy link
Member

@martincostello martincostello commented Jul 3, 2021

PR Title

Populate ActionDescriptor.EndpointMetadata.

PR Description

Populate the EndpointMetadata property of the ActionDescriptor on ApiDescription instances for minimal actions so that the attributes associated with the request delegate can be inspected.

This seems to me like it's something that should be done and is just missing, but in case it was intentional and this use case is intended to be solved in a different way, I've just opened this as a draft for now pending confirmation (or not) of the principle.

Relates to #34061.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 3, 2021
@martincostello
Copy link
Member Author

I used the below change to SimpleWebSiteWithWebApplicationBuilder and the JsonResult_Works functional test to work through this.

app.MapGet("/json", [ApiExplorerSettings(IgnoreApi = true)] (IApiDescriptionGroupCollectionProvider provider) =>
{
    if (provider.ApiDescriptionGroups is not null)
    {
        // Used to inspect and debug ApiDescriptionGroups
    }

    return Json(new Person("John", 42));
});

@martincostello
Copy link
Member Author

Also, with this and #34063, it's possible there's other things missing from the ActionDescriptor that should be being set/populated, but these are the two that I've specifically noticed.

Populate the EndpointMetadata property of the ActionDescriptor on
ActionDescriptor instances for minimal actions so that the attributes
associated with the request delegate can be inspected.
Relates to dotnet#34061.
@martincostello martincostello force-pushed the Set-ActionDescriptor-Metadata-34061 branch from 74bf7b5 to 2032c68 Compare July 3, 2021 14:37
@martincostello
Copy link
Member Author

martincostello commented Jul 3, 2021

Digging into it more this isn't a magic fix for #34061 for [ApiExplorerSettings] to just work, but it seems to be a sensible change as otherwise none of the attributes on the action can be inspected on the ApiDescription for customisation by application code.

For example:

builder.Services.AddSwaggerGen(options =>
{
    options.SwaggerDoc("v1", new OpenApiInfo { Title = "Todo API", Version = "v1" });

    options.DocInclusionPredicate(
        (_, description) =>
            !description.ActionDescriptor.EndpointMetadata.OfType<ApiExplorerSettingsAttribute>().Any((p) => p.IgnoreApi));
});

@javiercn
Copy link
Member

javiercn commented Jul 5, 2021

It's not ok for API explorer to modify the endpoint metadata on action descriptor. The action descriptors should only be modified by the IActionDescriptor providers.

While this potentially solves the problem you are facing, we don't think this is the correct way to solve it.

The endpoint metadata on the action descriptor is "Incomplete" by design and the one on the actual endpoint should be used instead. The metadata on the descriptor only reflects the metadata at the time the descriptor was created and doesn't include anything that has been added after the endpoint was generated. Updating it there, means that other libraries will see a different result after navigating to the swagger page and that is not acceptable.

@martincostello
Copy link
Member Author

Then is it the case that part of the implementation in the provider is in the wrong place, given that the ActionDescriptor itself is created here?

ActionDescriptor = new ActionDescriptor
{
DisplayName = routeEndpoint.DisplayName,
RouteValues =
{
["controller"] = controllerName,
},
},

The class modified here implements IActionDescriptorProvider, so I don't quite follow the argument that only action descriptor providers should modify it because it is one.

It's also not really modified per se, as it's just completely empty - there is no metadata at all, as it was constructed just a few lines earlier.

The application has no prior access to the ActionDescriptor before the method returns because it is the source of it.

If that's still incorrect, then I'll close this PR. In that case though, what is the intended design that would allow the action's metadata to be accessible to the API Explorer for inspection?

@javiercn
Copy link
Member

javiercn commented Jul 5, 2021

@martincostello I might have overlooked something, it's been a while since I've been in that part of the codebase. I'll take another look and update my comment here as necessary.

@mkArtakMSFT mkArtakMSFT added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-routing area-runtime and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jul 7, 2021
@mkArtakMSFT mkArtakMSFT assigned halter73 and unassigned javiercn Jul 7, 2021
// Intentionally not readonly to prevent defensive struct copies
private readonly object[] _items;
private object[] _items;
Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavkm This got changed by #34215 but the comment suggests it shouldn't have. I put it back when resolving merge conflicts.

Remove filtering and just directly populate the list.
Fix typo in comment.
@davidfowl
Copy link
Member

@pranavkm @halter73 What's the deal here?

@halter73
Copy link
Member

I think we should expose all the method's attributes on the ActionDescriptors generated by EndpointMetadataApiDescriptionProvider somehow, and those are all in RouteEndpoint.Metadata. This makes a lot of sense to me if @pranavkm has no objections.

@pranavkm pranavkm merged commit 3accbd6 into dotnet:main Jul 16, 2021
@ghost ghost added this to the 6.0-rc1 milestone Jul 16, 2021
@martincostello martincostello deleted the Set-ActionDescriptor-Metadata-34061 branch July 16, 2021 18:42
@DamianEdwards
Copy link
Member

I was literally just looking for this trying to configure a custom operation id builder for Swashbuckle for minimal APIs. This will let us get to the endpoint name.

@ghost
Copy link

ghost commented Jul 16, 2021

Hi @DamianEdwards. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member feature-routing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants