Feature/better errors

SpiffWorkflow/bpmn/FeelLikeScriptEngine.py

@@ -168,7 +168,7 @@ def feelParseISODuration(input):
 
     """
     if input[0] != 'P':
-        raise Exception("Oh Crap!")
+        raise Exception("ISO Duration format must begin with the letter P")
     input = input[1:]
     days, time = input.split("T")
     lookups = [("Y",days,timedelta(days=365)),
@@ -239,7 +239,7 @@ def feelParseISODuration(input):
          ('true','True'),
          ('false','False')
     ]
-         
+
 externalFuncs = {
     'feelConvertTime':feelConvertTime,
     'FeelInterval':FeelInterval,

SpiffWorkflow/bpmn/PythonScriptEngine.py

@@ -4,7 +4,7 @@
 import sys
 import traceback
 
-from SpiffWorkflow.bpmn.exceptions import WorkflowTaskExecException
+from ..exceptions import SpiffWorkflowException, WorkflowTaskException
 from ..operators import Operator
 
 
@@ -118,10 +118,11 @@ def evaluate(self, task, expression, external_methods=None):
                 return expression._matches(task)
             else:
                 return self._evaluate(expression, task.data, external_methods)
+        except SpiffWorkflowException as se:
+            se.add_note(f"Error evaluating expression '{expression}'")
+            raise se
         except Exception as e:
-            raise WorkflowTaskExecException(task,
-                                            f"Error evaluating expression {expression}",
-                                            e)
+            raise WorkflowTaskException(f"Error evaluating expression '{expression}'", task=task, exception=e)
 
     def execute(self, task, script, external_methods=None):
         """
@@ -141,25 +142,33 @@ def call_service(self, operation_name, operation_params, task_data):
         raise NotImplementedError("To call external services override the script engine and implement `call_service`.")
 
     def create_task_exec_exception(self, task, script, err):
-
-        if isinstance(err, WorkflowTaskExecException):
+        line_number, error_line = self.get_error_line_number_and_content(script, err)
+        if isinstance(err, SpiffWorkflowException):
+            err.line_number = line_number
+            err.error_line = error_line
+            err.add_note(f"Python script error on line {line_number}: '{error_line}'")
             return err
-
         detail = err.__class__.__name__
         if len(err.args) > 0:
             detail += ":" + err.args[0]
+        return WorkflowTaskException(detail, task=task, exception=err, line_number=line_number, error_line=error_line)
+
+    def get_error_line_number_and_content(self, script, err):
         line_number = 0
         error_line = ''
