refactor(retrofit): if not null, use SpinnakerServerException.getHttp…

…Method() instead of reflection in SpinnakerServerExceptionHandler
spinnaker · Mar 14, 2024 · 5a58ab9 · 5a58ab9
1 parent 8a828b3
commit 5a58ab9
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 4 deletions.
diff --git a/...groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy b/...groovy/com/netflix/spinnaker/orca/retrofit/exceptions/BaseRetrofitExceptionHandler.groovy
@@ -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
     }
@@ -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) {
@@ -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) {

diff --git a/.../java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java b/.../java/com/netflix/spinnaker/orca/retrofit/exceptions/SpinnakerServerExceptionHandler.java
@@ -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";
@@ -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)
@@ -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));
   }
 }