Log warning if @RequestMapping method has no explicit mapping

Commit c0b52d09f5 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

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 <T> 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<T> extends AbstractHandlerMapping implements InitializingBean {
 
@@ -207,8 +209,8 @@ public abstract class AbstractHandlerMethodMapping<T> 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<T> 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<String> 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<T> 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<T> 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.");
 			}
 		}
 

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<String> 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

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<String> 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() {
 		}
 	}
+
 }

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<String> getMappingPathPatterns(RequestMappingInfo info) {
+			return info.getPatternsCondition().getPatterns().stream().map(PathPattern::getPatternString).collect(Collectors.toSet());
+		}
 	}
 
 }

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 <T> 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<T> extends AbstractHandlerMapping implements InitializingBean {
 
@@ -496,7 +496,7 @@ public abstract class AbstractHandlerMethodMapping<T> 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<String> getMappingPathPatterns(T mapping);
 
@@ -616,11 +616,11 @@ public abstract class AbstractHandlerMethodMapping<T> 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()));
 			}

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<String> getMappingPathPatterns(RequestMappingInfo info) {

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");
 		}
 	}