refactor(front50): Enhance Exception Handling in Front50Service Retro…

…fit Client

These changes lay the groundwork for a smooth and successful upgrade to retrofit2.x. The refactoring ensures that the retrofit client in 'Front50Service' is well-prepared for the upcoming version, and the accompanying test coverage guarantees the stability of the exception handling logic.

Key Changes:

1. Error Handler Addition:
    - Integrated the ‘SpinnakerRetrofitErrorHandler’ as the error handler in the front50 retrofit configuration. This sets the stage for a seamless transition to retrofit2.x, ensuring a standardised approach to error handling.

2. Catch Block Replacement:
    - Replaced the catch block that previously caught ‘RetrofitError’ with catching ‘Spinnaker*Exception’. This aligns the exception handling logic with the new retrofit version, enhancing compatibility and maintainability.

3. Test Cases Coverage:
    - Covered test cases for each of the changes made, ensuring comprehensive testing of the refactored exception handling logic. This step is crucial to validate the correctness and reliability of the updated error handling.

orca-applications/src/main/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTask.groovy

@@ -18,6 +18,8 @@ package com.netflix.spinnaker.orca.applications.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.front50.Front50Service
@@ -74,26 +76,43 @@ class DeleteApplicationTask extends AbstractFront50Task {
         front50Service.delete(application.name)
         try {
           front50Service.deletePermission(application.name)
-        } catch (RetrofitError re) {
-          if (re.response?.status == 404) {
+        } catch (SpinnakerHttpException re) {
+          if (re.responseCode == 404) {
             return TaskResult.SUCCEEDED
           }
           log.error("Could not delete application permission", re)
           return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+        } catch (SpinnakerServerException serverException){
+          log.error("Could not delete application permission", serverException)
+          return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
         }
         // delete Managed Delivery data
         if (keelService != null) {
           log.debug("Deleting Managed Delivery data for {}", application.name)
           keelService.deleteDeliveryConfig(application.name)
         }
       }
-    } catch (RetrofitError e) {
+    }/**
+     * @see KeelService#deleteDeliveryConfig(java.lang.String) is invoked in the above try block.
+     * Since it throws RetrofitError, the below catch block with RetrofitError is still relevant.
+     * */
+    catch (RetrofitError e) {
       if (e.response?.status == 404) {
         return TaskResult.SUCCEEDED
       }
       log.error("Could not delete application", e)
       return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
     }
+    catch (SpinnakerHttpException e){
+      if (e.responseCode == 404) {
+        return TaskResult.SUCCEEDED
+      }
+      log.error("Could not delete application", e)
+      return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+    } catch (SpinnakerServerException e){
+      log.error("Could not delete application", e)
+      return TaskResult.builder(ExecutionStatus.TERMINAL).outputs(outputs).build()
+    }
     return TaskResult.builder(ExecutionStatus.SUCCEEDED).outputs(outputs).build()
   }
 

orca-applications/src/test/groovy/com/netflix/spinnaker/orca/applications/tasks/DeleteApplicationTaskSpec.groovy

@@ -18,11 +18,19 @@ package com.netflix.spinnaker.orca.applications.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.front50.model.Application
 import com.netflix.spinnaker.orca.KeelService
+import org.springframework.http.HttpStatus
+import retrofit.RetrofitError
 import retrofit.client.Response
+import retrofit.converter.ConversionException
+import retrofit.converter.JacksonConverter
 import spock.lang.Specification
 import spock.lang.Subject
 
@@ -105,4 +113,295 @@ class DeleteApplicationTaskSpec extends Specification {
     then:
     taskResult.status == ExecutionStatus.SUCCEEDED
   }
