Optimize API layer: using from_db_model() instead of from_dict()

* Using method from_db_model() of REST resources where possible
  which is more efficient than from_dict() that requires one more
  object in memory (a dict)
* Minor style changes

Change-Id: Ie1f3137bee94328f2af676a0831e30c1cf212f47

mistral/api/controllers/resource.py

@@ -16,6 +16,8 @@ import json
 
 from wsme import types as wtypes
 
+from mistral import utils
+
 
 class Resource(wtypes.Base):
     """REST API Resource."""
@@ -39,15 +41,21 @@ class Resource(wtypes.Base):
 
         for key, val in d.items():
             if hasattr(obj, key):
-                setattr(obj, key, val)
+                # Convert all datetime values to strings.
+                setattr(obj, key, utils.datetime_to_str(val))
 
         return obj
 
     @classmethod
-    def from_db_model(cls):
+    def from_db_model(cls, db_model):
-        # TODO(rakhmerov): Once we implement this method,
+        obj = cls()
-        # this will significantly reduce memory footprint.
+
-        raise NotImplementedError
+        for col_name, col_val in db_model.iter_columns():
+            if hasattr(obj, col_name):
+                # Convert all datetime values to strings.
+                setattr(obj, col_name, utils.datetime_to_str(col_val))
+
+        return obj
 
     def __str__(self):
         """WSME based implementation of __str__."""

mistral/api/controllers/v2/action.py

@@ -54,11 +54,12 @@ class ActionsController(rest.RestController, hooks.HookController):
         """
 
         acl.enforce('actions:get', context.ctx())
+
         LOG.info("Fetch action [identifier=%s]", identifier)
 
         db_model = db_api.get_action_definition(identifier)
 
-        return resources.Action.from_dict(db_model.to_dict())
+        return resources.Action.from_db_model(db_model)
 
     @rest_utils.wrap_pecan_controller_exception
     @pecan.expose(content_type="text/plain")
@@ -69,8 +70,11 @@ class ActionsController(rest.RestController, hooks.HookController):
             of multiple actions. In this case they all will be updated.
         """
         acl.enforce('actions:update', context.ctx())
+
         definition = pecan.request.text
+
         LOG.info("Update action(s) [definition=%s]", definition)
+
         scope = pecan.request.GET.get('scope', 'private')
 
         if scope not in resources.SCOPE_TYPES.values:
@@ -86,8 +90,9 @@ class ActionsController(rest.RestController, hooks.HookController):
                 identifier=identifier
             )
 
-        models_dicts = [db_act.to_dict() for db_act in db_acts]
+        action_list = [
-        action_list = [resources.Action.from_dict(act) for act in models_dicts]
+            resources.Action.from_db_model(db_act) for db_act in db_acts
+        ]
 
         return resources.Actions(actions=action_list).to_json()
 
