Expose mapped handler as request attribute in spring-webmvc [SPR-17518]

[spring-projects-issues]

Johnny Lim opened SPR-17518 and commented

Javadoc for HandlerMapping.getHandler() says it will return null if no mapping found but RequestMappingHandlerMapping.getHandler() throws HttpRequestMethodNotSupportedException instead.

 

There's a sample project to reproduce it in the "Reference URL".

---

Affects: 4.3.20

Reference URL: spring-projects/spring-boot#15204

Referenced from: commits 736b3c4, a55ca56, abacc6d

Backported to: 5.0.11, 4.3.21

[spring-projects-issues]

Rossen Stoyanchev commented

The HttpRequestMethodNotSupportedException, along with other possible similar exceptions, is raised when there is a partial match. Basically the idea is that when the URL path matches, the RequestMappingHandlerMapping considers it a partial match, and instead of returning null, it raises an exception that is then translated into the appropriate status code, in this case 405.

[spring-projects-issues]

Andy Wilkinson commented

If the behaviour cannot be changed to align with the documented contract, can the javadoc please be updated? It would also be useful if it was possible for callers to distinguish between exceptions that are internal errors and exceptions that are being used as a way of affecting the response status.

[spring-projects-issues]

Rossen Stoyanchev commented

RequestMappinghandlerMapping sees this as a match by URL pattern, and in that sense it "feels" that it owns the handling of it, and from there it just happens to reject specific values of the request in attempt to be helpful to the client. To put it differently, there is no expectation of split-handling for the same URL path across multiple handler mappings.

In practice I know split handling of a URL can sometimes occur. For example Spring Data REST is careful to use a different stereotype annotation. We could consider some improvement to the assumptions that RequestMappingHandlerMapping makes, or how the contract is defined, but for that it would be useful to precise the needs. I'm not sure I understand the scenario very well.

In any case adding an extra comment in the contract would be no problem. As for differentiating exceptions, I don't really have a good answer other than it's the ones handled in DefaultHandlerExceptionResolver. Suppose you could differentiate in some way, it would help to know, how would you actually make use of it?

[spring-projects-issues]

Andy Wilkinson commented

There's a bit of background in the referenced Boot issue and the Micrometer issue to which it links. Both Boot and Micrometer use HandlerMappingIntrospector.getMatchableHandlerMapping(HttpServletRequest) to determine the handler for an inbound request. This can throw an HttpRequestMethodNotSupportedException exception for the reasons discussed here. I'm not totally sure yet exactly how it will pan out, but it feels like it would be useful in the metrics filter to distinguish between an exception that's expected and an exception that's an internal error. For exceptions that are expected, like HttpRequestMethodNotSupportedException, I don't think the filter should log anything, even at debug or trace level, whereas for those that are unexpected, retaining the existing debug logging may have some merit.

[spring-projects-issues]

Rossen Stoyanchev commented

As evident from MatchableHandlerMapping and the RequestMatchResult it returns, the HandlerMappingIntrospector was designed specifically to allow Spring Security to help align its URL authorization with Spring MVC path matching options, which could vary by handler mapping. It was a compromise solution for a problem with hard consequences (security bypass). It was never intended as a way for filters to casually look up the handler, which is not what Spring Security does either. 

Performance implications aside, looking up a handler outside the DispatcherServlet processing lifecycle is not something I want to endorse formally with all the cases and issues that may arise from it. In that sense I see any code that decides to do this on its own in terms of the consequences resulting from the decision. 

For Micrometer specifically, what makes it a requirement to look up the handler before, as opposed to after the filter chain, for example by getting it from a request attribute? On the WebFlux side we have such an attribute HandlerMapping.BEST_MATCHING_HANDLER_ATTRIBUTE based on #20123 with metrics and tracing in mind. We could add the same to Spring MVC side. Jon Schneider would that help to avoid the whole looking up of the handler up front?

 

[spring-projects-issues]

Andy Wilkinson commented

I'm a bit hazy on the specifics as the code was developed in Micrometer before it moved into Boot and I'm guilty of not giving it the scrutiny it deserved when it moved over. That said, I believe that the handler is needed to support @Timed on request-mapped methods. The handler is needed to find the annotation and apply its configuration when recording the metric for the request. I had thought that the timer was only started if @Timed was present or all requests were being timed, however that's not the case. Given that, I think an attribute for the handler would be very useful. Hopefully Jon can confirm, but it looks to me like the timer could always be started as it is now and then @Timed could be looked up from the handler via the request attribute once the dispatcher servlet has handled the request.

[spring-projects-issues]

Rossen Stoyanchev commented

Okay I'll go ahead and add the request attribute for now, providing parity with Webflux. How far back do you need it backported to? It should be free of side effects, but nevertheless..

[spring-projects-issues]

Rossen Stoyanchev commented

Now available in master with #a55ca5.

[spring-projects-issues]

Andy Wilkinson commented

Thanks, Rossen. For Boot it would be useful to have the change in Framework 5.0 in addition to 5.1. For Micrometer, it may be useful to have it in 4.3 as well for use in their Spring Boot 1.5 integration.