Prevent duplicate controller registrations through class-level @RequestMapping.

When we detected @BasePathAwareController and @RepositoryRestController instances, we now reject types that use @RequestMapping on the class level as doing so causes an inevitable registration of the controller with Spring MVC.

Fixes #1342, #1628, #1686, #1946.

Author: odrotbohm

spring-data-rest-tests/spring-data-rest-tests-jpa/src/test/java/org/springframework/data/rest/webmvc/jpa/JpaRepositoryConfig.java

@@ -59,10 +59,9 @@ void someMethod(@PathVariable String id) {}
 	}
 
 	@RepositoryRestController
-	@RequestMapping("orders")
 	static class OrdersJsonController {
 
-		@RequestMapping(value = "/search/sort", method = RequestMethod.POST, produces = "application/hal+json")
+		@RequestMapping(value = "/orders/search/sort", method = RequestMethod.POST, produces = "application/hal+json")
 		void someMethodWithArgs(Sort sort, Pageable pageable, DefaultedPageable defaultedPageable) {}
 	}
 }

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/BasePathAwareHandlerMapping.java

@@ -28,12 +28,14 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
 
+import org.springframework.core.annotation.AnnotatedElementUtils;
 import org.springframework.data.rest.core.config.RepositoryRestConfiguration;
 import org.springframework.data.util.ProxyUtils;
 import org.springframework.http.HttpHeaders;
 import org.springframework.http.MediaType;
 import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
+import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.method.HandlerMethod;
 import org.springframework.web.servlet.mvc.condition.ProducesRequestCondition;
 import org.springframework.web.servlet.mvc.method.RequestMappingInfo;
@@ -46,6 +48,7 @@
  */
 public class BasePathAwareHandlerMapping extends RequestMappingHandlerMapping {
 
+	private static final String AT_REQUEST_MAPPING_ON_TYPE = "Spring Data REST controller %s must not use @RequestMapping on class level as this would cause double registration with Spring MVC!";
 	private final RepositoryRestConfiguration configuration;
 
 	/**
@@ -140,15 +143,38 @@ protected ProducesRequestCondition customize(ProducesRequestCondition condition)
 		return condition;
 	}
 
-	/*
-	 * (non-Javadoc)
+	/**
+	 * Returns whether the given type is considered a handler. Performs additional configuration checks. If you only want
+	 * to customize the handler selection criteria, override {@link #isHandlerInternal(Class)}. Will be made final in 4.0.
+	 *
 	 * @see org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping#isHandler(java.lang.Class)
+	 * @deprecated for overriding in 3.6. Will be made final in 4.0.
 	 */
 	@Override
+	@Deprecated
 	protected boolean isHandler(Class<?> beanType) {
 
 		Class<?> type = ProxyUtils.getUserClass(beanType);
+		boolean isSpringDataRestHandler = isHandlerInternal(type);
+
+		if (!isSpringDataRestHandler) {
+			return false;
+		}
+
+		if (AnnotatedElementUtils.hasAnnotation(type, RequestMapping.class)) {
+			throw new IllegalStateException(String.format(AT_REQUEST_MAPPING_ON_TYPE, beanType.getName()));
+		}
+
+		return isSpringDataRestHandler;
+	}
 
+	/**
+	 * Returns whether the given controller type is considered a handler.
+	 *
+	 * @param type will never be {@literal null}.
+	 * @return
+	 */
+	protected boolean isHandlerInternal(Class<?> type) {
 		return type.isAnnotationPresent(BasePathAwareController.class);
 	}
 

spring-data-rest-webmvc/src/main/java/org/springframework/data/rest/webmvc/RepositoryRestHandlerMapping.java

@@ -25,14 +25,12 @@
 import javax.servlet.http.HttpServletRequest;
 
 import org.springframework.core.annotation.AnnotatedElementUtils;
-import org.springframework.core.annotation.AnnotationUtils;
 import org.springframework.data.repository.support.Repositories;
 import org.springframework.data.rest.core.config.RepositoryRestConfiguration;
 import org.springframework.data.rest.core.mapping.HttpMethods;
 import org.springframework.data.rest.core.mapping.ResourceMappings;
 import org.springframework.data.rest.core.mapping.ResourceMetadata;
 import org.springframework.data.rest.webmvc.support.JpaHelper;
-import org.springframework.data.util.ProxyUtils;
 import org.springframework.data.util.Streamable;
 import org.springframework.hateoas.MediaTypes;
 import org.springframework.http.HttpMethod;
@@ -182,14 +180,11 @@ protected HandlerMethod handleNoMatch(Set<RequestMappingInfo> requestMappingInfo
 
 	/*
 	 * (non-Javadoc)
-	 * @see org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping#isHandler(java.lang.Class)
+	 * @see org.springframework.data.rest.webmvc.BasePathAwareHandlerMapping#isHandlerInternal(java.lang.Class)
 	 */
 	@Override
-	protected boolean isHandler(Class<?> beanType) {
-
-		Class<?> type = ProxyUtils.getUserClass(beanType);
-
-		return AnnotationUtils.findAnnotation(type, RepositoryRestController.class) != null;
+	protected boolean isHandlerInternal(Class<?> type) {
+		return AnnotatedElementUtils.hasAnnotation(type, RepositoryRestController.class);
 	}
 
 	/*

spring-data-rest-webmvc/src/test/java/org/springframework/data/rest/webmvc/BasePathAwareHandlerMappingUnitTests.java

@@ -25,6 +25,8 @@
 import org.springframework.aop.framework.ProxyFactory;
 import org.springframework.aop.support.AopUtils;
 import org.springframework.data.rest.core.config.RepositoryRestConfiguration;
+import org.springframework.stereotype.Controller;
+import org.springframework.web.bind.annotation.RequestMapping;
 
 /**
  * Unit tests for {@link BasePathAwareHandlerMapping}.
@@ -60,6 +62,23 @@ public void doesNotConsiderMetaAnnotation() {
 		assertThat(mapping.isHandler(type)).isFalse();
 	}
 
+	@Test // #1342, #1628, #1686, #1946
+	public void rejectsAtRequestMappingOnCustomController() {
+
+		assertThatIllegalStateException()
+				.isThrownBy(() -> {
+					mapping.isHandler(InvalidController.class);
+				})
+				.withMessageContaining(InvalidController.class.getName());
+	}
+
+	@Test // #1342, #1628, #1686, #1946
+	public void doesNotRejectAtRequestMappingOnStandardMvcController() {
+
+		assertThatNoException()
+				.isThrownBy(() -> mapping.isHandler(ValidController.class));
+	}
+
 	private static Class<?> createProxy(Object source) {
 
 		ProxyFactory factory = new ProxyFactory(source);
@@ -87,4 +106,12 @@ public boolean isHandler(Class<?> beanType) {
 			return super.isHandler(beanType);
 		}
 	}
+
+	@BasePathAwareController
+	@RequestMapping("/sample")
+	static class InvalidController {}
+
+	@Controller
+	@RequestMapping("/sample")
+	static class ValidController {}
 }