Skip to content

Analyzer: warn when marking a route parameter as optional if it isn't at the end of a route #39486

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

Open
Jcparkyn opened this issue Jan 13, 2022 · 2 comments
Labels
analyzer Indicates an issue which is related to analyzer experience area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing
Milestone

Comments

@Jcparkyn
Copy link

Jcparkyn commented Jan 13, 2022

Is your feature request related to a problem? Please describe.

Route parameters in an ASP.NET Core Web API can be marked as optional with a ?, e.g.,

app.MapGet("/test1/{foo?}", (string? foo) => foo ?? "none"); // "/test1" returns "none"

If a route parameter marked as optional is followed by a non-optional route segment, the optional annotation is ignored. E.g.,

app.MapGet("/test2/{foo?}/bar", (string? foo) => foo ?? "none"); // "/test2//bar" returns 404

It is not immediately obvious (to me at least) that it would not be possible to have an optional parameter within a route, and there is no run-time error or warning when doing this.

The behavior appears to be the same for nullable value types (e.g. int?).

Describe the solution you'd like

An analyzer that checks for routes containing optional parameters followed by non-optional route segments, and provides an appropriate warning.

I also couldn't find any mention in the documentation stating that this is not possible.

Additional context

Optional route parameters are allowed to be followed by other optional parameters, so the analyzer should consider this case. E.g.,

app.MapGet("/test3/{foo?}/{bar?}", (string? foo, string? bar) => foo ?? bar ?? "none"); // "/test3" returns "none"

Related: #36637

@javiercn javiercn added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-routing analyzer Indicates an issue which is related to analyzer experience labels Jan 13, 2022
@mkArtakMSFT mkArtakMSFT added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jan 13, 2022
@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Jan 13, 2022
@ghost
Copy link

ghost commented Jan 13, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@rjgotten
Copy link

rjgotten commented Jan 19, 2022

I don't really (or rather: really don't) understand why optional segments are not allowed in the middle of a pattern to begin with.

Regular optional segments, i.e. that are not marked catch-all, have a {0,1} cardinality creating a well-defined upper bound. A pattern that has N consecutive optional segments in the middle, followed by some required (incl. literal) segments, should always be able to be matched using an Nth degree lookahead.

It's completely silly that the framework implementation handling the routing DFA can't handle this and ruins simple solutions with a lot of practical gain, like multilingual routing using an BCP-47 tag as an optional first path segment to force selection of a certain language, overruling e.g. culture-selection based on the Accept-Language header.

@captainsafia captainsafia added area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer Indicates an issue which is related to analyzer experience area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-routing
Projects
None yet
Development

No branches or pull requests

5 participants