From 549ec1f3bfcaa0015cfcba8619cba3b09f5c54f9 Mon Sep 17 00:00:00 2001 From: Dougal Matthews Date: Mon, 9 Jul 2018 16:17:39 +0100 Subject: [PATCH] Return the result of the MistralHTTPAction If the HTTP request fails, we need to fail the task. Returning the error from the parent class will do this. While this means we also return the success result it will be ignored by the Mistral engine. Credit to @lijianying10 on GitHub for sending this fix via a pull request. Tests were then added to verify the change. Closes-Bug: #1779973 Change-Id: Ib8754c8de2d6677d71383b3793d0fa168be575f5 --- mistral/actions/std_actions.py | 2 +- ...for-std-mistral-http-b852b6d8f0034477.yaml | 6 + .../actions/test_std_mistral_http_action.py | 124 ++++++++++++++++++ .../tests/unit/engine/test_error_result.py | 57 +++++++- 4 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 mistral/tests/releasenotes/notes/return-errors-for-std-mistral-http-b852b6d8f0034477.yaml create mode 100644 mistral/tests/unit/actions/test_std_mistral_http_action.py diff --git a/mistral/actions/std_actions.py b/mistral/actions/std_actions.py index 4002717dc..6a6f04d06 100644 --- a/mistral/actions/std_actions.py +++ b/mistral/actions/std_actions.py @@ -272,7 +272,7 @@ class MistralHTTPAction(HTTPAction): 'Mistral-Callback-URL': exec_ctx.callback_url, }) - super(MistralHTTPAction, self).run(context) + return super(MistralHTTPAction, self).run(context) def is_sync(self): return False diff --git a/mistral/tests/releasenotes/notes/return-errors-for-std-mistral-http-b852b6d8f0034477.yaml b/mistral/tests/releasenotes/notes/return-errors-for-std-mistral-http-b852b6d8f0034477.yaml new file mode 100644 index 000000000..64ab544d1 --- /dev/null +++ b/mistral/tests/releasenotes/notes/return-errors-for-std-mistral-http-b852b6d8f0034477.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + The action std.mistral_http will now retrun an error if the HTTP request + fails. Previously the task would still go into the RUNNING state and wait + to be completed by the external resource. diff --git a/mistral/tests/unit/actions/test_std_mistral_http_action.py b/mistral/tests/unit/actions/test_std_mistral_http_action.py new file mode 100644 index 000000000..7d0fbddbe --- /dev/null +++ b/mistral/tests/unit/actions/test_std_mistral_http_action.py @@ -0,0 +1,124 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import json + +import mock +import requests + +from mistral.actions import std_actions as std +from mistral.tests.unit import base +from mistral_lib import actions as mistral_lib_actions + + +URL = 'http://some_url' + +DATA = { + 'server': { + 'id': '12345', + 'metadata': { + 'name': 'super_server' + } + } +} + + +def get_fake_response(content, code, **kwargs): + return base.FakeHTTPResponse( + content, + code, + **kwargs + ) + + +def get_success_fake_response(): + return get_fake_response( + json.dumps(DATA), + 200, + headers={'Content-Type': 'application/json'} + ) + + +def get_error_fake_response(): + return get_fake_response( + json.dumps(DATA), + 401 + ) + + +class MistralHTTPActionTest(base.BaseTest): + @mock.patch.object(requests, 'request') + def test_http_action(self, mocked_method): + mocked_method.return_value = get_success_fake_response() + mock_ctx = mock.Mock() + + action = std.MistralHTTPAction( + url=URL, + method='POST', + body=DATA, + timeout=20, + allow_redirects=True + ) + + DATA_STR = json.dumps(DATA) + + self.assertEqual(DATA_STR, action.body) + self.assertEqual(URL, action.url) + + result = action.run(mock_ctx) + + self.assertIsInstance(result, dict) + self.assertEqual(DATA, result['content']) + self.assertIn('headers', result) + self.assertEqual(200, result['status']) + + mock_ex = mock_ctx.execution + + headers = { + 'Mistral-Workflow-Name': mock_ex.workflow_name, + 'Mistral-Task-Id': mock_ex.task_execution_id, + 'Mistral-Callback-URL': mock_ex.callback_url, + 'Mistral-Action-Execution-Id': mock_ex.action_execution_id, + 'Mistral-Workflow-Execution-Id': mock_ex.workflow_execution_id + } + + mocked_method.assert_called_with( + 'POST', + URL, + data=DATA_STR, + headers=headers, + cookies=None, + params=None, + timeout=20, + auth=None, + allow_redirects=True, + proxies=None, + verify=None + ) + + @mock.patch.object(requests, 'request') + def test_http_action_error_result(self, mocked_method): + mocked_method.return_value = get_error_fake_response() + mock_ctx = mock.Mock() + + action = std.MistralHTTPAction( + url=URL, + method='POST', + body=DATA, + timeout=20, + allow_redirects=True + ) + + result = action.run(mock_ctx) + + self.assertIsInstance(result, mistral_lib_actions.Result) + self.assertEqual(401, result.error['status']) diff --git a/mistral/tests/unit/engine/test_error_result.py b/mistral/tests/unit/engine/test_error_result.py index 29b70476e..980a70467 100644 --- a/mistral/tests/unit/engine/test_error_result.py +++ b/mistral/tests/unit/engine/test_error_result.py @@ -38,7 +38,7 @@ wf: tasks: task1: - action: my_action + action: {action_name} input: success_result: <% $.success_result %> error_result: <% $.error_result %> @@ -71,14 +71,21 @@ class MyAction(actions_base.Action): raise NotImplementedError +class MyAsyncAction(MyAction): + + def is_sync(self): + return False + + class ErrorResultTest(base.EngineTestCase): def setUp(self): super(ErrorResultTest, self).setUp() test_base.register_action_class('my_action', MyAction) + test_base.register_action_class('my_async_action', MyAsyncAction) def test_error_result1(self): - wf_service.create_workflows(WF) + wf_service.create_workflows(WF.format(action_name="my_action")) # Start workflow. wf_ex = self.engine.start_workflow( @@ -111,7 +118,7 @@ class ErrorResultTest(base.EngineTestCase): self.assertEqual(2, data_flow.get_task_execution_result(task1)) def test_error_result2(self): - wf_service.create_workflows(WF) + wf_service.create_workflows(WF.format(action_name="my_action")) # Start workflow. wf_ex = self.engine.start_workflow( @@ -144,7 +151,7 @@ class ErrorResultTest(base.EngineTestCase): self.assertEqual(3, data_flow.get_task_execution_result(task1)) def test_success_result(self): - wf_service.create_workflows(WF) + wf_service.create_workflows(WF.format(action_name="my_action")) # Start workflow. wf_ex = self.engine.start_workflow( @@ -176,3 +183,45 @@ class ErrorResultTest(base.EngineTestCase): 'success', data_flow.get_task_execution_result(task1) ) + + def test_async_error_result(self): + wf_service.create_workflows(WF.format(action_name="my_async_action")) + + # Start workflow. + wf_ex = self.engine.start_workflow( + 'wf', + wf_input={ + 'success_result': None, + 'error_result': 2 + } + ) + + # If the action errors, we expect the workflow to continue. The + # on-error means the workflow ends in success. + self.await_workflow_success(wf_ex.id) + + def test_async_success_result(self): + wf_service.create_workflows(WF.format(action_name="my_async_action")) + + # Start workflow. + wf_ex = self.engine.start_workflow( + 'wf', + wf_input={ + 'success_result': 'success', + 'error_result': None + } + ) + + # When the action is successful, the workflow will wait in the RUNNING + # state for it to complete. + self.await_workflow_running(wf_ex.id) + + with db_api.transaction(): + # Note: We need to reread execution to access related tasks. + wf_ex = db_api.get_workflow_execution(wf_ex.id) + + tasks = wf_ex.task_executions + self.assertEqual(1, len(tasks)) + + task1 = self._assert_single_item(tasks, name='task1') + self.assertEqual(states.RUNNING, task1.state)