[Analyzer] Warn on ambiguous route patterns

[captainsafia]

Background and Motivation

ASP.NET Core throws an error if a URL maps to two routes. This is fine, but it can cause confusion with new ASP.NET Core devs who are use to method overloading in C#. New developers might think that minimal or action endpoints also support overloading based on the bound type.

app.MapGet("/product/{name}", (string name) => ...);
app.MapGet("/product/{id}", (int id) => ...);

For example, the routes above produces an ambiguous match because they have the same form from routing's perspective:

1. A literal product segment
2. An unconstrained route parameter segment.

It doesn't matter that the route parameters have different names or are bound to a minimal API method with different .NET types.

This limitation is easy to learn and fix. However, it's not obvious. The analyzer is aimed at new developers who I writing their first app. They can learn in the IDE instead of a runtime error.

Proposed Analyzer

An analyzer that looks at routes and figures out whether they are ambiguous at coding time. This analyzer will only detect obvious and easily detectable situations. There are still many ways to get ambiguous route errors that the analyzer won't address.

Analyzer Behavior and Message

The analyzer will only warn for ambiguous routes mapped in the same method (MapGet calls in a method could be conditional because they're in if statements) scope and using the same route builder. It won't attempt to detect matching routes across methods because they may be passed route builders created by Group("...").

The analyzer message is the same message that ASP.NET Core throws at runtime.

Route handler analyzer

• Title: "Route conflict detected between route handlers"
• Message: "Route '{0}' conflicts with another handler route. An HTTP request that matches multiple routes results in an ambiguous match error. Fix the conflict by changing the route's path, HTTP method, or route constraints."

MVC route analyzer

• Title: "Route conflict detected between controller actions"
• Message: "Route '{0}' conflicts with another action route. An HTTP request that matches multiple routes results in an ambiguous match error. Fix the conflict by changing the route's path, HTTP method, or route constraints."

Category

• Design
• Documentation
• Globalization
• Interoperability
• Maintainability
• Naming
• Performance
• Reliability
• Security
• Style
• Usage

Severity Level

• Error
• Warning
• Info
• Hidden

Routing is complex. I (@JamesNK) feel more comfortable if we start this as a warning. If there are no false positives, then we can make it an error in the future.

Usage Scenarios

app.MapGet("/product/{name}", (string name) => ...);
app.MapGet("/product/{id}", (int id) => ...);

Risks

False positives. When in doubt, the analyzer shouldn't warn.

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

API Review Notes:

• How conservative is this check? Very. Any unknown extension method on the IEndpointConventionBuilder causes this not to fire. Same if the builder is stored in the variable.
• Why not error level? We're a little worried about false positives still. If we don't get feedback about false positives, we can consider increasing the severity. Also, other endpoints can still work when there are conflicting routes.
• We consider any false positives a bug. We want to error on the side of false negatives in all cases.
• Does this fire once for each conflict, or each endpoint that conflicts? It will fire for each endpoint that conflicts.
• What do we think about the titles and messages?
  • Should we show what patterns it conflicts with? No. It could get really noisy, and each conflict is already highlighted.
  • "route's path" vs "route's pattern"? "route's pattern"
• What about the Usage category? Makes sense.

Analyzer approved as proposed with s/path/pattern.