@RequestMethod with "params" doesn't distinguish whether a value is specified or not [SPR-16674]

[spring-projects-issues]

Graham Cox opened SPR-16674 and commented

If I have handler methods annotated with @RequestMethod, with varying degrees of the "params" property, Spring doesn't differentiate as I'd hoped.

Specifically, in this case:

@Controller
@RequestMapping(value = ["/oauth2/auth"], method = [RequestMethod.GET])
class AuthorizationController {
    @RequestMapping(params = ["response_type=code"])
    fun startAuthorizationCode() {
        LOG.debug("Starting authorization with response_type=code")
    }
    @RequestMapping(params = ["response_type"])
    fun startAuthorizationUnknownType(@RequestParam("response_type") responseType: String) {
        LOG.warn("Unknown response_type value: {}", responseType)
    }
    @RequestMapping
    fun startAuthorizationNoType() {
        LOG.warn("Missing mandatory parameter: response_type")
    }
}

If I call "GET /oauth2/auth" then I correctly trigger "startAuthorizationNoType" - as the only matching handler.

If I call "GET /oauth2/auth?response_type=different" then I correctly trigger "startAuthorizationUnknownType" - as the more specific match.

Unfortunately, the other case fails. A call to "GET /oauth2/auth?response_type=code" triggers:

java.lang.IllegalStateException: Ambiguous handler methods mapped for HTTP path 'http://localhost:8080/oauth2/auth': {public void uk.co.grahamcox.worlds.openid.AuthorizationController.startAuthorizationCode(), public void uk.co.grahamcox.worlds.openid.AuthorizationController.startAuthorizationUnknownType(java.lang.String)}
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.lookupHandlerMethod(AbstractHandlerMethodMapping.java:370)
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.getHandlerInternal(AbstractHandlerMethodMapping.java:317)
	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.getHandlerInternal(AbstractHandlerMethodMapping.java:62)
	at org.springframework.web.servlet.handler.AbstractHandlerMapping.getHandler(AbstractHandlerMapping.java:351)
	at org.springframework.boot.actuate.metrics.web.servlet.WebMvcMetricsFilter.getHandler(WebMvcMetricsFilter.java:132)
	at org.springframework.boot.actuate.metrics.web.servlet.WebMvcMetricsFilter.filterAndRecordMetrics(WebMvcMetricsFilter.java:119)
	at org.springframework.boot.actuate.metrics.web.servlet.WebMvcMetricsFilter.doFilterInternal(WebMvcMetricsFilter.java:111)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)

I was hoping that this would instead trigger "startAuthorizationCode" as the most specific match of the three, but this proved not to be the case.

---

Affects: 5.0.4

[spring-projects-issues]

Graham Cox commented

It looks like this can be handled better by improving ParamsRequestCondition.compareTo to pay attention to the actual params being matched, and not just the number of them.

Something like:

• If one side has more matches than the other, it wins.
• If both side has equal number of matches, but one side has more where ParamExpression.value != null then it wins.
• If both side has equal number of matches with/without values, but one side has more that are not negated then it wins.

[spring-projects-issues]

Graham Cox commented

I've just also worked out that I can work around this by:

@RequestMapping(params = ["response_type", "response_type=code"])

This now has two "params" entries - even though one is a more specific version of the other - and so it has higher precedence.

[spring-projects-issues]

Rossen Stoyanchev commented

I'd go for a straight forward improvement along the lines of the example in this request, checking the overall count, and then if equal, compare the count of:

expression.getValue() != null && !expression.isNegated()

I think think we should not include negated matches. Arguably q!=foo is a little more specific than just q, but only marginally so, and it's hardly intuitive when you start thinking of more complex scenarios. Certainly a negated match by value is vastly less specific than a positive match by value.

[spring-projects-issues]

Rossen Stoyanchev commented

I've applied the change to param and header conditions.