Adopt CLI sorting argument guidelines

CLI sorting guidelines are defined in the cross project specs
repository: [1]

This patch deprecates --sort_key and --sort_dir and adds support for
--sort in the 'cinder list' command with the following syntax:

  cinder list --sort <key>[:<direction>]

The --sort parameter is comma-separated and is used to specify
one or more sort keys and directions.

Server-side work for sorting by multiple keys is handled in [2].

[1] https://github.com/openstack/openstack-specs/blob/master/specs/
    cli-sorting-args.rst
[2] https://review.openstack.org/#/c/141915/

Implements: blueprint cinder-pagination

Change-Id: Ie8c2c62b8c129f647f85d66d5bcfe4c8f0f4eedb

cinderclient/tests/v2/test_shell.py

@@ -16,6 +16,7 @@
 import fixtures
 import mock
 from requests_mock.contrib import fixture as requests_mock_fixture
+from six.moves.urllib import parse
 
 from cinderclient import client
 from cinderclient import exceptions
@@ -108,11 +109,16 @@ class ShellTest(utils.TestCase):
         self.run_command('list --sort_key=id --sort_dir=asc')
         self.assert_called('GET', '/volumes/detail?sort_dir=asc&sort_key=id')
 
-    def test_list_sort_name(self):
+    def test_list_sort_key_name(self):
         # Client 'name' key is mapped to 'display_name'
         self.run_command('list --sort_key=name')
         self.assert_called('GET', '/volumes/detail?sort_key=display_name')
 
+    def test_list_sort_name(self):
+        # Client 'name' key is mapped to 'display_name'
+        self.run_command('list --sort=name')
+        self.assert_called('GET', '/volumes/detail?sort=display_name')
+
     def test_list_sort_key_invalid(self):
         self.assertRaises(ValueError,
                           self.run_command,
@@ -123,11 +129,45 @@ class ShellTest(utils.TestCase):
                           self.run_command,
                           'list --sort_key=id --sort_dir=foo')
 
+    def test_list_mix_sort_args(self):
+        cmds = ['list --sort name:desc --sort_key=status',
+                'list --sort name:desc --sort_dir=asc',
+                'list --sort name:desc --sort_key=status --sort_dir=asc']
+        for cmd in cmds:
+            self.assertRaises(exceptions.CommandError, self.run_command, cmd)
+
+    def test_list_sort_single_key_only(self):
+        self.run_command('list --sort=id')
+        self.assert_called('GET', '/volumes/detail?sort=id')
+
+    def test_list_sort_single_key_trailing_colon(self):
+        self.run_command('list --sort=id:')
+        self.assert_called('GET', '/volumes/detail?sort=id')
+
+    def test_list_sort_single_key_and_dir(self):
+        self.run_command('list --sort=id:asc')
+        url = '/volumes/detail?%s' % parse.urlencode([('sort', 'id:asc')])
+        self.assert_called('GET', url)
+
+    def test_list_sort_multiple_keys_only(self):
+        self.run_command('list --sort=id,status,size')
+        url = ('/volumes/detail?%s' %
+               parse.urlencode([('sort', 'id,status,size')]))
+        self.assert_called('GET', url)
+
+    def test_list_sort_multiple_keys_and_dirs(self):
+        self.run_command('list --sort=id:asc,status,size:desc')
+        url = ('/volumes/detail?%s' %
+               parse.urlencode([('sort', 'id:asc,status,size:desc')]))
+        self.assert_called('GET', url)
+
     def test_list_reorder_with_sort(self):
         # sortby_index is None if there is sort information
         for cmd in ['list --sort_key=name',
                     'list --sort_dir=asc',
-                    'list --sort_key=name --sort_dir=asc']:
+                    'list --sort_key=name --sort_dir=asc',
+                    'list --sort=name',
+                    'list --sort=name:asc']:
             with mock.patch('cinderclient.utils.print_list') as mock_print:
                 self.run_command(cmd)
                 mock_print.assert_called_once_with(

cinderclient/tests/v2/test_volumes.py

@@ -223,3 +223,57 @@ class VolumesTest(utils.TestCase):
     def test_get_pools_detail(self):
         cs.volumes.get_pools('--detail')
         cs.assert_called('GET', '/scheduler-stats/get_pools?detail=True')
+
+
+class FormatSortParamTestCase(utils.TestCase):
+
+    def test_format_sort_empty_input(self):
+        for s in [None, '', []]:
+            self.assertEqual(None, cs.volumes._format_sort_param(s))
+
+    def test_format_sort_string_single_key(self):
+        s = 'id'
+        self.assertEqual('id', cs.volumes._format_sort_param(s))
+
+    def test_format_sort_string_single_key_and_dir(self):
+        s = 'id:asc'
+        self.assertEqual('id:asc', cs.volumes._format_sort_param(s))
+
+    def test_format_sort_string_multiple(self):
+        s = 'id:asc,status,size:desc'
+        self.assertEqual('id:asc,status,size:desc',
+                         cs.volumes._format_sort_param(s))
+
+    def test_format_sort_string_mappings(self):
+        s = 'id:asc,name,size:desc'
+        self.assertEqual('id:asc,display_name,size:desc',
+                         cs.volumes._format_sort_param(s))
+
+    def test_format_sort_whitespace_trailing_comma(self):
+        s = ' id : asc ,status,  size:desc,'
+        self.assertEqual('id:asc,status,size:desc',
+                         cs.volumes._format_sort_param(s))
+
+    def test_format_sort_list_of_strings(self):
+        s = ['id:asc', 'status', 'size:desc']
+        self.assertEqual('id:asc,status,size:desc',
+                         cs.volumes._format_sort_param(s))
+
+    def test_format_sort_list_of_tuples(self):
+        s = [('id', 'asc'), 'status', ('size', 'desc')]
+        self.assertEqual('id:asc,status,size:desc',
+                         cs.volumes._format_sort_param(s))
+
+    def test_format_sort_list_of_strings_and_tuples(self):
+        s = [('id', 'asc'), 'status', 'size:desc']
+        self.assertEqual('id:asc,status,size:desc',
+                         cs.volumes._format_sort_param(s))
+
+    def test_format_sort_invalid_direction(self):
+        for s in ['id:foo',
+                  'id:asc,status,size:foo',
+                  ['id', 'status', 'size:foo'],
+                  ['id', 'status', ('size', 'foo')]]:
+            self.assertRaises(ValueError,
+                              cs.volumes._format_sort_param,
+                              s)

cinderclient/v2/shell.py

@@ -28,6 +28,7 @@ from cinderclient import exceptions
 from cinderclient import utils
 from cinderclient.openstack.common import strutils
 from cinderclient.v2 import availability_zones
+from cinderclient.v2 import volumes
 
 
 def _poll_for_status(poll_fn, obj_id, action, final_ok_states,
@@ -179,14 +180,18 @@ def _extract_metadata(args):
 @utils.arg('--sort_key',
            metavar='<sort_key>',
            default=None,
-           help='Key to be sorted, should be (`id`, `status`, `size`, '
-           '`availability_zone`, `name`, `bootable`, `created_at`). '
-           'OPTIONAL: Default=None.')
+           help=argparse.SUPPRESS)
 @utils.arg('--sort_dir',
            metavar='<sort_dir>',
            default=None,
-           help='Sort direction, should be `desc` or `asc`. '
-           'OPTIONAL: Default=None.')
+           help=argparse.SUPPRESS)
+@utils.arg('--sort',
+           metavar='<key>[:<direction>]',
+           default=None,
+           help=(('Comma-separated list of sort keys and directions in the '
+           'form of <key>[:<asc|desc>]. '
+           'Valid keys: %s. '
+           'OPTIONAL: Default=None.') % ', '.join(volumes.SORT_KEY_VALUES)))
 @utils.service_type('volumev2')
 def do_list(cs, args):
     """Lists all volumes."""
@@ -201,9 +206,17 @@ def do_list(cs, args):
         'status': args.status,
         'metadata': _extract_metadata(args) if args.metadata else None,
     }
+
+    # --sort_key and --sort_dir deprecated in kilo and is not supported
+    # with --sort
+    if args.sort and (args.sort_key or args.sort_dir):
+        raise exceptions.CommandError(
+            'The --sort_key and --sort_dir arguments are deprecated and are '
+            'not supported with --sort.')
+
     volumes = cs.volumes.list(search_opts=search_opts, marker=args.marker,
                               limit=args.limit, sort_key=args.sort_key,
-                              sort_dir=args.sort_dir)
+                              sort_dir=args.sort_dir, sort=args.sort)
     _translate_volume_keys(volumes)
 
     # Create a list of servers to which the volume is attached
@@ -217,7 +230,7 @@ def do_list(cs, args):
     else:
         key_list = ['ID', 'Status', 'Name',
                     'Size', 'Volume Type', 'Bootable', 'Attached to']
-    if args.sort_key or args.sort_dir:
+    if args.sort_key or args.sort_dir or args.sort:
         sortby_index = None
     else:
         sortby_index = 0

cinderclient/v2/volumes.py

