Enhance endpoint selection by resolving ambiguities through constraint specificity analysis #62756
Conversation
…t specificity analysis
Reduce ambiguity between constraints by defining a level of specificity between the constraints.
Previously, if you had two endpoints as follows:
```
GET test/{id:required}
```
and
```
GET test/{id:guid:required}
```
The DefaultEndpointSelector would not be able to determine which endpoint is the correct
one among the two, as neither violates the Route Constraint policy and therefore are
considered valid candidates. From a human perspective, the endpoint that has the constraint
of the GUID is more specific than simply REQUIRED, but the engine did not make that
distinction. Now, the DefaultEndpointSelector can eliminate ambiguities based on their
constraint level of priority.
Lets say the same routes described above. The engine will get two candidates available for it,
but it can remove the first one cause its less specific than second one. But it is interesting
to note that if the system cannot determine the priority, it will still report the ambiguity.
Let's assume that the second request was equal to the first, it would trigger AmbiguousMatchException.
The currently specifity order is defined as (from higher to lower one):
1 - Strong typed route constraint (e.g int, guid, long etc.)
2 - Ranged route contraint (e.g min, max, range)
3 - Length route constraint (e.g length, minlength, maxlength)
4 - String patterns (e.g regex, alphanumerical)
5 - File and non file
6 - Unknown route constraint
7 - Required route constraint
As mentioned earlier, if after processing the ambiguities he still hasn't been able to
determine a specificity, he will report the ambiguity, since there is nothing to be
done in this case : ).
sami-daniel
requested review from
halter73,
BrennanConroy and
captainsafia
as code owners
github-actions
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
martincostello
added
area-routing
and removed
needs-area-label
|
I am not sure if those who failed are related with the changes. |
|
@sami-daniel These failures appear to be flakes in our build and not related to your change. I can kick the build here. |
|
This is a hot path. Are we adding allocations? Why do we need this logic in the default selector instead of one you can opt into? |
There was a problem hiding this comment.
Thanks for taking the time to work on this change, @sami-daniel!
I left some notes on the current implementation but have two big picture thoughts:
- Given the nature of the change, we'll likely have to document this as a breaking behavioral change in routing given it changes the contract the router makes.
- I wonder if it might be better to support this feature by changing the logic that calculates precedence for route templates to account for constraints (ref).
| @@ -60,6 +60,8 @@ private static void ProcessFinalCandidates( | |||
| Endpoint? endpoint = null; | |||
| RouteValueDictionary? values = null; | |||
| int? foundScore = null; | |||
| var candidatesWithSameScore = new List<CandidateState>(); | |||
There was a problem hiding this comment.
Can we initialize this the first time two candidates with the same score are detected?
| // Assert 2 | ||
| Assert.Same(endpoint2, httpContext.GetEndpoint()); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can we add a test for the case where the match is still ambiguous if there are specificity constraints that match?
| { | ||
| // Strong typed constraints that are very restrictive and has | ||
| // the highest specificity | ||
| "guid" => 100, |
There was a problem hiding this comment.
Using .Equals with a StringComparison to ignore casing might be more consistent than calling ToLowerInvariant for every constraint.
|
I’d follow up here by looking for benchmarks, for scenarios where this new code should not impact simple scenarios. At a glance, it does not look pay for play. |
Few things. I don't believe that the endpoint selector is the right place to make this change. Factoring it on the route precedence would make more sense. That said, I think this change is problematic for several reasons:
I think before we even consider a change in this area, we need to discuss whether this is even a change that we want to make at all, and that should be something we discuss on an issue rather than on a PR. |
Yes, we are adding allocations, and worse. We are copying the candidateState unnecessarily to the list when we could use it by reference. I will change that. |
Placing these changes in the DefaultEndpointSelector was a mistake, as it's not actually the source of the problem. The issue lies with its caller — the DFAMatcher. The DefaultEndpointSelector is just the default implementation used by the engine when no custom endpoint selector is defined. In contrast, the DFAMatcher cannot be replaced or customized with its own selector, so it makes more sense for the changes to be applied there. The real problem is that the DFAMatcher does not consider the weight of constraints, as described on the pr — it only checks whether they are valid. I confess that I did not notice or pay attention to the use of RoutePrecedence in the code. From what I saw, it is used in the DFAMatcher builder, and in some other places that I am not sure are related. I am not sure if it makes sense to make changes to RoutePrecedence, since it is not the direct cause of the problem. I would need to study a bit more to see where it applies... |
Yes, those are great points to consider. It may really not be worth pursuing. The DFA/DFAMatcher is already a solid algorithm that performs its task well. As you mentioned, introducing additional overhead to this hot path might not be a good idea — even if it’s just to eliminate ambiguity and align more closely with how framework users think. From what I can tell, making this change without affecting a critical path in the router is nearly impossible, since its a hot path. It seems like a very costly tradeoff. On top of that, as you pointed out, there are other cases that would also need to be handled. This could potentially require an entirely new pattern matching layer to cover all scenarios, adding even more overhead. Maybe your approach to negating a constraint is much more interesting and much cheaper. The control of this would be in the user's hands, not altering the current behavior of the Router and not adding any overhead. Such a constraint would pave the way for a new range of other possible routes to be created with similar templates. I really would like to do this 😁😁. |
|
As @javiercn said, perhaps it would be better, instead of adding overhead to |
Enhance endpoint selection by resolving ambiguities through constraint specificity analysis
Improved endpoint selection via constraint specificity analysis
Description
Previously, if you had two endpoints as follows:
and
The DefaultEndpointSelector would not be able to determine which endpoint is the correct
one among the two, as neither violates the Route Constraint policy and therefore are
considered valid candidates. From a human perspective, the endpoint that has the constraint of the GUID is more specific than simply REQUIRED, but the engine did not make that distinction. Now, the DefaultEndpointSelector can eliminate ambiguities based on their constraint level of priority.
Lets say the same routes described above. The engine will get two candidates available for it, but it can remove the first one cause its less specific than second one. But it is interesting to note that if the system cannot determine the priority, it will still report the ambiguity. Let's assume that the second request was equal to the first, it would trigger AmbiguousMatchException.
The currently specifity order is defined as (from higher to lower one):
1 - Strong typed route constraint (e.g int, guid, long etc.)
2 - Ranged route contraint (e.g min, max, range)
3 - Length route constraint (e.g length, minlength, maxlength)
4 - String patterns (e.g regex, alphanumerical)
5 - File and non file
6 - Unknown route constraint
7 - Required route constraint
As mentioned earlier, if after processing the ambiguities he still hasn't been able to
determine a specificity, he will report the ambiguity, since there is nothing to be
done in this case : ).
Fixes #62278