@RequestMapping method resolution is not deterministic [SPR-4551] #9228
Description
Ari Miller opened SPR-4551 and commented
My description of the problem below uses the 2.5.2 version of the classes described. Note the problem also occurs in 2.5.1.
We believe we have discovered an issue with the RequestMapping method resolution logic, that can cause particular requests to be handled by the wrong RequestMapping method based on the order in which the controller methods are returned by Class.getDeclaredMethods() (which is not deterministic).
The AnnotationMethodHandlerAdapter.ServletHandlerMethodResolver.resolveHandlerMethod will return the appropriate method to invoke for a given request. It starts with a series of methods that might potentially be used to handle the request, based on the @RequestMapping annotating those methods in the target handler. The problems stems from the dependency on initial order in the logic used to select from multiple potential methods:
Lines 452 - 472 of AnnotationMethodHandlerAdapter:
[CODE]
else if (!targetHandlerMethods.isEmpty()) {
RequestMappingInfo bestMappingMatch = null;
String bestPathMatch = null;
for (RequestMappingInfo mapping : targetHandlerMethods.keySet()) {
String mappedPath = targetPathMatches.get(mapping);
if (bestMappingMatch == null) {
bestMappingMatch = mapping;
bestPathMatch = mappedPath;
}
else {
if ((mappedPath != null && (bestPathMatch == null ||
mappedPath.equals(lookupPath) || bestPathMatch.length() < mappedPath.length())) ||
(bestMappingMatch.methods.length == 0 && mapping.methods.length > 0) ||
bestMappingMatch.params.length < mapping.params.length) {
bestMappingMatch = mapping;
bestPathMatch = mappedPath;
}
}
}
return targetHandlerMethods.get(bestMappingMatch);
}
[/CODE]
Note that the if((mappedPath != null ... ) logic gives multiple possible opportunities for a method to be deemed a betterMappingMatch than the current best match. Given certain requestMapping annotations, this results in whichever method is last in the LinkedHashMap targetHandlerMethods being returned, because the last method will meet one of the || conditions in that if statement.
Here is a specific example, with two methods on a single controller that could handle an incoming request with the path /enterAccessCode.do:
@RequestMapping("/**/enterAccessCode.do")
public ModelAndView methodWithPathMapping -- this is the method we want to handle the request
@RequestMapping(method = {RequestMethod.GET, RequestMethod.POST})
public ModelAndView methodWithMethodMapping()
Say the LinkedHashMap targetHandlerMethods has the following order:
- methodWithPathMapping
- methodWithMethodMapping
Initially, the bestMappingMatch starts with methodWithPathMapping.
When it gets to the if statement, methodWithMethodMapping becomes the bestMappingMatch, because this part of the statement is true:
(bestMappingMatch.methods.length == 0 && mapping.methods.length > 0)
This is not the desired behavior.
If you have the reverse order:
- methodWithMethodMapping
- methodWithPathMapping
methodWithMethodMapping starts out as the bestMappingMatch, but know the first part of the if statement is true, so methodWithPathMapping becomes the best match.
As to this not being deterministic:
HandlerMethodResolver.handlerMethods is a LinkedHashSet created based on ReflectionUtils.doWithMethods, which in turn depends on the
result of targetClass.getDeclaredMethods() (Javadoc declares: The elements in the array returned are not sorted and are not in any particular order). HandlerMethodResolver.handlerMethods is iterated through to create targetHandlerMethods, which is once again then not in a deterministic order. We've seen this result in different behavior depending on the JVM (and I think hardware) -- some hardware consistently uses the appropriate method to handle the incoming request, some hardware, because of the different method order, uses the inappropriate methodWithMethodMapping.
Our workaround for this is to avoid having our general handler use method = {RequestMethod.GET, RequestMethod.POST} for the methodWithMethodMapping -- this makes all of the if statement || blocks false.
My claim is that the logic to determine the best mapping match in the if block should be insensitive to initial order when finding the bestMappingMatch.
Here is a crude and untested way to accomplish that:
Replace:
[CODE]
if ((mappedPath != null && (bestPathMatch == null ||
mappedPath.equals(lookupPath) || bestPathMatch.length() < mappedPath.length())) ||
(bestMappingMatch.methods.length == 0 && mapping.methods.length > 0) ||
bestMappingMatch.params.length < mapping.params.length) {
bestMappingMatch = mapping;
bestPathMatch = mappedPath;
}
[/CODE]
with
[CODE]
private boolean isBetterPathMatch(String mappedPath, String mappedPathToCompare, String lookupPath) {
return (mappedPath != null && (mappedPathToCompare == null ||
mappedPath.equals(lookupPath) || mappedPathToCompare.length() < mappedPath.length()));
}
private boolean isBetterMethodMatch(RequestMappingInfo mapping, RequestMappingInfo mappingToCompare) {
return mappingToCompare.methods.length == 0 && mapping.methods.length > 0;
}
private boolean isBetterParamMatch(RequestMappingInfo mapping, RequestMappingInfo mappingToCompare) {
return mappingToCompare.params.length < mapping.params.length;
}
if (isBetterPathMatch(mapppedPath, bestPathMatch, lookupPath) ||
(! isBetterPathMatch(bestPathMatch, mappedPath, lookupPath) && isBetterMethodMatch(mapping, bestMappingMatch)) ||
(! isBetterPathMatch(bestPathMatch, mappedPath, lookupPath) && ! isBetterMethodMatch(bestMappingMatch, mapping) && isBetterParamMatch(mapping, bestMappingMatch)) {
bestMappingMatch = mapping;
bestPathMatch = mappedPath;
}
[/CODE]
Affects: 2.5.1, 2.5.2
Attachments:
- 4551Fix.diff (2.63 kB)
- AnnotationMethodHandlerAdapter.java (29.97 kB)
- SpringAnnotationMethodHandlerAdapterNotDeterministicTest.java (3.74 kB)
Issue Links:
- @RequestMapping method resolution issue with duplicate paths [SPR-5069] #9744
@RequestMappingmethod resolution issue with duplicate paths