From 72027b1746c5dfd3f6e35b57e522b78290a4a4f0 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Fri, 5 Apr 2019 19:48:30 +0200 Subject: [PATCH] Log warning if @RequestMapping method has no explicit mapping Commit c0b52d09f57fcabd75e664b7994dea12ba28f408 introduced support for throwing an exception if a @RequestMapping handler method in a Spring MVC controller was mapped to an empty path. This had negative side effects for applications that intentionally mapped to an empty path, potentially alongside a mapping to an explicit path for the same handler method. This commit addresses this by logging a warning (instead of throwing an exception) if a @RequestMapping method is mapped only to empty paths. This commit also introduces the same support for WebFlux-based @RequestMapping handler methods. Closes gh-22543 --- .../method/AbstractHandlerMethodMapping.java | 39 ++++++++++++----- .../RequestMappingHandlerMapping.java | 15 ++++++- .../method/HandlerMethodMappingTests.java | 11 ++++- ...RequestMappingInfoHandlerMappingTests.java | 10 ++++- .../handler/AbstractHandlerMethodMapping.java | 14 +++---- .../RequestMappingInfoHandlerMapping.java | 2 +- ...nnotationControllerHandlerMethodTests.java | 42 +++++++++++++++---- 7 files changed, 104 insertions(+), 29 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java index a4c02f75f09..cd16ed962b3 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java @@ -41,6 +41,7 @@ import org.springframework.http.server.RequestPath; import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.StringUtils; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.cors.reactive.CorsUtils; import org.springframework.web.method.HandlerMethod; @@ -57,9 +58,10 @@ import org.springframework.web.server.ServerWebExchange; * * @author Rossen Stoyanchev * @author Brian Clozel + * @author Sam Brannen * @since 5.0 * @param the mapping for a {@link HandlerMethod} containing the conditions - * needed to match the handler method to incoming request. + * needed to match the handler method to an incoming request. */ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMapping implements InitializingBean { @@ -207,8 +209,8 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap if (logger.isTraceEnabled()) { logger.trace(formatMappings(userType, methods)); } - methods.forEach((key, mapping) -> { - Method invocableMethod = AopUtils.selectInvocableMethod(key, userType); + methods.forEach((method, mapping) -> { + Method invocableMethod = AopUtils.selectInvocableMethod(method, userType); registerHandlerMethod(handler, invocableMethod, mapping); }); } @@ -412,6 +414,12 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap @Nullable protected abstract T getMappingForMethod(Method method, Class handlerType); + /** + * Extract and return the URL paths contained in the supplied mapping. + * @since 5.2 + */ + protected abstract Set getMappingPathPatterns(T mapping); + /** * Check if a mapping matches the current request and return a (potentially * new) mapping with conditions relevant to the current request. @@ -482,8 +490,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap this.readWriteLock.writeLock().lock(); try { HandlerMethod handlerMethod = createHandlerMethod(handler, method); - assertUniqueMethodMapping(handlerMethod, mapping); - + validateMethodMapping(handlerMethod, mapping); this.mappingLookup.put(mapping, handlerMethod); CorsConfiguration corsConfig = initCorsConfiguration(handler, method, mapping); @@ -498,13 +505,23 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } } - private void assertUniqueMethodMapping(HandlerMethod newHandlerMethod, T mapping) { - HandlerMethod handlerMethod = this.mappingLookup.get(mapping); - if (handlerMethod != null && !handlerMethod.equals(newHandlerMethod)) { + private void validateMethodMapping(HandlerMethod handlerMethod, T mapping) { + // Log a warning if the supplied mapping maps the supplied HandlerMethod + // only to empty paths. + if (logger.isWarnEnabled() && getMappingPathPatterns(mapping).stream().noneMatch(StringUtils::hasText)) { + logger.warn(String.format( + "Handler method '%s' in bean '%s' is not mapped to an explicit path. " + + "If you wish to map to all paths, please map explicitly to \"/**\" or \"**\".", + handlerMethod, handlerMethod.getBean())); + } + + // Assert that the supplied mapping is unique. + HandlerMethod existingHandlerMethod = this.mappingLookup.get(mapping); + if (existingHandlerMethod != null && !existingHandlerMethod.equals(handlerMethod)) { throw new IllegalStateException( - "Ambiguous mapping. Cannot map '" + newHandlerMethod.getBean() + "' method \n" + - newHandlerMethod + "\nto " + mapping + ": There is already '" + - handlerMethod.getBean() + "' bean method\n" + handlerMethod + " mapped."); + "Ambiguous mapping. Cannot map '" + handlerMethod.getBean() + "' method \n" + + handlerMethod + "\nto " + mapping + ": There is already '" + + existingHandlerMethod.getBean() + "' bean method\n" + existingHandlerMethod + " mapped."); } } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java index 40fca4b8ffc..ef6d4435bdb 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/RequestMappingHandlerMapping.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -21,7 +21,9 @@ import java.lang.reflect.Method; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; +import java.util.Set; import java.util.function.Predicate; +import java.util.stream.Collectors; import org.springframework.context.EmbeddedValueResolverAware; import org.springframework.core.annotation.AnnotatedElementUtils; @@ -41,6 +43,7 @@ import org.springframework.web.reactive.accept.RequestedContentTypeResolverBuild import org.springframework.web.reactive.result.condition.RequestCondition; import org.springframework.web.reactive.result.method.RequestMappingInfo; import org.springframework.web.reactive.result.method.RequestMappingInfoHandlerMapping; +import org.springframework.web.util.pattern.PathPattern; /** * An extension of {@link RequestMappingInfoHandlerMapping} that creates @@ -48,6 +51,7 @@ import org.springframework.web.reactive.result.method.RequestMappingInfoHandlerM * {@link RequestMapping @RequestMapping} annotations. * * @author Rossen Stoyanchev + * @author Sam Brannen * @since 5.0 */ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMapping @@ -161,6 +165,15 @@ public class RequestMappingHandlerMapping extends RequestMappingInfoHandlerMappi return info; } + /** + * Get the URL path patterns associated with the supplied {@link RequestMappingInfo}. + * @since 5.2 + */ + @Override + protected Set getMappingPathPatterns(RequestMappingInfo info) { + return info.getPatternsCondition().getPatterns().stream().map(PathPattern::getPatternString).collect(Collectors.toSet()); + } + /** * Delegates to {@link #createRequestMappingInfo(RequestMapping, RequestCondition)}, * supplying the appropriate custom {@link RequestCondition} depending on whether diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java index e9c4c21ad89..0f2cfa32150 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/HandlerMethodMappingTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,7 +17,9 @@ package org.springframework.web.reactive.result.method; import java.lang.reflect.Method; +import java.util.Collections; import java.util.Comparator; +import java.util.Set; import org.hamcrest.Matchers; import org.junit.Before; @@ -44,6 +46,7 @@ import static org.junit.Assert.assertThat; * Unit tests for {@link AbstractHandlerMethodMapping}. * * @author Rossen Stoyanchev + * @author Sam Brannen */ public class HandlerMethodMappingTests { @@ -155,6 +158,11 @@ public class HandlerMethodMappingTests { return methodName.startsWith("handler") ? methodName : null; } + @Override + protected Set getMappingPathPatterns(String mapping) { + return Collections.emptySet(); + } + @Override protected String getMatchingMapping(String pattern, ServerWebExchange exchange) { PathContainer lookupPath = exchange.getRequest().getPath().pathWithinApplication(); @@ -182,4 +190,5 @@ public class HandlerMethodMappingTests { public void handlerMethod2() { } } + } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java index 9120a300c4b..2d268956bec 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -26,6 +26,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.function.Consumer; +import java.util.stream.Collectors; import org.junit.Before; import org.junit.Test; @@ -69,7 +70,9 @@ import static org.springframework.web.reactive.result.method.RequestMappingInfo. /** * Unit tests for {@link RequestMappingInfoHandlerMapping}. + * * @author Rossen Stoyanchev + * @author Sam Brannen */ public class RequestMappingInfoHandlerMappingTests { @@ -475,6 +478,11 @@ public class RequestMappingInfoHandlerMappingTests { return null; } } + + @Override + protected Set getMappingPathPatterns(RequestMappingInfo info) { + return info.getPatternsCondition().getPatterns().stream().map(PathPattern::getPatternString).collect(Collectors.toSet()); + } } } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java index 0c93d8d7d6a..f75eb4c318c 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java @@ -62,7 +62,7 @@ import org.springframework.web.servlet.HandlerMapping; * @author Sam Brannen * @since 3.1 * @param the mapping for a {@link HandlerMethod} containing the conditions - * needed to match the handler method to incoming request. + * needed to match the handler method to an incoming request. */ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMapping implements InitializingBean { @@ -496,7 +496,7 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap protected abstract T getMappingForMethod(Method method, Class handlerType); /** - * Extract and return the URL paths contained in a mapping. + * Extract and return the URL paths contained in the supplied mapping. */ protected abstract Set getMappingPathPatterns(T mapping); @@ -616,11 +616,11 @@ public abstract class AbstractHandlerMethodMapping extends AbstractHandlerMap } private void validateMethodMapping(HandlerMethod handlerMethod, T mapping) { - // Assert that the supplied mapping maps the supplied HandlerMethod - // to explicit, non-empty paths. - if (!getMappingPathPatterns(mapping).stream().allMatch(StringUtils::hasText)) { - throw new IllegalStateException(String.format("Missing path mapping. " + - "Handler method '%s' in bean '%s' must be mapped to a non-empty path. " + + // Log a warning if the supplied mapping maps the supplied HandlerMethod + // only to empty paths. + if (logger.isWarnEnabled() && getMappingPathPatterns(mapping).stream().noneMatch(StringUtils::hasText)) { + logger.warn(String.format( + "Handler method '%s' in bean '%s' is not mapped to an explicit path. " + "If you wish to map to all paths, please map explicitly to \"/**\" or \"**\".", handlerMethod, handlerMethod.getBean())); } diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java index fecb47fe18b..8a65f5fe8bf 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfoHandlerMapping.java @@ -75,7 +75,7 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe /** - * Get the URL path patterns associated with this {@link RequestMappingInfo}. + * Get the URL path patterns associated with the supplied {@link RequestMappingInfo}. */ @Override protected Set getMappingPathPatterns(RequestMappingInfo info) { diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index 4cc37b9d188..6930a06442f 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java @@ -797,11 +797,28 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl } @Test - public void unmappedPathMapping() { - assertThatThrownBy(() -> initServletWithControllers(UnmappedPathController.class)) - .isInstanceOf(BeanCreationException.class) - .hasCauseInstanceOf(IllegalStateException.class) - .hasMessageContaining("Missing path mapping"); + public void unmappedPathMapping() throws Exception { + initServletWithControllers(UnmappedPathController.class); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/bogus-unmapped"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertEquals("get", response.getContentAsString()); + } + + @Test + public void explicitAndEmptyPathsControllerMapping() throws Exception { + initServletWithControllers(ExplicitAndEmptyPathsController.class); + + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/"); + MockHttpServletResponse response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertEquals("get", response.getContentAsString()); + + request = new MockHttpServletRequest("GET", ""); + response = new MockHttpServletResponse(); + getServlet().service(request, response); + assertEquals("get", response.getContentAsString()); } @Test @@ -2733,11 +2750,22 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl } @Controller - @RequestMapping // path intentionally omitted + // @RequestMapping intentionally omitted static class UnmappedPathController { @GetMapping // path intentionally omitted - public void get(@RequestParam(required = false) String id) { + public void get(Writer writer) throws IOException { + writer.write("get"); + } + } + + @Controller + // @RequestMapping intentionally omitted + static class ExplicitAndEmptyPathsController { + + @GetMapping({"/", ""}) + public void get(Writer writer) throws IOException { + writer.write("get"); } }