From 1f4a32f0ad9f4e156c558d42169b9490d698643c Mon Sep 17 00:00:00 2001 From: Stephane Nicoll Date: Tue, 23 Jan 2018 14:57:13 +0100 Subject: [PATCH] Add a way to signal that an endpoint request is invalid This commit adds InvalidEndpointRequestException as a technology agnostic way to signal that an endpoint request is invalid. When such exception is thrown, the web layer translates that to a 400. Rather than overriding the reason, this commit makes sure to reuse the error infrastructure. Closes gh-10618 --- .../InvalidEndpointRequestException.java | 48 +++++++++++++++++++ .../invoke/MissingParametersException.java | 9 +++- .../invoke/ParameterMappingException.java | 6 ++- .../actuate/endpoint/jmx/EndpointMBean.java | 5 +- .../jersey/JerseyEndpointResourceFactory.java | 5 +- ...AbstractWebFluxEndpointHandlerMapping.java | 11 ++--- .../AbstractWebMvcEndpointHandlerMapping.java | 17 +++++-- .../AbstractWebEndpointIntegrationTests.java | 27 +++++++++-- .../JerseyWebEndpointIntegrationTests.java | 8 ++++ .../WebFluxEndpointIntegrationTests.java | 3 ++ .../MvcWebEndpointIntegrationTests.java | 19 ++++---- 11 files changed, 126 insertions(+), 32 deletions(-) create mode 100644 spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/InvalidEndpointRequestException.java diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/InvalidEndpointRequestException.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/InvalidEndpointRequestException.java new file mode 100644 index 0000000000..b11a359c64 --- /dev/null +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/InvalidEndpointRequestException.java @@ -0,0 +1,48 @@ +/* + * Copyright 2012-2018 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. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.boot.actuate.endpoint; + +/** + * Indicate that an endpoint request is invalid. + * + * @author Stephane Nicoll + * @since 2.0.0 + */ +public class InvalidEndpointRequestException extends RuntimeException { + + private final String reason; + + public InvalidEndpointRequestException(String message, String reason) { + super(message); + this.reason = reason; + } + + public InvalidEndpointRequestException(String message, String reason, + Throwable cause) { + super(message, cause); + this.reason = reason; + } + + /** + * Return the reason explaining why the request is invalid, potentially {@code null}. + * @return the reason for the failure + */ + public String getReason() { + return this.reason; + } + +} diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/MissingParametersException.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/MissingParametersException.java index 8a00070058..d02fb43131 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/MissingParametersException.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/MissingParametersException.java @@ -17,7 +17,9 @@ package org.springframework.boot.actuate.endpoint.invoke; import java.util.Set; +import java.util.stream.Collectors; +import org.springframework.boot.actuate.endpoint.InvalidEndpointRequestException; import org.springframework.util.StringUtils; /** @@ -28,14 +30,17 @@ import org.springframework.util.StringUtils; * @author Phillip Webb * @since 2.0.0 */ -public class MissingParametersException extends RuntimeException { +public final class MissingParametersException extends InvalidEndpointRequestException { private final Set missingParameters; public MissingParametersException(Set missingParameters) { super("Failed to invoke operation because the following required " + "parameters were missing: " - + StringUtils.collectionToCommaDelimitedString(missingParameters)); + + StringUtils.collectionToCommaDelimitedString(missingParameters), + "Missing parameters: " + missingParameters.stream() + .map(OperationParameter::getName) + .collect(Collectors.joining(","))); this.missingParameters = missingParameters; } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/ParameterMappingException.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/ParameterMappingException.java index c928cc5da4..7043be7029 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/ParameterMappingException.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/invoke/ParameterMappingException.java @@ -16,6 +16,8 @@ package org.springframework.boot.actuate.endpoint.invoke; +import org.springframework.boot.actuate.endpoint.InvalidEndpointRequestException; + /** * A {@code ParameterMappingException} is thrown when a failure occurs during * {@link ParameterValueMapper#mapParameterValue operation parameter mapping}. @@ -23,7 +25,7 @@ package org.springframework.boot.actuate.endpoint.invoke; * @author Andy Wilkinson * @since 2.0.0 */ -public class ParameterMappingException extends RuntimeException { +public final class ParameterMappingException extends InvalidEndpointRequestException { private final OperationParameter parameter; @@ -39,7 +41,7 @@ public class ParameterMappingException extends RuntimeException { public ParameterMappingException(OperationParameter parameter, Object value, Throwable cause) { super("Failed to map " + value + " of type " + value.getClass() + " to " - + parameter, cause); + + parameter, "Parameter mapping failure", cause); this.parameter = parameter; this.value = value; } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/jmx/EndpointMBean.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/jmx/EndpointMBean.java index 372210c73c..c15abfe146 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/jmx/EndpointMBean.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/jmx/EndpointMBean.java @@ -31,8 +31,7 @@ import javax.management.ReflectionException; import reactor.core.publisher.Mono; -import org.springframework.boot.actuate.endpoint.invoke.MissingParametersException; -import org.springframework.boot.actuate.endpoint.invoke.ParameterMappingException; +import org.springframework.boot.actuate.endpoint.InvalidEndpointRequestException; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; @@ -103,7 +102,7 @@ public class EndpointMBean implements DynamicMBean { } return this.responseMapper.mapResponse(result); } - catch (MissingParametersException | ParameterMappingException ex) { + catch (InvalidEndpointRequestException ex) { throw new IllegalArgumentException(ex.getMessage(), ex); } } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyEndpointResourceFactory.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyEndpointResourceFactory.java index d44967d40f..45d0163bc6 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyEndpointResourceFactory.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyEndpointResourceFactory.java @@ -38,8 +38,7 @@ import org.glassfish.jersey.server.model.Resource; import org.glassfish.jersey.server.model.Resource.Builder; import reactor.core.publisher.Mono; -import org.springframework.boot.actuate.endpoint.invoke.MissingParametersException; -import org.springframework.boot.actuate.endpoint.invoke.ParameterMappingException; +import org.springframework.boot.actuate.endpoint.InvalidEndpointRequestException; import org.springframework.boot.actuate.endpoint.web.EndpointLinksResolver; import org.springframework.boot.actuate.endpoint.web.EndpointMediaTypes; import org.springframework.boot.actuate.endpoint.web.ExposableWebEndpoint; @@ -155,7 +154,7 @@ public class JerseyEndpointResourceFactory { Object response = this.operation.invoke(arguments); return convertToJaxRsResponse(response, data.getRequest().getMethod()); } - catch (MissingParametersException | ParameterMappingException ex) { + catch (InvalidEndpointRequestException ex) { return Response.status(Status.BAD_REQUEST).build(); } } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/reactive/AbstractWebFluxEndpointHandlerMapping.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/reactive/AbstractWebFluxEndpointHandlerMapping.java index 9fd2d34172..91e7d9f4db 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/reactive/AbstractWebFluxEndpointHandlerMapping.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/reactive/AbstractWebFluxEndpointHandlerMapping.java @@ -26,10 +26,9 @@ import reactor.core.publisher.Mono; import reactor.core.publisher.MonoSink; import reactor.core.scheduler.Schedulers; +import org.springframework.boot.actuate.endpoint.InvalidEndpointRequestException; import org.springframework.boot.actuate.endpoint.OperationType; -import org.springframework.boot.actuate.endpoint.invoke.MissingParametersException; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; -import org.springframework.boot.actuate.endpoint.invoke.ParameterMappingException; import org.springframework.boot.actuate.endpoint.web.EndpointMediaTypes; import org.springframework.boot.actuate.endpoint.web.ExposableWebEndpoint; import org.springframework.boot.actuate.endpoint.web.WebEndpointResponse; @@ -52,6 +51,7 @@ import org.springframework.web.reactive.result.condition.ProducesRequestConditio import org.springframework.web.reactive.result.condition.RequestMethodsRequestCondition; import org.springframework.web.reactive.result.method.RequestMappingInfo; import org.springframework.web.reactive.result.method.RequestMappingInfoHandlerMapping; +import org.springframework.web.server.ResponseStatusException; import org.springframework.web.server.ServerWebExchange; import org.springframework.web.util.pattern.PathPatternParser; @@ -288,10 +288,9 @@ public abstract class AbstractWebFluxEndpointHandlerMapping private Mono> handleResult(Publisher result, HttpMethod httpMethod) { return Mono.from(result).map(this::toResponseEntity) - .onErrorReturn(MissingParametersException.class, - new ResponseEntity<>(HttpStatus.BAD_REQUEST)) - .onErrorReturn(ParameterMappingException.class, - new ResponseEntity<>(HttpStatus.BAD_REQUEST)) + .onErrorMap(InvalidEndpointRequestException.class, (ex) -> + new ResponseStatusException(HttpStatus.BAD_REQUEST, + ex.getReason())) .defaultIfEmpty(new ResponseEntity<>(httpMethod == HttpMethod.GET ? HttpStatus.NOT_FOUND : HttpStatus.NO_CONTENT)); } diff --git a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java index 9602432094..bfae89e9cd 100644 --- a/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java +++ b/spring-boot-project/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/endpoint/web/servlet/AbstractWebMvcEndpointHandlerMapping.java @@ -27,9 +27,8 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.springframework.beans.factory.InitializingBean; -import org.springframework.boot.actuate.endpoint.invoke.MissingParametersException; +import org.springframework.boot.actuate.endpoint.InvalidEndpointRequestException; import org.springframework.boot.actuate.endpoint.invoke.OperationInvoker; -import org.springframework.boot.actuate.endpoint.invoke.ParameterMappingException; import org.springframework.boot.actuate.endpoint.web.EndpointMediaTypes; import org.springframework.boot.actuate.endpoint.web.ExposableWebEndpoint; import org.springframework.boot.actuate.endpoint.web.WebEndpointResponse; @@ -44,6 +43,7 @@ import org.springframework.util.StringUtils; import org.springframework.web.bind.annotation.RequestBody; import org.springframework.web.bind.annotation.RequestMethod; import org.springframework.web.bind.annotation.ResponseBody; +import org.springframework.web.bind.annotation.ResponseStatus; import org.springframework.web.cors.CorsConfiguration; import org.springframework.web.servlet.HandlerMapping; import org.springframework.web.servlet.mvc.condition.ConsumesRequestCondition; @@ -243,8 +243,8 @@ public abstract class AbstractWebMvcEndpointHandlerMapping return handleResult(this.invoker.invoke(arguments), HttpMethod.valueOf(request.getMethod())); } - catch (MissingParametersException | ParameterMappingException ex) { - return new ResponseEntity(HttpStatus.BAD_REQUEST); + catch (InvalidEndpointRequestException ex) { + throw new BadOperationRequestException(ex.getReason()); } } @@ -300,4 +300,13 @@ public abstract class AbstractWebMvcEndpointHandlerMapping } + @ResponseStatus(code = HttpStatus.BAD_REQUEST) + private static class BadOperationRequestException extends RuntimeException { + + BadOperationRequestException(String message) { + super(message); + } + + } + } diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/AbstractWebEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/AbstractWebEndpointIntegrationTests.java index 1bce93ac22..1ab71f71b3 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/AbstractWebEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/annotation/AbstractWebEndpointIntegrationTests.java @@ -42,6 +42,7 @@ import org.springframework.context.annotation.Import; import org.springframework.core.env.MapPropertySource; import org.springframework.core.io.ByteArrayResource; import org.springframework.core.io.Resource; +import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.lang.Nullable; import org.springframework.test.web.reactive.server.WebTestClient; @@ -157,8 +158,13 @@ public abstract class AbstractWebEndpointIntegrationTests client.get() - .uri("/query?two=two").exchange().expectStatus().isBadRequest()); + load(QueryEndpointConfiguration.class, (client) -> { + WebTestClient.BodyContentSpec body = client.get() + .uri("/query?two=two").exchange().expectStatus().isBadRequest() + .expectBody(); + validateErrorBody(body, HttpStatus.BAD_REQUEST, "/endpoints/query", + "Missing parameters: one"); + }); } @Test @@ -275,8 +281,13 @@ public abstract class AbstractWebEndpointIntegrationTests client.get() - .uri("/requiredparameters").exchange().expectStatus().isBadRequest()); + load(RequiredParameterEndpointConfiguration.class, (client) -> { + WebTestClient.BodyContentSpec body = client.get() + .uri("/requiredparameters").exchange().expectStatus().isBadRequest() + .expectBody(); + validateErrorBody(body, HttpStatus.BAD_REQUEST, + "/endpoints/requiredparameters", "Missing parameters: foo"); + }); } @Test @@ -321,6 +332,14 @@ public abstract class AbstractWebEndpointIntegrationTests configuration, BiConsumer consumer) { load(configuration, "/endpoints", consumer); diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyWebEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyWebEndpointIntegrationTests.java index 6899c3f4fd..91d089a34d 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyWebEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/jersey/JerseyWebEndpointIntegrationTests.java @@ -37,6 +37,8 @@ import org.springframework.boot.web.servlet.context.AnnotationConfigServletWebSe import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.env.Environment; +import org.springframework.http.HttpStatus; +import org.springframework.test.web.reactive.server.WebTestClient; /** * Integration tests for web endpoints exposed using Jersey. @@ -64,6 +66,12 @@ public class JerseyWebEndpointIntegrationTests extends return context.getWebServer().getPort(); } + @Override + protected void validateErrorBody(WebTestClient.BodyContentSpec body, + HttpStatus status, String path, String message) { + // Jersey doesn't support the general error page handling + } + @Configuration static class JerseyConfiguration { diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/reactive/WebFluxEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/reactive/WebFluxEndpointIntegrationTests.java index 4f6e29e310..3e935ba171 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/reactive/WebFluxEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/reactive/WebFluxEndpointIntegrationTests.java @@ -23,6 +23,8 @@ import org.junit.Test; import org.springframework.boot.actuate.endpoint.web.EndpointMediaTypes; import org.springframework.boot.actuate.endpoint.web.annotation.AbstractWebEndpointIntegrationTests; import org.springframework.boot.actuate.endpoint.web.annotation.WebEndpointDiscoverer; +import org.springframework.boot.autoconfigure.ImportAutoConfiguration; +import org.springframework.boot.autoconfigure.web.reactive.error.ErrorWebFluxAutoConfiguration; import org.springframework.boot.endpoint.web.EndpointMapping; import org.springframework.boot.web.embedded.netty.NettyReactiveWebServerFactory; import org.springframework.boot.web.reactive.context.AnnotationConfigReactiveWebServerApplicationContext; @@ -95,6 +97,7 @@ public class WebFluxEndpointIntegrationTests @Configuration @EnableWebFlux + @ImportAutoConfiguration(ErrorWebFluxAutoConfiguration.class) static class ReactiveConfiguration { private int port; diff --git a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java index b062b24a9f..66c72fe661 100644 --- a/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java +++ b/spring-boot-project/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/endpoint/web/servlet/MvcWebEndpointIntegrationTests.java @@ -23,6 +23,13 @@ import org.junit.Test; import org.springframework.boot.actuate.endpoint.web.EndpointMediaTypes; import org.springframework.boot.actuate.endpoint.web.annotation.AbstractWebEndpointIntegrationTests; import org.springframework.boot.actuate.endpoint.web.annotation.WebEndpointDiscoverer; +import org.springframework.boot.autoconfigure.ImportAutoConfiguration; +import org.springframework.boot.autoconfigure.http.HttpMessageConvertersAutoConfiguration; +import org.springframework.boot.autoconfigure.jackson.JacksonAutoConfiguration; +import org.springframework.boot.autoconfigure.web.servlet.DispatcherServletAutoConfiguration; +import org.springframework.boot.autoconfigure.web.servlet.ServletWebServerFactoryAutoConfiguration; +import org.springframework.boot.autoconfigure.web.servlet.WebMvcAutoConfiguration; +import org.springframework.boot.autoconfigure.web.servlet.error.ErrorMvcAutoConfiguration; import org.springframework.boot.endpoint.web.EndpointMapping; import org.springframework.boot.web.embedded.tomcat.TomcatServletWebServerFactory; import org.springframework.boot.web.servlet.context.AnnotationConfigServletWebServerApplicationContext; @@ -32,8 +39,6 @@ import org.springframework.core.env.Environment; import org.springframework.http.HttpStatus; import org.springframework.http.MediaType; import org.springframework.web.cors.CorsConfiguration; -import org.springframework.web.servlet.DispatcherServlet; -import org.springframework.web.servlet.config.annotation.EnableWebMvc; import static org.assertj.core.api.Assertions.assertThat; @@ -89,7 +94,10 @@ public class MvcWebEndpointIntegrationTests extends } @Configuration - @EnableWebMvc + @ImportAutoConfiguration({ JacksonAutoConfiguration.class, + HttpMessageConvertersAutoConfiguration.class, + ServletWebServerFactoryAutoConfiguration.class, WebMvcAutoConfiguration.class, + DispatcherServletAutoConfiguration.class, ErrorMvcAutoConfiguration.class }) static class WebMvcConfiguration { @Bean @@ -97,11 +105,6 @@ public class MvcWebEndpointIntegrationTests extends return new TomcatServletWebServerFactory(0); } - @Bean - public DispatcherServlet dispatcherServlet() { - return new DispatcherServlet(); - } - @Bean public WebMvcEndpointHandlerMapping webEndpointHandlerMapping( Environment environment, WebEndpointDiscoverer endpointDiscoverer,