From 0d8a3a8c64561e48a32c99e066d04a6f0e22778f Mon Sep 17 00:00:00 2001 From: Marcos Fermin Lobo Date: Mon, 6 Jun 2016 14:55:58 +0200 Subject: [PATCH] Allow to use both name and id to access action definitions - Support name and id to access via GET /v2/actions/ - Support name and id to access via DELETE /v2/actions/ TODO: - Make other apis of database layer consistent with workflow and action api. - Support name and id to access via PUT /v2/actions/ Change-Id: I8ce55c0eb1cab7581ba719fbbb6be7b7dced27e0 Implements: blueprint mistral-actions-identifier Closes-Bug: #1584643 --- mistral/api/controllers/v2/action.py | 24 +++++---- mistral/db/v2/sqlalchemy/api.py | 63 +++++++++++------------ mistral/tests/unit/api/v2/test_actions.py | 47 +++++++++++++---- 3 files changed, 83 insertions(+), 51 deletions(-) diff --git a/mistral/api/controllers/v2/action.py b/mistral/api/controllers/v2/action.py index 8f4c977b..25a13b8b 100644 --- a/mistral/api/controllers/v2/action.py +++ b/mistral/api/controllers/v2/action.py @@ -102,12 +102,16 @@ class ActionsController(rest.RestController, hooks.HookController): @rest_utils.wrap_wsme_controller_exception @wsme_pecan.wsexpose(Action, wtypes.text) - def get(self, name): - """Return the named action.""" - acl.enforce('actions:get', context.ctx()) - LOG.info("Fetch action [name=%s]" % name) + def get(self, identifier): + """Return the named action. - db_model = db_api.get_action_definition(name) + :param identifier: ID or name of the Action to get. + """ + + acl.enforce('actions:get', context.ctx()) + LOG.info("Fetch action [identifier=%s]" % identifier) + + db_model = db_api.get_action_definition(identifier) return Action.from_dict(db_model.to_dict()) @@ -169,19 +173,19 @@ class ActionsController(rest.RestController, hooks.HookController): @rest_utils.wrap_wsme_controller_exception @wsme_pecan.wsexpose(None, wtypes.text, status_code=204) - def delete(self, name): + def delete(self, identifier): """Delete the named action.""" acl.enforce('actions:delete', context.ctx()) - LOG.info("Delete action [name=%s]" % name) + LOG.info("Delete action [identifier=%s]" % identifier) with db_api.transaction(): - db_model = db_api.get_action_definition(name) + db_model = db_api.get_action_definition(identifier) if db_model.is_system: - msg = "Attempt to delete a system action: %s" % name + msg = "Attempt to delete a system action: %s" % identifier raise exc.DataAccessException(msg) - db_api.delete_action_definition(name) + db_api.delete_action_definition(identifier) @wsme_pecan.wsexpose(Actions, types.uuid, int, types.uniquelist, types.list, types.uniquelist, wtypes.text, diff --git a/mistral/db/v2/sqlalchemy/api.py b/mistral/db/v2/sqlalchemy/api.py index 864e5d09..48a1357e 100644 --- a/mistral/db/v2/sqlalchemy/api.py +++ b/mistral/db/v2/sqlalchemy/api.py @@ -21,7 +21,7 @@ from oslo_db import exception as db_exc from oslo_db import sqlalchemy as oslo_sqlalchemy from oslo_db.sqlalchemy import utils as db_utils from oslo_log import log as logging -from oslo_utils import uuidutils +from oslo_utils import uuidutils # noqa import sqlalchemy as sa from mistral.db.sqlalchemy import base as b @@ -224,6 +224,18 @@ def _get_db_object_by_id(model, id): return _secure_query(model).filter_by(id=id).first() +def _get_db_object_by_name_or_id(model, identifier): + query = _secure_query(model) + query = query.filter( + sa.or_( + model.id == identifier, + model.name == identifier + ) + ) + + return query.first() + + # Workbook definitions. def get_workbook(name): @@ -313,9 +325,10 @@ def get_workflow_definition(identifier): uuid. :return: Workflow definition. """ - wf_def = (_get_workflow_definition_by_id(identifier) - if uuidutils.is_uuid_like(identifier) - else _get_workflow_definition(identifier)) + wf_def = _get_db_object_by_name_or_id( + models.WorkflowDefinition, + identifier + ) if not wf_def: raise exc.DBEntityNotFoundError( @@ -326,7 +339,7 @@ def get_workflow_definition(identifier): def get_workflow_definition_by_id(id): - wf_def = _get_workflow_definition_by_id(id) + wf_def = _get_db_object_by_id(models.WorkflowDefinition, id) if not wf_def: raise exc.DBEntityNotFoundError( @@ -337,7 +350,7 @@ def get_workflow_definition_by_id(id): def load_workflow_definition(name): - return _get_workflow_definition(name) + return _get_db_object_by_name(models.WorkflowDefinition, name) def get_workflow_definitions(sort_keys=['created_at'], fields=None, **kwargs): @@ -403,7 +416,7 @@ def update_workflow_definition(identifier, values, session=None): @b.session_aware() def create_or_update_workflow_definition(name, values, session=None): - if not _get_workflow_definition(name): + if not _get_db_object_by_name(models.WorkflowDefinition, name): return create_workflow_definition(values) else: return update_workflow_definition(name, values) @@ -458,14 +471,6 @@ def delete_workflow_definitions(**kwargs): return _delete_all(models.WorkflowDefinition, **kwargs) -def _get_workflow_definition(name): - return _get_db_object_by_name(models.WorkflowDefinition, name) - - -def _get_workflow_definition_by_id(id): - return _get_db_object_by_id(models.WorkflowDefinition, id) - - # Action definitions. def get_action_definition_by_id(id): @@ -479,19 +484,22 @@ def get_action_definition_by_id(id): return action_def -def get_action_definition(name): - a_def = _get_action_definition(name) +def get_action_definition(identifier): + a_def = _get_db_object_by_name_or_id( + models.ActionDefinition, + identifier + ) if not a_def: raise exc.DBEntityNotFoundError( - "Action definition not found [action_name=%s]" % name + "Action definition not found [action_name=%s]" % identifier ) return a_def def load_action_definition(name): - return _get_action_definition(name) + return _get_db_object_by_name(models.ActionDefinition, name) def get_action_definitions(sort_keys=['name'], **kwargs): @@ -520,7 +528,7 @@ def create_action_definition(values, session=None): @b.session_aware() def update_action_definition(name, values, session=None): - a_def = _get_action_definition(name) + a_def = _get_db_object_by_name(models.ActionDefinition, name) if not a_def: raise exc.DBEntityNotFoundError( @@ -534,20 +542,15 @@ def update_action_definition(name, values, session=None): @b.session_aware() def create_or_update_action_definition(name, values, session=None): - if not _get_action_definition(name): + if not _get_db_object_by_name(models.ActionDefinition, name): return create_action_definition(values) else: return update_action_definition(name, values) @b.session_aware() -def delete_action_definition(name, session=None): - a_def = _get_action_definition(name) - - if not a_def: - raise exc.DBEntityNotFoundError( - "Action definition not found [action_name=%s]" % name - ) +def delete_action_definition(identifier, session=None): + a_def = get_action_definition(identifier) session.delete(a_def) @@ -557,10 +560,6 @@ def delete_action_definitions(**kwargs): return _delete_all(models.ActionDefinition, **kwargs) -def _get_action_definition(name): - return _get_db_object_by_name(models.ActionDefinition, name) - - # Common executions. def get_execution(id): diff --git a/mistral/tests/unit/api/v2/test_actions.py b/mistral/tests/unit/api/v2/test_actions.py index 1c19ad8e..920d9d2f 100644 --- a/mistral/tests/unit/api/v2/test_actions.py +++ b/mistral/tests/unit/api/v2/test_actions.py @@ -124,21 +124,44 @@ MOCK_DUPLICATE = mock.MagicMock(side_effect=exc.DBDuplicateEntryError()) class TestActionsController(base.APITest): - @mock.patch.object(db_api, "get_action_definition", MOCK_ACTION) + @mock.patch.object( + db_api, "get_action_definition", MOCK_ACTION) def test_get(self): resp = self.app.get('/v2/actions/my_action') self.assertEqual(200, resp.status_int) self.assertDictEqual(ACTION, resp.json) - @mock.patch.object(db_api, "get_action_definition", MOCK_NOT_FOUND) + @mock.patch.object( + db_api, "get_action_definition", MOCK_NOT_FOUND) def test_get_not_found(self): resp = self.app.get('/v2/actions/my_action', expect_errors=True) self.assertEqual(404, resp.status_int) - @mock.patch.object(db_api, "get_action_definition", MOCK_ACTION) @mock.patch.object(db_api, "update_action_definition", MOCK_UPDATED_ACTION) + @mock.patch.object( + db_api, "get_action_definition", MOCK_ACTION) + def test_get_by_id(self): + url = '/v2/actions/{0}'.format(ACTION['id']) + resp = self.app.get(url) + + self.assertEqual(200, resp.status_int) + self.assertEqual(ACTION['id'], resp.json['id']) + + @mock.patch.object( + db_api, "get_action_definition", MOCK_NOT_FOUND) + def test_get_by_id_not_found(self): + url = '/v2/actions/1234' + resp = self.app.get(url, expect_errors=True) + + self.assertEqual(404, resp.status_int) + + @mock.patch.object( + db_api, "get_action_definition", MOCK_ACTION) + @mock.patch.object( + db_api, "update_action_definition", MOCK_UPDATED_ACTION + ) def test_put(self): resp = self.app.put( '/v2/actions', @@ -178,7 +201,8 @@ class TestActionsController(base.APITest): self.assertEqual(404, resp.status_int) - @mock.patch.object(db_api, "get_action_definition", MOCK_SYSTEM_ACTION) + @mock.patch.object( + db_api, "get_action_definition", MOCK_SYSTEM_ACTION) def test_put_system(self): resp = self.app.put( '/v2/actions', @@ -257,7 +281,8 @@ class TestActionsController(base.APITest): self.assertEqual(409, resp.status_int) - @mock.patch.object(db_api, "get_action_definition", MOCK_ACTION) + @mock.patch.object( + db_api, "get_action_definition", MOCK_ACTION) @mock.patch.object(db_api, "delete_action_definition", MOCK_DELETE) def test_delete(self): resp = self.app.delete('/v2/actions/my_action') @@ -270,7 +295,8 @@ class TestActionsController(base.APITest): self.assertEqual(404, resp.status_int) - @mock.patch.object(db_api, "get_action_definition", MOCK_SYSTEM_ACTION) + @mock.patch.object( + db_api, "get_action_definition", MOCK_SYSTEM_ACTION) def test_delete_system(self): resp = self.app.delete('/v2/actions/std.echo', expect_errors=True) @@ -278,7 +304,8 @@ class TestActionsController(base.APITest): self.assertIn('Attempt to delete a system action: std.echo', resp.json['faultstring']) - @mock.patch.object(db_api, "get_action_definitions", MOCK_ACTIONS) + @mock.patch.object( + db_api, "get_action_definitions", MOCK_ACTIONS) def test_get_all(self): resp = self.app.get('/v2/actions') @@ -287,7 +314,8 @@ class TestActionsController(base.APITest): self.assertEqual(1, len(resp.json['actions'])) self.assertDictEqual(ACTION, resp.json['actions'][0]) - @mock.patch.object(db_api, "get_action_definitions", MOCK_EMPTY) + @mock.patch.object( + db_api, "get_action_definitions", MOCK_EMPTY) def test_get_all_empty(self): resp = self.app.get('/v2/actions') @@ -295,7 +323,8 @@ class TestActionsController(base.APITest): self.assertEqual(0, len(resp.json['actions'])) - @mock.patch.object(db_api, "get_action_definitions", MOCK_ACTIONS) + @mock.patch.object( + db_api, "get_action_definitions", MOCK_ACTIONS) def test_get_all_pagination(self): resp = self.app.get( '/v2/actions?limit=1&sort_keys=id,name')