mirror of
https://github.com/spring-projects/spring-boot.git
synced 2024-08-29 03:06:45 +08:00
Error handling for WebFlux should log HTTP 5xx errors
This commit ensures that all errors handled by the `DefaultErrorWebExceptionHandler` (Spring WebFlux error convetion support) logs an error with request information and exception stacktrace. This is limited to errors that result in an HTTP 5xx error. Exceptions that extend `ResponseStatusException` and set a non-5xx status will not be logged. Closes gh-10904
This commit is contained in:
parent
bfe2f85a4f
commit
fa0926b178
@ -114,6 +114,20 @@ public abstract class AbstractErrorWebExceptionHandler
|
||||
return this.errorAttributes.getErrorAttributes(request, includeStackTrace);
|
||||
}
|
||||
|
||||
/**
|
||||
* Extract the original error from the current request.
|
||||
* @param request the source request
|
||||
* @return the error
|
||||
*/
|
||||
protected Throwable getError(ServerRequest request) {
|
||||
return this.errorAttributes.getError(request);
|
||||
}
|
||||
|
||||
/**
|
||||
* Check whether the trace attribute has been set on the given request.
|
||||
* @param request the source request
|
||||
* @return {@code true} if the error trace has been requested, {@code false} otherwise
|
||||
*/
|
||||
protected boolean isTraceEnabled(ServerRequest request) {
|
||||
String parameter = request.queryParam("trace").orElse("false");
|
||||
return !"false".equals(parameter.toLowerCase());
|
||||
|
@ -21,6 +21,8 @@ import java.util.HashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
|
||||
import org.apache.commons.logging.Log;
|
||||
import org.apache.commons.logging.LogFactory;
|
||||
import reactor.core.publisher.Flux;
|
||||
import reactor.core.publisher.Mono;
|
||||
|
||||
@ -72,6 +74,9 @@ public class DefaultErrorWebExceptionHandler extends AbstractErrorWebExceptionHa
|
||||
|
||||
private static final Map<HttpStatus.Series, String> SERIES_VIEWS;
|
||||
|
||||
private static final Log logger = LogFactory
|
||||
.getLog(DefaultErrorWebExceptionHandler.class);
|
||||
|
||||
static {
|
||||
Map<HttpStatus.Series, String> views = new HashMap<>();
|
||||
views.put(HttpStatus.Series.CLIENT_ERROR, "4xx");
|
||||
@ -117,7 +122,8 @@ public class DefaultErrorWebExceptionHandler extends AbstractErrorWebExceptionHa
|
||||
.just("error/" + errorStatus.toString(),
|
||||
"error/" + SERIES_VIEWS.get(errorStatus.series()), "error/error")
|
||||
.flatMap((viewName) -> renderErrorView(viewName, response, error))
|
||||
.switchIfEmpty(renderDefaultErrorView(response, error)).next();
|
||||
.switchIfEmpty(renderDefaultErrorView(response, error)).next()
|
||||
.doOnNext(resp -> logError(request, errorStatus));
|
||||
}
|
||||
|
||||
/**
|
||||
@ -128,9 +134,11 @@ public class DefaultErrorWebExceptionHandler extends AbstractErrorWebExceptionHa
|
||||
protected Mono<ServerResponse> renderErrorResponse(ServerRequest request) {
|
||||
boolean includeStackTrace = isIncludeStackTrace(request, MediaType.ALL);
|
||||
Map<String, Object> error = getErrorAttributes(request, includeStackTrace);
|
||||
HttpStatus errorStatus = getHttpStatus(error);
|
||||
return ServerResponse.status(getHttpStatus(error))
|
||||
.contentType(MediaType.APPLICATION_JSON_UTF8)
|
||||
.body(BodyInserters.fromObject(error));
|
||||
.body(BodyInserters.fromObject(error))
|
||||
.doOnNext(resp -> logError(request, errorStatus));
|
||||
}
|
||||
|
||||
/**
|
||||
@ -178,4 +186,18 @@ public class DefaultErrorWebExceptionHandler extends AbstractErrorWebExceptionHa
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Log the original exception if handling it results in a Server Error.
|
||||
* @param request the source request
|
||||
* @param errorStatus the HTTP error status
|
||||
*/
|
||||
protected void logError(ServerRequest request, HttpStatus errorStatus) {
|
||||
if (errorStatus.is5xxServerError()) {
|
||||
Throwable error = getError(request);
|
||||
final String message = "Failed to handle request ["
|
||||
+ request.methodName() + " " + request.uri() + "]";
|
||||
logger.error(message, error);
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
@ -28,6 +28,7 @@ import java.util.List;
|
||||
import javax.validation.Valid;
|
||||
|
||||
import org.junit.After;
|
||||
import org.junit.Rule;
|
||||
import org.junit.Test;
|
||||
import reactor.core.publisher.Mono;
|
||||
|
||||
@ -38,6 +39,7 @@ import org.springframework.boot.autoconfigure.mustache.MustacheAutoConfiguration
|
||||
import org.springframework.boot.autoconfigure.web.reactive.HttpHandlerAutoConfiguration;
|
||||
import org.springframework.boot.autoconfigure.web.reactive.ReactiveWebServerAutoConfiguration;
|
||||
import org.springframework.boot.autoconfigure.web.reactive.WebFluxAutoConfiguration;
|
||||
import org.springframework.boot.test.rule.OutputCapture;
|
||||
import org.springframework.context.ConfigurableApplicationContext;
|
||||
import org.springframework.context.annotation.Configuration;
|
||||
import org.springframework.context.annotation.Import;
|
||||
@ -52,6 +54,9 @@ import org.springframework.web.bind.annotation.RestController;
|
||||
import org.springframework.web.server.ResponseStatusException;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.hamcrest.Matchers.allOf;
|
||||
import static org.hamcrest.Matchers.containsString;
|
||||
import static org.hamcrest.Matchers.not;
|
||||
|
||||
/**
|
||||
* Integration tests for {@link DefaultErrorWebExceptionHandler}
|
||||
@ -64,6 +69,9 @@ public class DefaultErrorWebExceptionHandlerIntegrationTest {
|
||||
|
||||
private WebTestClient webTestClient;
|
||||
|
||||
@Rule
|
||||
public OutputCapture output = new OutputCapture();
|
||||
|
||||
@After
|
||||
public void closeContext() {
|
||||
if (this.context != null) {
|
||||
@ -81,6 +89,8 @@ public class DefaultErrorWebExceptionHandlerIntegrationTest {
|
||||
.jsonPath("path").isEqualTo(("/")).jsonPath("message")
|
||||
.isEqualTo("Expected!").jsonPath("exception").doesNotExist()
|
||||
.jsonPath("trace").doesNotExist();
|
||||
this.output.expect(allOf(containsString("Failed to handle request [GET /]"),
|
||||
containsString("IllegalStateException")));
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -101,6 +111,8 @@ public class DefaultErrorWebExceptionHandlerIntegrationTest {
|
||||
.expectHeader().contentType(MediaType.TEXT_HTML).expectBody(String.class)
|
||||
.returnResult().getResponseBody();
|
||||
assertThat(body).contains("status: 500").contains("message: Expected!");
|
||||
this.output.expect(allOf(containsString("Failed to handle request [GET /]"),
|
||||
containsString("IllegalStateException")));
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -160,6 +172,7 @@ public class DefaultErrorWebExceptionHandlerIntegrationTest {
|
||||
.isEqualTo("400").jsonPath("error")
|
||||
.isEqualTo(HttpStatus.BAD_REQUEST.getReasonPhrase()).jsonPath("exception")
|
||||
.isEqualTo(ResponseStatusException.class.getName());
|
||||
this.output.expect(not(containsString("ResponseStatusException")));
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -171,6 +184,8 @@ public class DefaultErrorWebExceptionHandlerIntegrationTest {
|
||||
.returnResult().getResponseBody();
|
||||
assertThat(body).contains("Whitelabel Error Page")
|
||||
.contains("<div>Expected!</div>");
|
||||
this.output.expect(allOf(containsString("Failed to handle request [GET /]"),
|
||||
containsString("IllegalStateException")));
|
||||
}
|
||||
|
||||
private void load(String... arguments) {
|
||||
@ -188,10 +203,10 @@ public class DefaultErrorWebExceptionHandlerIntegrationTest {
|
||||
@Target(ElementType.TYPE)
|
||||
@Retention(RetentionPolicy.RUNTIME)
|
||||
@Documented
|
||||
@Import({ ReactiveWebServerAutoConfiguration.class,
|
||||
@Import({ReactiveWebServerAutoConfiguration.class,
|
||||
HttpHandlerAutoConfiguration.class, WebFluxAutoConfiguration.class,
|
||||
ErrorWebFluxAutoConfiguration.class,
|
||||
PropertyPlaceholderAutoConfiguration.class })
|
||||
PropertyPlaceholderAutoConfiguration.class})
|
||||
private @interface MinimalWebConfiguration {
|
||||
|
||||
}
|
||||
|
Loading…
Reference in New Issue
Block a user