From d97fcfd23852dbcdffe8e8d742a5bdd48eaea3ec Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 8 Feb 2023 11:53:20 -0500 Subject: [PATCH 1/4] Assure that when something goes wrong calling a service task that we get as much good information about the problem as possible. --- .../SpiffWorkflow/spiff/specs/service_task.py | 13 +++-- .../services/service_task_service.py | 49 +++++++++++++++++-- .../unit/test_service_task_delegate.py | 12 ++++- 3 files changed, 66 insertions(+), 8 deletions(-) diff --git a/SpiffWorkflow/SpiffWorkflow/spiff/specs/service_task.py b/SpiffWorkflow/SpiffWorkflow/spiff/specs/service_task.py index 80e3f345..c31e2fb5 100644 --- a/SpiffWorkflow/SpiffWorkflow/spiff/specs/service_task.py +++ b/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()} - result = task.workflow.script_engine.call_service(self.operation_name, - evaluated_params, task.data) - + 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 diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py index 37af3956..a78d11c7 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py @@ -7,6 +7,7 @@ import sentry_sdk from flask import current_app from flask import g +from spiffworkflow_backend.exceptions.api_error import ApiError from spiffworkflow_backend.services.file_system_service import FileSystemService from spiffworkflow_backend.services.secret_service import SecretService from spiffworkflow_backend.services.user_service import UserService @@ -43,6 +44,28 @@ class ServiceTaskDelegate: return value + @staticmethod + def get_message_for_status(code): + """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,20 +78,40 @@ 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}'. " + f"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"]) + class ServiceTaskService: """ServiceTaskService.""" diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py index 95b55756..5057afb6 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py @@ -1,9 +1,10 @@ """Test_various_bpmn_constructs.""" +import pytest from flask.app import Flask 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 ServiceTaskDelegate +from spiffworkflow_backend.services.service_task_service import ServiceTaskDelegate, ConnectorProxyError class TestServiceTaskDelegate(BaseTest): @@ -31,3 +32,12 @@ 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 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) From f387ab6c6c296c32b480225e4343e4a9777ca5ad Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 8 Feb 2023 12:00:27 -0500 Subject: [PATCH 2/4] run_pyl --- .../src/spiffworkflow_backend/__init__.py | 2 +- .../services/service_task_service.py | 56 ++++++++++++++----- .../unit/test_service_task_delegate.py | 10 +++- 3 files changed, 49 insertions(+), 19 deletions(-) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/__init__.py b/spiffworkflow-backend/src/spiffworkflow_backend/__init__.py index b3e85cd7..9aec289d 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/__init__.py +++ b/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 diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py index a78d11c7..bf7c5ddc 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py @@ -7,7 +7,6 @@ import sentry_sdk from flask import current_app from flask import g -from spiffworkflow_backend.exceptions.api_error import ApiError from spiffworkflow_backend.services.file_system_service import FileSystemService from spiffworkflow_backend.services.secret_service import SecretService from spiffworkflow_backend.services.user_service import UserService @@ -47,23 +46,44 @@ class ServiceTaskDelegate: @staticmethod def get_message_for_status(code): """Given a code like 404, return a string like 'The requested resource was not found.'""" - msg = f'HTTP Status Code {code}.' + 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.' + 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.' + 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.' + 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.' + 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.' + 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.' + 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.' + 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).' + msg = ( + "501 (Not Implemented) - This service needs to be called with the" + " different method (like POST not GET)." + ) return msg @staticmethod @@ -91,15 +111,22 @@ class ServiceTaskDelegate: 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) + 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." + 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}'. " - f"Responses for connectors must be in JSON format. ") + 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 response_text @@ -111,7 +138,6 @@ class ServiceTaskDelegate: return json.dumps(parsed_response["api_response"]) - class ServiceTaskService: """ServiceTaskService.""" diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py index 5057afb6..6de51b8a 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py @@ -4,7 +4,8 @@ from flask.app import Flask 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 ServiceTaskDelegate, ConnectorProxyError +from spiffworkflow_backend.services.service_task_service import ConnectorProxyError +from spiffworkflow_backend.services.service_task_service import ServiceTaskDelegate class TestServiceTaskDelegate(BaseTest): @@ -37,7 +38,10 @@ class TestServiceTaskDelegate(BaseTest): self, app: Flask, with_db_and_bpmn_file_cleanup: None ) -> None: with pytest.raises(ConnectorProxyError) as ae: - ServiceTaskDelegate.call_connector('my_invalid_operation', {}, {}) + 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) + assert ( + "A critical component (The connector proxy) is not responding correctly." + in str(ae) + ) From 4c7dfaffa6f3e1f4e1c7fe584748824d16d43b4d Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 8 Feb 2023 12:27:55 -0500 Subject: [PATCH 3/4] fixing some missing types --- .../src/spiffworkflow_backend/services/service_task_service.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py b/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py index bf7c5ddc..da3a65bb 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/services/service_task_service.py @@ -44,7 +44,7 @@ class ServiceTaskDelegate: return value @staticmethod - def get_message_for_status(code): + 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: From f9372effd9dc66b1118854d173be47e8980ed69b Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 8 Feb 2023 13:14:42 -0500 Subject: [PATCH 4/4] Use a mock when making external calls in tests. --- .../unit/test_service_task_delegate.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py index 6de51b8a..188722fa 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/unit/test_service_task_delegate.py @@ -1,8 +1,8 @@ """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 @@ -37,8 +37,12 @@ class TestServiceTaskDelegate(BaseTest): def test_invalid_call_returns_good_error_message( self, app: Flask, with_db_and_bpmn_file_cleanup: None ) -> None: - with pytest.raises(ConnectorProxyError) as ae: - ServiceTaskDelegate.call_connector("my_invalid_operation", {}, {}) + 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 (