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

Remove constraints from route parameters in path #2231

Conversation

captainsafia
Copy link
Contributor

Fixes dotnet/aspnetcore#36413.

This fix expands on the changes made in #2209.

Currently, the route constraints that are included in the route template path are included in the generated schema which breaks things like the curl command generated in the UI. This resolves that issue by stripping out constraints/qualifiers from the path.

Note that there is work tracked in dotnet/aspnetcore#36525 to ensure that these route constraints show up in the schema.

@captainsafia captainsafia marked this pull request as ready for review September 15, 2021 20:09
@@ -47,5 +47,26 @@ public static IEnumerable<object> CustomAttributes(this ApiDescription apiDescri

customAttributes = Enumerable.Empty<object>();
}

internal static string RelativePathSansParameterConstraints(this ApiDescription apiDescription)
{
Copy link
Owner

Choose a reason for hiding this comment

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

Could this be done with a Regex replace? I feel that would be a little cleaner but not sure about perf implications.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps! Admittedly, I'm by now means a regex expert but things get pretty gnarly when you try to capture all of the requirements for the matcher in regex form (matching '?', '=', or ':', handling regex constraints, etc).

In generally, we've avoid parsing routes with regex over in ASP.NET Core. We'll usually roll out token enumerator implementation (ref) to handle this.

I do agree that some code comments might make this a little bit easier to read/maintain.

@domaindrivendev domaindrivendev merged commit 923c7e6 into domaindrivendev:master Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minimal APIs route constraints break OpenAPI requests
2 participants