Skip to content

Commit

Permalink
refactor(retrofit): if not null, use SpinnakerServerException.getHttp…
Browse files Browse the repository at this point in the history 
…Method() instead of reflection in SpinnakerServerExceptionHandler
  • Loading branch information
abe garcia committed Mar 13, 2024
1 parent 74ba9ca commit 6cdc8d0
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 6 deletions.
1 change: 1 addition & 0 deletions orca-retrofit/orca-retrofit.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ dependencies {
implementation("io.spinnaker.kork:kork-retrofit")
implementation("com.jakewharton.retrofit:retrofit1-okhttp3-client:1.1.0")
implementation "com.google.guava:guava"
implementation "com.squareup.retrofit2:converter-jackson"

testImplementation("com.squareup.retrofit:retrofit-mock")
testImplementation("org.assertj:assertj-core")
Expand Down
14 changes: 11 additions & 3 deletions ...groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import static java.net.HttpURLConnection.*

abstract class BaseRetrofitExceptionHandler implements ExceptionHandler {
boolean shouldRetry(Exception e, String kind, Integer responseCode) {
return shouldRetry(e, kind, null, responseCode)
}

boolean shouldRetry(Exception e, String kind, String httpMethod, Integer responseCode) {
if (isMalformedRequest(kind, e.getMessage())) {
return false
}
Expand All @@ -33,7 +37,11 @@ abstract class BaseRetrofitExceptionHandler implements ExceptionHandler {
return true
}

return isIdempotentRequest(e) && (isNetworkError(kind) || isGatewayErrorCode(kind, responseCode) || isThrottle(kind, responseCode))
if(httpMethod == null) {
httpMethod = findHttpMethodAnnotation(e)
}

return isIdempotentRequest(httpMethod) && (isNetworkError(kind) || isGatewayErrorCode(kind, responseCode) || isThrottle(kind, responseCode))
}

private boolean isGatewayErrorCode(String kind, Integer responseCode) {
Expand All @@ -55,8 +63,8 @@ abstract class BaseRetrofitExceptionHandler implements ExceptionHandler {
return "UNEXPECTED".equals(kind) && exceptionMessage?.contains("Path parameter")
}

private static boolean isIdempotentRequest(Exception e) {
findHttpMethodAnnotation(e) in ["GET", "HEAD", "DELETE", "PUT"]
private static boolean isIdempotentRequest(String httpMethod) {
httpMethod in ["GET", "HEAD", "DELETE", "PUT"]
}

private static String findHttpMethodAnnotation(Exception exception) {
Expand Down
9 changes: 8 additions & 1 deletion .../java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public Response handle(String taskName, Exception exception) {

String kind;
Integer responseCode = null;
String httpMethod = ex.getHttpMethod();

if (ex instanceof SpinnakerNetworkException) {
kind = "NETWORK";
Expand Down Expand Up @@ -108,6 +109,12 @@ public Response handle(String taskName, Exception exception) {

responseDetails.put("kind", kind);

// http method may be null if exception is created from RetrofitError
// so only include in responseDetails when value is valid
if (httpMethod != null) {
responseDetails.put("method", httpMethod);
}

// Although Spinnaker*Exception has a retryable property that other parts of
// spinnaker use, ignore it here for compatibility with
// RetrofitExceptionHandler, specifically because that doesn't retry (most)
Expand All @@ -116,6 +123,6 @@ public Response handle(String taskName, Exception exception) {
ex.getClass().getSimpleName(),
taskName,
responseDetails,
shouldRetry(ex, kind, responseCode));
shouldRetry(ex, kind, httpMethod, responseCode));
}
}
112 changes: 110 additions & 2 deletions ...a/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableMap;
import com.google.gson.Gson;
import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
Expand All @@ -33,10 +34,13 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.TimeUnit;
import java.util.stream.Stream;
import okhttp3.*;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.springframework.http.HttpStatus;
import retrofit.RestAdapter;
import retrofit.RetrofitError;
import retrofit.client.Client;
Expand All @@ -45,6 +49,8 @@
import retrofit.converter.GsonConverter;
import retrofit.converter.JacksonConverter;
import retrofit.mime.TypedString;
import retrofit2.Retrofit;
import retrofit2.converter.jackson.JacksonConverterFactory;

class SpinnakerServerExceptionHandlerTest {
private static final String URL = "https://some-url";
Expand Down Expand Up @@ -153,12 +159,82 @@ private static Stream<Arguments> exceptionsForHandlesTest() {
}

@ParameterizedTest(name = "{index} => testRetryable {0}")
@MethodSource("exceptionsForRetryTest")
@MethodSource({"exceptionsForRetryTest", "retrofit2Exceptions"})
void testRetryable(SpinnakerServerException ex, boolean expectedRetryable) {
ExceptionHandler.Response response = spinnakerServerExceptionHandler.handle(TASK_NAME, ex);
assertThat(response.isShouldRetry()).isEqualTo(expectedRetryable);
}

private static Stream<Arguments> retrofit2Exceptions() {
Retrofit retrofit2Service =
new Retrofit.Builder()
.baseUrl(URL)
.client(
new OkHttpClient.Builder()
.callTimeout(1, TimeUnit.SECONDS)
.connectTimeout(1, TimeUnit.SECONDS)
.build())
.addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
.addConverterFactory(JacksonConverterFactory.create())
.build();

// HTTP 503 is always retryable
retrofit2.Response response =
retrofit2.Response.error(HttpStatus.SERVICE_UNAVAILABLE.value(), getDummyResponseBody());
SpinnakerServerException retryable = new SpinnakerHttpException(response, retrofit2Service);

// not retryable because POST is not idempotent
okhttp3.Request notRetryableRequest =
new okhttp3.Request.Builder().url(URL).post(getDummyRequestBody()).build();

retrofit2.Response notRetryableResponse =
makeRetrofit2Response(notRetryableRequest, HttpStatus.BAD_REQUEST.value());
SpinnakerServerException notRetryable =
new SpinnakerHttpException(notRetryableResponse, retrofit2Service);

// not retryable despite idempotent method because response code not within 429, 502, 503, 504
okhttp3.Request notRetryableIdempotentRequest =
new okhttp3.Request.Builder().url(URL).get().build();

retrofit2.Response notRetryableIdempotentResponse =
makeRetrofit2Response(notRetryableIdempotentRequest, HttpStatus.UNAUTHORIZED.value());
SpinnakerServerException notRetryableIdempotent =
new SpinnakerHttpException(notRetryableIdempotentResponse, retrofit2Service);

// idempotent network errors are retryable
okhttp3.Request retryableRequest = new okhttp3.Request.Builder().url(URL).get().build();

Throwable cause = new Throwable("error");
SpinnakerServerException idempotentNetworkException =
new SpinnakerNetworkException(cause, retryableRequest);

// throttling is retryable if idempotent
okhttp3.Request throttledIdempotentRequest =
new okhttp3.Request.Builder().url(URL).get().build();

retrofit2.Response throttledIdempotentResponse =
makeRetrofit2Response(throttledIdempotentRequest, HttpStatus.TOO_MANY_REQUESTS.value());
SpinnakerServerException throttledIdempotent =
new SpinnakerHttpException(throttledIdempotentResponse, retrofit2Service);

// throttling is not retryable if not idempotent
okhttp3.Request throttledNonIdempotentRequest =
new okhttp3.Request.Builder().url(URL).post(getDummyRequestBody()).build();

retrofit2.Response throttledNonIdempotentResponse =
makeRetrofit2Response(throttledNonIdempotentRequest, HttpStatus.TOO_MANY_REQUESTS.value());
SpinnakerServerException throttledNonIdempotent =
new SpinnakerHttpException(throttledNonIdempotentResponse, retrofit2Service);

return Stream.of(
Arguments.of(retryable, true),
Arguments.of(notRetryable, false),
Arguments.of(notRetryableIdempotent, false),
Arguments.of(idempotentNetworkException, true),
Arguments.of(throttledIdempotent, true),
Arguments.of(throttledNonIdempotent, false));
}

private static Stream<Arguments> exceptionsForRetryTest() throws Exception {
// This isn't retryable because it's not considered an idempotent request
// since there's no retrofit http method annotation in the exception's stack
Expand Down Expand Up @@ -220,7 +296,7 @@ private static Stream<Arguments> exceptionsForRetryTest() throws Exception {
}

@ParameterizedTest(name = "{index} => verifyResponseDetails {0}")
@MethodSource("exceptionsForResponseDetailsTest")
@MethodSource({"exceptionsForResponseDetailsTest", "nonRetryableRetrofit2Exceptions"})
void verifyResponseDetails(SpinnakerHttpException spinnakerHttpException) {
ExceptionHandler.Response response =
spinnakerServerExceptionHandler.handle(TASK_NAME, spinnakerHttpException);
Expand All @@ -229,6 +305,7 @@ void verifyResponseDetails(SpinnakerHttpException spinnakerHttpException) {
// expected. This duplicates some logic in SpinnakerServerExceptionHandler,
// but at least it helps detect future changes.
Map<String, Object> responseBody = spinnakerHttpException.getResponseBody();
String httpMethod = spinnakerHttpException.getHttpMethod();
String error = null;
List<String> errors = null;

Expand All @@ -238,6 +315,10 @@ void verifyResponseDetails(SpinnakerHttpException spinnakerHttpException) {
.put("url", spinnakerHttpException.getUrl())
.put("status", spinnakerHttpException.getResponseCode());

if (httpMethod != null) {
builder.put("method", httpMethod);
}

if (responseBody == null) {
error = spinnakerHttpException.getMessage();
errors = List.of();
Expand Down Expand Up @@ -285,6 +366,12 @@ private static Stream<SpinnakerHttpException> exceptionsForResponseDetailsTest()
.map(SpinnakerServerExceptionHandlerTest::makeSpinnakerHttpException);
}

private static Stream<SpinnakerHttpException> nonRetryableRetrofit2Exceptions() {
return retrofit2Exceptions()
.filter(args -> args.get()[1].equals(false))
.map(args -> ((SpinnakerHttpException) args.get()[0]));
}

private void compareResponse(
ExceptionHandler.Response expectedResponse, ExceptionHandler.Response actualResponse) {
assertThat(actualResponse).isNotNull();
Expand Down Expand Up @@ -312,4 +399,25 @@ private static <V> SpinnakerServerException expectingException(Callable<V> actio
return e;
}
}

private static ResponseBody getDummyResponseBody() {
return ResponseBody.create(MediaType.get("application/json"), "{\"test\":\"test\"}");
}

private static RequestBody getDummyRequestBody() {
return RequestBody.create(MediaType.get("application/json"), "{\"test\":\"test\"}");
}

private static retrofit2.Response makeRetrofit2Response(
okhttp3.Request request, Integer responseCode) {
okhttp3.Response rawResponse =
new okhttp3.Response.Builder()
.request(request)
.code(responseCode)
.protocol(Protocol.HTTP_1_1)
.message("test")
.build();

return retrofit2.Response.error(getDummyResponseBody(), rawResponse);
}
}

0 comments on commit 6cdc8d0

Please sign in to comment.