Skip to content

Commit

Permalink
refactor(retrofit): Dismantle RetrofitExceptionHandler
Browse files Browse the repository at this point in the history
As the final step towards cleaning up RetrofitError and to incorporate the use of retrofit 2.x, this commit aims at dismantling RetrofitExceptionHandler.

Class: https://github.com/spinnaker/orca/blob/a1b32d7398eb9b1ff610d7f3afd980b51f6cf7b1/orca-retrofit/src/main/java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java has already taken place as the replacement for RetrofitExceptionHandler as part of migration process. This class is used in many places across ORCA to handle the upcoming changes of retrofit 2.x.

Since there are no relevant usage found for RetrofitExceptionHandler, its dismantled.
  • Loading branch information
Pranav-b-7 authored and Pranav-b-7 committed Apr 30, 2024
1 parent bc76db0 commit 8fb69a5
Show file tree
Hide file tree
Showing 7 changed files with 3 additions and 498 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import com.netflix.spinnaker.orca.clouddriver.utils.CloudProviderAware

import com.netflix.spinnaker.orca.clouddriver.utils.ClusterDescriptor
import com.netflix.spinnaker.orca.clouddriver.utils.ServerGroupDescriptor
import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler
import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler
import groovy.util.logging.Slf4j
import org.springframework.beans.factory.annotation.Autowired
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import com.netflix.spinnaker.orca.pipeline.util.StageNavigator;
import com.netflix.spinnaker.orca.q.DummyTask;
import com.netflix.spinnaker.orca.q.RunTask;
import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler;
import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler;
import com.netflix.spinnaker.q.Queue;
import java.time.Clock;
Expand All @@ -75,7 +74,6 @@
import retrofit.RetrofitError;
import retrofit.client.Response;
import retrofit.converter.GsonConverter;
import retrofit.mime.TypedByteArray;
import retrofit.mime.TypedString;

/**
Expand Down Expand Up @@ -146,14 +144,13 @@ class RunTaskHandlerExceptionHandlerTest {
new TypedString(
"{ message: \"arbitrary message\", error: \"error property\", errors: [\"error one\", \"error two\"], messages: [\"message one\", \"message two\"] }"));

private RetrofitExceptionHandler retrofitExceptionHandler = new RetrofitExceptionHandler();
private SpinnakerServerExceptionHandler spinnakerServerExceptionHandler =
new SpinnakerServerExceptionHandler();
private DefaultExceptionHandler defaultExceptionHandler = new DefaultExceptionHandler();

/** Put RetrofitExceptionHandler first since its bean is marked as highest precedence */
private List<ExceptionHandler> exceptionHandlers =
List.of(retrofitExceptionHandler, spinnakerServerExceptionHandler, defaultExceptionHandler);
List.of(spinnakerServerExceptionHandler, defaultExceptionHandler);

private Queue queue = mock(Queue.class);

Expand Down Expand Up @@ -249,57 +246,6 @@ void setup() {
.thenReturn(pipeline);
}

@ParameterizedTest(name = "{index} => taskThrowsRetrofitErrorNoRetry {0}")
@MethodSource("nonRetryableRetrofitErrors")
void taskThrowsRetrofitErrorNoRetry(RetrofitError retrofitError) {
// given an arbitrary RetrofitError that RetrofitExceptionHandler doesn't consider retryable
doThrow(retrofitError).when(dummyTask).execute(any());

// when
runTaskHandler.handle(runTaskMessage);

// verify that the exception wasn't retried
verify(queue, never()).push(eq(runTaskMessage), any());

// verify that exception handling has populated the stage context as
// expected. This duplicates some logic in RetrofitExceptionHandler, but at
// least it helps detect future changes.
Map<String, Object> responseBody = (Map<String, Object>) retrofitError.getBodyAs(Map.class);
Response response = retrofitError.getResponse();
String responseBodyString = new String(((TypedByteArray) response.getBody()).getBytes());
String error = (String) responseBody.getOrDefault("error", response.getReason());
List<String> errors =
(List<String>)
responseBody.getOrDefault("errors", responseBody.getOrDefault("messages", List.of()));
String message = (String) responseBody.get("message");
if (errors.isEmpty() && (message != null)) {
errors = List.of(message);
}

ImmutableMap.Builder<String, Object> builder =
ImmutableMap.<String, Object>builder()
.put("responseBody", responseBodyString)
.put("kind", RetrofitError.Kind.HTTP)
.put("error", error)
.put("errors", errors)
.put("url", response.getUrl())
.put("status", response.getStatus());

Object exception = responseBody.get("exception");
if (exception != null) {
builder.put("rootException", exception);
}

Map<String, Object> responseDetails = builder.build();

ExceptionHandler.Response expectedResponse =
new ExceptionHandler.Response(
"RetrofitError", "unspecified", responseDetails, false /* shouldRetry */);

compareResponse(
expectedResponse, (ExceptionHandler.Response) stageExecution.getContext().get("exception"));
}

