From e9d8152ab2b25ba6ec51fadf5206e7c82d7a23e3 Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Wed, 29 Jun 2016 17:34:45 -0400 Subject: [PATCH] Port fix for SPR-14397 This is a port of the following commit, adapted for Java 8+: https://github.com/spring-projects/spring-framework/commit/89396ff01ff159aa7df18e332f3888cf9ce3dc20 --- .../RequestMappingInfoHandlerMapping.java | 273 ++++++++++++------ ...RequestMappingInfoHandlerMappingTests.java | 74 ++--- 2 files changed, 214 insertions(+), 133 deletions(-) diff --git a/spring-web-reactive/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java b/spring-web-reactive/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java index 8712b1c69b9..3a9dbd36d89 100644 --- a/spring-web-reactive/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java +++ b/spring-web-reactive/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMapping.java @@ -20,7 +20,6 @@ import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; -import java.util.HashSet; import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; @@ -35,18 +34,16 @@ import org.springframework.http.HttpMethod; import org.springframework.http.InvalidMediaTypeException; import org.springframework.http.MediaType; import org.springframework.http.server.reactive.ServerHttpRequest; -import org.springframework.util.CollectionUtils; import org.springframework.util.LinkedMultiValueMap; import org.springframework.util.MultiValueMap; import org.springframework.util.StringUtils; -import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.method.HandlerMethod; import org.springframework.web.reactive.HandlerMapping; -import org.springframework.web.reactive.result.condition.ParamsRequestCondition; -import org.springframework.web.server.ServerWebInputException; +import org.springframework.web.reactive.result.condition.NameValueExpression; import org.springframework.web.server.MethodNotAllowedException; import org.springframework.web.server.NotAcceptableStatusException; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.ServerWebInputException; import org.springframework.web.server.UnsupportedMediaTypeStatusException; /** @@ -207,59 +204,29 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe * method but not by query parameter conditions */ @Override - protected HandlerMethod handleNoMatch(Set requestMappingInfos, - String lookupPath, ServerWebExchange exchange) throws Exception { + protected HandlerMethod handleNoMatch(Set infos, String lookupPath, + ServerWebExchange exchange) throws Exception { - Set allowedMethods = new LinkedHashSet<>(4); + PartialMatchHelper helper = new PartialMatchHelper(infos, exchange); - Set patternMatches = new HashSet<>(); - Set patternAndMethodMatches = new HashSet<>(); - - for (RequestMappingInfo info : requestMappingInfos) { - if (info.getPatternsCondition().getMatchingCondition(exchange) != null) { - patternMatches.add(info); - if (info.getMethodsCondition().getMatchingCondition(exchange) != null) { - patternAndMethodMatches.add(info); - } - else { - for (RequestMethod method : info.getMethodsCondition().getMethods()) { - allowedMethods.add(method.name()); - } - } - } + if (helper.isEmpty()) { + return null; } ServerHttpRequest request = exchange.getRequest(); - if (patternMatches.isEmpty()) { - return null; - } - else if (patternAndMethodMatches.isEmpty()) { + + if (helper.hasMethodsMismatch()) { HttpMethod httpMethod = request.getMethod(); + Set methods = helper.getAllowedMethods(); if (HttpMethod.OPTIONS.matches(httpMethod.name())) { - HttpOptionsHandler handler = new HttpOptionsHandler(allowedMethods); + HttpOptionsHandler handler = new HttpOptionsHandler(methods); return new HandlerMethod(handler, HTTP_OPTIONS_HANDLE_METHOD); } - else if (!allowedMethods.isEmpty()) { - throw new MethodNotAllowedException(httpMethod.name(), allowedMethods); - } + throw new MethodNotAllowedException(httpMethod.name(), methods); } - Set consumableMediaTypes; - Set producibleMediaTypes; - List> paramConditions; - - if (patternAndMethodMatches.isEmpty()) { - consumableMediaTypes = getConsumableMediaTypes(exchange, patternMatches); - producibleMediaTypes = getProducibleMediaTypes(exchange, patternMatches); - paramConditions = getRequestParams(exchange, patternMatches); - } - else { - consumableMediaTypes = getConsumableMediaTypes(exchange, patternAndMethodMatches); - producibleMediaTypes = getProducibleMediaTypes(exchange, patternAndMethodMatches); - paramConditions = getRequestParams(exchange, patternAndMethodMatches); - } - - if (!consumableMediaTypes.isEmpty()) { + if (helper.hasConsumesMismatch()) { + Set mediaTypes = helper.getConsumableMediaTypes(); MediaType contentType; try { contentType = request.getHeaders().getContentType(); @@ -267,62 +234,176 @@ public abstract class RequestMappingInfoHandlerMapping extends AbstractHandlerMe catch (InvalidMediaTypeException ex) { throw new UnsupportedMediaTypeStatusException(ex.getMessage()); } - throw new UnsupportedMediaTypeStatusException(contentType, new ArrayList<>(consumableMediaTypes)); + throw new UnsupportedMediaTypeStatusException(contentType, new ArrayList<>(mediaTypes)); } - else if (!producibleMediaTypes.isEmpty()) { - throw new NotAcceptableStatusException(new ArrayList<>(producibleMediaTypes)); + + if (helper.hasProducesMismatch()) { + Set mediaTypes = helper.getProducibleMediaTypes(); + throw new NotAcceptableStatusException(new ArrayList<>(mediaTypes)); } - else { - if (!CollectionUtils.isEmpty(paramConditions)) { - Map params = request.getQueryParams().entrySet().stream() - .collect(Collectors.toMap(Entry::getKey, - entry -> entry.getValue().toArray(new String[entry.getValue().size()])) - ); - throw new ServerWebInputException("Unsatisfied query parameter conditions: " + - paramConditions + ", actual parameters: " + params); + + if (helper.hasParamsMismatch()) { + throw new ServerWebInputException( + "Unsatisfied query parameter conditions: " + helper.getParamConditions() + + ", actual parameters: " + request.getQueryParams()); + } + + return null; + } + + + /** + * Aggregate all partial matches and expose methods checking across them. + */ + private static class PartialMatchHelper { + + private final List partialMatches = new ArrayList<>(); + + + public PartialMatchHelper(Set infos, ServerWebExchange exchange) { + this.partialMatches.addAll(infos.stream(). + filter(info -> info.getPatternsCondition().getMatchingCondition(exchange) != null). + map(info -> new PartialMatch(info, exchange)). + collect(Collectors.toList())); + } + + + /** + * Whether there any partial matches. + */ + public boolean isEmpty() { + return this.partialMatches.isEmpty(); + } + + /** + * Any partial matches for "methods"? + */ + public boolean hasMethodsMismatch() { + return !this.partialMatches.stream(). + filter(PartialMatch::hasMethodsMatch).findAny().isPresent(); + } + + /** + * Any partial matches for "methods" and "consumes"? + */ + public boolean hasConsumesMismatch() { + return !this.partialMatches.stream(). + filter(PartialMatch::hasConsumesMatch).findAny().isPresent(); + } + + /** + * Any partial matches for "methods", "consumes", and "produces"? + */ + public boolean hasProducesMismatch() { + return !this.partialMatches.stream(). + filter(PartialMatch::hasProducesMatch).findAny().isPresent(); + } + + /** + * Any partial matches for "methods", "consumes", "produces", and "params"? + */ + public boolean hasParamsMismatch() { + return !this.partialMatches.stream(). + filter(PartialMatch::hasParamsMatch).findAny().isPresent(); + } + + /** + * Return declared HTTP methods. + */ + public Set getAllowedMethods() { + return this.partialMatches.stream(). + flatMap(m -> m.getInfo().getMethodsCondition().getMethods().stream()). + map(Enum::name). + collect(Collectors.toCollection(LinkedHashSet::new)); + } + + /** + * Return declared "consumable" types but only among those that also + * match the "methods" condition. + */ + public Set getConsumableMediaTypes() { + return this.partialMatches.stream().filter(PartialMatch::hasMethodsMatch). + flatMap(m -> m.getInfo().getConsumesCondition().getConsumableMediaTypes().stream()). + collect(Collectors.toCollection(LinkedHashSet::new)); + } + + /** + * Return declared "producible" types but only among those that also + * match the "methods" and "consumes" conditions. + */ + public Set getProducibleMediaTypes() { + return this.partialMatches.stream().filter(PartialMatch::hasConsumesMatch). + flatMap(m -> m.getInfo().getProducesCondition().getProducibleMediaTypes().stream()). + collect(Collectors.toCollection(LinkedHashSet::new)); + } + + /** + * Return declared "params" conditions but only among those that also + * match the "methods", "consumes", and "params" conditions. + */ + public List>> getParamConditions() { + return this.partialMatches.stream().filter(PartialMatch::hasProducesMatch). + map(match -> match.getInfo().getParamsCondition().getExpressions()). + collect(Collectors.toList()); + } + + + /** + * Container for a RequestMappingInfo that matches the URL path at least. + */ + private static class PartialMatch { + + private final RequestMappingInfo info; + + private final boolean methodsMatch; + + private final boolean consumesMatch; + + private final boolean producesMatch; + + private final boolean paramsMatch; + + + /** + * @param info RequestMappingInfo that matches the URL path + * @param exchange the current exchange + */ + public PartialMatch(RequestMappingInfo info, ServerWebExchange exchange) { + this.info = info; + this.methodsMatch = info.getMethodsCondition().getMatchingCondition(exchange) != null; + this.consumesMatch = info.getConsumesCondition().getMatchingCondition(exchange) != null; + this.producesMatch = info.getProducesCondition().getMatchingCondition(exchange) != null; + this.paramsMatch = info.getParamsCondition().getMatchingCondition(exchange) != null; } - else { - return null; + + + public RequestMappingInfo getInfo() { + return this.info; + } + + public boolean hasMethodsMatch() { + return this.methodsMatch; + } + + public boolean hasConsumesMatch() { + return hasMethodsMatch() && this.consumesMatch; + } + + public boolean hasProducesMatch() { + return hasConsumesMatch() && this.producesMatch; + } + + public boolean hasParamsMatch() { + return hasProducesMatch() && this.paramsMatch; + } + + @Override + public String toString() { + return this.info.toString(); } } } - private Set getConsumableMediaTypes(ServerWebExchange exchange, - Set partialMatches) { - - Set result = new HashSet<>(); - for (RequestMappingInfo partialMatch : partialMatches) { - if (partialMatch.getConsumesCondition().getMatchingCondition(exchange) == null) { - result.addAll(partialMatch.getConsumesCondition().getConsumableMediaTypes()); - } - } - return result; - } - - private Set getProducibleMediaTypes(ServerWebExchange exchange, - Set partialMatches) { - - Set result = new HashSet<>(); - for (RequestMappingInfo partialMatch : partialMatches) { - if (partialMatch.getProducesCondition().getMatchingCondition(exchange) == null) { - result.addAll(partialMatch.getProducesCondition().getProducibleMediaTypes()); - } - } - return result; - } - - private List> getRequestParams(ServerWebExchange exchange, - Set partialMatches) { - - return partialMatches.stream() - .map(RequestMappingInfo::getParamsCondition) - .filter(condition -> condition.getMatchingCondition(exchange) == null) - .map(ParamsRequestCondition::getExpressions) - .map(expressions -> expressions.stream().map(Object::toString).collect(Collectors.toList())) - .collect(Collectors.toList()); - } - - /** * Default handler for HTTP OPTIONS. */ diff --git a/spring-web-reactive/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java b/spring-web-reactive/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java index 1fe69c50fb4..00042ec69f8 100644 --- a/spring-web-reactive/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java +++ b/spring-web-reactive/src/test/java/org/springframework/web/reactive/result/method/RequestMappingInfoHandlerMappingTests.java @@ -32,6 +32,7 @@ import org.junit.Test; import reactor.core.publisher.Mono; import reactor.core.test.TestSubscriber; +import org.springframework.core.annotation.AnnotatedElementUtils; import org.springframework.core.annotation.AnnotationUtils; import org.springframework.http.HttpHeaders; import org.springframework.http.HttpMethod; @@ -43,6 +44,8 @@ import org.springframework.stereotype.Controller; import org.springframework.ui.ExtendedModelMap; import org.springframework.ui.ModelMap; import org.springframework.util.MultiValueMap; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.PutMapping; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestMethod; @@ -50,18 +53,20 @@ import org.springframework.web.method.HandlerMethod; import org.springframework.web.reactive.HandlerMapping; import org.springframework.web.reactive.HandlerResult; import org.springframework.web.reactive.result.method.RequestMappingInfo.BuilderConfiguration; -import org.springframework.web.server.ServerWebInputException; import org.springframework.web.server.MethodNotAllowedException; import org.springframework.web.server.NotAcceptableStatusException; import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.ServerWebInputException; import org.springframework.web.server.UnsupportedMediaTypeStatusException; import org.springframework.web.server.adapter.DefaultServerWebExchange; import org.springframework.web.server.session.WebSessionManager; import org.springframework.web.util.HttpRequestPathHelper; +import static org.hamcrest.CoreMatchers.containsString; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; @@ -147,41 +152,28 @@ public class RequestMappingInfoHandlerMappingTests { public void getHandlerRequestMethodNotAllowed() throws Exception { ServerWebExchange exchange = createExchange(HttpMethod.POST, "/bar"); Mono mono = this.handlerMapping.getHandler(exchange); + assertError(mono, MethodNotAllowedException.class, ex -> assertEquals(new HashSet<>(Arrays.asList("GET", "HEAD")), ex.getSupportedMethods())); } - // SPR-9603 - - @Test + @Test // SPR-9603 public void getHandlerRequestMethodMatchFalsePositive() throws Exception { ServerWebExchange exchange = createExchange(HttpMethod.GET, "/users"); exchange.getRequest().getHeaders().setAccept(Collections.singletonList(MediaType.APPLICATION_XML)); this.handlerMapping.registerHandler(new UserController()); Mono mono = this.handlerMapping.getHandler(exchange); - TestSubscriber - .subscribe(mono) - .assertError(NotAcceptableStatusException.class); + TestSubscriber.subscribe(mono).assertError(NotAcceptableStatusException.class); } - // SPR-8462 - - @Test + @Test // SPR-8462 public void getHandlerMediaTypeNotSupported() throws Exception { testHttpMediaTypeNotSupportedException("/person/1"); testHttpMediaTypeNotSupportedException("/person/1/"); testHttpMediaTypeNotSupportedException("/person/1.json"); } - @Test - public void getHandlerHttpOptions() throws Exception { - testHttpOptions("/foo", "GET,HEAD"); - testHttpOptions("/person/1", "PUT"); - testHttpOptions("/persons", "GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS"); - testHttpOptions("/something", "PUT,POST"); - } - @Test public void getHandlerTestInvalidContentType() throws Exception { ServerWebExchange exchange = createExchange(HttpMethod.PUT, "/person/1"); @@ -196,24 +188,32 @@ public class RequestMappingInfoHandlerMappingTests { // SPR-8462 @Test - public void getHandlerMediaTypeNotAccepted() throws Exception { - testHttpMediaTypeNotAcceptableException("/persons"); - testHttpMediaTypeNotAcceptableException("/persons/"); - testHttpMediaTypeNotAcceptableException("/persons.json"); + public void getHandlerTestMediaTypeNotAcceptable() throws Exception { + testMediaTypeNotAcceptable("/persons"); + testMediaTypeNotAcceptable("/persons/"); + testMediaTypeNotAcceptable("/persons.json"); } // SPR-12854 @Test - public void getHandlerUnsatisfiedServletRequestParameterException() throws Exception { + public void getHandlerTestRequestParamMismatch() throws Exception { ServerWebExchange exchange = createExchange(HttpMethod.GET, "/params"); Mono mono = this.handlerMapping.getHandler(exchange); assertError(mono, ServerWebInputException.class, ex -> { - assertEquals(ex.getReason(), "Unsatisfied query parameter conditions: " + - "[[bar=baz], [foo=bar]], actual parameters: {}"); + assertThat(ex.getReason(), containsString("[foo=bar]")); + assertThat(ex.getReason(), containsString("[bar=baz]")); }); } + @Test + public void getHandlerHttpOptions() throws Exception { + testHttpOptions("/foo", "GET,HEAD"); + testHttpOptions("/person/1", "PUT"); + testHttpOptions("/persons", "GET,HEAD,POST,PUT,PATCH,DELETE,OPTIONS"); + testHttpOptions("/something", "PUT,POST"); + } + @Test public void getHandlerProducibleMediaTypesAttribute() throws Exception { ServerWebExchange exchange = createExchange(HttpMethod.GET, "/content"); @@ -398,7 +398,7 @@ public class RequestMappingInfoHandlerMappingTests { assertEquals(allowHeader, ((HttpHeaders) value.get()).getFirst("Allow")); } - private void testHttpMediaTypeNotAcceptableException(String url) throws Exception { + private void testMediaTypeNotAcceptable(String url) throws Exception { ServerWebExchange exchange = createExchange(HttpMethod.GET, url); exchange.getRequest().getHeaders().setAccept(Collections.singletonList(MediaType.APPLICATION_JSON)); Mono mono = this.handlerMapping.getHandler(exchange); @@ -431,15 +431,15 @@ public class RequestMappingInfoHandlerMappingTests { @Controller private static class TestController { - @RequestMapping(value = "/foo", method = RequestMethod.GET) + @GetMapping("/foo") public void foo() { } - @RequestMapping(value = "/foo", method = RequestMethod.GET, params="p") + @GetMapping(path = "/foo", params="p") public void fooParam() { } - @RequestMapping(value = "/ba*", method = { RequestMethod.GET, RequestMethod.HEAD }) + @RequestMapping(path = "/ba*", method = { RequestMethod.GET, RequestMethod.HEAD }) public void bar() { } @@ -447,7 +447,7 @@ public class RequestMappingInfoHandlerMappingTests { public void empty() { } - @RequestMapping(value = "/person/{id}", method = RequestMethod.PUT, consumes="application/xml") + @PutMapping(path = "/person/{id}", consumes="application/xml") public void consumes(@RequestBody String text) { } @@ -488,11 +488,11 @@ public class RequestMappingInfoHandlerMappingTests { @Controller private static class UserController { - @RequestMapping(value = "/users", method = RequestMethod.GET, produces = "application/json") + @GetMapping(path = "/users", produces = "application/json") public void getUser() { } - @RequestMapping(value = "/users", method = RequestMethod.PUT) + @PutMapping(path = "/users") public void saveUser() { } } @@ -510,16 +510,16 @@ public class RequestMappingInfoHandlerMappingTests { @Override protected RequestMappingInfo getMappingForMethod(Method method, Class handlerType) { - RequestMapping annotation = AnnotationUtils.findAnnotation(method, RequestMapping.class); - if (annotation != null) { + RequestMapping annot = AnnotatedElementUtils.findMergedAnnotation(method, RequestMapping.class); + if (annot != null) { BuilderConfiguration options = new BuilderConfiguration(); options.setPathHelper(getPathHelper()); options.setPathMatcher(getPathMatcher()); options.setSuffixPatternMatch(true); options.setTrailingSlashMatch(true); - return RequestMappingInfo.paths(annotation.value()).methods(annotation.method()) - .params(annotation.params()).headers(annotation.headers()) - .consumes(annotation.consumes()).produces(annotation.produces()) + return RequestMappingInfo.paths(annot.value()).methods(annot.method()) + .params(annot.params()).headers(annot.headers()) + .consumes(annot.consumes()).produces(annot.produces()) .options(options).build(); } else {