@@ -100,6 +105,7 @@ class ActionsController(rest.RestController, hooks.HookController):
             of multiple actions. In this case they all will be created.
         """
         acl.enforce('actions:create', context.ctx())
+
         definition = pecan.request.text
         scope = pecan.request.GET.get('scope', 'private')
         pecan.response.status = 201
@@ -115,8 +121,9 @@ class ActionsController(rest.RestController, hooks.HookController):
         with db_api.transaction():
             db_acts = actions.create_actions(definition, scope=scope)
 
-        models_dicts = [db_act.to_dict() for db_act in db_acts]
+        action_list = [
-        action_list = [resources.Action.from_dict(act) for act in models_dicts]
+            resources.Action.from_db_model(db_act) for db_act in db_acts
+        ]
 
         return resources.Actions(actions=action_list).to_json()
 
@@ -125,6 +132,7 @@ class ActionsController(rest.RestController, hooks.HookController):
     def delete(self, identifier):
         """Delete the named action."""
         acl.enforce('actions:delete', context.ctx())
+
         LOG.info("Delete action [identifier=%s]", identifier)
 
         with db_api.transaction():

mistral/api/controllers/v2/action_execution.py

@@ -56,7 +56,7 @@ def _get_action_execution_resource_for_list(action_ex):
 
     # TODO(nmakhotkin): Get rid of using dicts for constructing resources.
     # TODO(nmakhotkin): Use db_model for this instead.
-    res = resources.ActionExecution.from_dict(action_ex.to_dict())
+    res = resources.ActionExecution.from_db_model(action_ex)
 
     task_name = (action_ex.task_execution.name
                  if action_ex.task_execution else None)
@@ -142,14 +142,14 @@ class ActionExecutionsController(rest.RestController):
                 "Please provide at least action name to run action."
             )
 
-        action_ex = rpc.get_engine_client().start_action(
+        values = rpc.get_engine_client().start_action(
             name,
             action_input,
             description=description,
             **params
         )
 
-        return resources.ActionExecution.from_dict(action_ex)
+        return resources.ActionExecution.from_dict(values)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(

mistral/api/controllers/v2/cron_trigger.py

@@ -40,7 +40,7 @@ class CronTriggersController(rest.RestController):
 
         db_model = db_api.get_cron_trigger(name)
 
-        return resources.CronTrigger.from_dict(db_model.to_dict())
+        return resources.CronTrigger.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(
@@ -71,7 +71,7 @@ class CronTriggersController(rest.RestController):
             workflow_id=values.get('workflow_id')
         )
 
-        return resources.CronTrigger.from_dict(db_model.to_dict())
+        return resources.CronTrigger.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(None, wtypes.text, status_code=204)

mistral/api/controllers/v2/environment.py

@@ -109,7 +109,7 @@ class EnvironmentController(rest.RestController):
 
         db_model = db_api.get_environment(name)
 
-        return resources.Environment.from_dict(db_model.to_dict())
+        return resources.Environment.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(
@@ -130,7 +130,7 @@ class EnvironmentController(rest.RestController):
 
         db_model = db_api.create_environment(env.to_dict())
 
-        return resources.Environment.from_dict(db_model.to_dict())
+        return resources.Environment.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(resources.Environment, body=resources.Environment)
@@ -155,7 +155,7 @@ class EnvironmentController(rest.RestController):
 
         db_model = db_api.update_environment(env.name, env.to_dict())
 
-        return resources.Environment.from_dict(db_model.to_dict())
+        return resources.Environment.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(None, wtypes.text, status_code=204)

mistral/api/controllers/v2/event_trigger.py

@@ -43,7 +43,7 @@ class EventTriggersController(rest.RestController):
 
         db_model = db_api.get_event_trigger(id)
 
-        return resources.EventTrigger.from_dict(db_model.to_dict())
+        return resources.EventTrigger.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(resources.EventTrigger, body=resources.EventTrigger,
@@ -73,7 +73,7 @@ class EventTriggersController(rest.RestController):
             workflow_params=values.get('workflow_params'),
         )
 
-        return resources.EventTrigger.from_dict(db_model.to_dict())
+        return resources.EventTrigger.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(resources.EventTrigger, types.uuid,
@@ -103,7 +103,7 @@ class EventTriggersController(rest.RestController):
 
             db_model = triggers.update_event_trigger(id, values)
 
-        return resources.EventTrigger.from_dict(db_model.to_dict())
+        return resources.EventTrigger.from_db_model(db_model)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(None, types.uuid, status_code=204)

mistral/api/controllers/v2/execution.py

@@ -47,12 +47,12 @@ STATE_TYPES = wtypes.Enum(
 )
 
 
-def _get_execution_resource(ex):
+def _get_execution_resource(wf_ex):
     # We need to refer to this lazy-load field explicitly in
     # order to make sure that it is correctly loaded.
-    hasattr(ex, 'output')
+    hasattr(wf_ex, 'output')
 
-    return resources.Execution.from_dict(ex.to_dict())
+    return resources.Execution.from_db_model(wf_ex)
 
 
 # TODO(rakhmerov): Make sure to make all needed renaming on public API.
@@ -77,7 +77,7 @@ class ExecutionsController(rest.RestController):
             # amount of DB queries and network traffic.
             hasattr(wf_ex, 'output')
 
-        return resources.Execution.from_dict(wf_ex.to_dict())
+        return resources.Execution.from_db_model(wf_ex)
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(

mistral/api/controllers/v2/member.py

@@ -66,13 +66,13 @@ class MembersController(rest.RestController):
             member_id
         )
 
-        member_dict = db_api.get_resource_member(
+        member_db = db_api.get_resource_member(
             self.resource_id,
             self.type,
             member_id
-        ).to_dict()
+        )
 
-        return resources.Member.from_dict(member_dict)
+        return resources.Member.from_db_model(member_db)
 
     @rest_utils.wrap_pecan_controller_exception
     @auth_enable_check
@@ -91,9 +91,10 @@ class MembersController(rest.RestController):
             self.resource_id,
             self.type
         )
+
         members = [
-            resources.Member.from_dict(member.to_dict())
+            resources.Member.from_db_model(db_member)
-            for member in db_members
+            for db_member in db_members
         ]
 
         return resources.Members(members=members)
@@ -118,15 +119,15 @@ class MembersController(rest.RestController):
         )
 
         if not member_info.member_id:
-            msg = "Member id must be provided."
+            raise exc.WorkflowException("Member id must be provided.")
-            raise exc.WorkflowException(msg)
 
         with db_api.transaction():
             wf_db = db_api.get_workflow_definition(self.resource_id)
 
             if wf_db.scope != 'private':
-                msg = "Only private resource could be shared."
+                raise exc.WorkflowException(
-                raise exc.WorkflowException(msg)
+                    "Only private resource could be shared."
+                )
 
             resource_member = {
                 'resource_id': self.resource_id,
@@ -137,7 +138,7 @@ class MembersController(rest.RestController):
 
             db_member = db_api.create_resource_member(resource_member)
 
-        return resources.Member.from_dict(db_member.to_dict())
+        return resources.Member.from_db_model(db_member)
 
     @rest_utils.wrap_pecan_controller_exception
     @auth_enable_check
@@ -165,7 +166,7 @@ class MembersController(rest.RestController):
             {'status': member_info.status}
         )
 
-        return resources.Member.from_dict(db_member.to_dict())
+        return resources.Member.from_db_model(db_member)
 
     @rest_utils.wrap_pecan_controller_exception
     @auth_enable_check

mistral/api/controllers/v2/resources.py

@@ -95,16 +95,12 @@ class Workflow(resource.Resource):
                    updated_at='1970-01-01T00:00:00.000000')
 
     @classmethod
-    def from_dict(cls, d):
+    def _set_input(cls, obj, wf_spec):
-        e = cls()
         input_list = []
 
-        for key, val in d.items():
+        if wf_spec:
-            if hasattr(e, key):
+            input = wf_spec.get('input', [])
-                setattr(e, key, val)
 
-        if 'spec' in d:
-            input = d.get('spec', {}).get('input', [])
             for param in input:
                 if isinstance(param, dict):
                     for k, v in param.items():
@@ -112,9 +108,21 @@ class Workflow(resource.Resource):
                 else:
                     input_list.append(param)
 
-            setattr(e, 'input', ", ".join(input_list) if input_list else '')
+            setattr(obj, 'input', ", ".join(input_list) if input_list else '')
 
-        return e
+        return obj
+
+    @classmethod
+    def from_dict(cls, d):
+        obj = super(Workflow, cls).from_dict(d)
+
+        return cls._set_input(obj, d.get('spec'))
+
+    @classmethod
+    def from_db_model(cls, db_model):
+        obj = super(Workflow, cls).from_db_model(db_model)
+
+        return cls._set_input(obj, db_model.spec)
 
 
 class Workflows(resource.ResourceList):

mistral/api/controllers/v2/service.py

@@ -58,10 +58,18 @@ class ServicesController(rest.RestController):
         try:
             for group in service_group:
                 members = service_coordinator.get_members(group)
-                services_list.extend(
+
-                    [resources.Service.from_dict(
+                members_list = [
-                        {'type': group, 'name': member}) for member in members]
+                    resources.Service.from_dict(
+                        {
+                            'type': group,
+                            'name': member
+                        }
                     )
+                    for member in members
+                ]
+
+                services_list.extend(members_list)
         except tooz.coordination.ToozError as e:
             # In the scenario of network interruption or manually shutdown
             # connection shutdown, ToozError will be raised.

mistral/api/controllers/v2/task.py

@@ -41,7 +41,8 @@ STATE_TYPES = wtypes.Enum(str, states.IDLE, states.RUNNING, states.SUCCESS,
 
 
 def _get_task_resource_with_result(task_ex):
-    task = resources.Task.from_dict(task_ex.to_dict())
+    task = resources.Task.from_db_model(task_ex)
+
     task.result = json.dumps(data_flow.get_task_execution_result(task_ex))
 
     return task

mistral/api/controllers/v2/workbook.py

@@ -52,7 +52,7 @@ class WorkbooksController(rest.RestController, hooks.HookController):
 
         db_model = db_api.get_workbook(name)
 
-        return resources.Workbook.from_dict(db_model.to_dict())
+        return resources.Workbook.from_db_model(db_model)
 
     @rest_utils.wrap_pecan_controller_exception
     @pecan.expose(content_type="text/plain")
@@ -66,7 +66,7 @@ class WorkbooksController(rest.RestController, hooks.HookController):
 
         wb_db = workbooks.update_workbook_v2(definition)
 
-        return resources.Workbook.from_dict(wb_db.to_dict()).to_json()
+        return resources.Workbook.from_db_model(wb_db).to_json()
 
     @rest_utils.wrap_pecan_controller_exception
     @pecan.expose(content_type="text/plain")
@@ -79,9 +79,10 @@ class WorkbooksController(rest.RestController, hooks.HookController):
         LOG.info("Create workbook [definition=%s]" % definition)
 
         wb_db = workbooks.create_workbook_v2(definition)
+
         pecan.response.status = 201
 
-        return resources.Workbook.from_dict(wb_db.to_dict()).to_json()
+        return resources.Workbook.from_db_model(wb_db).to_json()
 
     @rest_utils.wrap_wsme_controller_exception
     @wsme_pecan.wsexpose(None, wtypes.text, status_code=204)

mistral/api/controllers/v2/workflow.py

@@ -85,7 +85,7 @@ class WorkflowsController(rest.RestController, hooks.HookController):
 
         db_model = db_api.get_workflow_definition(identifier)
 
-        return resources.Workflow.from_dict(db_model.to_dict())
+        return resources.Workflow.from_db_model(db_model)
 
     @rest_utils.wrap_pecan_controller_exception
     @pecan.expose(content_type="text/plain")
@@ -117,9 +117,8 @@ class WorkflowsController(rest.RestController, hooks.HookController):
             identifier=identifier
         )
 
-        models_dicts = [db_wf.to_dict() for db_wf in db_wfs]
         workflow_list = [
-            resources.Workflow.from_dict(wf) for wf in models_dicts
+            resources.Workflow.from_db_model(db_wf) for db_wf in db_wfs
         ]
 
         return (workflow_list[0].to_json() if identifier
@@ -148,10 +147,9 @@ class WorkflowsController(rest.RestController, hooks.HookController):
         LOG.info("Create workflow(s) [definition=%s]", definition)
 
         db_wfs = workflows.create_workflows(definition, scope=scope)
-        models_dicts = [db_wf.to_dict() for db_wf in db_wfs]
 
         workflow_list = [
-            resources.Workflow.from_dict(wf) for wf in models_dicts
+            resources.Workflow.from_db_model(db_wf) for db_wf in db_wfs
         ]
 
         return resources.Workflows(workflows=workflow_list).to_json()
@@ -161,6 +159,7 @@ class WorkflowsController(rest.RestController, hooks.HookController):
     def delete(self, identifier):
         """Delete a workflow."""
         acl.enforce('workflows:delete', context.ctx())
+
         LOG.info("Delete workflow [identifier=%s]", identifier)
 
         with db_api.transaction():

mistral/service/coordination.py

@@ -144,6 +144,7 @@ class ServiceCoordinator(object):
             return []
 
         get_members_req = self._coordinator.get_members(group_id)
+
         try:
             members = get_members_req.get()
 

mistral/tests/unit/api/test_resource_base.py

@@ -0,0 +1,74 @@
+# Copyright 2016 NEC Corporation. All rights reserved.
+#
+#    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 copy
+import datetime
+
+from mistral.api.controllers.v2 import resources
+from mistral.db.v2 import api as db_api
+from mistral.tests.unit import base
+from mistral import utils
+
+
+WF_EXEC = {
+    'id': 'c0f3be41-88b9-4c86-a669-83e77cd0a1b8',
+    'spec': {},
+    'params': {'task': 'my_task1'},
+    'project_id': '<default-project>',
+    'scope': 'PUBLIC',
+    'state': 'IDLE',
+    'state_info': "Running...",
+    'created_at': datetime.datetime(2016, 12, 1, 15, 0, 0),
+    'updated_at': None,
+    'context': None,
+    'task_execution_id': None,
+    'description': None,
+    'output': None,
+    'accepted': False,
+    'some_invalid_field': "foobar"
+}
+
+
+class TestRestResource(base.DbTestCase):
+    def test_from_db_model(self):
+        wf_ex = db_api.create_workflow_execution(WF_EXEC)
+
+        self.assertIsNotNone(wf_ex)
+
+        wf_ex_resource = resources.Execution.from_db_model(wf_ex)
+
+        self.assertIsNotNone(wf_ex_resource)
+
+        expected = copy.copy(WF_EXEC)
+
+        del expected['some_invalid_field']
+        utils.datetime_to_str_in_dict(expected, 'created_at')
+
+        self.assertDictEqual(expected, wf_ex.to_dict())
+
+    def test_from_dict(self):
+        wf_ex = db_api.create_workflow_execution(WF_EXEC)
+
+        self.assertIsNotNone(wf_ex)
+
+        wf_ex_resource = resources.Execution.from_dict(wf_ex.to_dict())
+
+        self.assertIsNotNone(wf_ex_resource)
+
+        expected = copy.copy(WF_EXEC)
+
+        del expected['some_invalid_field']
+        utils.datetime_to_str_in_dict(expected, 'created_at')
+
+        self.assertDictEqual(expected, wf_ex.to_dict())

mistral/tests/unit/api/v2/test_workflows.py

@@ -412,6 +412,9 @@ class TestWorkflowsController(base.APITest):
         self.assertEqual(200, resp.status_int)
 
         self.assertEqual(1, len(resp.json['workflows']))
+
+        print(resp.json['workflows'][0])
+
         self.assertDictEqual(WF, resp.json['workflows'][0])
 
     @mock.patch.object(db_api, "get_workflow_definitions", MOCK_EMPTY)

mistral/utils/__init__.py

@@ -474,6 +474,9 @@ def utc_now_sec():
 def datetime_to_str(val, sep=' '):
     """Converts datetime value to string.
 
+    If the given value is not an instance of datetime then the method
+    returns the same value.
+
     :param val: datetime value.
     :param sep: Separator between date and time.
     :return: Datetime as a string.

mistral/utils/rest_utils.py

@@ -196,11 +196,14 @@ def get_all(list_cls, cls, get_all_function, get_function,
                         if hasattr(obj, f):
                             data.append(getattr(obj, f))
 
-                    dict_data = dict(zip(fields, data))
+                    # TODO(rakhmerov): This still can be expensive
+                    # due to creation of extra dictionary.
+                    list_to_return.append(
+                        cls.from_dict(dict(zip(fields, data)))
+                    )
                 else:
-                    dict_data = obj.to_dict()
+                    list_to_return.append(obj)
 
-                list_to_return.append(cls.from_dict(dict_data))
     else:
         db_list = get_all_function(
             limit=limit,
@@ -213,10 +216,12 @@ def get_all(list_cls, cls, get_all_function, get_function,
         )
 
         for data in db_list:
-            dict_data = (dict(zip(fields, data)) if fields else
+            if fields:
-                         data.to_dict())
+                list_to_return.append(
-
+                    cls.from_dict(dict(zip(fields, data)))
-            list_to_return.append(cls.from_dict(dict_data))
+                )
+            else:
+                list_to_return.append(cls.from_db_model(data))
 
     return list_cls.convert_with_links(
         list_to_return,