+
+  void "should handle SpinnakerHttpException if the response code is 404 while fetching applications and return the task status as SUCCEEDED"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      get(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        Response response = new Response(url, HttpStatus.NOT_FOUND.value(), HttpStatus.NOT_FOUND.name(), [], null)
+        RetrofitError httpError = RetrofitError.httpError(url, response, new JacksonConverter(), null)
+        throw new SpinnakerHttpException(httpError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.SUCCEEDED
+
+  }
+
+  void "should catch SpinnakerHttpException if the response code is not 404 while fetching applications and return the task status as TERMINAL"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      get(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        Response response = new Response(url, HttpStatus.BAD_REQUEST.value(), HttpStatus.BAD_REQUEST.name(), [], null)
+        RetrofitError httpError = RetrofitError.httpError(url, response, new JacksonConverter(), null)
+        throw new SpinnakerHttpException(httpError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+
+  }
+
+  void "should catch SpinnakerNetworkException  and return the task status as TERMINAL"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      get(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        RetrofitError networkError = RetrofitError.networkError(url, new IOException("Failed to connect to the host : front50service.com"))
+        throw new SpinnakerNetworkException(networkError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+
+  }
+
+  void "should catch SpinnakerNetworkException which occurs while deleting application  and return the task status as TERMINAL"() {
+    given:
+    def application = new Application()
+    application.name = "testapp"
+    application.email = "[email protected]"
+    application.user = "testuser"
+    task.front50Service = Mock(Front50Service) {
+      1 * get(config.application.name) >> application
+      1 * delete(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        RetrofitError networkError = RetrofitError.networkError(url, new IOException("Failed to connect to the host : front50service.com"))
+        throw new SpinnakerNetworkException(networkError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+    taskResult.context.previousState == application
+  }
+
+  void "should catch SpinnakerServerException which occurs while deleting application  and return the task status as TERMINAL"() {
+    given:
+    def application = new Application()
+    application.name = "testapp"
+    application.email = "[email protected]"
+    application.user = "testuser"
+    task.front50Service = Mock(Front50Service) {
+      1 * get(config.application.name) >> application
+      1 * delete(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        RetrofitError unexpectedError = RetrofitError.unexpectedError(url, new IOException("Something went wrong, please try again"))
+        throw new SpinnakerServerException(unexpectedError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+    taskResult.context.previousState == application
+  }
+
+  void "should catch SpinnakerHttpException which occurs while deleting application  and return the task status as TERMINAL"() {
+    given:
+    def application = new Application()
+    application.name = "testapp"
+    application.email = "[email protected]"
+    application.user = "testuser"
+    task.front50Service = Mock(Front50Service) {
+      1 * get(config.application.name) >> application
+      1 * delete(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        Response response = new Response(url, HttpStatus.UNAUTHORIZED.value(), HttpStatus.UNAUTHORIZED.name(), [], null)
+        RetrofitError httpError = RetrofitError.httpError(url, response, new JacksonConverter(), null)
+        throw new SpinnakerHttpException(httpError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+    taskResult.context.previousState == application
+  }
+
+  void "should catch SpinnakerHttpException  with 404 response code which occurs while deleting application  and return the task status as SUCCEEDED"() {
+    given:
+    def application = new Application()
+    application.name = "testapp"
+    application.email = "[email protected]"
+    application.user = "testuser"
+    task.front50Service = Mock(Front50Service) {
+      1 * get(config.application.name) >> application
+      1 * delete(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        Response response = new Response(url, HttpStatus.NOT_FOUND.value(), HttpStatus.NOT_FOUND.name(), [], null)
+        RetrofitError httpError = RetrofitError.httpError(url, response, new JacksonConverter(), null)
+        throw new SpinnakerHttpException(httpError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.SUCCEEDED
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+  }
+
+  void "should catch SpinnakerConversionException which occurs while deleting application  and return the task status as TERMINAL"() {
+    given:
+    def application = new Application()
+    application.name = "testapp"
+    application.email = "[email protected]"
+    application.user = "testuser"
+    task.front50Service = Mock(Front50Service) {
+      1 * get(config.application.name) >> application
+      1 * delete(config.application.name) >> {
+        def url = "https://front50service.com/v2/applications/"+config.application.name
+        Response response = new Response(url, HttpStatus.NOT_ACCEPTABLE.value(), HttpStatus.NOT_ACCEPTABLE.name(), [], null)
+        RetrofitError conversionError = RetrofitError.conversionError(url, response, new JacksonConverter(), null, new ConversionException("Failed to parse the http error body"))
+        throw new SpinnakerConversionException(conversionError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+    taskResult.context.previousState == application
+  }
+
+  void "should catch SpinnakerHttpException  with 404 response code which occurs while deleting application permission  and return the task status as SUCCEEDED"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      def url = "https://front50service.com/v2/applications/"+config.application.name
+      1 * get(config.application.name) >>  new Application()
+      1 * delete(config.application.name) >> new Response(url, HttpStatus.NO_CONTENT.value(), HttpStatus.NO_CONTENT.name(), Collections.emptyList(), null)
+      1 * deletePermission(config.application.name) >> {
+        Response response = new Response(url, HttpStatus.NOT_FOUND.value(), HttpStatus.NOT_FOUND.name(), [], null)
+        RetrofitError httpError = RetrofitError.httpError(url, response, new JacksonConverter(), null)
+        throw new SpinnakerHttpException(httpError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.SUCCEEDED
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+  }
+
+  void "should catch SpinnakerHttpException  with 400 response code which occurs while deleting application permission  and return the task status as TERMINAL"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      def url = "https://front50service.com/v2/applications/"+config.application.name
+      1 * get(config.application.name) >>  new Application()
+      1 * delete(config.application.name) >> new Response(url, HttpStatus.NO_CONTENT.value(), HttpStatus.NO_CONTENT.name(), Collections.emptyList(), null)
+      1 * deletePermission(config.application.name) >> {
+        Response response = new Response(url, HttpStatus.BAD_REQUEST.value(), HttpStatus.BAD_REQUEST.name(), [], null)
+        RetrofitError httpError = RetrofitError.httpError(url, response, new JacksonConverter(), null)
+        throw new SpinnakerHttpException(httpError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+  }
+
+  void "should catch SpinnakerConversionException which occurs while deleting application permission  and return the task status as TERMINAL"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      def url = "https://front50service.com/v2/applications/"+config.application.name
+      1 * get(config.application.name) >>  new Application()
+      1 * delete(config.application.name) >> new Response(url, HttpStatus.NO_CONTENT.value(), HttpStatus.NO_CONTENT.name(), Collections.emptyList(), null)
+      1 * deletePermission(config.application.name) >> {
+        Response response = new Response(url, HttpStatus.NOT_ACCEPTABLE.value(), HttpStatus.NOT_ACCEPTABLE.name(), [], null)
+        RetrofitError conversionError = RetrofitError.conversionError(url, response, new JacksonConverter(), null, new ConversionException("Failed to parse"))
+        throw new SpinnakerConversionException(conversionError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+  }
+
+  void "should catch SpinnakerNetworkException which occurs while deleting application permission  and return the task status as TERMINAL"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      def url = "https://front50service.com/v2/applications/"+config.application.name
+      1 * get(config.application.name) >>  new Application()
+      1 * delete(config.application.name) >> new Response(url, HttpStatus.NO_CONTENT.value(), HttpStatus.NO_CONTENT.name(), Collections.emptyList(), null)
+      1 * deletePermission(config.application.name) >> {
+        RetrofitError networkError = RetrofitError.networkError(url, new IOException("Failed to connect to the host : front50service.com"))
+        throw new SpinnakerNetworkException(networkError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+  }
+
+  void "should catch SpinnakerServerException which occurs while deleting application permission  and return the task status as TERMINAL"() {
+    given:
+    task.front50Service = Mock(Front50Service) {
+      def url = "https://front50service.com/v2/applications/"+config.application.name
+      1 * get(config.application.name) >>  new Application()
+      1 * delete(config.application.name) >> new Response(url, HttpStatus.NO_CONTENT.value(), HttpStatus.NO_CONTENT.name(), Collections.emptyList(), null)
+      1 * deletePermission(config.application.name) >> {
+        RetrofitError unexpectedError = RetrofitError.unexpectedError(url, new IOException("Something went wrong, Please try again"))
+        throw new SpinnakerServerException(unexpectedError)
+      }
+    }
+
+    when:
+    def taskResult = task.execute(pipeline.stages.first())
+
+    then:
+    taskResult.status == ExecutionStatus.TERMINAL
+    taskResult.context.get("notification.type") == "deleteapplication"
+    taskResult.context.get("application.name") == "application"
+  }
 }

orca-bakery/orca-bakery.gradle

@@ -38,6 +38,7 @@ dependencies {
   testImplementation("org.junit.jupiter:junit-jupiter-api")
   testImplementation("org.assertj:assertj-core")
   testImplementation("org.mockito:mockito-core")
+  testImplementation(("io.spinnaker.kork:kork-test"))
 
   testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine")
 }