Merge pull request #131 from sartography/feature/improved_service_task_errors

Feature/improved service task errors

SpiffWorkflow/SpiffWorkflow/spiff/specs/service_task.py

@@ -1,6 +1,7 @@
 from copy import deepcopy
 import json
 from SpiffWorkflow.bpmn.specs.ServiceTask import ServiceTask
+from SpiffWorkflow.exceptions import WorkflowTaskException
 from SpiffWorkflow.spiff.specs.spiff_task import SpiffBpmnTask
 
 class ServiceTask(SpiffBpmnTask, ServiceTask):
@@ -31,9 +32,13 @@ class ServiceTask(SpiffBpmnTask, ServiceTask):
         operation_params_copy = deepcopy(self.operation_params)
         evaluated_params = {k: evaluate(v) for k, v in operation_params_copy.items()}
 
+        try:
             result = task.workflow.script_engine.call_service(self.operation_name,
                     evaluated_params, task.data)
-
+        except Exception as e:
+            wte = WorkflowTaskException("Error executing Service Task",
+                                        task=task, exception=e)
+            wte.add_note(str(e))
+            raise wte
         parsed_result = json.loads(result)
-
         task.data[self._result_variable(task)] = parsed_result

spiffworkflow-backend/src/spiffworkflow_backend/__init__.py

@@ -1,7 +1,7 @@
 """__init__."""
+import faulthandler
 import os
 import sys
-import faulthandler
 from typing import Any
 
 import connexion  # type: ignore

spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py

@@ -43,6 +43,49 @@ class ServiceTaskDelegate:
 
         return value
 
+    @staticmethod
+    def get_message_for_status(code: int) -> str:
+        """Given a code like 404, return a string like 'The requested resource was not found.'"""
+        msg = f"HTTP Status Code {code}."
+        if code == 301:
+            msg = (
+                "301 (Permanent Redirect) - you may need to use a different URL in this"
+                " service task."
+            )
+        if code == 302:
+            msg = (
+                "302 (Temporary Redirect) - you may need to use a different URL in this"
+                " service task."
+            )
+        if code == 400:
+            msg = (
+                "400 (Bad Request) - The request was received by the service, but it"
+                " was not understood."
+            )
+        if code == 401:
+            msg = (
+                "401 (Unauthorized Error) - this end point requires some form of"
+                " authentication."
+            )
+        if code == 403:
+            msg = (
+                "403 (Forbidden) - The service you called refused to accept the"
+                " request."
+            )
+        if code == 404:
+            msg = "404 (Not Found) - The service did not find the requested resource."
+        if code == 500:
+            msg = (
+                "500 (Internal Server Error) - The service you called is experiencing"
+                " technical difficulties."
+            )
+        if code == 501:
+            msg = (
+                "501 (Not Implemented) - This service needs to be called with the"
+                " different method (like POST not GET)."
+            )
+        return msg
+
     @staticmethod
     def call_connector(name: str, bpmn_params: Any, task_data: Any) -> str:
         """Calls a connector via the configured proxy."""
@@ -55,17 +98,43 @@ class ServiceTaskDelegate:
             params["spiff__task_data"] = task_data
 
             proxied_response = requests.post(call_url, json=params)
+            response_text = proxied_response.text
+            json_parse_error = None
 
-            parsed_response = json.loads(proxied_response.text)
+            if response_text == "":
+                response_text = "{}"
+            try:
+                parsed_response = json.loads(response_text)
+            except Exception as e:
+                json_parse_error = e
+                parsed_response = {}
+
+            if proxied_response.status_code >= 300:
+                error = f"Received an unexpected response from the service : "
+                error += ServiceTaskDelegate.get_message_for_status(
+                    proxied_response.status_code
+                )
+                if "error" in parsed_response:
+                    error += parsed_response["error"]
+                if json_parse_error:
+                    error += (
+                        "A critical component (The connector proxy) is not responding"
+                        " correctly."
+                    )
+                raise ConnectorProxyError(error)
+            elif json_parse_error:
+                raise ConnectorProxyError(
+                    f"There is a problem with this connector: '{name}'. "
+                    "Responses for connectors must be in JSON format. "
+                )
 
             if "refreshed_token_set" not in parsed_response:
-                return proxied_response.text
+                return response_text
 
             secret_key = parsed_response["auth"]
             refreshed_token_set = json.dumps(parsed_response["refreshed_token_set"])
             user_id = g.user.id if UserService.has_user() else None
             SecretService().update_secret(secret_key, refreshed_token_set, user_id)
-
             return json.dumps(parsed_response["api_response"])
 
 

spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py

@@ -1,8 +1,10 @@
 """Test_various_bpmn_constructs."""
+import pytest
 from flask.app import Flask
+from unittest.mock import Mock, patch
 from tests.spiffworkflow_backend.helpers.base_test import BaseTest
-
 from spiffworkflow_backend.services.secret_service import SecretService
+from spiffworkflow_backend.services.service_task_service import ConnectorProxyError
 from spiffworkflow_backend.services.service_task_service import ServiceTaskDelegate
 
 
@@ -31,3 +33,19 @@ class TestServiceTaskDelegate(BaseTest):
         SecretService().add_secret("hot_secret", "my_secret_value", user.id)
         result = ServiceTaskDelegate.check_prefixes("secret:hot_secret")
         assert result == "my_secret_value"
+
+    def test_invalid_call_returns_good_error_message(
+        self, app: Flask, with_db_and_bpmn_file_cleanup: None
+    ) -> None:
+        with patch('requests.post') as mock_post:
+            mock_post.return_value.status_code = 404
+            mock_post.return_value.ok = True
+            mock_post.return_value.json.return_value = ""
+            with pytest.raises(ConnectorProxyError) as ae:
+                ServiceTaskDelegate.call_connector("my_invalid_operation", {}, {})
+        assert "404" in str(ae)
+        assert "The service did not find the requested resource." in str(ae)
+        assert (
+            "A critical component (The connector proxy) is not responding correctly."
+            in str(ae)
+        )