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

@@ -130,9 +130,10 @@ class ActionsController(rest.RestController, hooks.HookController):
                 "%s" % (SCOPE_TYPES.values, scope)
             )
 
+        with db_api.transaction():
             db_acts = actions.update_actions(definition, scope=scope)
-        models_dicts = [db_act.to_dict() for db_act in db_acts]
 
+        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)
 
+        with db_api.transaction():
             db_acts = actions.create_actions(definition, scope=scope)
-        models_dicts = [db_act.to_dict() for db_act in db_acts]
 
+        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()

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',
+            scope='public'
-            run_in_tx=False
         )
 
 

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))
 
     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:
+    for action_spec in action_list_spec.get_actions():
-        with db_api.transaction():
+        db_actions.append(update_action(action_spec, definition, scope))
-            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
-        )
 
     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(
+            create_or_update_action(action_spec, definition, scope)
-                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())
 

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(
+    @mock.patch.object(db_api, "update_action_definition", MOCK_UPDATED_ACTION)
-        db_api, "create_or_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(
+    @mock.patch.object(db_api, "update_action_definition")
-        db_api, "create_or_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(
+    @mock.patch.object(db_api, "update_action_definition", MOCK_NOT_FOUND)
-        db_api, "create_or_update_action_definition", MOCK_NOT_FOUND
-    )
     def test_put_not_found(self):
         resp = self.app.put(
             '/v2/actions',