From 7b773bba05b2c9bd21141bede87c447ec867b064 Mon Sep 17 00:00:00 2001 From: Dougal Matthews Date: Mon, 20 Jun 2016 09:27:35 +0100 Subject: [PATCH] Don't create actions when attempting to update one that doesn't exist The API called create_or_update_action_definition when doing a PUT request. This was incorrect, as updating an action that doesn't exist shouldn't create a new action - rather a 404 is expected. This change adds a new create_or_update_actions method that has has the previous behaviour and updates the action_manager.py to use this as it required the old behaviour. The update_actions method is then updated to use the DB API call that will error if the action doesn't exist. Change-Id: Iea9ce71078058224172d31e20d84746052a28b2f Closes-Bug: 1568541 --- mistral/api/controllers/v2/action.py | 10 +++-- mistral/services/action_manager.py | 5 +-- mistral/services/actions.py | 50 +++++++++++------------ mistral/tests/unit/api/v2/test_actions.py | 11 ++--- 4 files changed, 36 insertions(+), 40 deletions(-) diff --git a/mistral/api/controllers/v2/action.py b/mistral/api/controllers/v2/action.py index 58a1d2d8..646e6d6d 100644 --- a/mistral/api/controllers/v2/action.py +++ b/mistral/api/controllers/v2/action.py @@ -130,9 +130,10 @@ class ActionsController(rest.RestController, hooks.HookController): "%s" % (SCOPE_TYPES.values, scope) ) - db_acts = actions.update_actions(definition, scope=scope) - models_dicts = [db_act.to_dict() for db_act in db_acts] + with db_api.transaction(): + db_acts = actions.update_actions(definition, scope=scope) + models_dicts = [db_act.to_dict() for db_act in db_acts] action_list = [Action.from_dict(act) for act in models_dicts] return Actions(actions=action_list).to_json() @@ -158,9 +159,10 @@ class ActionsController(rest.RestController, hooks.HookController): LOG.info("Create action(s) [definition=%s]" % definition) - db_acts = actions.create_actions(definition, scope=scope) - models_dicts = [db_act.to_dict() for db_act in db_acts] + 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.from_dict(act) for act in models_dicts] return Actions(actions=action_list).to_json() diff --git a/mistral/services/action_manager.py b/mistral/services/action_manager.py index 6be34f03..0ecfc17c 100644 --- a/mistral/services/action_manager.py +++ b/mistral/services/action_manager.py @@ -42,10 +42,9 @@ def register_standard_actions(): for action_path in action_paths: action_definition = open(action_path).read() - actions.update_actions( + actions.create_or_update_actions( action_definition, - scope='public', - run_in_tx=False + scope='public' ) diff --git a/mistral/services/actions.py b/mistral/services/actions.py index 1c01b708..d570e8d0 100644 --- a/mistral/services/actions.py +++ b/mistral/services/actions.py @@ -24,45 +24,31 @@ def create_actions(definition, scope='private'): db_actions = [] - with db_api.transaction(): - for action_spec in action_list_spec.get_actions(): - db_actions.append(create_action(action_spec, definition, scope)) + for action_spec in action_list_spec.get_actions(): + db_actions.append(create_action(action_spec, definition, scope)) return db_actions -def update_actions(definition, scope='private', run_in_tx=True): +def update_actions(definition, scope='private'): action_list_spec = spec_parser.get_action_list_spec_from_yaml(definition) db_actions = [] - if run_in_tx: - with db_api.transaction(): - db_actions = _append_all_actions( - action_list_spec, - db_actions, - definition, - scope - ) - else: - db_actions = _append_all_actions( - action_list_spec, - db_actions, - definition, - scope - ) + for action_spec in action_list_spec.get_actions(): + db_actions.append(update_action(action_spec, definition, scope)) return db_actions -def _append_all_actions(action_list_spec, db_actions, definition, scope): +def create_or_update_actions(definition, scope='private'): + action_list_spec = spec_parser.get_action_list_spec_from_yaml(definition) + + db_actions = [] + for action_spec in action_list_spec.get_actions(): db_actions.append( - create_or_update_action( - action_spec, - definition, - scope - ) + create_or_update_action(action_spec, definition, scope) ) return db_actions @@ -74,6 +60,20 @@ def create_action(action_spec, definition, scope): ) +def update_action(action_spec, definition, scope): + action = db_api.load_action_definition(action_spec.get_name()) + + if action and action.is_system: + raise exc.InvalidActionException( + "Attempt to modify a system action: %s" % + action.name + ) + + values = _get_action_values(action_spec, definition, scope) + + return db_api.update_action_definition(values['name'], values) + + def create_or_update_action(action_spec, definition, scope): action = db_api.load_action_definition(action_spec.get_name()) diff --git a/mistral/tests/unit/api/v2/test_actions.py b/mistral/tests/unit/api/v2/test_actions.py index 62324a50..1c19ad8e 100644 --- a/mistral/tests/unit/api/v2/test_actions.py +++ b/mistral/tests/unit/api/v2/test_actions.py @@ -138,9 +138,7 @@ class TestActionsController(base.APITest): self.assertEqual(404, resp.status_int) @mock.patch.object(db_api, "get_action_definition", MOCK_ACTION) - @mock.patch.object( - db_api, "create_or_update_action_definition", MOCK_UPDATED_ACTION - ) + @mock.patch.object(db_api, "update_action_definition", MOCK_UPDATED_ACTION) def test_put(self): resp = self.app.put( '/v2/actions', @@ -153,8 +151,7 @@ class TestActionsController(base.APITest): self.assertEqual({"actions": [UPDATED_ACTION]}, resp.json) @mock.patch.object(db_api, "load_action_definition", MOCK_ACTION) - @mock.patch.object( - db_api, "create_or_update_action_definition") + @mock.patch.object(db_api, "update_action_definition") def test_put_public(self, mock_mtd): mock_mtd.return_value = UPDATED_ACTION_DB @@ -170,9 +167,7 @@ class TestActionsController(base.APITest): self.assertEqual("public", mock_mtd.call_args[0][1]['scope']) - @mock.patch.object( - db_api, "create_or_update_action_definition", MOCK_NOT_FOUND - ) + @mock.patch.object(db_api, "update_action_definition", MOCK_NOT_FOUND) def test_put_not_found(self): resp = self.app.put( '/v2/actions',