-        cl, exc, tb = sys.exc_info()
-        # Loop back through the stack trace to find the file called
-        # 'string' - which is the script we are executing, then use that
-        # to parse and pull out the offending line.
-        for frame_summary in traceback.extract_tb(tb):
-            if frame_summary.filename == '<string>':
-                line_number = frame_summary.lineno
-                error_line = script.splitlines()[line_number - 1]
-        return WorkflowTaskExecException(task, detail, err, line_number,
-                                         error_line)
+        if isinstance(err, SyntaxError):
+            line_number = err.lineno
+        else:
+            cl, exc, tb = sys.exc_info()
+            # Loop back through the stack trace to find the file called
+            # 'string' - which is the script we are executing, then use that
+            # to parse and pull out the offending line.
+            for frame_summary in traceback.extract_tb(tb):
+                if frame_summary.filename == '<string>':
+                    line_number = frame_summary.lineno
+        if line_number > 0:
+            error_line = script.splitlines()[line_number - 1]
+        return line_number, error_line
 
     def check_for_overwrite(self, task, external_methods):
         """It's possible that someone will define a variable with the
@@ -172,7 +181,7 @@ def check_for_overwrite(self, task, external_methods):
             msg = f"You have task data that overwrites a predefined " \
                   f"function(s). Please change the following variable or " \
                   f"field name(s) to something else: {func_overwrites}"
-            raise WorkflowTaskExecException(task, msg)
+            raise WorkflowTaskException(msg, task=task)
 
     def convert_to_box(self, data):
         if isinstance(data, dict):

SpiffWorkflow/bpmn/exceptions.py

@@ -1,50 +1,4 @@
-import re
-
-from SpiffWorkflow.exceptions import WorkflowException, WorkflowTaskException
-from SpiffWorkflow.util import levenshtein
-
-class WorkflowTaskExecException(WorkflowTaskException):
-    """
-    Exception during execution of task "payload". For example:
-
-    * ScriptTask during execution of embedded script,
-    * ServiceTask during external service call.
-    """
-
-    def __init__(self, task, error_msg, exception=None, line_number=0, error_line=""):
-        """
-        Exception initialization.
-
-        :param task: the task that threw the exception
-        :type task: Task
-        :param exception: a human readable error message
-        :type exception: Exception
-
-        """
-
-        self.offset = 0
-        self.line_number = line_number
-        self.error_line = error_line
-
-        if isinstance(exception, SyntaxError):
-            # Prefer line number from syntax error if available.
-            self.line_number = exception.lineno
-            self.offset = exception.offset
-        elif isinstance(exception, NameError):
-            def_match = re.match("name '(.+)' is not defined", str(exception))
-            if def_match:
-                bad_variable = re.match("name '(.+)' is not defined", str(exception)).group(1)
-                most_similar = levenshtein.most_similar(bad_variable, task.data.keys(), 3)
-                error_msg = f'something you are referencing does not exist: ' \
-                            f'"{exception}".'
-                if len(most_similar) == 1:
-                    error_msg += f' Did you mean \'{most_similar[0]}\'?'
-                if len(most_similar) > 1:
-                    error_msg += f' Did you mean one of \'{most_similar}\'?'
-
-            else:
-                error_msg = str(exception)
-        super().__init__(task, error_msg, exception)
+from SpiffWorkflow.exceptions import WorkflowException
 
 
 class WorkflowDataException(WorkflowException):

SpiffWorkflow/bpmn/parser/BpmnParser.py

@@ -21,7 +21,7 @@
 import os
 
 from lxml import etree
-from lxml.etree import DocumentInvalid
+from lxml.etree import DocumentInvalid, LxmlError
 
 from SpiffWorkflow.bpmn.specs.events.event_definitions import NoneEventDefinition
 
@@ -72,8 +72,13 @@ def __init__(self, xsd_path=XSD_PATH, imports=None):
     def validate(self, bpmn, filename=None):
         try:
             self.validator.assertValid(bpmn)
-        except DocumentInvalid as di:
-            raise DocumentInvalid(str(di) + "file: " + filename)
+        except ValidationException as ve:
+            ve.file_name = filename
+            ve.line_number = self.validator.error_log.last_error.line
+        except LxmlError as le:
+            last_error = self.validator.error_log.last_error
+            raise ValidationException(last_error.message, file_name=filename,
+                                      line_number=last_error.line)
 
 class BpmnParser(object):
     """
@@ -211,8 +216,7 @@ def _add_correlations(self, bpmn):
             correlation_identifier = correlation.attrib.get("id")
             if correlation_identifier is None:
                 raise ValidationException(
-                    "Correlation identifier is missing from bpmn xml"
-                )
+                    "Correlation identifier is missing from bpmn xml"                )
             correlation_property_retrieval_expressions = correlation.xpath(
                 "//bpmn:correlationPropertyRetrievalExpression", namespaces = self.namespaces)
             if not correlation_property_retrieval_expressions:
@@ -243,9 +247,9 @@ def _find_dependencies(self, process):
     def create_parser(self, node, filename=None, lane=None):
         parser = self.PROCESS_PARSER_CLASS(self, node, self.namespaces, filename=filename, lane=lane)
         if parser.get_id() in self.process_parsers:
-            raise ValidationException('Duplicate process ID', node=node, filename=filename)
+            raise ValidationException('Duplicate process ID', node=node, file_name=filename)
         if parser.get_name() in self.process_parsers_by_name:
-            raise ValidationException('Duplicate process name', node=node, filename=filename)
+            raise ValidationException('Duplicate process name', node=node, file_name=filename)
         self.process_parsers[parser.get_id()] = parser
         self.process_parsers_by_name[parser.get_name()] = parser
 

SpiffWorkflow/bpmn/parser/ProcessParser.py

@@ -93,7 +93,7 @@ def parse_node(self, node):
         (node_parser, spec_class) = self.parser._get_parser_class(node.tag)
         if not node_parser or not spec_class:
             raise ValidationException("There is no support implemented for this task type.",
-                node=node, filename=self.filename)
+                                      node=node, file_name=self.filename)
         np = node_parser(self, spec_class, node, lane=self.lane)
         task_spec = np.parse_node()
         return task_spec
@@ -103,7 +103,7 @@ def _parse(self):
         # bpmn:startEvent if we have a subworkflow task
         start_node_list = self.xpath('./bpmn:startEvent')
         if not start_node_list and self.process_executable:
-            raise ValidationException("No start event found", node=self.node, filename=self.filename)
+            raise ValidationException("No start event found", node=self.node, file_name=self.filename)
         self.spec = BpmnProcessSpec(name=self.get_id(), description=self.get_name(), filename=self.filename)
 
         # Check for an IO Specification.

SpiffWorkflow/bpmn/parser/TaskParser.py

@@ -72,7 +72,7 @@ def _set_multiinstance_attributes(self, is_sequential, expanded, loop_count,
             raise ValidationException(
                 f'Unsupported MultiInstance Task: {self.task.__class__}',
                 node=self.node,
-                filename=self.filename)
+                file_name=self.filename)
 
         self.task.loopTask = loop_task
         self.task.isSequential = is_sequential
@@ -133,8 +133,8 @@ def _add_boundary_event(self, children):
             if isinstance(child.event_definition, CancelEventDefinition) \
               and not isinstance(self.task, TransactionSubprocess):
                 raise ValidationException('Cancel Events may only be used with transactions',
-                    node=self.node,
-                    filename=self.filename)
+                                          node=self.node,
+                                          file_name=self.filename)
             parent.connect(child)
         return parent
 
@@ -167,7 +167,7 @@ def parse_node(self):
                     'Multiple outgoing flows are not supported for '
                     'tasks of type',
                     node=self.node,
-                    filename=self.filename)
+                    file_name=self.filename)
             for sequence_flow in outgoing:
                 target_ref = sequence_flow.get('targetRef')
                 try:
@@ -177,7 +177,7 @@ def parse_node(self):
                         'When looking for a task spec, we found two items, '
                         'perhaps a form has the same ID? (%s)' % target_ref,
                         node=self.node,
-                        filename=self.filename)
+                        file_name=self.filename)
 
                 c = self.process_parser.parse_node(target_node)
                 position = c.position
@@ -197,13 +197,10 @@ def parse_node(self):
                     self.connect_outgoing(c, sequence_flow, sequence_flow.get('id') == default_outgoing)
 
             return parent if boundary_event_nodes else self.task
-        except ValidationException:
-            raise
+        except ValidationException as ve:
+            raise ve
         except Exception as ex:
