fix(retrofit): handle non-message properties when deserializing response body in SpinnakerServerException (spinnaker#913)

Co-authored-by: calvintse <[email protected]>

Author: dbyron-sf

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

@@ -17,6 +17,7 @@
 package com.netflix.spinnaker.kork.retrofit.exceptions;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
@@ -74,6 +75,12 @@ final String getRawMessage() {
   }
 
   @Getter
+  // Use JsonIgnoreProperties because some responses contain properties that
+  // cannot be mapped to the RetrofitErrorResponseBody class.  If the default
+  // JacksonConverter (with no extra configurations) is used to deserialize the
+  // response body and properties other than "message" exist in the JSON
+  // response, there will be an UnrecognizedPropertyException.
+  @JsonIgnoreProperties(ignoreUnknown = true)
   private static final class RetrofitErrorResponseBody {
     private final String message;
 

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

@@ -21,15 +21,22 @@
 import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
+import java.util.HashMap;
+import java.util.Map;
 import okhttp3.mockwebserver.MockResponse;
 import okhttp3.mockwebserver.MockWebServer;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.springframework.http.HttpStatus;
 import retrofit.RestAdapter;
 import retrofit.client.Response;
+import retrofit.converter.JacksonConverter;
 import retrofit.http.GET;
 
 public class SpinnakerRetrofitErrorHandlerTest {
@@ -55,16 +62,76 @@ public static void shutdownOnce() throws Exception {
   }
 
   @Test
-  public void testNotFoundIsNotRetryable() throws Exception {
+  public void testNotFoundIsNotRetryable() {
     mockWebServer.enqueue(new MockResponse().setResponseCode(HttpStatus.NOT_FOUND.value()));
     NotFoundException notFoundException =
         assertThrows(NotFoundException.class, () -> retrofitService.getFoo());
     assertNotNull(notFoundException.getRetryable());
     assertFalse(notFoundException.getRetryable());
   }
 
+  @ParameterizedTest(name = "Deserialize response using {0}")
+  // Test the different converters used to deserialize the response body to the
+  // SpinnakerServerException.RetrofitErrorResponseBody class:
+  //
+  // - the JacksonConverter constructed without an ObjectMapper is used in
+  //   Clouddriver's RestAdapter to communicate with Front50Service
+  //
+  // - the JacksonConverter constructed with an ObjectMapper is used in Rosco's RestAdapter to
+  //   communicate with Clouddriver
+  //
+  // - GSONConverter is the default converter used by Retrofit if no converter
+  //   is set when building out the RestAdapter
+  @ValueSource(
+      strings = {"Default_GSONConverter", "JacksonConverter", "JacksonConverterWithObjectMapper"})
+  public void testResponseWithExtraField(String retrofitConverter) throws Exception {
+    Map<String, String> responseBodyMap = new HashMap<>();
+    responseBodyMap.put("timestamp", "123123123123");
+    responseBodyMap.put("message", "Not Found error Message");
+    String responseBodyString = new ObjectMapper().writeValueAsString(responseBodyMap);
+
+    RestAdapter.Builder restAdapter =
+        new RestAdapter.Builder()
+            .setEndpoint(mockWebServer.url("/").toString())
+            .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance());
+
+    if (retrofitConverter.equals("JacksonConverter")) {
+      restAdapter.setConverter(new JacksonConverter());
+    } else if (retrofitConverter.equals("JacksonConverterWithObjectMapper")) {
+      ObjectMapper objectMapper =
+          new ObjectMapper()
+              .enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL)
+              .disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+
+      restAdapter.setConverter(new JacksonConverter(objectMapper));
+    }
+
+    RetrofitService retrofitServiceTestConverter =
+        restAdapter.build().create(RetrofitService.class);
+
+    mockWebServer.enqueue(
+        new MockResponse()
+            .setBody(responseBodyString)
+            // an arbitrary response code -- one that
+            // SpinnakerRetrofitErrorHandler converts to a
+            // SpinnakerServerException (or one of its children).
+            .setResponseCode(HttpStatus.INTERNAL_SERVER_ERROR.value()));
+
+    // If the converter can not deserialize the response body to the
+    // SpinnakerServerException.RetrofitErrorResponseBody
+    // class, then a RuntimeException will be thrown with a ConversionException nested inside.
+    //
+    // java.lang.RuntimeException:
+    //     retrofit.converter.ConversionException:
+    //         com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException: Unrecognized field
+    // "..."
+    //
+    // so make sure we get a SpinnakerHttpException from calling getFoo
+    assertThrows(SpinnakerHttpException.class, retrofitServiceTestConverter::getFoo);
+  }
+
   @Test
-  public void testBadRequestIsNotRetryable() throws Exception {
+  public void testBadRequestIsNotRetryable() {
     mockWebServer.enqueue(new MockResponse().setResponseCode(HttpStatus.BAD_REQUEST.value()));
     SpinnakerHttpException spinnakerHttpException =
         assertThrows(SpinnakerHttpException.class, () -> retrofitService.getFoo());
@@ -73,7 +140,7 @@ public void testBadRequestIsNotRetryable() throws Exception {
   }
 
   @Test
-  public void testOtherClientErrorHasNullRetryable() throws Exception {
+  public void testOtherClientErrorHasNullRetryable() {
     // Arbitrarily choose GONE as an example of a client (e.g. 4xx) error that
     // we expect to have null retryable
     mockWebServer.enqueue(new MockResponse().setResponseCode(HttpStatus.GONE.value()));