Skip to content

Support setting ApiParameterRouetInfo for endpoints #37470

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 5 commits into from
Oct 21, 2021

Conversation

captainsafia
Copy link
Member

Contributes towards #36525.

The ApiExplorer for MVC populates each route parameter with an ApiParameterRouteInfo object that contains information about the constraints defined on a route parameter. This metadata can be used by OpenAPI libraries to generate more complete schemas for route parameters. This PR updates the ApiDescriptorProvider for endpoints to generate the same metadata.

Note: both Swashbuckle and NSwag don't currently respect the values in the RouteInfo property so there is some additional work to read from this info.

@captainsafia captainsafia requested review from javiercn and a team October 12, 2021 03:15
@captainsafia captainsafia marked this pull request as ready for review October 12, 2021 03:15
@Pilchie Pilchie added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Oct 12, 2021

return new ApiParameterRouteInfo()
{
Constraints = constraints.AsEnumerable(),
Copy link
Member

Choose a reason for hiding this comment

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

Are we worried about people casting and mutating this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's possible for someone to mutate this in a harmful way (say removing the constraints that we discovered). I don't anticipate this being common but it does't hurt to lock this down.

Copy link
Contributor

Choose a reason for hiding this comment

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

AsEnumerable doesn't do much though - https://source.dot.net/#System.Linq/System/Linq/Enumerable.cs,10. You would need to use AsReadOnly or ReadOnlyCollection if you truly want to lock this down.

But I'm of the shoot you own foot, now you have to go figure out how to bandage it school of thought. If somebody messes with the constraints and things don't work well, they're kinda at fault, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

You would need to use AsReadOnly or ReadOnlyCollection if you truly want to lock this down.

Yep, to clarify by "it doesn't hurt to lock this down" I meant use AsReadOnly here as in 43fd3cd.

But I'm of the shoot you own foot, now you have to go figure out how to bandage it school of thought. If somebody messes with the constraints and things don't work well, they're kinda at fault, no?

Agreed, but it doesn't hurt to be explicit about the fact that we don't recommend doing things like this by making it read-only.

@rafikiassumani-msft rafikiassumani-msft added this to the 7.0-preview1 milestone Oct 14, 2021
@captainsafia captainsafia changed the title Support setting ApiParameterRoutInfo for endpoints Support setting ApiParameterRouetInfo for endpoints Oct 15, 2021
@rafikiassumani-msft rafikiassumani-msft removed this from the 7.0-preview1 milestone Oct 15, 2021
@captainsafia captainsafia enabled auto-merge (squash) October 20, 2021 20:02
@captainsafia captainsafia merged commit 1b07f2a into main Oct 21, 2021
@captainsafia captainsafia deleted the safia/constraints-route-schema branch October 21, 2021 00:16
@ghost ghost added this to the 7.0-preview1 milestone Oct 21, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants