Skip to content

Commit

Permalink
fix(retrofit): use SpinnakerServerException.getHttpMethod() instead o…
Browse files Browse the repository at this point in the history 
…f reflection in SpinnakerServerExceptionHandler (#4675)

* tests(retrofit): add retrofit2 exceptions to SpinnakerServerExceptionHandlerTest

* fix(retrofit): update shouldRetry method in SpinnakerServerExceptionHandler to default to using SpinnakerServerException.getHttpMethod() since findHttpMethodAnnotation returns null for retrofit2 exceptions

---------

Co-authored-by: abe garcia <[email protected]>
  • Loading branch information
@alice485
alice485 and abe garcia authored Mar 15, 2024
1 parent ad1b83c commit da1f0bb
Show file tree
Hide file tree
Showing 4 changed files with 140 additions and 7 deletions.
2 changes: 2 additions & 0 deletions orca-retrofit/orca-retrofit.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ dependencies {
testImplementation("org.junit.jupiter:junit-jupiter-params")
testImplementation("org.mockito:mockito-core")
testImplementation("org.springframework.boot:spring-boot-test")
testImplementation("com.squareup.okhttp3:mockwebserver")
testImplementation("com.squareup.retrofit2:converter-jackson")
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine")
testRuntimeOnly("org.springframework:spring-test")

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));
}
}
122 changes: 119 additions & 3 deletions ...a/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
package com.netflix.spinnaker.orca.retrofit.exceptions;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.catchThrowableOfType;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

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 +35,18 @@
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.OkHttpClient;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
import okhttp3.mockwebserver.SocketPolicy;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
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,9 +55,16 @@
import retrofit.converter.GsonConverter;
import retrofit.converter.JacksonConverter;
import retrofit.mime.TypedString;
import retrofit2.Call;
import retrofit2.Retrofit;
import retrofit2.converter.jackson.JacksonConverterFactory;

class SpinnakerServerExceptionHandlerTest {
private static final String URL = "https://some-url";
private static Retrofit2Service retrofit2Service;

private static final MockWebServer mockWebServer = new MockWebServer();

private static final String URL = mockWebServer.url("https://some-url").toString();

private static final String TASK_NAME = "task name";

Expand Down Expand Up @@ -135,6 +152,35 @@ class SpinnakerServerExceptionHandlerTest {
SpinnakerServerExceptionHandler spinnakerServerExceptionHandler =
new SpinnakerServerExceptionHandler();

@BeforeAll
static void setupOnce() {
retrofit2Service =
new Retrofit.Builder()
.baseUrl(mockWebServer.url("/"))
.client(
new OkHttpClient.Builder()
.callTimeout(1, TimeUnit.SECONDS)
.connectTimeout(1, TimeUnit.SECONDS)
.build())
.addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance())
.addConverterFactory(JacksonConverterFactory.create())
.build()
.create(Retrofit2Service.class);
}

@AfterAll
static void shutdownOnce() throws Exception {
mockWebServer.shutdown();
}

interface Retrofit2Service {
@retrofit2.http.GET("/retrofit2")
Call<String> getRetrofit2();

@retrofit2.http.POST("/retrofit2")
Call<String> postRetrofit2();
}

@ParameterizedTest(name = "{index} => onlyHandlesSpinnakerServerExceptionAndSubclasses {0}")
@MethodSource("exceptionsForHandlesTest")
void onlyHandlesSpinnakerServerExceptionAndSubclasses(Exception ex, boolean supported) {
Expand All @@ -153,12 +199,71 @@ 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() {
String responseBody = "{\"test\":\"test\"}";

// HTTP 503 is always retryable
mockWebServer.enqueue(
new MockResponse()
.setResponseCode(HttpStatus.SERVICE_UNAVAILABLE.value())
.setBody(responseBody));
SpinnakerServerException retryable =
catchThrowableOfType(
() -> retrofit2Service.getRetrofit2().execute(), SpinnakerServerException.class);

// not retryable because POST is not idempotent
mockWebServer.enqueue(
new MockResponse().setResponseCode(HttpStatus.BAD_REQUEST.value()).setBody(responseBody));
SpinnakerServerException notRetryable =
catchThrowableOfType(
() -> retrofit2Service.postRetrofit2().execute(), SpinnakerServerException.class);

// not retryable despite idempotent method because response code not within 429, 502, 503, 504
mockWebServer.enqueue(
new MockResponse().setResponseCode(HttpStatus.UNAUTHORIZED.value()).setBody(responseBody));
SpinnakerServerException notRetryableIdempotent =
catchThrowableOfType(
() -> retrofit2Service.getRetrofit2().execute(), SpinnakerServerException.class);

// idempotent network errors are retryable
mockWebServer.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE));
SpinnakerServerException idempotentNetworkException =
catchThrowableOfType(
() -> retrofit2Service.getRetrofit2().execute(), SpinnakerNetworkException.class);

// throttling is retryable if idempotent
mockWebServer.enqueue(
new MockResponse()
.setResponseCode(HttpStatus.TOO_MANY_REQUESTS.value())
.setBody(responseBody));
SpinnakerServerException throttledIdempotent =
catchThrowableOfType(
() -> retrofit2Service.getRetrofit2().execute(), SpinnakerServerException.class);

// throttling is not retryable if not idempotent
mockWebServer.enqueue(
new MockResponse()
.setResponseCode(HttpStatus.TOO_MANY_REQUESTS.value())
.setBody(responseBody));
SpinnakerServerException throttledNonIdempotent =
catchThrowableOfType(
() -> retrofit2Service.postRetrofit2().execute(), SpinnakerServerException.class);

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 +325,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 +334,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 +344,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 +395,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

0 comments on commit da1f0bb

Please sign in to comment.