Improve snapshots list API filtering

1) Added new params for filtering by:
- source share
- usage of snapshot*
- limit and offset a la pagination
- direction (asc, desc)*
- key of snapshot*

* requires server-side update

2) Added aliases for params, mostly in perspective to avoid mess of underscore
and dash symbols.

Implements blueprint improve-snapshot-list-filtering

Change-Id: I3727911f179ea01dd4cb68149fa7f4d79ca75a5c

manilaclient/common/constants.py

@@ -27,3 +27,14 @@ SHARE_SORT_KEY_VALUES = (
     'share_network_id', 'share_network',
     'snapshot_id', 'snapshot',
 )
+
+SNAPSHOT_SORT_KEY_VALUES = (
+    'id',
+    'status',
+    'size',
+    'share_id',
+    'user_id',
+    'project_id',
+    'progress',
+    'name', 'display_name',
+)

manilaclient/tests/unit/v1/fakes.py

@@ -90,8 +90,19 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
             raise AssertionError("Unexpected action: %s" % action)
         return (resp, {}, _body)
 
+    def get_snapshots(self, **kw):
+        snapshots = {
+            'snapshots': [
+                {
+                    'id': 1234,
+                    'status': 'available',
+                    'name': 'sharename',
+                }
+            ]
+        }
+        return (200, {}, snapshots)
+
     def get_snapshots_detail(self, **kw):
