-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Enhance endpoint selection by resolving ambiguities through constraint specificity analysis #62756
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,8 @@ private static void ProcessFinalCandidates( | |
Endpoint? endpoint = null; | ||
RouteValueDictionary? values = null; | ||
int? foundScore = null; | ||
var candidatesWithSameScore = new List<CandidateState>(); | ||
|
||
for (var i = 0; i < candidateState.Length; i++) | ||
{ | ||
ref var state = ref candidateState[i]; | ||
|
@@ -74,6 +76,7 @@ private static void ProcessFinalCandidates( | |
endpoint = state.Endpoint; | ||
values = state.Values; | ||
foundScore = state.Score; | ||
candidatesWithSameScore.Add(state); | ||
} | ||
else if (foundScore < state.Score) | ||
{ | ||
|
@@ -85,15 +88,27 @@ private static void ProcessFinalCandidates( | |
} | ||
else if (foundScore == state.Score) | ||
{ | ||
// This is the second match we've found of the same score, so there | ||
// must be an ambiguity. | ||
// | ||
// Don't worry about the 'null == state.Score' case, it returns false. | ||
// Same score - collect for constraint specificity analysis. We cant | ||
// just dismiss these candidate and report an ambiguity, we need to | ||
// analyze them for constraint specificity. | ||
candidatesWithSameScore.Add(state); | ||
} | ||
} | ||
|
||
// If we have multiple candidates with the same score, try to resolve using | ||
// constraint specificity rules | ||
if (candidatesWithSameScore.Count > 1) | ||
{ | ||
var mostSpecific = SelectMostSpecificEndpoint(candidatesWithSameScore); | ||
if (mostSpecific.HasValue) | ||
{ | ||
endpoint = mostSpecific.Value.Endpoint; | ||
values = mostSpecific.Value.Values; | ||
} | ||
else | ||
{ | ||
// Still ambiguous after constraint analysis | ||
ReportAmbiguity(candidateState); | ||
|
||
// Unreachable, ReportAmbiguity always throws. | ||
throw new NotSupportedException(); | ||
} | ||
} | ||
|
||
|
@@ -104,6 +119,104 @@ private static void ProcessFinalCandidates( | |
} | ||
} | ||
|
||
private static CandidateState? SelectMostSpecificEndpoint(List<CandidateState> candidates) | ||
{ | ||
CandidateState? mostSpecific = null; | ||
var highestSpecificity = -1; | ||
var hasAmbiguity = false; | ||
|
||
foreach (var candidate in candidates) | ||
{ | ||
if (candidate.Endpoint is not RouteEndpoint routeEndpoint) | ||
{ | ||
continue; | ||
} | ||
|
||
var specificity = CalculateConstraintSpecificity(routeEndpoint); | ||
|
||
if (specificity > highestSpecificity) | ||
{ | ||
highestSpecificity = specificity; | ||
mostSpecific = candidate; | ||
hasAmbiguity = false; | ||
} | ||
else if (specificity == highestSpecificity) | ||
{ | ||
// Okay, note the ambiguity and continue trying | ||
// to determine a higher level of specificity. | ||
hasAmbiguity = true; | ||
} | ||
} | ||
|
||
return hasAmbiguity ? null : mostSpecific; | ||
} | ||
|
||
private static int CalculateConstraintSpecificity(RouteEndpoint endpoint) | ||
{ | ||
var specificity = 0; | ||
var routePattern = endpoint.RoutePattern; | ||
|
||
foreach (var parameter in routePattern.Parameters) | ||
{ | ||
// We may have parameter without constraints, e.g. "id" in "/products/{id}" | ||
if (parameter.ParameterPolicies?.Count > 0) | ||
{ | ||
foreach (var policy in parameter.ParameterPolicies) | ||
{ | ||
if (policy.Content != null) | ||
{ | ||
specificity += GetConstraintSpecificityWeight(policy.Content); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return specificity; | ||
} | ||
|
||
private static int GetConstraintSpecificityWeight(string constraintName) | ||
{ | ||
return constraintName.ToLowerInvariant() switch | ||
{ | ||
// Strong typed constraints that are very restrictive and has | ||
// the highest specificity | ||
"guid" => 100, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using .Equals with a StringComparison to ignore casing might be more consistent than calling ToLowerInvariant for every constraint. |
||
"datetime" => 90, | ||
"decimal" => 85, | ||
"double" => 80, | ||
"float" => 75, | ||
"long" => 70, | ||
"int" => 65, | ||
"bool" => 60, | ||
|
||
// Range constraint are more restrictive than other types | ||
var range when range.StartsWith("range(", StringComparison.OrdinalIgnoreCase) => 55, | ||
var min when min.StartsWith("min(", StringComparison.OrdinalIgnoreCase) => 50, | ||
var max when max.StartsWith("max(", StringComparison.OrdinalIgnoreCase) => 50, | ||
|
||
// This one is a bit odd, but we will consider it less specific than range, | ||
// since it defines only the length of the value and will consider it as raw | ||
// string not a number. | ||
var length when length.StartsWith("length(", StringComparison.OrdinalIgnoreCase) => 45, | ||
var minlength when minlength.StartsWith("minlength(", StringComparison.OrdinalIgnoreCase) => 40, | ||
var maxlength when maxlength.StartsWith("maxlength(", StringComparison.OrdinalIgnoreCase) => 40, | ||
|
||
// String patterns, which are less specific than length | ||
"alpha" => 35, | ||
var regex when regex.StartsWith("regex(", StringComparison.OrdinalIgnoreCase) => 30, | ||
|
||
// File constraints | ||
"file" => 25, | ||
"nonfile" => 25, | ||
|
||
// Least specific just requires non empty | ||
"required" => 10, | ||
|
||
// Unknown constraint, assign medium specificity to it | ||
_ => 20 | ||
}; | ||
} | ||
|
||
private static void ReportAmbiguity(Span<CandidateState> candidateState) | ||
{ | ||
// If we get here it's the result of an ambiguity - we're OK with this | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,6 +58,67 @@ private DataSourceDependentMatcher CreateDfaMatcher( | |
return Assert.IsType<DataSourceDependentMatcher>(factory.CreateMatcher(dataSource)); | ||
} | ||
|
||
[Fact] | ||
public async Task MatchAsync_SimilarEndpoints_CanDetermine_MostSpecificOne() | ||
{ | ||
// Arrange | ||
var dataSource = new DefaultEndpointDataSource(new List<Endpoint> | ||
{ | ||
CreateEndpoint("/test/{name:required}", 0), | ||
CreateEndpoint("/test/{id:guid:required}", 0), | ||
}); | ||
|
||
var matcher = CreateDfaMatcher(dataSource); | ||
|
||
var httpContext = CreateContext(); | ||
httpContext.Request.Path = "/test/" + Guid.NewGuid().ToString(); | ||
|
||
// Act | ||
await matcher.MatchAsync(httpContext); | ||
|
||
// Assert | ||
Assert.Same(dataSource.Endpoints[1], httpContext.GetEndpoint()); | ||
} | ||
|
||
[Fact] | ||
public async Task MatchAsync_MultipleEndpointsWithSameRequiredValues_EndpointMatched() | ||
{ | ||
// Arrange | ||
var endpoint1 = CreateEndpoint( | ||
"{controller}/{action}/{id?}", | ||
0, | ||
requiredValues: new { controller = "Home", action = "Index", area = (string)null, page = (string)null }); | ||
var endpoint2 = CreateEndpoint( | ||
"{controller}/{action}/{id?}", | ||
0, | ||
requiredValues: new { controller = "Login", action = "Index", area = (string)null, page = (string)null }); | ||
|
||
var dataSource = new DefaultEndpointDataSource(new List<Endpoint> | ||
{ | ||
endpoint1, | ||
endpoint2 | ||
}); | ||
|
||
var matcher = CreateDfaMatcher(dataSource); | ||
|
||
var httpContext = CreateContext(); | ||
httpContext.Request.Path = "/Home/Index/123"; | ||
|
||
// Act 1 | ||
await matcher.MatchAsync(httpContext); | ||
|
||
// Assert 1 | ||
Assert.Same(endpoint1, httpContext.GetEndpoint()); | ||
|
||
httpContext.Request.Path = "/Login/Index/123"; | ||
|
||
// Act 2 | ||
await matcher.MatchAsync(httpContext); | ||
|
||
// Assert 2 | ||
Assert.Same(endpoint2, httpContext.GetEndpoint()); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test for the case where the match is still ambiguous if there are specificity constraints that match? |
||
[Fact] | ||
public async Task MatchAsync_ValidRouteConstraint_EndpointMatched() | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we initialize this the first time two candidates with the same score are detected?