RFR: Fix action registration validation

CHANGELOG.rst

@@ -31,6 +31,10 @@ Docs: http://docks.stackstorm.com/latest
   and ``set_poll_interval`` methods respectively. (new-feature)
 * Add support for a ``standalone`` mode to the st2auth service. In the standalone mode,
   authentication is handled inside the st2auth service using the defined backend. (new feature)
+* Timer is not a sensor anymore. It is spun as part of rules_engine process (refactor)
+* Fix a bug with action registration where action with invalid schema for
+  parameters get registered. (bug-fix)
+* Fix a bug with 'default' param values inheritance in runner/actions. (bug-fix)
 
 v0.6.0 - December 8, 2014
 -------------------------

contrib/core/actions/sendmail.json

@@ -23,17 +23,8 @@
             "required": true,
             "position": 2
         },
-        "user": {
-            "immutable": true
-        },
-        "dir": {
-            "immutable": true
-        },
         "sudo": {
             "immutable": true
-        },
-        "parallel": {
-            "immutable": true
         }
     }
 }

contrib/core/actions/stormbot_say.json

@@ -7,12 +7,6 @@
         "sudo": {
             "immutable": true
         },
-        "user": {
-            "immutable": true
-        },
-        "dir": {
-            "immutable": true
-        },
         "source": {
             "type": "string",
             "position": 0

contrib/examples/actions/ubuntu_pkg_info.json

@@ -11,17 +11,8 @@
             "required": true,
             "position": 0
         },
-        "user": {
-            "immutable": true
-        },
-        "dir": {
-            "immutable": true
-        },
         "sudo": {
             "immutable": true
-        },
-        "parallel": {
-            "immutable": true
         }
     }
 }

st2actions/st2actions/bootstrap/actionsregistrar.py

@@ -24,9 +24,10 @@
 from st2common.content.loader import (ContentPackLoader, MetaLoader)
 from st2common.content.validators import RequirementsValidator
 from st2common.persistence.action import Action
-from st2common.models.db.action import ActionDB
+from st2common.models.api.action import ActionAPI
 from st2common.models.system.common import ResourceReference
 import st2common.util.action_db as action_utils
+import st2common.validators.api.action as action_validator
 
 LOG = logging.getLogger(__name__)
 
@@ -48,23 +49,26 @@ def _get_actions_from_pack(self, actions_dir):
 
     def _register_action(self, pack, action):
         content = self._meta_loader.load(action)
+        pack_field = content.get('pack', None)
+        if not pack_field:
+            content['pack'] = pack
+            pack_field = pack
+        if pack_field != pack:
+            raise Exception('Model is in pack "%s" but field "pack" is different: %s' %
+                            (pack, pack_field))
+
+        action_api = ActionAPI(**content)
+        action_validator.validate_action(action_api)
+        model = ActionAPI.to_model(action_api)
+
         action_ref = ResourceReference(pack=pack, name=str(content['name']))
-        model = action_utils.get_action_by_ref(action_ref)
-        if not model:
-            model = ActionDB()
-        model.name = content['name']
-        model.description = content['description']
-        model.enabled = content['enabled']
-        model.pack = pack
-        model.entry_point = content['entry_point']
-        model.parameters = content.get('parameters', {})
-        runner_type = str(content['runner_type'])
-        valid_runner_type, runner_type_db = self._has_valid_runner_type(runner_type)
-        if valid_runner_type:
-            model.runner_type = {'name': runner_type_db.name}
+        existing = action_utils.get_action_by_ref(action_ref)
+        if not existing:
+            LOG.info('Action %s not found. Creating new one with: %s', action_ref, content)
         else:
-            LOG.exception('Runner type %s doesn\'t exist.', runner_type)
-            raise
+            LOG.info('Action %s found. Will be updated from: %s to: %s',
+                     action_ref, existing, model)
+            model.id = existing.id
 
         try:
             model = Action.add_or_update(model)
