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

ApiExplorer Support for HttpResults types for Controllers #57464

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jgarciadelanoceda
Copy link
Contributor

@jgarciadelanoceda jgarciadelanoceda commented Aug 22, 2024

Fixes issue #44988.
The ControllerActionDescriptor is created in the ControllerActionDescriptorBuilder, but as it does not has the EndpointBuilder aspNetCore does not bind the ResponseTypes that are obtained in the method EndpointMetadataPopulator.PopulateMetadata.

To do not break anything the first approach that I did is to add to the EndpointMetadata of the ControllerActionDescriptor pass the types that are obtained in the EndpointBuilder.

It will be interesting to backPort this to dotnet8, since is the latest LTS, but I added the OpenApi test to show that everything is working as expected

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Aug 22, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 22, 2024
@jgarciadelanoceda jgarciadelanoceda changed the title Support for HttpResults types Support for HttpResults types in ApiExplorer Aug 22, 2024
@jgarciadelanoceda jgarciadelanoceda changed the title Support for HttpResults types in ApiExplorer ApiExplorer Support for HttpResults types for Controllers Aug 22, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Aug 30, 2024
@jgarciadelanoceda
Copy link
Contributor Author

jgarciadelanoceda commented Sep 5, 2024

I tried rebasing this with master version in order to see if I broke the tests that are failing but I have not found a way to execute the tests on master version. Is it required to have a Preview version of Visual Studio to run AspNetCore?

@captainsafia
Copy link
Member

but I have not found a way to execute the tests on master version. Is it required to have a Preview version of Visual Studio to run AspNetCore?

What issues are you seeing when you try to execute the tests locally? The main branch currently relies on 9.0.0-rc.2. I don't use Visual Studio so not sure if we have a hard requirement on a certain VS version as part of that SDK target.

@jgarciadelanoceda
Copy link
Contributor Author

but I have not found a way to execute the tests on master version. Is it required to have a Preview version of Visual Studio to run AspNetCore?

What issues are you seeing when you try to execute the tests locally? The main branch currently relies on 9.0.0-rc.2. I don't use Visual Studio so not sure if we have a hard requirement on a certain VS version as part of that SDK target.

The error is
Severity Code Description Project File Line Suppression State Details
Error (active) NETSDK1060 Error reading assets file: Error loading lock file 'C:\Git\Github\aspnetcore\artifacts\obj\Microsoft.AspNetCore.Cryptography.Internal\project.assets.json' : Could not load file or assembly 'System.Text.Json, Version=8.0.0.4, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified. Microsoft.AspNetCore.Cryptography.Internal (net462), Microsoft.AspNetCore.Cryptography.Internal (net9.0), Microsoft.AspNetCore.Cryptography.Internal (netstandard2.0) C:\Git\Github\aspnetcore.dotnet\sdk\9.0.100-rc.2.24452.3\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets 266

It seems there could be a process which is using those files

@martincostello
Copy link
Member

Looks like that's a known issue: dotnet/core#9489

@captainsafia
Copy link
Member

@jgarciadelanoceda I'm still a little wary of this approach given the feedback left here. This implementation is focused on the ProducesResponseTypeMetadata and doesn't handle any metadata that might be injected by an IEndpointMetadataProvider implementation. In the past, we approached this problem with an implementation like this:

// Add metadata inferred from the parameter and/or return type before action-specific metadata.
// MethodInfo *should* never be null given a ControllerActionDescriptor, but this is unenforced.
if (controllerActionDescriptor?.MethodInfo is not null)
{
EndpointMetadataPopulator.PopulateMetadata(controllerActionDescriptor.MethodInfo, builder);
}
// Add action-specific metadata early so it has a low precedence
if (action.EndpointMetadata != null)
{
foreach (var d in action.EndpointMetadata)
{
builder.Metadata.Add(d);
}
}

But as I recall had to revert it because we broke some scenarios. Can you use the code above as the basis for the implementation?

@jgarciadelanoceda
Copy link
Contributor Author

jgarciadelanoceda commented Sep 24, 2024

@jgarciadelanoceda I'm still a little wary of this approach given the feedback left here. This implementation is focused on the ProducesResponseTypeMetadata and doesn't handle any metadata that might be injected by an IEndpointMetadataProvider implementation. In the past, we approached this problem with an implementation like this:

// Add metadata inferred from the parameter and/or return type before action-specific metadata.
// MethodInfo *should* never be null given a ControllerActionDescriptor, but this is unenforced.
if (controllerActionDescriptor?.MethodInfo is not null)
{
EndpointMetadataPopulator.PopulateMetadata(controllerActionDescriptor.MethodInfo, builder);
}
// Add action-specific metadata early so it has a low precedence
if (action.EndpointMetadata != null)
{
foreach (var d in action.EndpointMetadata)
{
builder.Metadata.Add(d);
}
}

But as I recall had to revert it because we broke some scenarios. Can you use the code above as the basis for the implementation?

Hi @captainsafia, I have just looked in all the implementations of IEndpointMetadataProvider, and I saw that all of them populated into the EndpointBuilder.Metadata an object of type IProducesResponseTypeMetadata, because of this I wrote the PR using this object, the risk it has is if in the future the implementations change you could miss all that info.
I am just copying back to the ControllerActionDescriptor the metadata which is obtained in the call to EndpointMetadataPopulator.PopulateMetadata, which is almost the same of what you suggested.

The problem I have is that the builder.Metadata is just a ListOfobject, so I just copy the objects that I am interested about. I think I cannot do anything more unless I add a new method in IEndpointMetadataProvider but I do not think that is neccesary for this PR

@jgarciadelanoceda
Copy link
Contributor Author

@captainsafia thoughts?, I do not see any other alternative here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants