Avoid empty entity field in uri path

Currently, empty entity field is accepted but will fail when call cli,
it is because the empty field leads to incorrect rest api uri, which will
get 404 error.

This patch checks some required entity fields, which will present in uri path,
and raises CommandError if they are empty. Note the other required fields
will not cause the same problem, so they are not checked.

Change-Id: I59411b760ff7457064b386911e868518b3057e3a
Closes-Bug: #1313679

2 files changed, 105 insertions, 15 deletions

diff --git a/ceilometerclient/tests/v2/test_shell.py b/ceilometerclient/tests/v2/test_shell.py
index 80c57c8..5c127bc 100644
--- a/ceilometerclient/tests/v2/test_shell.py
+++ b/ceilometerclient/tests/v2/test_shell.py
@@ -22,6 +22,7 @@ import sys
 
 from testtools import matchers
 
+from ceilometerclient import exc
 from ceilometerclient import shell as base_shell
 from ceilometerclient.tests import utils
 from ceilometerclient.v2 import alarms
@@ -788,3 +789,83 @@ class ShellStatisticsTest(utils.BaseTestCase):
                 fields,
                 [self.displays.get(f, f) for f in fields],
             )
+
+
+class ShellEmptyIdTest(utils.BaseTestCase):
+    """Test empty field which will cause calling incorrect rest uri."""
+
+    def _test_entity_action_with_empty_values(self, entity, *args):
+        for value in ('', ' ', '   ', '\t'):
+            self._test_entity_action_with_empty_value(entity, value, *args)
+
+    def _test_entity_action_with_empty_value(self, entity, value, *args):
+        shell = base_shell.CeilometerShell()
+        argv = list(args) + ['--%s' % entity, value]
+
+        with mock.patch('ceilometerclient.exc.CommandError') as e:
+            e.return_value = exc.BaseException()
+            self.assertRaises(exc.BaseException, shell.parse_args, argv)
+            entity = entity.replace('-', '_')
+            e.assert_called_with('%s should not be empty' % entity)
+
+    def _test_alarm_action_with_empty_ids(self, method, *args):
+        args = [method] + list(args)
+        self._test_entity_action_with_empty_values('alarm_id', *args)
+
+    def test_alarm_show_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-show')
+
+    def test_alarm_update_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-update')
+
+    def test_alarm_threshold_update_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-threshold-update')
+
+    def test_alarm_combination_update_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-combination-update')
+
+    def test_alarm_delete_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-delete')
+
+    def test_alarm_state_get_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-state-get')
+
+    def test_alarm_state_set_with_empty_id(self):
+        args = ['alarm-state-set', '--state', 'ok']
+        self._test_alarm_action_with_empty_ids(*args)
+
+    def test_alarm_history_with_empty_id(self):
+        self._test_alarm_action_with_empty_ids('alarm-history')
+
+    def test_event_show_with_empty_message_id(self):
+        args = ['event-show']
+        self._test_entity_action_with_empty_values('message_id', *args)
+
+    def test_resource_show_with_empty_id(self):
+        args = ['resource-show']
+        self._test_entity_action_with_empty_values('resource_id', *args)
+
+    def test_sample_list_with_empty_meter(self):
+        args = ['sample-list']
+        self._test_entity_action_with_empty_values('meter', *args)
+
+    def test_sample_create_with_empty_meter(self):
+        args = ['sample-create', '-r', 'x', '--meter-type', 'gauge',
+                '--meter-unit', 'B', '--sample-volume', '1']
+        self._test_entity_action_with_empty_values('meter-name', *args)
+
+    def test_statistics_with_empty_meter(self):
+        args = ['statistics']
+        self._test_entity_action_with_empty_values('meter', *args)
+
+    def test_trait_description_list_with_empty_event_type(self):
+        args = ['trait-description-list']
+        self._test_entity_action_with_empty_values('event_type', *args)
+
+    def test_trait_list_with_empty_event_type(self):
+        args = ['trait-list', '--trait_name', 'x']
+        self._test_entity_action_with_empty_values('event_type', *args)
+
+    def test_trait_list_with_empty_trait_name(self):
+        args = ['trait-list', '--event_type', 'x']
+        self._test_entity_action_with_empty_values('trait_name', *args)
diff --git a/ceilometerclient/v2/shell.py b/ceilometerclient/v2/shell.py
index 961bd10..cc768b1 100644
--- a/ceilometerclient/v2/shell.py
+++ b/ceilometerclient/v2/shell.py
@@ -19,6 +19,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import argparse
 import functools
 import json
 import six
@@ -48,11 +49,18 @@ COMPLEX_OPERATORS = ['and', 'or']
 SIMPLE_OPERATORS = ["=", "!=", "<", "<=", '>', '>=']
 
 