@@ -73,12 +77,6 @@ def _register_action(self, pack, action):
             LOG.exception('Failed to write action to db %s.', model.name)
             raise
 
-    def _has_valid_runner_type(self, runner_type):
-        try:
-            return True, action_utils.get_runnertype_by_name(runner_type)
-        except:
-            return False, None
-
     def _register_actions_from_pack(self, pack, actions):
         for action in actions:
             try:

st2actions/st2actions/bootstrap/runnersregistrar.py

@@ -116,7 +116,8 @@ def register_runner_types():
                 },
                 'sudo': {
                     'description': 'The remote command will be executed with sudo.',
-                    'type': 'boolean'
+                    'type': 'boolean',
+                    'default': False
                 },
                 'dir': {
                     'description': 'The working directory where the command will be '
@@ -158,7 +159,8 @@ def register_runner_types():
                 },
                 'sudo': {
                     'description': 'The remote command will be executed with sudo.',
-                    'type': 'boolean'
+                    'type': 'boolean',
+                    'default': False
                 },
                 'dir': {
                     'description': 'The working directory where the command will be '

st2actions/st2actions/utils/param_utils.py

@@ -104,14 +104,18 @@ def _get_resolved_runner_params(runner_parameters, action_parameters,
         # No override if param is immutable
         if param_value.get('immutable', False):
             continue
+
         # Check if param exists in action_parameters and if it has a default value then
         # pickup the override.
-        if param_name in action_parameters and 'default' in action_parameters[param_name]:
+        if param_name in action_parameters:
             action_param = action_parameters[param_name]
-            resolved_params[param_name] = action_param['default']
-            # No further override if param is immutable
+            if action_param.get('default', False):
+                resolved_params[param_name] = action_param['default']
+
+            # No further override (from actionexecution) if param is immutable
             if action_param.get('immutable', False):
                 continue
+
         # Finally pick up override from actionexec_runner_parameters
         if param_name in actionexec_runner_parameters:
             resolved_params[param_name] = actionexec_runner_parameters[param_name]

st2actions/tests/unit/test_actions_registrar.py

@@ -17,33 +17,36 @@
     import simplejson as json
 except ImportError:
     import json
-import os
-
+import jsonschema
 import mock
 
 import st2actions.bootstrap.actionsregistrar as actions_registrar
 from st2common.persistence.action import Action
+import st2common.validators.api.action as action_validator
 from st2common.models.db.action import RunnerTypeDB
 import st2tests.base as tests_base
+import st2tests.fixturesloader as fixtures_loader
 
 MOCK_RUNNER_TYPE_DB = RunnerTypeDB()
 MOCK_RUNNER_TYPE_DB.name = 'run-local'
 MOCK_RUNNER_TYPE_DB.runner_module = 'st2.runners.local'
 
 
 class ActionsRegistrarTest(tests_base.DbTestCase):
-    @mock.patch.object(actions_registrar.ActionsRegistrar, '_has_valid_runner_type',
-                       mock.MagicMock(return_value=(True, MOCK_RUNNER_TYPE_DB)))
+    @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True))
+    @mock.patch.object(action_validator, '_get_runner_model',
+                       mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB))
     def test_register_all_actions(self):
         try:
-            packs_base_path = os.path.join(tests_base.get_fixtures_path())
+            packs_base_path = fixtures_loader.get_fixtures_base_path()
             all_actions_in_db = Action.get_all()
             actions_registrar.register_actions(packs_base_path=packs_base_path)
-            all_actions_in_db = Action.get_all()
-            self.assertTrue(len(all_actions_in_db) > 0)
         except Exception as e:
             print(str(e))
             self.fail('All actions must be registered without exceptions.')
+        else:
+            all_actions_in_db = Action.get_all()
+            self.assertTrue(len(all_actions_in_db) > 0)
 
     def test_register_actions_from_bad_pack(self):
         packs_base_path = tests_base.get_fixtures_path()
