refactor(retrofit/test): simplify the code by replacing RetrofitExcep…

…tion.httpError with SpinnakerHttpException (#1079)

* test(retrofit): demonstrate behavior of SpinnakerHttpException built from a RetrofitError

* refactor(retrofit): RetrofitException.getErrorBodyAs() returns a Map to allow to set custom properties in error response body

* refactor(retrofit): move getErrorBodyAs() in SpinnakerHttpException to remove RetrofitException class later

* refactor(retrofit): remove type parameter from getErrorBodyAs() since it will always be of type Map

* refactor(retrofit): move retryable condition to SpinnakerHttpException to remove createSpinnakerHttpException() later

* refactor(retrofit): remove createSpinnakerHttpException() to remove RetrofitException.httpError() later

* refactor(retrofit/test): remove reference to RetrofitException class in all files to simplify code

* refactor(retrofit): make retrofit as final for readability

* refactor(retrofit/test): delete RetrofitException class and tests to simplify code

* refactor(retrofit): changes to process different responses at runtime

* refactor(retrofit): make SpinnakerHttpException.getErrorBodyAs private

and pass it a Retrofit object so we no longer need a Retrofit member

* refactor(retrofit): replace @NonnullByDefault with @NullableByDefault in SpinnakerHttpException

since most members and methods are nullable

* refactor(retrofit): remove unnecessary final from SpinnakerHttpException.getRawMessage declaration

---------

Co-authored-by: David Byron <[email protected]>

kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/ErrorHandlingExecutorCallAdapterFactory.java

@@ -16,7 +16,6 @@
 
 package com.netflix.spinnaker.kork.retrofit;
 
-import com.netflix.spinnaker.kork.retrofit.exceptions.RetrofitException;
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
@@ -26,11 +25,9 @@
 import java.lang.reflect.Type;
 import java.util.Objects;
 import java.util.concurrent.Executor;
-import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import okhttp3.Request;
 import okio.Timeout;
-import org.springframework.http.HttpStatus;
 import retrofit2.Call;
 import retrofit2.CallAdapter;
 import retrofit2.Callback;
@@ -157,18 +154,7 @@ public Response<T> execute() {
  } catch (Exception e) {
  throw new SpinnakerServerException(e);
  }
- throw createSpinnakerHttpException(syncResp);
- }
-
- @Nonnull
- private SpinnakerHttpException createSpinnakerHttpException(Response<T> response) {
- SpinnakerHttpException retval =
- new SpinnakerHttpException(RetrofitException.httpError(response, retrofit));
- if ((response.code() == HttpStatus.NOT_FOUND.value())
- || (response.code() == HttpStatus.BAD_REQUEST.value())) {
- retval.setRetryable(false);
- }
- return retval;
+ throw new SpinnakerHttpException(syncResp, retrofit);
  }
 
  /**
@@ -249,7 +235,7 @@ public void run() {
  public void run() {
  callback.onFailure(
  executorCallbackCall,
- executorCallbackCall.createSpinnakerHttpException(response));
+ new SpinnakerHttpException(response, executorCallbackCall.retrofit));
  }
  });
  }

kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpException.java

@@ -17,25 +17,34 @@
 package com.netflix.spinnaker.kork.retrofit.exceptions;
 
 import com.google.common.base.Preconditions;
-import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
+import com.netflix.spinnaker.kork.annotations.NullableByDefault;
+import java.io.IOException;
+import java.lang.annotation.Annotation;
 import java.util.HashMap;
 import java.util.Map;
+import javax.annotation.Nonnull;
+import okhttp3.ResponseBody;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpStatus;
 import retrofit.RetrofitError;
 import retrofit.client.Response;
+import retrofit2.Converter;
+import retrofit2.Retrofit;
 
 /**
  * An exception that exposes the {@link Response} of a given HTTP {@link RetrofitError} or {@link
- * okhttp3.Response} of a {@link RetrofitException} if retrofit 2.x used and a detail message that
- * extracts useful information from the {@link Response} or {@link okhttp3.Response}. Both {@link
- * Response} and {@link okhttp3.Response} can't be set together..
+ * okhttp3.Response} if retrofit 2.x used and a detail message that extracts useful information from
+ * the {@link Response} or {@link okhttp3.Response}. Both {@link Response} and {@link
+ * okhttp3.Response} can't be set together.
  */