+class NotEmptyAction(argparse.Action):
+    def __call__(self, parser, namespace, values, option_string=None):
+        if not values or values.isspace():
+            raise exc.CommandError('%s should not be empty' % self.dest)
+        setattr(namespace, self.dest, values)
+
+
 @utils.arg('-q', '--query', metavar='<QUERY>',
            help='key[op]data_type::value; list. data_type is optional, '
                 'but if supplied must be string, integer, float, or boolean.')
 @utils.arg('-m', '--meter', metavar='<NAME>', required=True,
-           help='Name of meter to show samples for.')
+           action=NotEmptyAction, help='Name of meter to show samples for.')
 @utils.arg('-p', '--period', metavar='<PERIOD>',
            help='Period in seconds over which to group samples.')
 @utils.arg('-g', '--groupby', metavar='<FIELD>', action='append',
@@ -106,7 +114,7 @@ def do_statistics(cc, args):
            help='key[op]data_type::value; list. data_type is optional, '
                 'but if supplied must be string, integer, float, or boolean.')
 @utils.arg('-m', '--meter', metavar='<NAME>', required=True,
-           help='Name of meter to show samples for.')
+           action=NotEmptyAction, help='Name of meter to show samples for.')
 @utils.arg('-l', '--limit', metavar='<NUMBER>',
            help='Maximum number of samples to return.')
 def do_sample_list(cc, args):
@@ -136,7 +144,7 @@ def do_sample_list(cc, args):
 @utils.arg('-r', '--resource-id', metavar='<RESOURCE_ID>', required=True,
            help='ID of the resource.')
 @utils.arg('-m', '--meter-name', metavar='<METER_NAME>', required=True,
-           help='The meter name.')
+           action=NotEmptyAction, help='The meter name.')
 @utils.arg('--meter-type', metavar='<METER_TYPE>', required=True,
            help='The meter type.')
 @utils.arg('--meter-unit', metavar='<METER_UNIT>', required=True,
@@ -318,7 +326,7 @@ def _display_alarm(alarm):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm to show.')
+           action=NotEmptyAction, help='ID of the alarm to show.')
 def do_alarm_show(cc, args={}):
     '''Show an alarm.'''
     try:
@@ -476,7 +484,7 @@ def do_alarm_combination_create(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm to update.')
+           action=NotEmptyAction, help='ID of the alarm to update.')
 @common_alarm_arguments()
 @utils.arg('--remove-time-constraint', action='append',
            metavar='<Constraint names>',
@@ -517,7 +525,7 @@ def do_alarm_update(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm to update.')
+           action=NotEmptyAction, help='ID of the alarm to update.')
 @common_alarm_arguments()
 @utils.arg('--remove-time-constraint', action='append',
            metavar='<Constraint names>',
@@ -568,7 +576,7 @@ def do_alarm_threshold_update(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm to update.')
+           action=NotEmptyAction, help='ID of the alarm to update.')
 @common_alarm_arguments()
 @utils.arg('--remove-time-constraint', action='append',
            metavar='<Constraint names>',
@@ -600,7 +608,7 @@ def do_alarm_combination_update(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm to delete.')
+           action=NotEmptyAction, help='ID of the alarm to delete.')
 def do_alarm_delete(cc, args={}):
     '''Delete an alarm.'''
     try:
@@ -610,7 +618,7 @@ def do_alarm_delete(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm state to set.')
+           action=NotEmptyAction, help='ID of the alarm state to set.')
 @utils.arg('--state', metavar='<STATE>', required=True,
            help='State of the alarm, one of: ' + str(ALARM_STATES) +
            '.')
@@ -624,7 +632,7 @@ def do_alarm_state_set(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
-           help='ID of the alarm state to show.')
+           action=NotEmptyAction, help='ID of the alarm state to show.')
 def do_alarm_state_get(cc, args={}):
     '''Get the state of an alarm.'''
     try:
@@ -635,6 +643,7 @@ def do_alarm_state_get(cc, args={}):
 
 
 @utils.arg('-a', '--alarm_id', metavar='<ALARM_ID>', required=True,
+           action=NotEmptyAction,
            help='ID of the alarm for which history is shown.')
 @utils.arg('-q', '--query', metavar='<QUERY>',
            help='key[op]data_type::value; list. data_type is optional, '
@@ -668,7 +677,7 @@ def do_resource_list(cc, args={}):
 
 
 @utils.arg('-r', '--resource_id', metavar='<RESOURCE_ID>', required=True,
-           help='ID of the resource to show.')
+           action=NotEmptyAction, help='ID of the resource to show.')
 def do_resource_show(cc, args={}):
     '''Show the resource.'''
     try:
@@ -701,7 +710,7 @@ def do_event_list(cc, args={}):
 
 @utils.arg('-m', '--message_id', metavar='<message_id>',
            help='The ID of the event. Should be a UUID.',
-           required=True)
+           required=True, action=NotEmptyAction)
 def do_event_show(cc, args={}):
     '''Show a particular event.'''
     event = cc.events.get(args.message_id)
@@ -718,7 +727,7 @@ def do_event_type_list(cc, args={}):
 
 @utils.arg('-e', '--event_type', metavar='<EVENT_TYPE>',
            help='Type of the event for which traits will be shown.',
-           required=True)
+           required=True, action=NotEmptyAction)
 def do_trait_description_list(cc, args={}):
     '''List trait info for an event type.'''
     trait_descriptions = cc.trait_descriptions.list(args.event_type)
@@ -729,10 +738,10 @@ def do_trait_description_list(cc, args={}):
 
 @utils.arg('-e', '--event_type', metavar='<EVENT_TYPE>',
            help='Type of the event for which traits will listed.',
-           required=True)
+           required=True, action=NotEmptyAction)
 @utils.arg('-t', '--trait_name', metavar='<TRAIT_NAME>',
            help='The name of the trait to list.',
-           required=True)
+           required=True, action=NotEmptyAction)
 def do_trait_list(cc, args={}):
     '''List trait all traits with name <trait_name> for Event Type
     <event_type>.