private static Stream<RetrofitError> nonRetryableRetrofitErrors() {
return Stream.of(
makeRetrofitError(response),
Expand All @@ -310,56 +256,6 @@ private static Stream<RetrofitError> nonRetryableRetrofitErrors() {
makeRetrofitError(responseWithErrorAndErrorsAndMessages));
}

@Test
void taskThrowsRetrofitErrorRetryable() {
// given an arbitrary RetrofitError that RetrofitExceptionHandler considers retryable
RetrofitError retrofitError = makeRetrofitError(response503);
doThrow(retrofitError).when(dummyTask).execute(any());

// when
runTaskHandler.handle(runTaskMessage);

// verify that the exception was retried
verify(queue).push(eq(runTaskMessage), any());

// On retry, expect no exception info in the context
assertThat(stageExecution.getContext().get("exception")).isNull();
}

@Test
void taskThrowsRetrofitErrorNonJsonResponse() {
// given an arbitrary RetrofitError that RetrofitExceptionHandler doesn't consider retryable
Response response =
new Response(URL, 500, "arbitrary reason", List.of(), new TypedString("non-json response"));
RetrofitError retrofitError = makeRetrofitError(response);
doThrow(retrofitError).when(dummyTask).execute(any());

// when
runTaskHandler.handle(runTaskMessage);

// verify that the exception wasn't retried
verify(queue, never()).push(eq(runTaskMessage), any());

// verify that exception handling has populated the stage context as
// expected. This duplicates some logic in RetrofitExceptionHandler, but at
// least it helps detect future changes.
Map<String, Object> responseDetails =
Map.of(
"error", response.getReason(),
"errors", List.of(),
"responseBody", "non-json response",
"kind", RetrofitError.Kind.HTTP,
"url", response.getUrl(),
"status", response.getStatus());

ExceptionHandler.Response expectedResponse =
new ExceptionHandler.Response(
"RetrofitError", "unspecified", responseDetails, false /* shouldRetry */);

compareResponse(
expectedResponse, (ExceptionHandler.Response) stageExecution.getContext().get("exception"));
}

@ParameterizedTest(name = "{index} => taskThrowsSpinnakerHttpExceptionNoRetry {0}")
@MethodSource("nonRetryableSpinnakerHttpExceptions")
void taskThrowsSpinnakerHttpExceptionNoRetry(SpinnakerHttpException spinnakerHttpException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import com.jakewharton.retrofit.Ok3Client
import com.netflix.spectator.api.Registry
import com.netflix.spinnaker.config.OkHttp3ClientConfiguration
import com.netflix.spinnaker.config.OkHttpClientConfiguration
import com.netflix.spinnaker.orca.retrofit.exceptions.RetrofitExceptionHandler
import com.netflix.spinnaker.orca.retrofit.exceptions.SpinnakerServerExceptionHandler
import groovy.transform.CompileStatic
import okhttp3.Interceptor
Expand Down Expand Up @@ -99,18 +98,6 @@ class RetrofitConfiguration {
return LogLevel.valueOf(retrofitLogLevel)
}

/**
* Set the order such that this has higher precedence than the
* DefaultExceptionHandler bean. The order relative to the
* SpinnakerServerExceptionHandler bean isn't important since they
* handle different types.
*/
@Bean
@Order(Ordered.HIGHEST_PRECEDENCE)
RetrofitExceptionHandler retrofitExceptionHandler() {
new RetrofitExceptionHandler()
}

/**
* Set the order such that this has higher precedence than the
* DefaultExceptionHandler bean. The order relative to the
Expand Down

This file was deleted.

Loading

0 comments on commit 8fb69a5

Please sign in to comment.