@@ -53,12 +56,14 @@ def test_register_actions_from_bad_pack(self):
         except:
             pass
 
-    @mock.patch.object(actions_registrar.ActionsRegistrar, '_has_valid_runner_type',
-                       mock.MagicMock(return_value=(True, MOCK_RUNNER_TYPE_DB)))
+    @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True))
+    @mock.patch.object(action_validator, '_get_runner_model',
+                       mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB))
     def test_pack_name_missing(self):
         registrar = actions_registrar.ActionsRegistrar()
-        action_file = os.path.join(tests_base.get_fixtures_path(),
-                                   'wolfpack/actions/action_3_pack_missing.json')
+        loader = fixtures_loader.FixturesLoader()
+        action_file = loader.get_fixture_file_path_abs(
+            'generic', 'actions', 'action_3_pack_missing.json')
         registrar._register_action('dummy', action_file)
         action_name = None
         with open(action_file, 'r') as fd:
@@ -68,3 +73,37 @@ def test_pack_name_missing(self):
             self.assertEqual(action_db.pack, 'dummy', 'Content pack must be ' +
                              'set to dummy')
             Action.delete(action_db)
+
+    @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True))
+    @mock.patch.object(action_validator, '_get_runner_model',
+                       mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB))
+    def test_invalid_params_schema(self):
+        registrar = actions_registrar.ActionsRegistrar()
+        loader = fixtures_loader.FixturesLoader()
+        action_file = loader.get_fixture_file_path_abs(
+            'generic', 'actions', 'action-invalid-schema-params.yaml')
+        try:
+            registrar._register_action('dummy', action_file)
+            self.fail('Invalid action schema. Should have failed.')
+        except jsonschema.ValidationError:
+            pass
+
+    @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(return_value=True))
+    @mock.patch.object(action_validator, '_get_runner_model',
+                       mock.MagicMock(return_value=MOCK_RUNNER_TYPE_DB))
+    def test_action_update(self):
+        registrar = actions_registrar.ActionsRegistrar()
+        loader = fixtures_loader.FixturesLoader()
+        action_file = loader.get_fixture_file_path_abs(
+            'generic', 'actions', 'action1.json')
+        registrar._register_action('wolfpack', action_file)
+        # try registering again. this should not throw errors.
+        registrar._register_action('wolfpack', action_file)
+        action_name = None
+        with open(action_file, 'r') as fd:
+            content = json.load(fd)
+            action_name = str(content['name'])
+            action_db = Action.get_by_name(action_name)
+            self.assertEqual(action_db.pack, 'wolfpack', 'Content pack must be ' +
+                             'set to wolfpack')
+            Action.delete(action_db)

st2actions/tests/unit/test_param_utils.py

@@ -105,6 +105,8 @@ def test_get_resolved_params(self):
         self.assertEqual(runner_params.get('runnerint'), 555)
         # Assert that a runner param can be overriden by action param default.
         self.assertEqual(runner_params.get('runnerdummy'), 'actiondummy')
+        # Asser that runner param made immutable in action can use default value in runner.
+        self.assertEqual(runner_params.get('runnerfoo'), 'FOO')
         # Assert that an immutable param cannot be overriden by action param or execution param.
         self.assertEqual(runner_params.get('runnerimmutable'), 'runnerimmutable')
 
@@ -328,6 +330,10 @@ def _setup_runner_models(cls):
                     'description': 'Foo int param.',
                     'type': 'number'
                 },
+                'runnerfoo': {
+                    'description': 'Some foo param.',
+                    'default': 'FOO'
+                },
                 'runnerdummy': {
                     'description': 'Dummy param.',
                     'type': 'string',
@@ -358,6 +364,7 @@ def _setup_action_models(cls):
             'actionstr': {'type': 'string', 'required': True},
             'actionint': {'type': 'number', 'default': 10},
             'runnerdummy': {'type': 'string', 'default': 'actiondummy', 'immutable': True},
+            'runnerfoo': {'type': 'string', 'immutable': True},
             'runnerimmutable': {'type': 'string', 'default': 'failed_override'},
             'actionimmutable': {'type': 'string', 'default': 'actionimmutable', 'immutable': True}
         }