-        print(kw)
         snapshots = {'snapshots': [{
             'id': 1234,
             'created_at': '2012-08-27T00:00:00.000000',

manilaclient/tests/unit/v1/test_share_snapshots.py

@@ -1,6 +1,6 @@
 # Copyright 2010 Jacob Kaplan-Moss
-
 # Copyright 2011 OpenStack LLC.
+# Copyright 2014 Mirantis, Inc.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -44,6 +44,32 @@ class ShareSnapshotsTest(utils.TestCase):
         cs.assert_called('POST', '/snapshots/1234/action',
                          {'os-force_delete': None})
 
-    def test_list_share_snapshots(self):
+    def test_list_share_snapshots_index(self):
-        cs.share_snapshots.list()
+        cs.share_snapshots.list(detailed=False)
+        cs.assert_called('GET', '/snapshots')
+
+    def test_list_share_snapshots_index_with_search_opts(self):
+        search_opts = {'fake_str': 'fake_str_value', 'fake_int': 1}
+        cs.share_snapshots.list(detailed=False, search_opts=search_opts)
+        cs.assert_called(
+            'GET', '/snapshots?fake_int=1&fake_str=fake_str_value')
+
+    def test_list_share_snapshots_sort_by_asc_and_share_id(self):
+        cs.share_snapshots.list(
+            detailed=False, sort_key='share_id', sort_dir='asc')
+        cs.assert_called('GET', '/snapshots?sort_dir=asc&sort_key=share_id')
+
+    def test_list_share_snapshots_sort_by_desc_and_status(self):
+        cs.share_snapshots.list(
+            detailed=False, sort_key='status', sort_dir='desc')
+        cs.assert_called('GET', '/snapshots?sort_dir=desc&sort_key=status')
+
+    def test_list_share_snapshots_by_improper_direction(self):
+        self.assertRaises(ValueError, cs.share_snapshots.list, sort_dir='fake')
+
+    def test_list_share_snapshots_by_improper_key(self):
+        self.assertRaises(ValueError, cs.share_snapshots.list, sort_key='fake')
+
+    def test_list_share_snapshots_detail(self):
+        cs.share_snapshots.list(detailed=True)
         cs.assert_called('GET', '/snapshots/detail')

manilaclient/tests/unit/v1/test_shell.py

@@ -267,14 +267,102 @@ class ShellTest(test_utils.TestCase):
             'delete fake-not-found'
         )
 
-    def test_snapshot_list_filter_share_id(self):
+    def test_list_snapshots(self):
-        self.run_command('snapshot-list --share-id=1234')
+        self.run_command('snapshot-list')
-        self.assert_called('GET', '/snapshots/detail?share_id=1234')
+        self.assert_called('GET', '/snapshots/detail?usage=any')
 
-    def test_snapshot_list_filter_status_and_share_id(self):
+    def test_list_snapshots_all_tenants_only_key(self):
-        self.run_command('snapshot-list --status=available --share-id=1234')
+        self.run_command('snapshot-list --all-tenants')
-        self.assert_called('GET', '/snapshots/detail?'
+        self.assert_called('GET', '/snapshots/detail?all_tenants=1&usage=any')
-                           'share_id=1234&status=available')
+
+    def test_list_snapshots_all_tenants_key_and_value_1(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --all-tenants' + separator + '1')
+            self.assert_called(
+                'GET', '/snapshots/detail?all_tenants=1&usage=any')
+
+    def test_list_snapshots_all_tenants_key_and_value_0(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --all-tenants' + separator + '0')
+            self.assert_called('GET', '/snapshots/detail?usage=any')
+
+    def test_list_snapshots_filter_by_name(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --name' + separator + '1234')
+            self.assert_called(
+                'GET', '/snapshots/detail?name=1234&usage=any')
+
+    def test_list_snapshots_filter_by_status(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --status' + separator + '1234')
+            self.assert_called(
+                'GET', '/snapshots/detail?status=1234&usage=any')
+
+    def test_list_snapshots_filter_by_share_id(self):
+        aliases = ['--share_id', '--share-id']
+        for alias in aliases:
+            for separator in self.separators:
+                self.run_command('snapshot-list ' + alias + separator + '1234')
+                self.assert_called(
+                    'GET', '/snapshots/detail?share_id=1234&usage=any')
+
+    def test_list_snapshots_only_used(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --usage' + separator + 'used')
+            self.assert_called('GET', '/snapshots/detail?usage=used')
+
+    def test_list_snapshots_only_unused(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --usage' + separator + 'unused')
+            self.assert_called('GET', '/snapshots/detail?usage=unused')
+
+    def test_list_snapshots_with_limit(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --limit' + separator + '50')
+            self.assert_called(
+                'GET', '/snapshots/detail?limit=50&usage=any')
+
+    def test_list_snapshots_with_offset(self):
+        for separator in self.separators:
+            self.run_command('snapshot-list --offset' + separator + '50')
+            self.assert_called(
+                'GET', '/snapshots/detail?offset=50&usage=any')
+
+    def test_list_snapshots_with_sort_dir_verify_keys(self):
+        aliases = ['--sort_dir', '--sort-dir']
+        for alias in aliases:
+            for key in constants.SORT_DIR_VALUES:
+                for separator in self.separators:
+                    self.run_command(
+                        'snapshot-list ' + alias + separator + key)
+                    self.assert_called(
+                        'GET',
+                        '/snapshots/detail?sort_dir=' + key + '&usage=any')
+
+    def test_list_snapshots_with_fake_sort_dir(self):
+        self.assertRaises(
+            ValueError,
+            self.run_command,
+            'snapshot-list --sort-dir fake_sort_dir',
+        )
+
+    def test_list_snapshots_with_sort_key_verify_keys(self):
+        aliases = ['--sort_key', '--sort-key']
+        for alias in aliases:
+            for key in constants.SNAPSHOT_SORT_KEY_VALUES:
+                for separator in self.separators:
+                    self.run_command(
+                        'snapshot-list ' + alias + separator + key)
+                    self.assert_called(
+                        'GET',
+                        '/snapshots/detail?sort_key=' + key + '&usage=any')
+
+    def test_list_snapshots_with_fake_sort_key(self):
+        self.assertRaises(
+            ValueError,
+            self.run_command,
+            'snapshot-list --sort-key fake_sort_key',
+        )
 
     def test_rename(self):
         # basic rename with positional agruments

manilaclient/v1/share_snapshots.py

@@ -20,6 +20,7 @@ except ImportError:
     from urllib.parse import urlencode  # noqa
 
 from manilaclient import base
+from manilaclient.common import constants
 from manilaclient.openstack.common.apiclient import base as common_base
 
 
@@ -74,11 +75,34 @@ class ShareSnapshotManager(base.ManagerWithFind):
         """
         return self._get('/snapshots/%s' % snapshot_id, 'snapshot')
 
-    def list(self, detailed=True, search_opts=None):
+    def list(self, detailed=True, search_opts=None, sort_key=None,
-        """Get a list of all snapshots of shares.
+             sort_dir=None):
+        """Get a list of snapshots of shares.
 
+        :param search_opts: Search options to filter out shares.
+        :param sort_key: Key to be sorted.
+        :param sort_dir: Sort direction, should be 'desc' or 'asc'.
         :rtype: list of :class:`ShareSnapshot`
         """
+        if search_opts is None:
+            search_opts = {}
+
+        if sort_key is not None:
+            if sort_key in constants.SNAPSHOT_SORT_KEY_VALUES:
+                search_opts['sort_key'] = sort_key
+            else:
+                raise ValueError(
+                    'sort_key must be one of the following: %s.'
+                    % ', '.join(constants.SNAPSHOT_SORT_KEY_VALUES))
+
+        if sort_dir is not None:
+            if sort_dir in constants.SORT_DIR_VALUES:
+                search_opts['sort_dir'] = sort_dir
+            else:
+                raise ValueError(
+                    'sort_dir must be one of the following: %s.'
+                    % ', '.join(constants.SORT_DIR_VALUES))
+
         if search_opts:
             query_string = urlencode(
                 sorted([(k, v) for (k, v) in list(search_opts.items()) if v]))

manilaclient/v1/shell.py

@@ -726,22 +726,78 @@ def do_list(cs, args):
     help='Filter results by status.')
 @cliutils.arg(
     '--share-id',
-    metavar='<share-id>',
+    '--share_id',  # alias
+    metavar='<share_id>',
     default=None,
-    help='Filter results by share ID.')
+    action='single_alias',
+    help='Filter results by source share ID.')
+@cliutils.arg(
+    '--usage',
+    dest='usage',
+    metavar='any|used|unused',
+    nargs='?',
+    type=str,
+    const='any',
+    default='any',
+    choices=['any', 'used', 'unused', ],
+    help='Either filter or not snapshots by its usage. OPTIONAL: Default=any.')
+@cliutils.arg(
+    '--limit',
+    metavar='<limit>',
+    type=int,
+    default=None,
+    help='Maximum number of share snapshots to return. '
+         'OPTIONAL: Default=None.')
+@cliutils.arg(
+    '--offset',
+    metavar='<offset>',
+    type=int,
+    default=None,
+    help='Set offset to define start point of share snapshots listing. '
+         'OPTIONAL: Default=None.')
+@cliutils.arg(
+    '--sort-key',
+    '--sort_key',  # alias
+    metavar='<sort_key>',
+    type=str,
+    default=None,
+    action='single_alias',
+    help='Key to be sorted, available keys are %(keys)s. '
+         'OPTIONAL: Default=None.' % {
+             'keys': constants.SNAPSHOT_SORT_KEY_VALUES})
+@cliutils.arg(
+    '--sort-dir',
+    '--sort_dir',  # alias
+    metavar='<sort_dir>',
+    type=str,
+    default=None,
+    action='single_alias',
+    help='Sort direction, available values are %(values)s. '
+         'OPTIONAL: Default=None.' % {'values': constants.SORT_DIR_VALUES})
 @cliutils.service_type('share')
 def do_snapshot_list(cs, args):
     """List all the snapshots."""
+    list_of_keys = [
+        'ID', 'Share ID', 'Status', 'Name', 'Share Size',
+    ]
     all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants))
+    empty_obj = type('Empty', (object,), {'id': None})
+    share = _find_share(cs, args.share_id) if args.share_id else empty_obj
     search_opts = {
+        'offset': args.offset,
+        'limit': args.limit,
         'all_tenants': all_tenants,
         'name': args.name,
         'status': args.status,
-        'share_id': args.share_id,
+        'share_id': share.id,
+        'usage': args.usage,
     }
-    snapshots = cs.share_snapshots.list(search_opts=search_opts)
+    snapshots = cs.share_snapshots.list(
-    utils.print_list(snapshots,
+        search_opts=search_opts,
-                     ['ID', 'Share ID', 'Status', 'Name', 'Share Size'])
+        sort_key=args.sort_key,
+        sort_dir=args.sort_dir,
+    )
+    utils.print_list(snapshots, list_of_keys)
 
 
 @cliutils.arg(