From a788bc8c1ac534ba46ad0776d14590dc02cd1fa9 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 19 Jun 2017 11:23:29 +0200 Subject: [PATCH 1/7] Update Python runner to log an error if it fails to de-serialize the action result (e.g. due to non-simple type in the result or similar). --- contrib/runners/python_runner/python_runner.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/contrib/runners/python_runner/python_runner.py b/contrib/runners/python_runner/python_runner.py index dc5f5f7d1d..cb8e4b0107 100644 --- a/contrib/runners/python_runner/python_runner.py +++ b/contrib/runners/python_runner/python_runner.py @@ -178,8 +178,10 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out): # Parse the serialized action result object try: action_result = json.loads(action_result) - except: - pass + except Exception as e: + # Failed to de-serialize the result, probably it contains non-simple type or similar + # TODO: Should we throw instead? + LOG.warning('Failed to de-serialize result "%s": %s' % (str(action_result), str(e))) if action_result and isinstance(action_result, dict): result = action_result.get('result', None) From 1de143aa081546d7d7a6b3fd553015ede5ab9879 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 19 Jun 2017 11:48:31 +0200 Subject: [PATCH 2/7] Update Python runner code to simply return action result as is (serialized as string) in case the result contains non-simple type and we can't de-serialize it. --- contrib/runners/python_runner/python_runner.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/contrib/runners/python_runner/python_runner.py b/contrib/runners/python_runner/python_runner.py index cb8e4b0107..f81199584a 100644 --- a/contrib/runners/python_runner/python_runner.py +++ b/contrib/runners/python_runner/python_runner.py @@ -14,6 +14,7 @@ # limitations under the License. import os +import re import sys import json import uuid @@ -180,14 +181,20 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out): action_result = json.loads(action_result) except Exception as e: # Failed to de-serialize the result, probably it contains non-simple type or similar - # TODO: Should we throw instead? LOG.warning('Failed to de-serialize result "%s": %s' % (str(action_result), str(e))) + pass if action_result and isinstance(action_result, dict): result = action_result.get('result', None) status = action_result.get('status', None) else: - result = 'None' + # Failed to de-serialize return result as is aka as a string + match = re.search("'result': (.*?)$", action_result).groups() + + if match: + action_result = match[0] + + result = action_result status = None output = { From e4c5b9710414411b4aa640a2cdce6aec11c9edf4 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 19 Jun 2017 12:03:22 +0200 Subject: [PATCH 3/7] Update changelog. --- CHANGELOG.rst | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8baa0ebcd0..6f3c6953ca 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -4,6 +4,14 @@ Changelog in development -------------- +* Update Python runner so it mimics behavior from StackStorm pre 1.6 and returns action result as + is (serialized as string) in case we are unable to serialize action result because it contains + non-simple types (e.g. class instances) which can't be serialized. + + In v1.6 we introduced a change when in such instances, we simply returned ``None`` as result + and didn't log anything which was confusing. (improvement) + + Reported by Anthony Shaw. 2.3.0 - June 19, 2017 --------------------- From 86341d60ece84c2dd3d1853e2970a5d8798c44c1 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 19 Jun 2017 12:37:18 +0200 Subject: [PATCH 4/7] Also handle a case where action_result is None. --- .../runners/python_runner/python_runner.py | 23 +++++++++++-------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/contrib/runners/python_runner/python_runner.py b/contrib/runners/python_runner/python_runner.py index f81199584a..86a220db77 100644 --- a/contrib/runners/python_runner/python_runner.py +++ b/contrib/runners/python_runner/python_runner.py @@ -182,19 +182,22 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out): except Exception as e: # Failed to de-serialize the result, probably it contains non-simple type or similar LOG.warning('Failed to de-serialize result "%s": %s' % (str(action_result), str(e))) - pass - if action_result and isinstance(action_result, dict): - result = action_result.get('result', None) - status = action_result.get('status', None) - else: - # Failed to de-serialize return result as is aka as a string - match = re.search("'result': (.*?)$", action_result).groups() + if action_result: + if isinstance(action_result, dict): + result = action_result.get('result', None) + status = action_result.get('status', None) + else: + # Failed to de-serialize action result aka result is a string + match = re.search("'result': (.*?)$", action_result or '').groups() - if match: - action_result = match[0] + if match: + action_result = match[0] - result = action_result + result = action_result + status = None + else: + result = 'None' status = None output = { From b2c98d573fa68aed0b79f4077081e2cabfe08036 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 19 Jun 2017 12:43:05 +0200 Subject: [PATCH 5/7] Add a test case for it. --- .../tests/unit/test_pythonrunner.py | 22 +++++++++++++++++++ .../pythonactions/actions/non_simple_type.py | 15 +++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 st2tests/st2tests/resources/packs/pythonactions/actions/non_simple_type.py diff --git a/contrib/runners/python_runner/tests/unit/test_pythonrunner.py b/contrib/runners/python_runner/tests/unit/test_pythonrunner.py index 704fd99416..1fa1e8d0e2 100644 --- a/contrib/runners/python_runner/tests/unit/test_pythonrunner.py +++ b/contrib/runners/python_runner/tests/unit/test_pythonrunner.py @@ -14,6 +14,7 @@ # limitations under the License. import os +import re import mock @@ -42,6 +43,8 @@ 'packs/dummy_pack_9/actions/list_repos_doesnt_exist.py') ACTION_2_PATH = os.path.join(tests_base.get_fixtures_path(), 'packs/dummy_pack_9/actions/invalid_syntax.py') +NON_SIMPLE_TYPE_ACTION = os.path.join(tests_base.get_resources_path(), 'packs', + 'pythonactions/actions/non_simple_type.py') # Note: runner inherits parent args which doesn't work with tests since test pass additional # unrecognized args @@ -59,6 +62,25 @@ def test_runner_creation(self): self.assertTrue(runner is not None, 'Creation failed. No instance.') self.assertEqual(type(runner), python_runner.PythonRunner, 'Creation failed. No instance.') + def test_action_returns_non_serializable_result(self): + # Actions returns non-simple type which can't be serialized, verify result is simple str() + # representation of the result + runner = python_runner.get_runner() + runner.action = self._get_mock_action_obj() + runner.runner_parameters = {} + runner.entry_point = NON_SIMPLE_TYPE_ACTION + runner.container_service = service.RunnerContainerService() + runner.pre_run() + (status, output, _) = runner.run({}) + + self.assertEqual(status, LIVEACTION_STATUS_SUCCEEDED) + self.assertTrue(output is not None) + + expected_result_re = (r"\[{'a': '1'}, {'h': 3, 'c': 2}, {'e': " + "}\]") + match = re.match(expected_result_re, output['result']) + self.assertTrue(match) + def test_simple_action_with_result_no_status(self): runner = python_runner.get_runner() runner.action = self._get_mock_action_obj() diff --git a/st2tests/st2tests/resources/packs/pythonactions/actions/non_simple_type.py b/st2tests/st2tests/resources/packs/pythonactions/actions/non_simple_type.py new file mode 100644 index 0000000000..3a4328aa37 --- /dev/null +++ b/st2tests/st2tests/resources/packs/pythonactions/actions/non_simple_type.py @@ -0,0 +1,15 @@ +from st2common.runners.base_action import Action + + +class Test(object): + foo = 'bar' + + +class NonSimpleTypeAction(Action): + def run(self): + result = [ + {'a': '1'}, + {'c': 2, 'h': 3}, + {'e': Test()} + ] + return result From 5db64ec379bbd20574afd2bd1f0990e809781c35 Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Mon, 19 Jun 2017 12:46:04 +0200 Subject: [PATCH 6/7] Fix invalid syntax. --- contrib/runners/python_runner/python_runner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/runners/python_runner/python_runner.py b/contrib/runners/python_runner/python_runner.py index 86a220db77..221a38135a 100644 --- a/contrib/runners/python_runner/python_runner.py +++ b/contrib/runners/python_runner/python_runner.py @@ -189,10 +189,10 @@ def _get_output_values(self, exit_code, stdout, stderr, timed_out): status = action_result.get('status', None) else: # Failed to de-serialize action result aka result is a string - match = re.search("'result': (.*?)$", action_result or '').groups() + match = re.search("'result': (.*?)$", action_result or '') if match: - action_result = match[0] + action_result = match.groups()[0] result = action_result status = None From 21b8fa86271361a04fdd92620dcfe79d2c500cdb Mon Sep 17 00:00:00 2001 From: Tomaz Muraus Date: Wed, 21 Jun 2017 12:24:54 +0200 Subject: [PATCH 7/7] Add Github PR / issue numbers to changelog entries. --- CHANGELOG.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index c1b5992de7..32380c7008 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -6,13 +6,13 @@ in development * Update ``st2 run`` / ``st2 execution run`` command to display result of workflow actions when they finish. In the workflow case, result of the last task (action) of the workflow is used. - (improvement) + (improvement) #3481 * Update Python runner so it mimics behavior from StackStorm pre 1.6 and returns action result as is (serialized as string) in case we are unable to serialize action result because it contains non-simple types (e.g. class instances) which can't be serialized. In v1.6 we introduced a change when in such instances, we simply returned ``None`` as result - and didn't log anything which was confusing. (improvement) + and didn't log anything which was confusing. (improvement) #3489 Reported by Anthony Shaw.