-@NonnullByDefault
+@NullableByDefault
 public class SpinnakerHttpException extends SpinnakerServerException {
+
  private final Response response;
+
  private HttpHeaders headers;
 
- private final retrofit2.Response retrofit2Response;
+ private final retrofit2.Response<?> retrofit2Response;
 
  /**
  * A message derived from a RetrofitError's response body, or null if a custom message has been
@@ -57,18 +66,26 @@ public SpinnakerHttpException(RetrofitError e) {
  : e.getMessage();
  }
 
- public SpinnakerHttpException(RetrofitException e) {
- super(e);
+ /**
+ * The constructor handles the HTTP retrofit2 exception, similar to retrofit logic. It is used
+ * with {@link com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory}.
+ */
+ public SpinnakerHttpException(
+ retrofit2.Response<?> retrofit2Response, retrofit2.Retrofit retrofit) {
  this.response = null;
- this.retrofit2Response = e.getResponse();
- responseBody = (Map<String, Object>) e.getErrorBodyAs(HashMap.class);
+ this.retrofit2Response = retrofit2Response;
+ if ((retrofit2Response.code() == HttpStatus.NOT_FOUND.value())
+ || (retrofit2Response.code() == HttpStatus.BAD_REQUEST.value())) {
+ setRetryable(false);
+ }
+ responseBody = this.getErrorBodyAs(retrofit);
  this.rawMessage =
  responseBody != null
- ? (String) responseBody.getOrDefault("message", e.getMessage())
- : e.getMessage();
+ ? (String) responseBody.getOrDefault("message", retrofit2Response.message())
+ : retrofit2Response.message();
  }
 
- private final String getRawMessage() {
+ private String getRawMessage() {
  return rawMessage;
  }
 
@@ -108,6 +125,7 @@ public int getResponseCode() {
  }
  }
 
+ @Nonnull
  public HttpHeaders getHeaders() {
  if (headers == null) {
  headers = new HttpHeaders();
@@ -157,4 +175,24 @@ public SpinnakerHttpException newInstance(String message) {
  public Map<String, Object> getResponseBody() {
  return this.responseBody;
  }
+
+ /**
+ * HTTP response body converted to specified {@code type}. {@code null} if there is no response.
+ *
+ * @throws RuntimeException wrapping the underlying IOException if unable to convert the body to
+ * the specified {@code type}.
+ */
+ private Map<String, Object> getErrorBodyAs(Retrofit retrofit) {
+ if (retrofit2Response == null) {
+ return null;
+ }
+
+ Converter<ResponseBody, Map> converter =
+ retrofit.responseBodyConverter(Map.class, new Annotation[0]);
+ try {
+ return converter.convert(retrofit2Response.errorBody());
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }
 }

kork-retrofit/src/main/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerServerException.java

@@ -31,6 +31,8 @@ public SpinnakerServerException(Throwable cause) {
  super(cause);
  }
 
+ public SpinnakerServerException() {}
+
  @Override
  public SpinnakerServerException newInstance(String message) {
  return new SpinnakerServerException(message, this);

kork-retrofit/src/test/java/com/netflix/spinnaker/kork/retrofit/exceptions/SpinnakerHttpExceptionTest.java

@@ -17,10 +17,12 @@
 
 package com.netflix.spinnaker.kork.retrofit.exceptions;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
+import com.google.gson.Gson;
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
 import java.util.List;
 import java.util.Map;
@@ -30,12 +32,36 @@
 import org.springframework.http.HttpStatus;
 import retrofit.RetrofitError;
 import retrofit.client.Response;
+import retrofit.converter.GsonConverter;
+import retrofit.mime.TypedString;
 import retrofit2.Retrofit;
 import retrofit2.converter.jackson.JacksonConverterFactory;
 
 public class SpinnakerHttpExceptionTest {
  private static final String CUSTOM_MESSAGE = "custom message";
 
+ public void testSpinnakerHttpExceptionFromRetrofitError() {
+ String url = "http://localhost";
+ int statusCode = 200;
+ String message = "arbitrary message";
+ Response response =
+ new Response(
+ url,
+ statusCode,
+ "reason",
+ List.of(),
+ new TypedString("{ message: \"" + message + "\", name: \"test\" }"));
+ RetrofitError retrofitError =
+ RetrofitError.httpError(url, response, new GsonConverter(new Gson()), String.class);
+ SpinnakerHttpException spinnakerHttpException = new SpinnakerHttpException(retrofitError);
+ assertThat(spinnakerHttpException.getResponseBody()).isNotNull();
+ Map<String, Object> errorResponseBody = spinnakerHttpException.getResponseBody();
+ assertThat(errorResponseBody.get("name")).isEqualTo("test");
+ assertThat(spinnakerHttpException.getResponseCode()).isEqualTo(statusCode);
+ assertThat(spinnakerHttpException.getMessage())
+ .isEqualTo("Status: " + statusCode + ", URL: " + url + ", Message: " + message);
+ }
+
  @Test
  public void testSpinnakerHttpExceptionFromRetrofitException() {
  final String validJsonResponseBodyString = "{\"name\":\"test\"}";
@@ -50,8 +76,8 @@ public void testSpinnakerHttpExceptionFromRetrofitException() {
  .baseUrl("http://localhost")
  .addConverterFactory(JacksonConverterFactory.create())
  .build();
- RetrofitException retrofitException = RetrofitException.httpError(response, retrofit2Service);
- SpinnakerHttpException notFoundException = new SpinnakerHttpException(retrofitException);
+ SpinnakerHttpException notFoundException =
+  new SpinnakerHttpException(response, retrofit2Service);
  assertNotNull(notFoundException.getResponseBody());
  Map<String, Object> errorResponseBody = notFoundException.getResponseBody();
  assertEquals(errorResponseBody.get("name"), "test");