refactor(retrofit): remove use of RetrofitException.httpError to crea…

…te SpinnakerHttpException

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

@@ -18,11 +18,16 @@
 
 import com.google.common.base.Preconditions;
 import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
+import java.io.IOException;
+import java.lang.annotation.Annotation;
 import java.util.HashMap;
 import java.util.Map;
+import okhttp3.ResponseBody;
 import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpStatus;
 import retrofit.RetrofitError;
 import retrofit.client.Response;
+import retrofit2.Converter;
 
 /**
  * An exception that exposes the {@link Response} of a given HTTP {@link RetrofitError} or {@link
@@ -44,6 +49,10 @@ public class SpinnakerHttpException extends SpinnakerServerException {
  private final String rawMessage;
 
  private final Map<String, Object> responseBody;
+ private retrofit2.Retrofit retrofit;
+
+ private static Map<String, Object> jsonErrorResponseBody =
+ Map.of("message", "failed to parse response");
 
  public SpinnakerHttpException(RetrofitError e) {
  super(e);
@@ -57,17 +66,6 @@ public SpinnakerHttpException(RetrofitError e) {
  : e.getMessage();
  }
 
- public SpinnakerHttpException(RetrofitException e) {
- super(e);
- this.response = null;
- this.retrofit2Response = e.getResponse();
- responseBody = (Map<String, Object>) e.getErrorBodyAs(HashMap.class);
- this.rawMessage =
- responseBody != null
- ? (String) responseBody.getOrDefault("message", e.getMessage())
- : e.getMessage();
- }
-
  private final String getRawMessage() {
  return rawMessage;
  }
@@ -100,6 +98,28 @@ public SpinnakerHttpException(String message, SpinnakerHttpException cause) {
  this.responseBody = cause.responseBody;
  }
 
+ /**
+ * The constructor handles the HTTP retrofit2 exception, similar to retrofit logic. It is used
+ * with {@link com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory}.
+ */
+ public <T> SpinnakerHttpException(retrofit2.Response<T> syncResp, retrofit2.Retrofit retrofit) {
+ super(
+ syncResp.code() + " " + syncResp.message(),
+ new Throwable(syncResp.code() + " " + syncResp.message()));
+ this.retrofit2Response = syncResp;
+ this.response = null;
+ this.retrofit = retrofit;
+ responseBody = this.getErrorBodyAs();
+ this.rawMessage =
+ responseBody != null
+ ? (String) responseBody.getOrDefault("message", getMessage())
+ : getMessage();
+ if ((retrofit2Response.code() == HttpStatus.NOT_FOUND.value())
+ || (retrofit2Response.code() == HttpStatus.BAD_REQUEST.value())) {
+ setRetryable(false);
+ }
+ }
+
  public int getResponseCode() {
  if (response != null) {
  return response.getStatus();
@@ -157,4 +177,18 @@ public SpinnakerHttpException newInstance(String message) {
  public Map<String, Object> getResponseBody() {
  return this.responseBody;
  }
+
+ private Map<String, Object> getErrorBodyAs() {
+ 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) {
+ return jsonErrorResponseBody;
+ }
+ }
 }

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

@@ -96,13 +96,23 @@ 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");
  assertEquals(HttpStatus.NOT_FOUND.value(), notFoundException.getResponseCode());
- assertTrue(
- notFoundException.getMessage().contains(String.valueOf(HttpStatus.NOT_FOUND.value())));
+
+ // Note: Expect custom "Failed to parse response" message,
+ // instead of a RuntimeException when the converter fails to convert the response body. eg:
+ // JacksonResponseBodyConverter returns JsonParseException/JsonEOFException which are type of
+ // RuntimeException.
+ String expectedMessage =
+ String.format(
+ "Status: %s, URL: %s, Message: %s",
+ HttpStatus.NOT_FOUND.value(),
+ "http://localhost/",
+ HttpStatus.NOT_FOUND.value() + " " + "Response.error()");
+ assertEquals(expectedMessage, notFoundException.getMessage());
  }
 }