-            exc_info = sys.exc_info()
-            tb = "".join(traceback.format_exception(
-                exc_info[0], exc_info[1], exc_info[2]))
-            raise ValidationException("%r" % (ex), node=self.node, filename=self.filename)
+            raise ValidationException("%r" % (ex), node=self.node, file_name=self.filename)
 
     def get_task_spec_name(self, target_ref=None):
         """

SpiffWorkflow/bpmn/parser/ValidationException.py

@@ -1,5 +1,5 @@
 # -*- coding: utf-8 -*-
-# Copyright (C) 2012 Matthew Hampton
+# Copyright (C) 2012 Matthew Hampton, 2023 Dan Funk
 #
 # This library is free software; you can redistribute it and/or
 # modify it under the terms of the GNU Lesser General Public
@@ -17,34 +17,31 @@
 # 02110-1301  USA
 
 from .util import BPMN_MODEL_NS
+from ...exceptions import SpiffWorkflowException
 
 
-class ValidationException(Exception):
-
+class ValidationException(SpiffWorkflowException):
     """
     A ValidationException should be thrown with enough information for the user
     to diagnose the problem and sort it out.
 
     If available, please provide the offending XML node and filename.
     """
 
-    def __init__(self, msg, node=None, filename=None, *args, **kwargs):
+    def __init__(self, msg, node=None, file_name=None, *args, **kwargs):
         if node is not None:
             self.tag = self._shorten_tag(node.tag)
-            self.id = node.get('id', '<Unknown>')
-            self.name = node.get('name', '<Unknown>')
-            self.sourceline = getattr(node, 'sourceline', '<Unknown>')
+            self.id = node.get('id', '')
+            self.name = node.get('name', '')
+            self.line_number = getattr(node, 'line_number', '')
         else:
-            self.tag = '<Unknown>'
-            self.id = '<Unknown>'
-            self.name = '<Unknown>'
-            self.sourceline = '<Unknown>'
-        self.filename = filename or '<Unknown File>'
-        message = ('%s\nSource Details: '
-                   '%s (id:%s), name \'%s\', line %s in %s') % (
-            msg, self.tag, self.id, self.name, self.sourceline, self.filename)
+            self.tag = kwargs.get('tag', '')
+            self.id = kwargs.get('id', '')
+            self.name = kwargs.get('name', '')
+            self.line_number = kwargs.get('line_number', '')
+        self.file_name = file_name or ''
 
-        super(ValidationException, self).__init__(message, *args, **kwargs)
+        super(ValidationException, self).__init__(msg, *args)
 
     @classmethod
     def _shorten_tag(cls, tag):

SpiffWorkflow/bpmn/parser/event_parsers.py

@@ -93,9 +93,9 @@ def parse_timer_event(self):
             time_cycle = first(self.xpath('.//bpmn:timeCycle'))
             if time_cycle is not None:
                 return CycleTimerEventDefinition(label, time_cycle.text)
-            raise ValidationException("Unknown Time Specification", node=self.node, filename=self.filename)
+            raise ValidationException("Unknown Time Specification", node=self.node, file_name=self.filename)
         except Exception as e:
-            raise ValidationException("Time Specification Error. " + str(e), node=self.node, filename=self.filename)
+            raise ValidationException("Time Specification Error. " + str(e), node=self.node, file_name=self.filename)
 
     def get_message_correlations(self, message_ref):
 
@@ -254,4 +254,3 @@ def handles_multiple_outgoing(self):
     def connect_outgoing(self, outgoing_task, sequence_flow_node, is_default):
         self.task.event_definition.event_definitions.append(outgoing_task.event_definition)
         self.task.connect(outgoing_task)
-

SpiffWorkflow/bpmn/parser/node_parser.py

@@ -46,7 +46,7 @@ def parse_incoming_data_references(self):
             if ref is not None and ref.get('dataObjectRef') in self.process_parser.spec.data_objects:
                 specs.append(self.process_parser.spec.data_objects[ref.get('dataObjectRef')])
             else:
-                raise ValidationException(f'Cannot resolve dataInputAssociation {name}', self.node, self.filename)
+                raise ValidationException(f'Cannot resolve dataInputAssociation {name}', self.node, self.file_name)
         return specs
 
     def parse_outgoing_data_references(self):
@@ -56,7 +56,7 @@ def parse_outgoing_data_references(self):
             if ref is not None and ref.get('dataObjectRef') in self.process_parser.spec.data_objects:
                 specs.append(self.process_parser.spec.data_objects[ref.get('dataObjectRef')])
             else:
-                raise ValidationException(f'Cannot resolve dataOutputAssociation {name}', self.node, self.filename)
+                raise ValidationException(f'Cannot resolve dataOutputAssociation {name}', self.node, self.file_name)
         return specs
 
     def parse_extensions(self, node=None):