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

Fixed the issue #60096

Merged
merged 7 commits into from
Feb 4, 2025
Merged

Fixed the issue #60096

merged 7 commits into from
Feb 4, 2025

Conversation

mkArtakMSFT
Copy link
Member

@mkArtakMSFT mkArtakMSFT commented Jan 29, 2025

Include typed result metadata in the action descriptors for MVC Controller actions

Description

The endpoint metadata wasn't being captured in ActionModel during the ApplicationModel creation. This is addressed by the change in the DefaultApplicationModelProvider class. The metadata is now being extracted based on the return type of the action and then the selectors associated with the action are populated with that metadata, as that's what is being used later on for populating the ActionDescriptor's EndpointMetadata.

Fixes #44988

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Jan 29, 2025
@mkArtakMSFT mkArtakMSFT force-pushed the mkArtakMSFT/issue_44988 branch from 7886a25 to c274bff Compare January 31, 2025 02:01
@mkArtakMSFT mkArtakMSFT marked this pull request as ready for review January 31, 2025 02:02
@mkArtakMSFT mkArtakMSFT requested a review from a team as a code owner January 31, 2025 02:02
@mkArtakMSFT
Copy link
Member Author

@wtgodbe I assume these failures are all related to #59788?

@wtgodbe
Copy link
Member

wtgodbe commented Jan 31, 2025

Yes - I just merged that, so re-kicking this should fix CI

@mkArtakMSFT
Copy link
Member Author

@eerhardt somehow, I am unable to properly suppress these warnings, even though the suppressions have been generated. When running the suppression generation command locally, it doesn't make any other changes, which indicates that the suppressions are properly made in the file. So I'm not sure what else may be going on here. Can you take a look and see what I may be missing here?

@mkArtakMSFT mkArtakMSFT requested review from eerhardt and removed request for mgravell, halter73, JamesNK and BrennanConroy February 1, 2025 05:28
@mkArtakMSFT
Copy link
Member Author

So the manual "fix" that I did was done incorrectly and that was the culprit. Thank you, @eerhardt.

@captainsafia this is ready for review.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Looks good overall! Posted some notes about adding more tests to validate the behavior in the ApiExplorer/OpenAPI layer.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

LGTM!

@mkArtakMSFT mkArtakMSFT merged commit 43b5fbf into main Feb 4, 2025
27 checks passed
@mkArtakMSFT mkArtakMSFT deleted the mkArtakMSFT/issue_44988 branch February 4, 2025 05:39
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview2 milestone Feb 4, 2025
@mkArtakMSFT mkArtakMSFT changed the title Fixed the issue. Will be adding the tests as a separate commit. Fixed the issue Feb 6, 2025
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypedResults metadata are not inferred for API Controllers
4 participants