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

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:
+        for ex in self.client.executions:
-            self.client.delete_obj('executions', wf)
+            self.client.delete_obj('executions', ex)
         self.client.executions = []
 
         super(ExecutionTestsV2, self).tearDown()

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' "
+        self.assertIn("Invalid DSL", resp.body)
-                      "can't be specified both", 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' "
+        self.assertIn("Invalid DSL", resp.body)
-                      "can't be specified both", 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' "
+        self.assertIn("Invalid DSL", resp.json['error'])
-                      "can't be specified both", resp.json['error'])
 
     def test_validate_dsl_parse_exception(self):
         resp = self.app.post(

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' "
+        self.assertIn("Invalid DSL", resp.body)
-                      "can't be specified both", 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' "
+        self.assertIn("Invalid DSL", resp.body)
-                      "can't be specified both", 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' "
+        self.assertIn("Invalid DSL", resp.json['error'])
-                      "can't be specified both", resp.json['error'])
 
     def test_validate_dsl_parse_exception(self):
         resp = self.app.post(

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 = [

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': {}}}

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')
+        return base.instantiate_spec(tasks_v2.TaskSpec, spec_dict)
-
-        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 None

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

mistral/workbook/v2/workflows.py

@@ -57,10 +57,13 @@ class WorkflowSpec(base.BaseSpec):
             'task-defaults',
             task_defaults.TaskDefaultsSpec
         )
-        self._tasks = self._spec_property(
+
-            'tasks',
+        # Inject 'type' here, so instantiate_spec function can recognize the
-            tasks.TaskSpecList.get_class(self._type)
+        # 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()