st2common/st2common/content/bootstrap.py

@@ -29,8 +29,8 @@
 def register_opts():
     content_opts = [
         cfg.BoolOpt('all', default=False, help='Register sensors, actions and rules.'),
-        cfg.BoolOpt('sensors', default=True, help='Register sensors.'),
-        cfg.BoolOpt('actions', default=True, help='Register actions.'),
+        cfg.BoolOpt('sensors', default=False, help='Register sensors.'),
+        cfg.BoolOpt('actions', default=False, help='Register actions.'),
         cfg.BoolOpt('rules', default=False, help='Register rules.')
     ]
     try:

st2common/st2common/validators/api/action.py

@@ -28,13 +28,7 @@
 
 
 def validate_action(action_api):
-    runner_db = None
-    # Check if runner exists.
-    try:
-        runner_db = get_runnertype_by_name(action_api.runner_type)
-    except StackStormDBObjectNotFoundError:
-        msg = 'RunnerType %s is not found.' % action_api.runner_type
-        raise ValueValidationException(msg)
+    runner_db = _get_runner_model(action_api)
 
     # Check if pack is valid.
     if not _is_valid_pack(action_api.pack):
@@ -47,6 +41,17 @@ def validate_action(action_api):
     _validate_parameters(action_api.parameters, runner_db.runner_parameters)
 
 
+def _get_runner_model(action_api):
+    runner_db = None
+    # Check if runner exists.
+    try:
+        runner_db = get_runnertype_by_name(action_api.runner_type)
+    except StackStormDBObjectNotFoundError:
+        msg = 'RunnerType %s is not found.' % action_api.runner_type
+        raise ValueValidationException(msg)
+    return runner_db
+
+
 def _is_valid_pack(pack):
     base_path = cfg.CONF.content.packs_base_path
     return os.path.exists(os.path.join(base_path, pack, 'actions'))
@@ -61,6 +66,10 @@ def _validate_parameters(action_params=None, runner_params=None):
                     msg = 'Param %s is declared immutable in runner. ' % param + \
                           'Cannot override in action.'
                     raise ValueValidationException(msg)
-            if 'default' not in action_param_meta:
-                msg = 'Immutable param %s requires a default value.' % param
-                raise ValueValidationException(msg)
+                if 'default' not in action_param_meta and 'default' not in runner_param_meta:
+                    msg = 'Immutable param %s requires a default value.' % param
+                    raise ValueValidationException(msg)
+            else:
+                if 'default' not in action_param_meta:
+                    msg = 'Immutable param %s requires a default value.' % param
+                    raise ValueValidationException(msg)

st2common/tests/unit/test_action_api_validator.py

@@ -82,3 +82,17 @@ def test_validate_action_param_immutable(self):
             self.fail('Action validation should not have passed. %s' % json.dumps(action_api_dict))
         except ValueValidationException as e:
             self.assertTrue('requires a default value.' in e.message)
+
+    @mock.patch.object(action_validator, '_is_valid_pack', mock.MagicMock(
+        return_value=True))
+    def test_validate_action_param_immutable_no_default(self):
+        action_api_dict = fixture.ARTIFACTS['actions']['action-immutable-runner-param-no-default']
+        action_api = ActionAPI(**action_api_dict)
+
+        # Runner param sudo is decalred immutable in action but no defualt value
+        # supplied in action. We should pick up default value from runner.
+        try:
+            action_validator.validate_action(action_api)
+        except ValueValidationException as e:
+            print(e)
+            self.fail('Action validation should have passed. %s' % json.dumps(action_api_dict))