@@ -233,8 +233,57 @@ class VolumeManager(base.ManagerWithFind):
         """
         return self._get("/volumes/%s" % volume_id, "volume")
 
+    def _format_sort_param(self, sort):
+        '''Formats the sort information into the sort query string parameter.
+
+        The input sort information can be any of the following:
+        - Comma-separated string in the form of <key[:dir]>
+        - List of strings in the form of <key[:dir]>
+        - List of either string keys, or tuples of (key, dir)
+
+        For example, the following import sort values are valid:
+        - 'key1:dir1,key2,key3:dir3'
+        - ['key1:dir1', 'key2', 'key3:dir3']
+        - [('key1', 'dir1'), 'key2', ('key3', dir3')]
+
+        :param sort: Input sort information
+        :returns: Formatted query string parameter or None
+        :raise ValueError: If an invalid sort direction or invalid sort key is
+                           given
+        '''
+        if not sort:
+            return None
+
+        if isinstance(sort, six.string_types):
+            # Convert the string into a list for consistent validation
+            sort = [s for s in sort.split(',') if s]
+
+        sort_array = []
+        for sort_item in sort:
+            if isinstance(sort_item, tuple):
+                sort_key = sort_item[0]
+                sort_dir = sort_item[1]
+            else:
+                sort_key, _sep, sort_dir = sort_item.partition(':')
+            sort_key = sort_key.strip()
+            if sort_key in SORT_KEY_VALUES:
+                sort_key = SORT_KEY_MAPPINGS.get(sort_key, sort_key)
+            else:
+                raise ValueError('sort_key must be one of the following: %s.'
+                                 % ', '.join(SORT_KEY_VALUES))
+            if sort_dir:
+                sort_dir = sort_dir.strip()
+                if sort_dir not in SORT_DIR_VALUES:
+                    msg = ('sort_dir must be one of the following: %s.'
+                           % ', '.join(SORT_DIR_VALUES))
+                    raise ValueError(msg)
+                sort_array.append('%s:%s' % (sort_key, sort_dir))
+            else:
+                sort_array.append(sort_key)
+        return ','.join(sort_array)
+
     def list(self, detailed=True, search_opts=None, marker=None, limit=None,
-             sort_key=None, sort_dir=None):
+             sort_key=None, sort_dir=None, sort=None):
         """Lists all volumes.
 
         :param detailed: Whether to return detailed volume info.
@@ -242,8 +291,10 @@ class VolumeManager(base.ManagerWithFind):
         :param marker: Begin returning volumes that appear later in the volume
                        list than that represented by this volume id.
         :param limit: Maximum number of volumes to return.
-        :param sort_key: Key to be sorted.
-        :param sort_dir: Sort direction, should be 'desc' or 'asc'.
+        :param sort_key: Key to be sorted; deprecated in kilo
+        :param sort_dir: Sort direction, should be 'desc' or 'asc'; deprecated
+                         in kilo
+        :param sort: Sort information
         :rtype: list of :class:`Volume`
         """
         if search_opts is None:
@@ -261,19 +312,25 @@ class VolumeManager(base.ManagerWithFind):
         if limit:
             qparams['limit'] = limit
 
-        if sort_key is not None:
-            if sort_key in SORT_KEY_VALUES:
-                qparams['sort_key'] = SORT_KEY_MAPPINGS.get(sort_key, sort_key)
-            else:
-                raise ValueError('sort_key must be one of the following: %s.'
-                                 % ', '.join(SORT_KEY_VALUES))
-
-        if sort_dir is not None:
-            if sort_dir in SORT_DIR_VALUES:
-                qparams['sort_dir'] = sort_dir
-            else:
-                raise ValueError('sort_dir must be one of the following: %s.'
-                                 % ', '.join(SORT_DIR_VALUES))
+        # sort_key and sort_dir deprecated in kilo, prefer sort
+        if sort:
+            qparams['sort'] = self._format_sort_param(sort)
+        else:
+            if sort_key is not None:
+                if sort_key in SORT_KEY_VALUES:
+                    qparams['sort_key'] = SORT_KEY_MAPPINGS.get(sort_key,
+                                                                sort_key)
+                else:
+                    msg = ('sort_key must be one of the following: %s.'
+                           % ', '.join(SORT_KEY_VALUES))
+                    raise ValueError(msg)
+            if sort_dir is not None:
+                if sort_dir in SORT_DIR_VALUES:
+                    qparams['sort_dir'] = sort_dir
+                else:
+                    msg = ('sort_dir must be one of the following: %s.'
+                           % ', '.join(SORT_DIR_VALUES))
+                    raise ValueError(msg)
 
         # Transform the dict to a sequence of two-element tuples in fixed
         # order, then the encoded string will be consistent in Python 2&3.