Upgrade from 2.5.7 to 2.6.x breaks use of RequestMappingHandlerMapping in Filters

[william00179]

Hi,

Working version: 2.5.7
Broken version: 2.6.x

I have a Filter that will check if the matched handler method has a particular annotation and wrap the servlet response if that is the case.

In 2.6.x requestMappingHandlerMapping.getHandler(httpServletRequest) throws

java.lang.IllegalArgumentException: Expected parsed RequestPath in request attribute "org.springframework.web.util.ServletRequestPathUtils.PATH".
	at org.springframework.util.Assert.notNull(Assert.java:201) ~[spring-core-5.3.13.jar!/:5.3.13]
	at org.springframework.web.util.ServletRequestPathUtils.getParsedRequestPath(ServletRequestPathUtils.java:77) ~[spring-web-5.3.13.jar!/:5.3.13]
	at org.springframework.web.servlet.handler.AbstractHandlerMapping.initLookupPath(AbstractHandlerMapping.java:574) ~[spring-webmvc-5.3.13.jar!/:5.3.13]
 	at org.springframework.web.servlet.handler.AbstractHandlerMethodMapping.getHandlerInternal(AbstractHandlerMethodMapping.java:380) ~[spring-webmvc-5.3.13.jar!/:5.3.13]
	at org.springframework.web.servlet.mvc.method.RequestMappingInfoHandlerMapping.getHandlerInternal(RequestMappingInfoHandlerMapping.java:125) ~[spring-webmvc-5.3.13.jar!/:5.3.13]
 	at org.springframework.web.servlet.mvc.method.RequestMappingInfoHandlerMapping.getHandlerInternal(RequestMappingInfoHandlerMapping.java:67) ~[spring-webmvc-5.3.13.jar!/:5.3.13]
	at org.springframework.web.servlet.handler.AbstractHandlerMapping.getHandler(AbstractHandlerMapping.java:498) ~[spring-webmvc-5.3.13.jar!/:5.3.13]

A minimal example of how we are using this code

@Component
@ConditionalOnWebApplication
public class DataFilter implements Filter {

 @Autowired
    private RequestMappingHandlerMapping requestMappingHandlerMapping;
    
