From c4ba9f398dfb25f9c90c78b554fc2e6b035f1f26 Mon Sep 17 00:00:00 2001 From: Dan Date: Wed, 25 Jan 2023 14:13:21 -0500 Subject: [PATCH] Making sure we create informative messages when encountering jinja2 syntax errors. --- .../routes/tasks_controller.py | 68 +++++++++----- .../tests/data/error/instructions_error.bpmn | 45 +++++++++ .../simple_form_with_error/simple_form.json | 18 ++++ .../simple_form_ui.json | 11 +++ .../simple_form_with_error.bpmn | 63 +++++++++++++ .../helpers/test_data.py | 2 +- .../integration/test_for_good_errors.py | 93 +++++++++++++++++++ 7 files changed, 275 insertions(+), 25 deletions(-) create mode 100644 spiffworkflow-backend/tests/data/error/instructions_error.bpmn create mode 100644 spiffworkflow-backend/tests/data/simple_form_with_error/simple_form.json create mode 100644 spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_ui.json create mode 100644 spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_with_error.bpmn create mode 100644 spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_for_good_errors.py diff --git a/spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py b/spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py index 3126674e..0015b189 100644 --- a/spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py +++ b/spiffworkflow-backend/src/spiffworkflow_backend/routes/tasks_controller.py @@ -10,6 +10,7 @@ from typing import Union import flask.wrappers import jinja2 +from SpiffWorkflow.exceptions import WorkflowTaskException from flask import current_app from flask import g from flask import jsonify @@ -17,6 +18,7 @@ from flask import make_response from flask.wrappers import Response from SpiffWorkflow.task import Task as SpiffTask # type: ignore from SpiffWorkflow.task import TaskState +from jinja2 import TemplateSyntaxError from sqlalchemy import and_ from sqlalchemy import asc from sqlalchemy import desc @@ -251,7 +253,7 @@ def task_show(process_instance_id: int, task_id: str) -> flask.wrappers.Response form_contents = _prepare_form_data( form_schema_file_name, - task.data, + spiff_task, process_model_with_form, ) @@ -271,7 +273,7 @@ def task_show(process_instance_id: int, task_id: str) -> flask.wrappers.Response ) from exception if task.data: - _update_form_schema_with_task_data_as_needed(form_dict, task.data) + _update_form_schema_with_task_data_as_needed(form_dict, task) if form_contents: task.form_schema = form_dict @@ -279,7 +281,7 @@ def task_show(process_instance_id: int, task_id: str) -> flask.wrappers.Response if form_ui_schema_file_name: ui_form_contents = _prepare_form_data( form_ui_schema_file_name, - task.data, + task, process_model_with_form, ) if ui_form_contents: @@ -287,9 +289,13 @@ def task_show(process_instance_id: int, task_id: str) -> flask.wrappers.Response if task.properties and task.data and "instructionsForEndUser" in task.properties: if task.properties["instructionsForEndUser"]: - task.properties["instructionsForEndUser"] = _render_jinja_template( - task.properties["instructionsForEndUser"], task.data - ) + try: + task.properties["instructionsForEndUser"] = _render_jinja_template( + task.properties["instructionsForEndUser"], spiff_task + ) + except WorkflowTaskException as wfe: + wfe.add_note("Failed to render instructions for end user.") + raise ApiError.from_workflow_exception("instructions_error", str(wfe), exp=wfe) return make_response(jsonify(task), 200) @@ -499,23 +505,36 @@ def _get_tasks( def _prepare_form_data( - form_file: str, task_data: Union[dict, None], process_model: ProcessModelInfo + form_file: str, spiff_task: SpiffTask, process_model: ProcessModelInfo ) -> str: """Prepare_form_data.""" - if task_data is None: + if spiff_task.data is None: return "" file_contents = SpecFileService.get_data(process_model, form_file).decode("utf-8") - return _render_jinja_template(file_contents, task_data) + try: + return _render_jinja_template(file_contents, spiff_task) + except WorkflowTaskException as wfe: + wfe.add_note(f"Error in Json Form File '{form_file}'") + api_error = ApiError.from_workflow_exception("instructions_error", str(wfe), exp=wfe) + api_error.file_name = form_file + raise api_error - -def _render_jinja_template(unprocessed_template: str, data: dict[str, Any]) -> str: +def _render_jinja_template(unprocessed_template: str, spiff_task: SpiffTask) -> str: """Render_jinja_template.""" jinja_environment = jinja2.Environment( autoescape=True, lstrip_blocks=True, trim_blocks=True ) - template = jinja_environment.from_string(unprocessed_template) - return template.render(**data) + try: + template = jinja_environment.from_string(unprocessed_template) + return template.render(**spiff_task.data) + except jinja2.exceptions.TemplateError as template_error: + wfe = WorkflowTaskException(str(template_error), task=spiff_task, exception=template_error) + if isinstance(template_error, TemplateSyntaxError): + wfe.line_number = template_error.lineno + wfe.error_line = template_error.source.split('\n')[template_error.lineno - 1] + wfe.add_note("Jinja2 template errors can happen when trying to displaying task data") + raise wfe from template_error def _get_spiff_task_from_process_instance( @@ -542,7 +561,7 @@ def _get_spiff_task_from_process_instance( # originally from: https://bitcoden.com/answers/python-nested-dictionary-update-value-where-any-nested-key-matches def _update_form_schema_with_task_data_as_needed( - in_dict: dict, task_data: dict + in_dict: dict, task: SpiffTask ) -> None: """Update_nested.""" for k, value in in_dict.items(): @@ -559,19 +578,20 @@ def _update_form_schema_with_task_data_as_needed( "options_from_task_data_var:", "" ) - if task_data_var not in task_data: + if task_data_var not in task.data: + wte = WorkflowTaskException( + f"Error building form. Attempting to create a selection list" + f" with options from variable '{task_data_var}' but it doesn't" + f" exist in the Task Data.", task=task) raise ( - ApiError( + ApiError.from_workflow_exception( error_code="missing_task_data_var", - message=( - "Task data is missing variable:" - f" {task_data_var}" - ), - status_code=500, + message=str(wte), + exp=wte ) ) - select_options_from_task_data = task_data.get(task_data_var) + select_options_from_task_data = task.data.get(task_data_var) if isinstance(select_options_from_task_data, list): if all( "value" in d and "label" in d @@ -594,11 +614,11 @@ def _update_form_schema_with_task_data_as_needed( in_dict[k] = options_for_react_json_schema_form elif isinstance(value, dict): - _update_form_schema_with_task_data_as_needed(value, task_data) + _update_form_schema_with_task_data_as_needed(value, task) elif isinstance(value, list): for o in value: if isinstance(o, dict): - _update_form_schema_with_task_data_as_needed(o, task_data) + _update_form_schema_with_task_data_as_needed(o, task) def _get_potential_owner_usernames(assigned_user: AliasedClass) -> Any: diff --git a/spiffworkflow-backend/tests/data/error/instructions_error.bpmn b/spiffworkflow-backend/tests/data/error/instructions_error.bpmn new file mode 100644 index 00000000..24039bbb --- /dev/null +++ b/spiffworkflow-backend/tests/data/error/instructions_error.bpmn @@ -0,0 +1,45 @@ + + + + + Flow_0smvjir + + + + Flow_1boyhcj + + + + + Hello {{ name }} +Department: {{ department }} +{{ x +=- 1}} + + + Flow_0smvjir + Flow_1boyhcj + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form.json b/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form.json new file mode 100644 index 00000000..356f6de8 --- /dev/null +++ b/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form.json @@ -0,0 +1,18 @@ +{ + "title": "Simple form", + "description": "A simple form example with some bad Jinja2 Syntax in it {{ x +=- 1}}", + "type": "object", + "required": ["name"], + "properties": { + "name": { + "type": "string", + "title": "Name", + "default": "World" + }, + "department": { + "type": "string", + "title": "Department", + "enum": ["Finance", "HR", "IT"] + } + } +} diff --git a/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_ui.json b/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_ui.json new file mode 100644 index 00000000..dc6916e0 --- /dev/null +++ b/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_ui.json @@ -0,0 +1,11 @@ +{ + "name": { + "ui:title": "Name", + "ui:description": "(Your name)" + }, + "department": { + "ui:title": "Department", + "ui:description": "(Your department)" + }, + "ui:order": ["name", "department"] +} diff --git a/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_with_error.bpmn b/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_with_error.bpmn new file mode 100644 index 00000000..351d53a6 --- /dev/null +++ b/spiffworkflow-backend/tests/data/simple_form_with_error/simple_form_with_error.bpmn @@ -0,0 +1,63 @@ + + + + + Flow_0smvjir + + + + Flow_1boyhcj + + + + + Hello {{ name }} +Department: {{ department }} + + + Flow_1ly1khd + Flow_1boyhcj + + + + + + + + + + Flow_0smvjir + Flow_1ly1khd + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/test_data.py b/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/test_data.py index d6b4f730..5a7a969e 100644 --- a/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/test_data.py +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/helpers/test_data.py @@ -42,7 +42,7 @@ def load_test_spec( ) -> ProcessModelInfo: """Loads a bpmn file into the process model dir based on a directory in tests/data.""" if process_model_source_directory is None: - raise Exception("You must inclode a `process_model_source_directory`.") + raise Exception("You must include a `process_model_source_directory`.") spec = ExampleDataLoader.create_spec( process_model_id=process_model_id, diff --git a/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_for_good_errors.py b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_for_good_errors.py new file mode 100644 index 00000000..95525d06 --- /dev/null +++ b/spiffworkflow-backend/tests/spiffworkflow_backend/integration/test_for_good_errors.py @@ -0,0 +1,93 @@ +"""Test_various_bpmn_constructs.""" +from flask.app import Flask +from flask.testing import FlaskClient +from spiffworkflow_backend import db +from spiffworkflow_backend.models.human_task import HumanTaskModel +from tests.spiffworkflow_backend.helpers.base_test import BaseTest +from spiffworkflow_backend.models.user import UserModel +from tests.spiffworkflow_backend.helpers.test_data import load_test_spec + + +class TestForGoodErrors(BaseTest): + """Assure when certain errors happen when rendering a jinaj2 error that it makes + some sense.""" + + def get_next_user_task(self, process_instance_id: int, + client: FlaskClient, + with_super_admin_user: UserModel, + ): + human_tasks = ( + db.session.query(HumanTaskModel) + .filter(HumanTaskModel.process_instance_id == process_instance_id) + .all() + ) + assert len(human_tasks) > 0, "No human tasks found for process." + human_task = human_tasks[0] + response = client.get( + f"/v1.0/tasks/{process_instance_id}/{human_task.task_id}", + headers=self.logged_in_headers(with_super_admin_user), + ) + return response + + def test_invalid_form( + self, + app: Flask, + client: FlaskClient, + with_db_and_bpmn_file_cleanup: None, + with_super_admin_user: UserModel, + ) -> None: + process_model = load_test_spec( + process_model_id="group/simple_form_with_error", + process_model_source_directory="simple_form_with_error", + ) + response = self.create_process_instance_from_process_model_id_with_api( + client, + # process_model.process_group_id, + process_model.id, + headers=self.logged_in_headers(with_super_admin_user), + ) + assert response.json is not None + process_instance_id = response.json["id"] + response = client.post( + f"/v1.0/process-instances/{self.modify_process_identifier_for_path_param(process_model.id)}/{process_instance_id}/run", + headers=self.logged_in_headers(with_super_admin_user), + ) + assert response.status_code == 200 + response = self.get_next_user_task(process_instance_id, client, with_super_admin_user) + assert response.json is not None + assert response.json['error_type'] == 'TemplateSyntaxError' + assert response.json['line_number'] == 3 + assert response.json['file_name'] == 'simple_form.json' + + def test_jinja2_error_message_for_end_user_instructions ( + self, + app: Flask, + client: FlaskClient, + with_db_and_bpmn_file_cleanup: None, + with_super_admin_user: UserModel, + ) -> None: + """Test_task_data_is_set_even_if_process_instance_errors.""" + process_model = load_test_spec( + process_model_id="group/end_user_instructions_error", + bpmn_file_name="instructions_error.bpmn", + process_model_source_directory="error", + ) + process_instance = self.create_process_instance_from_process_model( + process_model=process_model, user=with_super_admin_user + ) + + response = client.post( + f"/v1.0/process-instances/{self.modify_process_identifier_for_path_param(process_model.id)}/{process_instance.id}/run", + headers=self.logged_in_headers(with_super_admin_user), + ) + response = self.get_next_user_task(process_instance.id, client, with_super_admin_user) + + assert response.status_code == 400 + assert response.json is not None + assert response.json['error_type'] == 'TemplateSyntaxError' + assert response.json['line_number'] == 3 + assert response.json['error_line'] == '{{ x +=- 1}}' + assert response.json['file_name'] == 'instructions_error.bpmn' + assert 'instructions for end user' in response.json['message'] + assert 'Jinja2' in response.json['message'] + assert 'unexpected \'=\'' in response.json['message'] \ No newline at end of file