spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.53k stars 38.12k forks source link

@RequestMapping method resolution is not deterministic [SPR-4551] #9228

Closed spring-projects-issues closed 12 years ago

spring-projects-issues commented 16 years ago

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:

  1. methodWithPathMapping
  2. 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:

  1. methodWithMethodMapping
  2. 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:

Issue Links:

spring-projects-issues commented 16 years ago

Ari Miller commented

Attached three files: SpringAnnotationMethodHandlerAdapterNotDeterministicTest.java A test, which depending on your JVM version/hardware (fundamentally, the return order of Class.getDeclaredMethods()), should fail because of this bug.

AnnotationMethodHandlerAdapter.java A fixed version of the Adapter (from 2.5.2 source)

A unified diff for AnnotationMethodHandlerAdapter.java which I believe fixes the issue (4551Fix.diff) This is a change to the version of AnnotationMethodHandlerAdapter.java released in 2.5.2.

spring-projects-issues commented 16 years ago

Ari Miller commented

Note that the 4551Fix.diff file uses version numbers specific to our local repository, but should work when applied to the version of AnnotationMethodHandlerAdapter.java was released in 2.5.2.

For the test code, I relied on the test structure given in http://jira.springframework.org/browse/SPR-4516

spring-projects-issues commented 16 years ago

Juergen Hoeller commented

Thanks for pointing this out - I've revised the @RequestMapping matching algorithm accordingly.

This will be available in tonight's 2.5.3 snapshot (http://static.springframework.org/downloads/nightly/snapshot-download.php?project=SPR) already. Please give it a try and let me know whether it works for you...

Juergen

spring-projects-issues commented 16 years ago

Ari Miller commented

This issue is fixed in 2.5.3-SNAPSHOT. Thanks. I tested with SpringAnnotationMethodHandlerAdapterNotDeterministicTest.java. It fails in 2.5.2, passes with 2.5.3-SNAPSHOT.

I used the directions here to grab the 2.5.3-SNAPSHOT for Maven: http://blog.springsource.com/main/2007/09/18/maven-artifacts-2/

I confirmed the 2.5.3-SNAPSHOT source code had this fix in it at the time I tested. I like your removal of my duplicate call to (!isBetterPathMatch(bestPathMatch, mappedPath, lookupPath) in the prioritization if block.

spring-projects-issues commented 16 years ago

William Butler commented

There is still an issue related to the following method:

[CODE] private boolean isBetterPathMatch(String mappedPath, String mappedPathToCompare, String lookupPath) { return (mappedPath != null && (mappedPathToCompare == null || mappedPath.equals(lookupPath) || mappedPathToCompare.length() < mappedPath.length())); } [/CODE]

If multiple methods are annotated with the same path value, one can be considered "better" than another based on the fact that the mappedPath equals lookupPath. There is no check to see if the mappedPathToCompare is also equal to the lookupPath. The code should probably be changed to something like:

[CODE] private boolean isBetterPathMatch(String mappedPath, String mappedPathToCompare, String lookupPath) { return (mappedPath != null && (mappedPathToCompare == null || mappedPath.equals(lookupPath) && !mappedPathToCompare.equals(lookupPath) || mappedPathToCompare.length() < mappedPath.length())); } [/CODE]