From 7a12e30d072210ba5b8f9b3bdde2a601e56e7bee Mon Sep 17 00:00:00 2001 From: Lingxian Kong Date: Wed, 9 Sep 2015 22:55:06 +0800 Subject: [PATCH] Refactor get_task_spec using mechanism of polymorphic DSL entities * Changing the way of getting task spec using polymorphic mechanism, which is the motivation of this patch. * Moving the type injection in TaskSpec to WorkflowSpec, the 'type' param is already there when TaskSpec initializing, and we could also continue to use that for polymorphic mechanism of TaskSpec, and the subclasses iteration mechanism can also be removed as well. * Improving the schema validation for TaskSpec, replace the code logic of prohibition of co-existance between action and workflow in task spec with jsonschema restriction. Change-Id: Ia88ce390298d54afc1ee72ebfb67de9056d9f444 --- .../api/v2/test_mistral_basic_v2.py | 4 +- mistral/tests/unit/api/v2/test_workbooks.py | 9 +-- mistral/tests/unit/api/v2/test_workflows.py | 9 +-- mistral/tests/unit/workbook/v2/test_tasks.py | 19 ----- .../tests/unit/workbook/v2/test_workflows.py | 5 -- mistral/workbook/parser.py | 9 +-- mistral/workbook/v2/tasks.py | 71 ++++++++----------- mistral/workbook/v2/workflows.py | 11 +-- 8 files changed, 44 insertions(+), 93 deletions(-) diff --git a/mistral/tests/functional/api/v2/test_mistral_basic_v2.py b/mistral/tests/functional/api/v2/test_mistral_basic_v2.py index efcc7942..1a21c4bf 100644 --- a/mistral/tests/functional/api/v2/test_mistral_basic_v2.py +++ b/mistral/tests/functional/api/v2/test_mistral_basic_v2.py @@ -356,8 +356,8 @@ class ExecutionTestsV2(base.TestCase): self.client.delete_obj('workflows', wf) self.client.workflows = [] - for wf in self.client.executions: - self.client.delete_obj('executions', wf) + for ex in self.client.executions: + self.client.delete_obj('executions', ex) self.client.executions = [] super(ExecutionTestsV2, self).tearDown() diff --git a/mistral/tests/unit/api/v2/test_workbooks.py b/mistral/tests/unit/api/v2/test_workbooks.py index 182bac47..e1ceac44 100644 --- a/mistral/tests/unit/api/v2/test_workbooks.py +++ b/mistral/tests/unit/api/v2/test_workbooks.py @@ -146,8 +146,7 @@ class TestWorkbooksController(base.FunctionalTest): ) self.assertEqual(resp.status_int, 400) - self.assertIn("Task properties 'action' and 'workflow' " - "can't be specified both", resp.body) + self.assertIn("Invalid DSL", resp.body) @mock.patch.object(workbooks, "create_workbook_v2", MOCK_WORKBOOK) def test_post(self): @@ -180,8 +179,7 @@ class TestWorkbooksController(base.FunctionalTest): ) self.assertEqual(resp.status_int, 400) - self.assertIn("Task properties 'action' and 'workflow' " - "can't be specified both", resp.body) + self.assertIn("Invalid DSL", resp.body) @mock.patch.object(db_api, "delete_workbook", MOCK_DELETE) def test_delete(self): @@ -232,8 +230,7 @@ class TestWorkbooksController(base.FunctionalTest): self.assertEqual(resp.status_int, 200) self.assertFalse(resp.json['valid']) - self.assertIn("Task properties 'action' and 'workflow' " - "can't be specified both", resp.json['error']) + self.assertIn("Invalid DSL", resp.json['error']) def test_validate_dsl_parse_exception(self): resp = self.app.post( diff --git a/mistral/tests/unit/api/v2/test_workflows.py b/mistral/tests/unit/api/v2/test_workflows.py index d9a9280c..66fdd7fe 100644 --- a/mistral/tests/unit/api/v2/test_workflows.py +++ b/mistral/tests/unit/api/v2/test_workflows.py @@ -221,8 +221,7 @@ class TestWorkflowsController(base.FunctionalTest): ) self.assertEqual(resp.status_int, 400) - self.assertIn("Task properties 'action' and 'workflow' " - "can't be specified both", resp.body) + self.assertIn("Invalid DSL", resp.body) @mock.patch.object(db_api, "create_workflow_definition") def test_post(self, mock_mtd): @@ -264,8 +263,7 @@ class TestWorkflowsController(base.FunctionalTest): ) self.assertEqual(resp.status_int, 400) - self.assertIn("Task properties 'action' and 'workflow' " - "can't be specified both", resp.body) + self.assertIn("Invalid DSL", resp.body) @mock.patch.object(db_api, "delete_workflow_definition", MOCK_DELETE) def test_delete(self): @@ -416,8 +414,7 @@ class TestWorkflowsController(base.FunctionalTest): self.assertEqual(resp.status_int, 200) self.assertFalse(resp.json['valid']) - self.assertIn("Task properties 'action' and 'workflow' " - "can't be specified both", resp.json['error']) + self.assertIn("Invalid DSL", resp.json['error']) def test_validate_dsl_parse_exception(self): resp = self.app.post( diff --git a/mistral/tests/unit/workbook/v2/test_tasks.py b/mistral/tests/unit/workbook/v2/test_tasks.py index 7569fd2e..777f06a9 100644 --- a/mistral/tests/unit/workbook/v2/test_tasks.py +++ b/mistral/tests/unit/workbook/v2/test_tasks.py @@ -15,33 +15,14 @@ from oslo_log import log as logging -from mistral import exceptions -from mistral.tests import base from mistral.tests.unit.workbook.v2 import base as v2_base from mistral import utils -from mistral.workbook.v2 import tasks from mistral.workbook.v2 import workflows LOG = logging.getLogger(__name__) -class TaskSpecListTest(base.BaseTest): - def test_get_class(self): - spec_list_cls = tasks.TaskSpecList.get_class('direct') - - self.assertIs(spec_list_cls, tasks.DirectWfTaskSpecList) - - def test_get_class_notfound(self): - exc = self.assertRaises( - exceptions.NotFoundException, - tasks.TaskSpecList.get_class, - "invalid" - ) - - self.assertIn("Can not find task list specification", str(exc)) - - class TaskSpecValidation(v2_base.WorkflowSpecValidationTestCase): def test_type_injection(self): tests = [ diff --git a/mistral/tests/unit/workbook/v2/test_workflows.py b/mistral/tests/unit/workbook/v2/test_workflows.py index 994f0d95..12263765 100644 --- a/mistral/tests/unit/workbook/v2/test_workflows.py +++ b/mistral/tests/unit/workbook/v2/test_workflows.py @@ -20,7 +20,6 @@ import yaml from mistral import exceptions as exc from mistral.tests.unit.workbook.v2 import base from mistral import utils -from mistral.workbook.v2 import tasks LOG = logging.getLogger(__name__) @@ -56,8 +55,6 @@ class WorkflowSpecValidation(base.WorkflowSpecValidationTestCase): self.assertEqual(1, len(wfs_spec.get_workflows())) self.assertEqual('test', wfs_spec.get_workflows()[0].get_name()) self.assertEqual('direct', wfs_spec.get_workflows()[0].get_type()) - self.assertIsInstance(wfs_spec.get_workflows()[0].get_tasks(), - tasks.DirectWfTaskSpecList) def test_direct_workflow_invalid_task(self): overlay = { @@ -102,8 +99,6 @@ class WorkflowSpecValidation(base.WorkflowSpecValidationTestCase): self.assertEqual(1, len(wfs_spec.get_workflows())) self.assertEqual('test', wfs_spec.get_workflows()[0].get_name()) self.assertEqual('reverse', wfs_spec.get_workflows()[0].get_type()) - self.assertIsInstance(wfs_spec.get_workflows()[0].get_tasks(), - tasks.ReverseWfTaskSpecList) def test_reverse_workflow_invalid_task(self): overlay = {'test': {'type': 'reverse', 'tasks': {}}} diff --git a/mistral/workbook/parser.py b/mistral/workbook/parser.py index 983686f2..6c6009f3 100644 --- a/mistral/workbook/parser.py +++ b/mistral/workbook/parser.py @@ -115,13 +115,6 @@ def get_workflow_list_spec_from_yaml(text): def get_task_spec(spec_dict): if _get_spec_version(spec_dict) == V2_0: - workflow_type = spec_dict.get('type') - - if workflow_type == 'direct': - return tasks_v2.DirectWorkflowTaskSpec(spec_dict) - elif workflow_type == 'reverse': - return tasks_v2.ReverseWorkflowTaskSpec(spec_dict) - else: - raise Exception('Unsupported workflow type "%s".' % workflow_type) + return base.instantiate_spec(tasks_v2.TaskSpec, spec_dict) return None diff --git a/mistral/workbook/v2/tasks.py b/mistral/workbook/v2/tasks.py index 5ce81d01..c213efd8 100644 --- a/mistral/workbook/v2/tasks.py +++ b/mistral/workbook/v2/tasks.py @@ -33,7 +33,7 @@ WITH_ITEMS_PTRN = re.compile( class TaskSpec(base.BaseSpec): # See http://json-schema.org - _type = None + _polymorphic_key = ('type', 'direct') _schema = { "type": "object", @@ -58,7 +58,27 @@ class TaskSpec(base.BaseSpec): "target": types.NONEMPTY_STRING, "keep-result": types.YAQL_OR_BOOLEAN }, - "additionalProperties": False + "additionalProperties": False, + "anyOf": [ + { + "not": { + "type": "object", + "required": ["action", "workflow"] + }, + }, + { + "oneOf": [ + { + "type": "object", + "required": ["action"] + }, + { + "type": "object", + "required": ["workflow"] + } + ] + } + ] } def __init__(self, data): @@ -83,22 +103,16 @@ class TaskSpec(base.BaseSpec): self._target = data.get('target') self._keep_result = data.get('keep-result', True) - self._inject_type() self._process_action_and_workflow() def validate_schema(self): super(TaskSpec, self).validate_schema() + self._transform_with_items() + action = self._data.get('action') workflow = self._data.get('workflow') - if action and workflow: - msg = ("Task properties 'action' and 'workflow' can't be" - " specified both: %s" % self._data) - raise exc.InvalidModelException(msg) - - self._transform_with_items() - # Validate YAQL expressions. if action or workflow: inline_params = self._parse_cmd_and_input(action or workflow)[1] @@ -146,10 +160,6 @@ class TaskSpec(base.BaseSpec): return with_items - def _inject_type(self): - if self._type: - self._data['type'] = self._type - def _process_action_and_workflow(self): params = {} @@ -198,7 +208,7 @@ class TaskSpec(base.BaseSpec): class DirectWorkflowTaskSpec(TaskSpec): - _type = 'direct' + _polymorphic_value = 'direct' _on_clause_type = { "oneOf": [ @@ -210,7 +220,7 @@ class DirectWorkflowTaskSpec(TaskSpec): _direct_workflow_schema = { "type": "object", "properties": { - "type": {"enum": [_type]}, + "type": {"enum": [_polymorphic_value]}, "join": { "oneOf": [ {"enum": ["all", "one"]}, @@ -270,12 +280,12 @@ class DirectWorkflowTaskSpec(TaskSpec): class ReverseWorkflowTaskSpec(TaskSpec): - _type = 'reverse' + _polymorphic_value = 'reverse' _reverse_workflow_schema = { "type": "object", "properties": { - "type": {"enum": [_type]}, + "type": {"enum": [_polymorphic_value]}, "requires": { "oneOf": [types.NONEMPTY_STRING, types.UNIQUE_STRING_LIST] } @@ -299,28 +309,3 @@ class ReverseWorkflowTaskSpec(TaskSpec): class TaskSpecList(base.BaseSpecList): item_class = TaskSpec - - @staticmethod - def get_class(wf_type): - """Gets a task specification list class by given workflow type. - - :param wf_type: Workflow type - :returns: Task specification list class - """ - for spec_list_cls in utils.iter_subclasses(TaskSpecList): - if wf_type == spec_list_cls.__type__: - return spec_list_cls - - msg = ("Can not find task list specification with workflow type:" - " %s" % wf_type) - raise exc.NotFoundException(msg) - - -class DirectWfTaskSpecList(TaskSpecList): - __type__ = 'direct' - item_class = DirectWorkflowTaskSpec - - -class ReverseWfTaskSpecList(TaskSpecList): - __type__ = 'reverse' - item_class = ReverseWorkflowTaskSpec diff --git a/mistral/workbook/v2/workflows.py b/mistral/workbook/v2/workflows.py index 9bcfce47..ddb26374 100644 --- a/mistral/workbook/v2/workflows.py +++ b/mistral/workbook/v2/workflows.py @@ -57,10 +57,13 @@ class WorkflowSpec(base.BaseSpec): 'task-defaults', task_defaults.TaskDefaultsSpec ) - self._tasks = self._spec_property( - 'tasks', - tasks.TaskSpecList.get_class(self._type) - ) + + # Inject 'type' here, so instantiate_spec function can recognize the + # specific subclass of TaskSpec. + for task in self._data.get('tasks').itervalues(): + task['type'] = self._type + + self._tasks = self._spec_property('tasks', tasks.TaskSpecList) def validate_schema(self): super(WorkflowSpec, self).validate_schema()