Skip to content

Commit

Permalink
Clean up springboot 1 and 2 references, and bring disconnected-client…
Browse files Browse the repository at this point in the history
… detection up to current spring logic
  • Loading branch information
nicmunroe committed Sep 12, 2024
1 parent 7d18bd9 commit d7ac08d
Show file tree
Hide file tree
Showing 21 changed files with 107 additions and 96 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,13 @@
@SuppressWarnings("WeakerAccess")
public class UnhandledServletContainerErrorHelper {

// TODO javax-to-jakarta: Test these things in with the new spring/springboot libs/frameworks.
protected static final List<String> DEFAULT_THROWABLE_REQUEST_ATTR_NAMES = Arrays.asList(
// Try the Springboot 2 attrs first.
// Try the Springboot 3 attrs first.
// Corresponds to org.springframework.boot.web.reactive.error.DefaultErrorAttributes.ERROR_ATTRIBUTE.
"org.springframework.boot.web.reactive.error.DefaultErrorAttributes.ERROR",
// Corresponds to org.springframework.boot.web.servlet.error.DefaultErrorAttributes.ERROR_ATTRIBUTE.
"org.springframework.boot.web.servlet.error.DefaultErrorAttributes.ERROR",

// Try the Springboot 1 attr next.
// Corresponds to org.springframework.boot.autoconfigure.web.DefaultErrorAttributes.ERROR_ATTRIBUTE.
"org.springframework.boot.autoconfigure.web.DefaultErrorAttributes.ERROR",

// Fall back to the Servlet API value last.
// Corresponds to jakarta.servlet.RequestDispatcher.ERROR_EXCEPTION.
"jakarta.servlet.error.exception"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,43 +60,35 @@ protected ProjectSpecificErrorCodeRange getProjectSpecificErrorCodeRange() {
}

@DataProvider(value = {
"true | true | true | true",
"false | true | true | true",
"false | false | true | true",
"false | false | false | true",
"false | false | false | false",
"true | true | true",
"false | true | true",
"false | false | true",
"false | false | true",
"false | false | false",
}, splitBy = "\\|")
@Test
public void extractOrGenerateErrorForRequest_returns_wrapped_exception_from_request_attrs_if_available(
boolean requestHasExInSpringboot2ReactiveAttr,
boolean requestHasExInSpringboot2ServletAttr,
boolean requestHasExInSpringboot1Attr,
boolean requestHasExInSpringboot3ReactiveAttr,
boolean requestHasExInSpringboot3ServletAttr,
boolean requestHasExInServletAttr
) {
// given
Throwable springboot2ReactiveAttrEx = new RuntimeException("some springboot 2 reactive request attr exception");
Throwable springboot2ServletAttrEx = new RuntimeException("some springboot 2 servlet request attr exception");
Throwable springboot1AttrEx = new RuntimeException("some springboot 1 request attr exception");
Throwable springboot3ReactiveAttrEx = new RuntimeException("some springboot 3 reactive request attr exception");
Throwable springboot3ServletAttrEx = new RuntimeException("some springboot 3 servlet request attr exception");
Throwable servletAttrEx = new RuntimeException("some servlet request attr exception");

if (requestHasExInSpringboot2ReactiveAttr) {
doReturn(springboot2ReactiveAttrEx)
if (requestHasExInSpringboot3ReactiveAttr) {
doReturn(springboot3ReactiveAttrEx)
.when(requestMock)
.getAttribute("org.springframework.boot.web.reactive.error.DefaultErrorAttributes.ERROR");
}

if (requestHasExInSpringboot2ServletAttr) {
doReturn(springboot2ServletAttrEx)
if (requestHasExInSpringboot3ServletAttr) {
doReturn(springboot3ServletAttrEx)
.when(requestMock)
.getAttribute("org.springframework.boot.web.servlet.error.DefaultErrorAttributes.ERROR");
}

if (requestHasExInSpringboot1Attr) {
doReturn(springboot1AttrEx)
.when(requestMock)
.getAttribute("org.springframework.boot.autoconfigure.web.DefaultErrorAttributes.ERROR");
}

if (requestHasExInServletAttr) {
doReturn(servletAttrEx).when(requestMock).getAttribute(RequestDispatcher.ERROR_EXCEPTION);
}
Expand All @@ -105,23 +97,17 @@ public void extractOrGenerateErrorForRequest_returns_wrapped_exception_from_requ
Throwable result = helper.extractOrGenerateErrorForRequest(requestMock, projectApiErrors);

// then
if (requestHasExInSpringboot2ReactiveAttr) {
assertThat(result)
.isInstanceOf(WrapperException.class)
.hasMessage("Caught a container exception.")
.hasCause(springboot2ReactiveAttrEx);
}
else if (requestHasExInSpringboot2ServletAttr) {
if (requestHasExInSpringboot3ReactiveAttr) {
assertThat(result)
.isInstanceOf(WrapperException.class)
.hasMessage("Caught a container exception.")
.hasCause(springboot2ServletAttrEx);
.hasCause(springboot3ReactiveAttrEx);
}
else if (requestHasExInSpringboot1Attr) {
else if (requestHasExInSpringboot3ServletAttr) {
assertThat(result)
.isInstanceOf(WrapperException.class)
.hasMessage("Caught a container exception.")
.hasCause(springboot1AttrEx);
.hasCause(springboot3ServletAttrEx);
}
else if (requestHasExInServletAttr) {
assertThat(result)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright 2002-2024 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
*
* https://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 com.nike.backstopper.handler.spring.webflux;

import org.springframework.core.NestedExceptionUtils;

import java.util.Set;

/**
* Copied from spring-web-6.1.12. We need some of the same logic in Backstopper.
* Modified slightly from the original to make the methods static, get rid of the logging and methods we don't need, etc.
*/
class DisconnectedClientHelper {

private static final Set<String> EXCEPTION_PHRASES =
Set.of("broken pipe", "connection reset");

private static final Set<String> EXCEPTION_TYPE_NAMES =
Set.of("AbortedException", "ClientAbortException",
"EOFException", "EofException", "AsyncRequestNotUsableException");

/**
* Whether the given exception indicates the client has gone away.
* <p>Known cases covered:
* <ul>
* <li>ClientAbortException or EOFException for Tomcat
* <li>EofException for Jetty
* <li>IOException "Broken pipe" or "connection reset by peer"
* <li>SocketException "Connection reset"
* </ul>
*/
public static boolean isClientDisconnectedException(Throwable ex) {
String message = NestedExceptionUtils.getMostSpecificCause(ex).getMessage();
if (message != null) {
String text = message.toLowerCase();
for (String phrase : EXCEPTION_PHRASES) {
if (text.contains(phrase)) {
return true;
}
}
}
return EXCEPTION_TYPE_NAMES.contains(ex.getClass().getSimpleName());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.ObjectProvider;
import org.springframework.core.NestedExceptionUtils;
import org.springframework.core.Ordered;
import org.springframework.http.codec.HttpMessageReader;
import org.springframework.http.codec.HttpMessageWriter;
Expand All @@ -27,20 +26,18 @@
import org.springframework.web.server.ServerWebExchange;
import org.springframework.web.server.WebExceptionHandler;

import java.util.Arrays;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import jakarta.inject.Inject;
import jakarta.inject.Named;
import jakarta.inject.Singleton;

import reactor.core.publisher.Mono;

import static com.nike.backstopper.handler.spring.webflux.DisconnectedClientHelper.isClientDisconnectedException;

/**
* An {@link ApiExceptionHandlerBase} extension that hooks into Spring WebFlux via its
* {@link WebExceptionHandler} interface, and specifically the
Expand All @@ -59,12 +56,6 @@ public class SpringWebfluxApiExceptionHandler extends ApiExceptionHandlerBase<Mo

private final Logger logger = LoggerFactory.getLogger(this.getClass());

/**
* Copied from Spring WebFlux {@link org.springframework.web.server.adapter.HttpWebHandlerAdapter}.
*/
public static final Set<String> DISCONNECTED_CLIENT_EXCEPTIONS = new HashSet<>(
Arrays.asList("AbortedException", "ClientAbortException", "EOFException", "EofException"));

/**
* The sort order for where this handler goes in the spring exception handler chain. We default to {@link
* Ordered#HIGHEST_PRECEDENCE}, so that this is tried first before any other handlers.
Expand Down Expand Up @@ -150,8 +141,8 @@ public Mono<Void> handle(ServerWebExchange exchange, Throwable ex) {
// Before we try to write the response, we should check to see if it's already committed, or if the client
// disconnected.
// This short circuit logic due to an already-committed response or disconnected client was copied from
// Spring Boot 2's AbstractErrorWebExceptionHandler class.
if (exchange.getResponse().isCommitted() || isDisconnectedClientError(ex)) {
// Spring Boot's AbstractErrorWebExceptionHandler class.
if (exchange.getResponse().isCommitted() || isClientDisconnectedException(ex)) {
return Mono.error(ex);
}

Expand All @@ -175,29 +166,13 @@ protected void processWebFluxResponse(
}
}

// Copied from Spring Boot 2's AbstractErrorWebExceptionHandler class.
// Copied and slightly modified from Spring Boot 3.3.3's AbstractErrorWebExceptionHandler class.
protected Mono<? extends Void> write(ServerWebExchange exchange, ServerResponse response) {
// force content-type since writeTo won't overwrite response header values
exchange.getResponse().getHeaders().setContentType(response.headers().getContentType());
return response.writeTo(exchange, new ResponseContext(messageWriters, viewResolvers));
}

/**
* Copied from Spring Boot 2's {@code AbstractErrorWebExceptionHandler} class.
*/
public static boolean isDisconnectedClientError(Throwable ex) {
return DISCONNECTED_CLIENT_EXCEPTIONS.contains(ex.getClass().getSimpleName())
|| isDisconnectedClientErrorMessage(NestedExceptionUtils.getMostSpecificCause(ex).getMessage());
}

/**
* Copied from Spring Boot 2's {@code AbstractErrorWebExceptionHandler} class.
*/
public static boolean isDisconnectedClientErrorMessage(String message) {
message = (message != null) ? message.toLowerCase() : "";
return (message.contains("broken pipe") || message.contains("connection reset by peer"));
}

/**
* See the javadocs for {@link #order} for info on what this is for.
*/
Expand All @@ -214,6 +189,7 @@ public void setOrder(int order) {
this.order = order;
}

// Copied and slightly modified from Springboot 3.3.3's AbstractErrorWebExceptionHandler.
public static class ResponseContext implements ServerResponse.Context {

private final List<HttpMessageWriter<?>> messageWriters;
Expand All @@ -228,12 +204,12 @@ protected ResponseContext(
}

@Override
public List<HttpMessageWriter<?>> messageWriters() {
public @NotNull List<HttpMessageWriter<?>> messageWriters() {
return messageWriters;
}

@Override
public List<ViewResolver> viewResolvers() {
public @NotNull List<ViewResolver> viewResolvers() {
return viewResolvers;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,9 @@
import jakarta.inject.Inject;
import jakarta.inject.Named;
import jakarta.inject.Singleton;

import reactor.core.publisher.Mono;

import static com.nike.backstopper.handler.spring.webflux.SpringWebfluxApiExceptionHandler.isDisconnectedClientError;
import static com.nike.backstopper.handler.spring.webflux.DisconnectedClientHelper.isClientDisconnectedException;

/**
* An extension of {@link UnhandledExceptionHandlerBase} that acts as a final catch-all exception handler for
Expand All @@ -55,7 +54,7 @@ public class SpringWebfluxUnhandledExceptionHandler

/**
* The sort order for where this handler goes in the spring exception handler chain. We default to -2 so this gets
* executed after any custom handlers, but before any default spring handlers (Spring Boot 2's
* executed after any custom handlers, but before any default spring handlers (Spring Boot 3's
* {@code DefaultErrorWebExceptionHandler} is ordered at -1, see {@code
* ErrorWebFluxAutoConfiguration.errorWebExceptionHandler(...)}. And {@code WebFluxResponseStatusExceptionHandler}
* is ordered at 0, see {@code WebFluxConfigurationSupport.responseStatusExceptionHandler(...)}).
Expand Down Expand Up @@ -151,8 +150,8 @@ public Mono<Void> handle(ServerWebExchange exchange, Throwable ex) {
// Before we try to write the response, we should check to see if it's already committed, or if the client
// disconnected.
// This short circuit logic due to an already-committed response or disconnected client was copied from
// Spring Boot 2's AbstractErrorWebExceptionHandler class.
if (exchange.getResponse().isCommitted() || isDisconnectedClientError(ex)) {
// Spring Boot's AbstractErrorWebExceptionHandler class.
if (exchange.getResponse().isCommitted() || isClientDisconnectedException(ex)) {
return Mono.error(ex);
}

Expand All @@ -176,7 +175,7 @@ protected void processWebFluxResponse(
}
}

// Copied from Spring Boot 2's AbstractErrorWebExceptionHandler class.
// Copied and slightly modified from Spring Boot 3.3.3's AbstractErrorWebExceptionHandler class.
protected Mono<? extends Void> write(ServerWebExchange exchange, ServerResponse response) {
// force content-type since writeTo won't overwrite response header values
exchange.getResponse().getHeaders().setContentType(response.headers().getContentType());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import reactor.core.publisher.Mono;
import reactor.netty.channel.AbortedException;

import static com.nike.backstopper.handler.spring.webflux.DisconnectedClientHelper.isClientDisconnectedException;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.catchThrowable;
import static org.mockito.ArgumentMatchers.any;
Expand Down Expand Up @@ -484,7 +485,7 @@ public static List<List<IsDisconnectedClientErrorScenario>> isDisconnectedClient
@Test
public void isDisconnectedClientError_works_as_expected(IsDisconnectedClientErrorScenario scenario) {
// when
boolean result = SpringWebfluxApiExceptionHandler.isDisconnectedClientError(scenario.ex);
boolean result = isClientDisconnectedException(scenario.ex);

// then
assertThat(result).isEqualTo(scenario.expectedResult);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,7 @@ public String triggerServerErrorExceptionEndpoint(@PathVariable(name = "notFoo")
return "we should never reach here";
}

// Can't figure out how to get springboot 2 to trigger a ConversionNotSupportedException naturally, so
// Can't figure out how to get springboot to trigger a ConversionNotSupportedException naturally, so
// we'll just throw one ourselves.
@GetMapping(path = CONVERSION_NOT_SUPPORTED_EXCEPTION_ENDPOINT_PATH)
@ResponseBody
Expand Down
2 changes: 0 additions & 2 deletions backstopper-spring-web-mvc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ concrete example of the information covered in this readme.**

### Spring / Spring Boot Versions

// TODO javax-to-jakarta: verify this statement about spring boot 3

This `backstopper-spring-web-mvc` library can be used in any Spring Web MVC `6.x.x` environment or later. This includes
Spring 6 and Spring Boot 3.

Expand Down
7 changes: 2 additions & 5 deletions backstopper-spring-web/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,14 @@ does not provide Spring+Backstopper integration by itself.
To integrate Backstopper with your Spring application, please choose the correct concrete integration library,
depending on which Spring environment your application is running in:

// TODO javax-to-jakarta: Fix these links to other libraries after the refactor is complete.

### Spring WebFlux based applications

* [backstopper-spring-web-flux](../backstopper-spring-web-flux) - For Spring WebFlux applications.

### Spring Web MVC based applications

* [backstopper-spring-boot1](../backstopper-spring-boot1) - For Spring Boot 1 applications.
* [backstopper-spring-boot2-webmvc](../backstopper-spring-boot2-webmvc) - For Spring Boot 2 applications using the
Spring MVC (Servlet) framework. If you want Spring Boot 2 with Spring WebFlux (Netty) framework, then see
* [backstopper-spring-boot3-webmvc](../backstopper-spring-boot3-webmvc) - For Spring Boot 3 applications using the
Spring MVC (Servlet) framework. If you want Spring Boot 3 with Spring WebFlux (Netty) framework, then see
[backstopper-spring-web-flux](../backstopper-spring-web-flux) instead.
* [backstopper-spring-web-mvc](../backstopper-spring-web-mvc) - For Spring Web MVC applications that are not
Spring Boot.
Expand Down
2 changes: 1 addition & 1 deletion testonly/testonly-springboot3_0-webflux/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,6 @@ dependencies {
)
}

// We're just running tests, not trying to stand up a real Springboot 2 server from gradle.
// We're just running tests, not trying to stand up a real Springboot 3 server from gradle.
// Disable the bootJar task so gradle doesn't fall over.
bootJar.enabled = false
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
*/
@SpringBootApplication
@ComponentScan(basePackages = {
// Component scan the core Backstopper+Springboot1 support.
// Component scan the core Backstopper+Springboot support.
"com.nike.backstopper",
// Component scan the controller.
"testonly.componenttest.spring.reusable.controller"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
*/
@SpringBootApplication
@Import({
// Import core Backstopper+Springboot1 support.
// Import core Backstopper+Springboot support.
BackstopperSpringboot3WebMvcConfig.class,
// Import the controller.
SampleController.class
Expand Down
Loading

0 comments on commit d7ac08d

Please sign in to comment.