From de54b63e20faaee722d169c5b406f6efe3db8212 Mon Sep 17 00:00:00 2001 From: Kelly McDonald Date: Fri, 17 Jul 2020 10:56:04 -0400 Subject: [PATCH] Process scripts with no shebang (#!) as a regular python script. If there is a shebang, we look up the class as we did before. I've also made it so that it falls back if we accidentally forget to add a shebang to a study as this would be a breaking change. With the fallback feature, it should work with unmodified bpmn documents. --- crc/services/workflow_processor.py | 82 +++++++++++-------- crc/static/bpmn/core_info/core_info.bpmn | 4 +- .../data_security_plan.bpmn | 4 +- .../documents_approvals.bpmn | 7 +- .../bpmn/ide_supplement/ide_supplement.bpmn | 4 +- .../ids_full_submission.bpmn | 4 +- .../bpmn/ind_supplement/ind_supplement.bpmn | 4 +- .../bpmn/irb_api_details/irb_api_details.bpmn | 4 +- .../irb_api_personnel/irb_api_personnel.bpmn | 24 +++--- .../bpmn/research_rampup/research_rampup.bpmn | 6 +- .../top_level_workflow.bpmn | 6 +- tests/data/invalid_script/invalid_script.bpmn | 2 +- .../data/invalid_script2/invalid_script2.bpmn | 39 +++++++++ tests/data/study_details/study_details.bpmn | 4 +- .../top_level_workflow.bpmn | 4 +- .../test_workflow_spec_validation_api.py | 11 +++ 16 files changed, 135 insertions(+), 74 deletions(-) create mode 100644 tests/data/invalid_script2/invalid_script2.bpmn diff --git a/crc/services/workflow_processor.py b/crc/services/workflow_processor.py index c59ad1a3..4ca3f20a 100644 --- a/crc/services/workflow_processor.py +++ b/crc/services/workflow_processor.py @@ -30,7 +30,7 @@ class CustomBpmnScriptEngine(BpmnScriptEngine): Rather than execute arbitrary code, this assumes the script references a fully qualified python class such as myapp.RandomFact. """ - def execute(self, task: SpiffTask, script, **kwargs): + def execute(self, task: SpiffTask, script, data): """ Assume that the script read in from the BPMN file is a fully qualified python class. Instantiate that class, pass in any data available to the current task so that it might act on it. @@ -40,43 +40,55 @@ class CustomBpmnScriptEngine(BpmnScriptEngine): """ # Shlex splits the whole string while respecting double quoted strings within commands = shlex.split(script) - print(commands) - if commands[0] != '#!': - super().execute_data(task,script,task.data) - else: - printable_comms = commands[1:] - path_and_command = commands[1].rsplit(".", 1) - if len(path_and_command) == 1: - module_name = "crc.scripts." + self.camel_to_snake(path_and_command[0]) - class_name = path_and_command[0] - else: - module_name = "crc.scripts." + path_and_command[0] + "." + self.camel_to_snake(path_and_command[1]) - class_name = path_and_command[1] - try: - mod = __import__(module_name, fromlist=[class_name]) - klass = getattr(mod, class_name) - study_id = task.workflow.data[WorkflowProcessor.STUDY_ID_KEY] - if WorkflowProcessor.WORKFLOW_ID_KEY in task.workflow.data: - workflow_id = task.workflow.data[WorkflowProcessor.WORKFLOW_ID_KEY] - else: - workflow_id = None - if not isinstance(klass(), Script): - raise ApiError.from_task("invalid_script", - "This is an internal error. The script '%s:%s' you called " % - (module_name, class_name) + - "does not properly implement the CRC Script class.", - task=task) - if task.workflow.data[WorkflowProcessor.VALIDATION_PROCESS_KEY]: - """If this is running a validation, and not a normal process, then we want to - mimic running the script, but not make any external calls or database changes.""" - klass().do_task_validate_only(task, study_id, workflow_id, *commands[2:]) - else: - klass().do_task(task, study_id, workflow_id, *commands[2:]) - except ModuleNotFoundError: + failedOnce = False + prevError = '' + if commands[0] != '#!': + try: + super().execute(task,script,data) + except SyntaxError as e: + failedOnce = True + prevError = script + else: + commands = commands[1:] + + printable_comms = commands + path_and_command = commands[0].rsplit(".", 1) + if len(path_and_command) == 1: + module_name = "crc.scripts." + self.camel_to_snake(path_and_command[0]) + class_name = path_and_command[0] + else: + module_name = "crc.scripts." + path_and_command[0] + "." + self.camel_to_snake(path_and_command[1]) + class_name = path_and_command[1] + try: + mod = __import__(module_name, fromlist=[class_name]) + klass = getattr(mod, class_name) + study_id = task.workflow.data[WorkflowProcessor.STUDY_ID_KEY] + if WorkflowProcessor.WORKFLOW_ID_KEY in task.workflow.data: + workflow_id = task.workflow.data[WorkflowProcessor.WORKFLOW_ID_KEY] + else: + workflow_id = None + + if not isinstance(klass(), Script): raise ApiError.from_task("invalid_script", - "Unable to locate Script: '%s:%s'" % (module_name, class_name), + "This is an internal error. The script '%s:%s' you called " % + (module_name, class_name) + + "does not properly implement the CRC Script class.", + task=task) + if task.workflow.data[WorkflowProcessor.VALIDATION_PROCESS_KEY]: + """If this is running a validation, and not a normal process, then we want to + mimic running the script, but not make any external calls or database changes.""" + klass().do_task_validate_only(task, study_id, workflow_id, *commands[1:]) + else: + klass().do_task(task, study_id, workflow_id, *commands[1:]) + except ModuleNotFoundError: + if failedOnce: + raise ApiError.from_task("invalid_script", + "Script had a syntax error: '%s'" % (prevError), task=task) + raise ApiError.from_task("invalid_script", + "Unable to locate Script: '%s:%s'" % (module_name, class_name), + task=task) def evaluate_expression(self, task, expression): """ diff --git a/crc/static/bpmn/core_info/core_info.bpmn b/crc/static/bpmn/core_info/core_info.bpmn index 8e790f98..8c69ffb3 100644 --- a/crc/static/bpmn/core_info/core_info.bpmn +++ b/crc/static/bpmn/core_info/core_info.bpmn @@ -1,5 +1,5 @@ - + Flow_1wqp7vf @@ -212,7 +212,7 @@ SequenceFlow_1r3yrhy Flow_09h1imz - StudyInfo details + #! StudyInfo details Flow_09h1imz diff --git a/crc/static/bpmn/data_security_plan/data_security_plan.bpmn b/crc/static/bpmn/data_security_plan/data_security_plan.bpmn index 0bf95e18..86426d6d 100644 --- a/crc/static/bpmn/data_security_plan/data_security_plan.bpmn +++ b/crc/static/bpmn/data_security_plan/data_security_plan.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_100w7co @@ -453,7 +453,7 @@ Indicate all the possible formats in which you will transmit your data outside o SequenceFlow_0k2r83n SequenceFlow_0t6xl9i SequenceFlow_16kyite - CompleteTemplate NEW_DSP_template.docx Study_DataSecurityPlan + #! CompleteTemplate NEW_DSP_template.docx Study_DataSecurityPlan ##### Instructions diff --git a/crc/static/bpmn/documents_approvals/documents_approvals.bpmn b/crc/static/bpmn/documents_approvals/documents_approvals.bpmn index bf39615b..12e85e34 100644 --- a/crc/static/bpmn/documents_approvals/documents_approvals.bpmn +++ b/crc/static/bpmn/documents_approvals/documents_approvals.bpmn @@ -41,8 +41,7 @@ {%- else -%} | {{doc.display_name}} | Not started | [?](/help/documents/{{doc.code}}) | No file yet | {%- endif %} -{% endif %}{% endfor %} - +{% endif %}{% endfor %} @@ -54,12 +53,12 @@ Flow_0c7ryff Flow_142jtxs - StudyInfo approvals + #! StudyInfo approvals Flow_1k3su2q Flow_0c7ryff - StudyInfo documents + #! StudyInfo documents diff --git a/crc/static/bpmn/ide_supplement/ide_supplement.bpmn b/crc/static/bpmn/ide_supplement/ide_supplement.bpmn index a886b4d4..7a83643b 100644 --- a/crc/static/bpmn/ide_supplement/ide_supplement.bpmn +++ b/crc/static/bpmn/ide_supplement/ide_supplement.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_1dhb8f4 @@ -36,7 +36,7 @@ SequenceFlow_1dhb8f4 SequenceFlow_1uzcl1f - StudyInfo details + #! StudyInfo details diff --git a/crc/static/bpmn/ids_full_submission/ids_full_submission.bpmn b/crc/static/bpmn/ids_full_submission/ids_full_submission.bpmn index 72fece25..25a9ad6e 100644 --- a/crc/static/bpmn/ids_full_submission/ids_full_submission.bpmn +++ b/crc/static/bpmn/ids_full_submission/ids_full_submission.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_1dexemq @@ -217,7 +217,7 @@ Protocol Owner: **(need to insert value here)** SequenceFlow_1dexemq Flow_1x9d2mo - StudyInfo documents + #! StudyInfo documents diff --git a/crc/static/bpmn/ind_supplement/ind_supplement.bpmn b/crc/static/bpmn/ind_supplement/ind_supplement.bpmn index b25e080b..e2a07df1 100644 --- a/crc/static/bpmn/ind_supplement/ind_supplement.bpmn +++ b/crc/static/bpmn/ind_supplement/ind_supplement.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_1dhb8f4 @@ -12,7 +12,7 @@ SequenceFlow_1dhb8f4 SequenceFlow_1uzcl1f - StudyInfo details + #! StudyInfo details diff --git a/crc/static/bpmn/irb_api_details/irb_api_details.bpmn b/crc/static/bpmn/irb_api_details/irb_api_details.bpmn index b5f0da02..b4f540f5 100644 --- a/crc/static/bpmn/irb_api_details/irb_api_details.bpmn +++ b/crc/static/bpmn/irb_api_details/irb_api_details.bpmn @@ -1,5 +1,5 @@ - + @@ -8,7 +8,7 @@ SequenceFlow_1fmyo77 SequenceFlow_18nr0gf - StudyInfo details + #! StudyInfo details diff --git a/crc/static/bpmn/irb_api_personnel/irb_api_personnel.bpmn b/crc/static/bpmn/irb_api_personnel/irb_api_personnel.bpmn index 99edb961..a5258cbe 100644 --- a/crc/static/bpmn/irb_api_personnel/irb_api_personnel.bpmn +++ b/crc/static/bpmn/irb_api_personnel/irb_api_personnel.bpmn @@ -1,5 +1,5 @@ - + Flow_0kcrx5l @@ -7,7 +7,7 @@ Flow_0kcrx5l Flow_1dcsioh - StudyInfo investigators + #! StudyInfo investigators ## The following information was gathered: @@ -54,28 +54,28 @@ - - + + - - + + - - + + - + - + - + - + diff --git a/crc/static/bpmn/research_rampup/research_rampup.bpmn b/crc/static/bpmn/research_rampup/research_rampup.bpmn index 19588731..5a4bb1bc 100644 --- a/crc/static/bpmn/research_rampup/research_rampup.bpmn +++ b/crc/static/bpmn/research_rampup/research_rampup.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_05ja25w @@ -598,7 +598,7 @@ Use the EHS [Lab Safety Plan During COVID 19 template](https://www.google.com/ur This step is internal to the system and do not require and user interaction Flow_11uqavk Flow_0aqgwvu - CompleteTemplate ResearchRampUpPlan.docx RESEARCH_RAMPUP + #! CompleteTemplate ResearchRampUpPlan.docx RESEARCH_RAMPUP @@ -764,7 +764,7 @@ This step is internal to the system and do not require and user interaction Flow_16y8glw Flow_0uc4o6c - UpdateStudy title:PIComputingID.label pi:PIComputingID.value + #! UpdateStudy title:PIComputingID.label pi:PIComputingID.value #### Weekly Personnel Schedule(s) diff --git a/crc/static/bpmn/top_level_workflow/top_level_workflow.bpmn b/crc/static/bpmn/top_level_workflow/top_level_workflow.bpmn index 6806fa5b..23d6ff72 100644 --- a/crc/static/bpmn/top_level_workflow/top_level_workflow.bpmn +++ b/crc/static/bpmn/top_level_workflow/top_level_workflow.bpmn @@ -11,7 +11,7 @@ SequenceFlow_1ees8ka SequenceFlow_17ct47v - StudyInfo documents + #! StudyInfo documents Flow_1m8285h @@ -62,7 +62,7 @@ Flow_0pwtiqm Flow_0eq6px2 - StudyInfo details + #! StudyInfo details Flow_14ce1d7 @@ -91,7 +91,7 @@ Flow_1qyrmzn Flow_0vo6ul1 - StudyInfo investigators + #! StudyInfo investigators diff --git a/tests/data/invalid_script/invalid_script.bpmn b/tests/data/invalid_script/invalid_script.bpmn index 83dcfe40..b85e2bc4 100644 --- a/tests/data/invalid_script/invalid_script.bpmn +++ b/tests/data/invalid_script/invalid_script.bpmn @@ -11,7 +11,7 @@ SequenceFlow_1pnq3kg SequenceFlow_12pf6um - NoSuchScript withArg1 + #! NoSuchScript withArg1 diff --git a/tests/data/invalid_script2/invalid_script2.bpmn b/tests/data/invalid_script2/invalid_script2.bpmn new file mode 100644 index 00000000..b061e76c --- /dev/null +++ b/tests/data/invalid_script2/invalid_script2.bpmn @@ -0,0 +1,39 @@ + + + + + SequenceFlow_1pnq3kg + + + + SequenceFlow_12pf6um + + + SequenceFlow_1pnq3kg + SequenceFlow_12pf6um + a really bad error that should fail + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/tests/data/study_details/study_details.bpmn b/tests/data/study_details/study_details.bpmn index b9aead94..2b46f935 100644 --- a/tests/data/study_details/study_details.bpmn +++ b/tests/data/study_details/study_details.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_1nfe5m9 @@ -8,7 +8,7 @@ SequenceFlow_1nfe5m9 SequenceFlow_1bqiin0 - StudyInfo info + #! StudyInfo info diff --git a/tests/data/top_level_workflow/top_level_workflow.bpmn b/tests/data/top_level_workflow/top_level_workflow.bpmn index cc6e1c57..8b1bb888 100644 --- a/tests/data/top_level_workflow/top_level_workflow.bpmn +++ b/tests/data/top_level_workflow/top_level_workflow.bpmn @@ -1,5 +1,5 @@ - + SequenceFlow_1ees8ka @@ -11,7 +11,7 @@ SequenceFlow_1ees8ka SequenceFlow_17ct47v - StudyInfo documents + #! StudyInfo documents Flow_1m8285h diff --git a/tests/workflow/test_workflow_spec_validation_api.py b/tests/workflow/test_workflow_spec_validation_api.py index d79986cf..597c4aa1 100644 --- a/tests/workflow/test_workflow_spec_validation_api.py +++ b/tests/workflow/test_workflow_spec_validation_api.py @@ -98,6 +98,17 @@ class TestWorkflowSpecValidation(BaseTest): self.assertEqual("An Invalid Script Reference", errors[0]['task_name']) self.assertEqual("invalid_script.bpmn", errors[0]['file_name']) + def test_invalid_script2(self): + self.load_example_data() + errors = self.validate_workflow("invalid_script2") + self.assertEqual(2, len(errors)) + self.assertEqual("error_loading_workflow", errors[0]['code']) + self.assertTrue("syntax error" in errors[0]['message']) + self.assertEqual("Invalid_Script_Task", errors[0]['task_id']) + self.assertEqual("An Invalid Script Reference", errors[0]['task_name']) + self.assertEqual("invalid_script2.bpmn", errors[0]['file_name']) + + def test_repeating_sections_correctly_populated(self): self.load_example_data() spec_model = self.load_test_spec('repeat_form')