-
Notifications
You must be signed in to change notification settings - Fork 10k
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
base: main
Are you sure you want to change the base?
ApiExplorer Support for HttpResults types for Controllers #57464
Conversation
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? |
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 It seems there could be a process which is using those files |
Looks like that's a known issue: dotnet/core#9489 |
@jgarciadelanoceda I'm still a little wary of this approach given the feedback left here. This implementation is focused on the aspnetcore/src/Mvc/Mvc.Core/src/Routing/ActionEndpointFactory.cs Lines 368 to 382 in 067c6c2
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. 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 |
@captainsafia thoughts?, I do not see any other alternative here |
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