    @Override
    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException {

        HttpServletRequest httpServletRequest = (HttpServletRequest) request;
        HttpServletResponse httpServletResponse = (HttpServletResponse) response;
        try {
            Optional<HandlerExecutionChain> handlerExecutionChain = Optional.ofNullable(requestMappingHandlerMapping.getHandler(httpServletRequest));
            if (handlerExecutionChain.isPresent()) {
                HandlerMethod handlerMethod = (HandlerMethod) handlerExecutionChain.get().getHandler();
                if (handlerMethod.getMethod().isAnnotationPresent(MonitorData.class)) {
                    //wrap the response
                    chain.doFilter(request, wrappedResponse);
                    }
       ...
                   

I that the default matching strategy has been changed in #24805, but I've been unable to resolve this issue in my codebase.

[william00179]

Rolling back to the old behaviour by setting spring.mvc.pathmatch.matching-strategy=ANT_PATH_MATCHER works around this issue in the mean time.

[zifnab87]

Rolling back to the old behaviour by setting spring.mvc.pathmatch.matching-strategy=ANT_PATH_MATCHER works around this issue in the mean time.

This indeed is a workaround to the problem but using rest-assured with those endpoints that involve filters is still failing the tests even with this setting in my application-test.yaml .

The issue is replicated in this simple example

@Component
public class ChatBotAuthenticationFilter extends OncePerRequestFilter {

    private final RequestMappingHandlerMapping reqMap;

    public ChatBotAuthenticationFilter(RequestMappingHandlerMapping reqMap) {
        this.reqMap = reqMap;
    }

    @Override
    protected void doFilterInternal(HttpServletRequest req, HttpServletResponse res, FilterChain chain) throws IOException, ServletException {
        boolean isChatBotOnly = isEndpointChatBotOnly(req);
        //[...] custom logic
        chain.doFilter(req, res);
    }

    private boolean isEndpointChatBotOnly(HttpServletRequest req) {
        HandlerMethod method;
        try {
            // this call of reqMap.getHandler(req) throws the aforementioned exception
            method = (HandlerMethod) reqMap.getHandler(req).getHandler();
        } catch (Exception e) {
            return false;
        }

        return method.getMethod().isAnnotationPresent(ChatbotOnlyEndpoint.class);
    }
}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface ChatbotOnlyEndpoint {
}

Here I am annotating some controller endpoints with @ChatbotOnlyEndpoint to enforce some kind of authorization filtering based on the headers that are being submitted (tokens/ids etc)

[bclozel]

This path matching strategy has been available since Spring Framework 5.3 and Spring Boot 2.4 (as an option first, in #21694). This strategy will also become the new default in Spring Framework.

The problem here is that those custom Servlet filters are leveraging the RequestMappingHandlerMapping without adapting to the changes made in Spring Framework 5.3.

To avoid parsing the request path (for efficient matching) multiple times per request, this is done once and cached as a request attribute. At the time those filters process the request, this hasn't been done yet. This is usually done by the DispatcherServlet itself or by a ServletRequestPathFilter, as explained in AbsractHandlerMapping's javadoc.

I'd suggest to ways to fix the filter implementation:

• check if the new path matching is configured with org.springframework.web.servlet.handler.AbstractHandlerMapping#getPatternParser and manually populate the cached request path with org.springframework.web.util.ServletRequestPathUtils#parseAndCache in your filter
• or simply declare a ServletRequestPathFilter ahead of your filter

This issue is really about adapting custom code to the new path matching - changing the default in Spring Boot merely brought an existing problem to your attention. I'm closing this issue as a result. Thanks!

[bclozel]

As an additional comment, I'd like to add that RequestMappingHandlerMapping is really a strategy for the DispatcherServlet, so using it outside of that scope is not supported and considered as a bad practice. The solution I've provided in my previous comment is really a workaround and further changes should be considered in custom filters doing this.

Without knowing more about the use case, it's hard to point to alternatives. Maybe a org.springframework.web.servlet.HandlerInterceptor would be a better choice? In the interest of keeping questions outside of the Spring Boot issue tracker, feel free to ask questions on StackOverflow and report the link back here.

[zifnab87]

@bclozel Thanks for your suggestions! I tried the first suggestion and this is what I changed:

I added these lines

     if (!ServletRequestPathUtils.hasParsedRequestPath(req)) {
          ServletRequestPathUtils.parseAndCache(req);
     }

just before method = (HandlerMethod) reqMap.getHandler(req).getHandler();

and it fixed my problem - is this the correct way to fix the issue or this could introduce performance problems?

Do I really need to check ServletRequestPathUtils.hasParsedRequestPath(req) beforehand?

Thanks in advance!

@Component
public class CustomAuthenticationFilter extends OncePerRequestFilter {

    private final RequestMappingHandlerMapping reqMap;

    public ChatBotAuthenticationFilter(RequestMappingHandlerMapping reqMap) {
        this.reqMap = reqMap;
    }

    @Override
    protected void doFilterInternal(HttpServletRequest req, HttpServletResponse res, FilterChain chain) throws IOException, ServletException {
        boolean isChatBotOnly = isEndpointCustomAuthenticationOnly(req);
        //[...] custom logic
        chain.doFilter(req, res);
    }

    private boolean isEndpointCustomAuthenticationOnly(HttpServletRequest req) {
        HandlerMethod method;
        try {
             if (!ServletRequestPathUtils.hasParsedRequestPath(req)) {
                ServletRequestPathUtils.parseAndCache(req);
            }
            method = (HandlerMethod) reqMap.getHandler(req).getHandler();
        } catch (Exception e) {
            return false;
        }

        return method.getMethod().isAnnotationPresent(CustomAuthenticationEndpoint.class);
    }
}

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.METHOD)
public @interface CustomAuthenticationEndpoint {
}

[bclozel]

Sorry for the late reply @zifnab87
This looks ok to me. The performance problem here is really about performing the matching process twice (once in this filter, and another time for the actual handling). Looking up/setting a request attribute is really cheap compared to that.