Improve share list API filtering

1) Added new params for filtering by:
- share network
- snapshot
- volume type
- host
- limit and offset a la pagination
- project id (useful with '--all-tenants')
- metadata*
- extra-specs*
- direction (asc, desc)*
- key of share*

* requires server-side update

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

3) Updated utils.print_list func to print results as is without reordering.

Implements  blueprint improve-share-list-filtering

Change-Id: I6fe92c5242f18c900e109271f9182bd5ed287b25

manilaclient/common/constants.py

@@ -0,0 +1,29 @@
+# Copyright 2014 Mirantis, Inc.
+# All Rights Reserved.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""Common constants that can be used all over the manilaclient."""
+
+# These are used for providing desired sorting params with list requests
+SORT_DIR_VALUES = ('asc', 'desc')
+SHARE_SORT_KEY_VALUES = (
+    'id', 'status', 'size', 'host', 'share_proto',
+    'export_location', 'availability_zone',
+    'user_id', 'project_id',
+    'created_at', 'updated_at',
+    'display_name', 'name',
+    'volume_type_id', 'volume_type',
+    'share_network_id', 'share_network',
+    'snapshot_id', 'snapshot',
+)

manilaclient/utils.py

@@ -20,15 +20,14 @@ import six
 from manilaclient.openstack.common import strutils
 
 
-def _print(pt, order):
+def _print(pt, order=None):
+    output = pt.get_string(sortby=order) if order else pt.get_string()
     if sys.version_info >= (3, 0):
-        print(pt.get_string(sortby=order))
+        print(output)
     else:
-        print(strutils.safe_encode(pt.get_string(sortby=order)))
+        print(strutils.safe_encode(output))
 
 
-# NOTE(vponomaryov): replace function 'print_list' and 'print_dict'
-#                    with functions from cliutils, when bug #1342050 is fixed
 def print_list(objs, fields, formatters={}, order_by=None):
     mixed_case_fields = ['serverId']
     pt = prettytable.PrettyTable([f for f in fields], caching=False)
@@ -47,10 +46,6 @@ def print_list(objs, fields, formatters={}, order_by=None):
                 data = getattr(o, field_name, '')
                 row.append(data)
         pt.add_row(row)
-
-    if order_by is None:
-        order_by = fields[0]
-
     _print(pt, order_by)
 
 

manilaclient/v1/shares.py

@@ -22,6 +22,7 @@ except ImportError:
     from urllib.parse import urlencode  # noqa
 
 from manilaclient import base
+from manilaclient.common import constants
 from manilaclient import exceptions
 from manilaclient.openstack.common.apiclient import base as common_base
 
@@ -175,11 +176,40 @@ class ShareManager(base.ManagerWithFind):
         body = {'share': kwargs, }
         return self._update("/shares/%s" % share.id, body)
 
-    def list(self, detailed=True, search_opts=None):
+    def list(self, detailed=True, search_opts=None,
+             sort_key=None, sort_dir=None):
         """Get a list of all shares.
 
+        :param detailed: Whether to return detailed share info.
+        :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:`Share`
         """
+        if search_opts is None:
+            search_opts = {}
+
+        if sort_key is not None:
+            if sort_key in constants.SHARE_SORT_KEY_VALUES:
+                search_opts['sort_key'] = sort_key
+                # NOTE(vponomaryov): Replace aliases with appropriate keys
+                if sort_key == 'volume_type':
+                    search_opts['sort_key'] = 'volume_type_id'
+                elif sort_key == 'snapshot':
+                    search_opts['sort_key'] = 'snapshot_id'
+                elif sort_key == 'share_network':
+                    search_opts['sort_key'] = 'share_network_id'
+            else:
+                raise ValueError('sort_key must be one of the following: %s.'
+                                 % ', '.join(constants.SHARE_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

@@ -19,6 +19,7 @@ import os
 import sys
 import time
 
+from manilaclient.common import constants
 from manilaclient import exceptions
 from manilaclient.openstack.common import cliutils
 from manilaclient import utils
@@ -96,6 +97,7 @@ def _translate_keys(collection, convert):
 
 def _extract_metadata(args):
     metadata = {}
+    if args.metadata:
         for metadatum in args.metadata:
             # unset doesn't require a val, so we have the if/else
             if '=' in metadatum:
@@ -108,6 +110,21 @@ def _extract_metadata(args):
     return metadata
 
 
+def _extract_extra_specs(args):
+    extra_specs = {}
+    if args.extra_specs:
+        for extra_spec in args.extra_specs:
+            # unset doesn't require a val, so we have the if/else
+            if '=' in extra_spec:
+                (key, value) = extra_spec.split('=', 1)
+            else:
+                key = extra_spec
+                value = None
+
+            extra_specs[key] = value
+    return extra_specs
+
+
 def do_endpoints(cs, args):
     """Discover endpoints that get returned from the authenticate services."""
     catalog = cs.client.service_catalog.catalog
@@ -545,32 +562,147 @@ def do_access_list(cs, args):
 @cliutils.arg(
     '--name',
     metavar='<name>',
+    type=str,
     default=None,
     help='Filter results by name.')
 @cliutils.arg(
     '--status',
     metavar='<status>',
+    type=str,
     default=None,
     help='Filter results by status.')
 @cliutils.arg(
     '--share-server-id',
+    '--share-server_id', '--share_server-id', '--share_server_id',  # aliases
     metavar='<share_server_id>',
+    type=str,
     default=None,
+    action='single_alias',
     help='Filter results by share server ID.')
+@cliutils.arg(
+    '--metadata',
+    type=str,
+    nargs='*',
+    metavar='<key=value>',
+    help='Filters results by a metadata key and value. OPTIONAL: Default=None',
+    default=None)
+@cliutils.arg(
+    '--extra-specs',
+    '--extra_specs',  # alias
+    type=str,
+    nargs='*',
+    metavar='<key=value>',
+    action='single_alias',
+    help='Filters results by a extra specs key and value of volume type that '
+         'was used for share creation. OPTIONAL: Default=None',
+    default=None)
+@cliutils.arg(
+    '--volume-type',
+    '--volume_type', '--volume-type-id',  # aliases
+    '--volume-type_id', '--volume_type-id', '--volume_type_id',  # aliases
+    metavar='<volume_type>',
+    type=str,
+    default=None,
+    action='single_alias',
+    help='Filter results by a volume type id or name that was used for share '
+         'creation.')
+@cliutils.arg(
+    '--limit',
+    metavar='<limit>',
+    type=int,
+    default=None,
+    help='Maximum number of shares to return. OPTIONAL: Default=None.')
+@cliutils.arg(
+    '--offset',
+    metavar='<offset>',
+    type=int,
+    default=None,
+    help='Set offset to define start point of share 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.SHARE_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.arg(
+    '--snapshot',
+    metavar='<snapshot>',
+    type=str,
+    default=None,
+    help='Filer results by snapshot name or id, that was used for share.')
+@cliutils.arg(
+    '--host',
+    metavar='<host>',
+    default=None,
+    help='Filter results by host.')
+@cliutils.arg(
+    '--share-network',
+    '--share_network',  # alias
+    metavar='<share_network>',
+    type=str,
+    default=None,
+    action='single_alias',
+    help='Filter results by share-network name or id.')
+@cliutils.arg(
+    '--project-id',
+    '--project_id',  # alias
+    metavar='<project_id>',
+    type=str,
+    default=None,
+    action='single_alias',
+    help="Filter results by project id. Useful with set key '--all-tenants'.")
 @cliutils.service_type('share')
 def do_list(cs, args):
-    """List all NAS shares."""
+    """List NAS shares with filters."""
+    list_of_keys = [
+        'ID', 'Name', 'Size', 'Share Proto', 'Status', 'Volume Type',
+        'Export location', 'Host',
+    ]
     all_tenants = int(os.environ.get("ALL_TENANTS", args.all_tenants))
+
+    empty_obj = type('Empty', (object,), {'id': None})
+    volume_type = (_find_volume_type(cs, args.volume_type)
+                   if args.volume_type else empty_obj)
+
+    snapshot = (_find_share_snapshot(cs, args.snapshot)
+                if args.snapshot else empty_obj)
+
+    share_network = (_find_share_network(cs, args.share_network)
+                     if args.share_network else empty_obj)
     search_opts = {
+        'offset': args.offset,
+        'limit': args.limit,
         'all_tenants': all_tenants,
         'name': args.name,
         'status': args.status,
+        'host': args.host,
+        'share_network_id': share_network.id,
+        'snapshot_id': snapshot.id,
+        'volume_type_id': volume_type.id,
+        'metadata': _extract_metadata(args),
+        'extra_specs': _extract_extra_specs(args),
         'share_server_id': args.share_server_id,
+        'project_id': args.project_id,
     }
-    shares = cs.shares.list(search_opts=search_opts)
+    shares = cs.shares.list(
-    utils.print_list(shares,
+        search_opts=search_opts,
-                     ['ID', 'Name', 'Size', 'Share Proto', 'Status',
+        sort_key=args.sort_key,
-                      'Export location'])
+        sort_dir=args.sort_dir,
+    )
+    utils.print_list(shares, list_of_keys)
 
 
 @cliutils.arg(

tests/test_shell.py

@@ -73,7 +73,7 @@ class ShellTest(utils.TestCase):
     def test_help_on_subcommand(self):
         required = [
             '.*?^usage: manila list',
-            '.*?(?m)^List all NAS shares.',
+            '.*?(?m)^List NAS shares with filters.',
         ]
         help_text = self.shell('help list')
         for r in required:

tests/v1/fakes.py

@@ -34,10 +34,43 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
         share = {'share': {'id': 1234, 'name': 'sharename'}}
         return (200, {}, share)
 
-    def get_shares_detail(self, **kw):
+    def get_shares(self, **kw):
-        shares = {'shares': [{'id': 1234,
+        endpoint = "http://127.0.0.1:8786/v1"
+        share_id = '1234'
+        shares = {
+            'shares': [
+                {
+                    'id': share_id,
                     'name': 'sharename',
-                              'attachments': [{'server_id': 111}]}]}
+                    'links': [
+                        {"href": endpoint + "/fake_project/shares/" + share_id,
+                         "rel": "self"},
+                    ],
+                },
+            ]
+        }
+        return (200, {}, shares)
+
+    def get_shares_detail(self, **kw):
+        endpoint = "http://127.0.0.1:8786/v1"
+        share_id = '1234'
+        shares = {
+            'shares': [
+                {
+                    'id': share_id,
+                    'name': 'sharename',
+                    'status': 'fake_status',
+                    'size': 1,
+                    'host': 'fake_host',
+                    'export_location': 'fake_export_location',
+                    'snapshot_id': 'fake_snapshot_id',
+                    'links': [
+                        {"href": endpoint + "/fake_project/shares/" + share_id,
+                         "rel": "self"},
+                    ],
+                },
+            ]
+        }
         return (200, {}, shares)
 
     def get_snapshots_1234(self, **kw):

tests/v1/test_shares.py

@@ -90,10 +90,57 @@ class SharesTest(utils.TestCase):
         cs.assert_called('POST', '/shares/1234/action',
                          {'os-force_delete': None})
 
-    def test_list_shares(self):
+    def test_list_shares_index(self):
-        cs.shares.list()
+        cs.shares.list(detailed=False)
+        cs.assert_called('GET', '/shares')
+
+    def test_list_shares_index_with_search_opts(self):
+        search_opts = {
+            'fake_str': 'fake_str_value',
+            'fake_int': 1,
+        }
+        cs.shares.list(detailed=False, search_opts=search_opts)
+        cs.assert_called('GET', '/shares?fake_int=1&fake_str=fake_str_value')
+
+    def test_list_shares_detailed(self):
+        cs.shares.list(detailed=True)
         cs.assert_called('GET', '/shares/detail')
 
+    def test_list_shares_detailed_with_search_opts(self):
+        search_opts = {
+            'fake_str': 'fake_str_value',
+            'fake_int': 1,
+        }
+        cs.shares.list(detailed=True, search_opts=search_opts)
+        cs.assert_called(
+            'GET', '/shares/detail?fake_int=1&fake_str=fake_str_value')
+
+    def test_list_shares_sort_by_asc_and_host_key(self):
+        cs.shares.list(detailed=False, sort_key='host', sort_dir='asc')
+        cs.assert_called('GET', '/shares?sort_dir=asc&sort_key=host')
+
+    def test_list_shares_sort_by_desc_and_size_key(self):
+        cs.shares.list(detailed=False, sort_key='size', sort_dir='desc')
+        cs.assert_called('GET', '/shares?sort_dir=desc&sort_key=size')
+
+    def test_list_shares_filter_by_share_network_alias(self):
+        cs.shares.list(detailed=False, sort_key='share_network')
+        cs.assert_called('GET', '/shares?sort_key=share_network_id')
+
+    def test_list_shares_filter_by_snapshot_alias(self):
+        cs.shares.list(detailed=False, sort_key='snapshot')
+        cs.assert_called('GET', '/shares?sort_key=snapshot_id')
+
+    def test_list_shares_filter_by_volume_type_alias(self):
+        cs.shares.list(detailed=False, sort_key='volume_type')
+        cs.assert_called('GET', '/shares?sort_key=volume_type_id')
+
+    def test_list_shares_by_improper_direction(self):
+        self.assertRaises(ValueError, cs.shares.list, sort_dir='fake')
+
+    def test_list_shares_by_improper_key(self):
+        self.assertRaises(ValueError, cs.shares.list, sort_key='fake')
+
     def test_allow_access_to_share(self):
         share = cs.shares.get(1234)
         ip = '192.168.0.1'

tests/v1/test_shell.py

@@ -1,4 +1,5 @@
 # Copyright 2013 OpenStack LLC.
+# Copyright 2014 Mirantis, Inc.
 # All Rights Reserved.
 #
 #    Licensed under the Apache License, Version 2.0 (the "License"); you may
@@ -18,7 +19,9 @@ import mock
 import requests
 
 from manilaclient import client
+from manilaclient.common import constants
 from manilaclient import exceptions
+from manilaclient.openstack.common import cliutils
 from manilaclient.openstack.common import jsonutils
 from manilaclient import shell
 from manilaclient.v1 import client as client_v1
@@ -51,6 +54,10 @@ class ShellTest(utils.TestCase):
         self.old_get_client_class = client.get_client_class
         client.get_client_class = lambda *_: fakes.FakeClient
 
+        # Following shows available separators for optional params
+        # and its values
+        self.separators = [' ', '=']
+
     def tearDown(self):
         # For some method like test_image_meta_bad_action we are
         # testing a SystemExit to be thrown and object self.shell has
@@ -79,20 +86,169 @@ class ShellTest(utils.TestCase):
         self.assert_called('GET', '/shares/detail')
 
     def test_list_filter_status(self):
-        self.run_command('list --status=available')
+        for separator in self.separators:
+            self.run_command('list --status' + separator + 'available')
             self.assert_called('GET', '/shares/detail?status=available')
 
     def test_list_filter_name(self):
-        self.run_command('list --name=1234')
+        for separator in self.separators:
+            self.run_command('list --name' + separator + '1234')
             self.assert_called('GET', '/shares/detail?name=1234')
 
-    def test_list_all_tenants(self):
+    def test_list_all_tenants_only_key(self):
-        self.run_command('list --all-tenants=1')
+        self.run_command('list --all-tenants')
         self.assert_called('GET', '/shares/detail?all_tenants=1')
 
-    def test_list_filter_share_server_id(self):
+    def test_list_all_tenants_key_and_value_1(self):
-        self.run_command('list --share-server-id=1234')
+        for separator in self.separators:
-        self.assert_called('GET', '/shares/detail?share_server_id=1234')
+            self.run_command('list --all-tenants' + separator + '1')
+            self.assert_called('GET', '/shares/detail?all_tenants=1')
+
+    def test_list_all_tenants_key_and_value_0(self):
+        for separator in self.separators:
+            self.run_command('list --all-tenants' + separator + '0')
+            self.assert_called('GET', '/shares/detail')
+
+    def test_list_filter_by_share_server_and_its_aliases(self):
+        aliases = [
+            '--share-server-id', '--share-server_id',
+            '--share_server-id', '--share_server_id',
+        ]
+        for alias in aliases:
+            for separator in self.separators:
+                self.run_command('list ' + alias + separator + '1234')
+                self.assert_called(
+                    'GET', '/shares/detail?share_server_id=1234')
+
+    def test_list_filter_by_metadata(self):
+        self.run_command('list --metadata key=value')
+        self.assert_called(
+            'GET', '/shares/detail?metadata=%7B%27key%27%3A+%27value%27%7D')
+
+    def test_list_filter_by_extra_specs_and_its_aliases(self):
+        aliases = ['--extra-specs', '--extra_specs', ]
+        for alias in aliases:
+            self.run_command('list ' + alias + ' key=value')
+            self.assert_called(
+                'GET',
+                '/shares/detail?extra_specs=%7B%27key%27%3A+%27value%27%7D',
+            )
+
+    def test_list_filter_by_volume_type_and_its_aliases(self):
+        fake_vt = type('Empty', (object,), {'id': 'fake_vt'})
+        aliases = [
+            '--volume-type', '--volume_type', '--volume-type-id',
+            '--volume-type_id', '--volume_type-id', '--volume_type_id',
+        ]
+        for alias in aliases:
+            for separator in self.separators:
+                with mock.patch.object(cliutils, 'find_resource',
+                                       mock.Mock(return_value=fake_vt)):
+                    self.run_command('list ' + alias + separator + fake_vt.id)
+                    self.assert_called(
+                        'GET', '/shares/detail?volume_type_id=' + fake_vt.id)
+
+    def test_list_filter_by_volume_type_not_found(self):
+        for separator in self.separators:
+            self.assertRaises(
+                exceptions.CommandError,
+                self.run_command,
+                'list --volume-type' + separator + 'not_found_expected',
+            )
+            self.assert_called('GET', '/types')
+
+    def test_list_with_limit(self):
+        for separator in self.separators:
+            self.run_command('list --limit' + separator + '50')
+            self.assert_called('GET', '/shares/detail?limit=50')
+
+    def test_list_with_offset(self):
+        for separator in self.separators:
+            self.run_command('list --offset' + separator + '50')
+            self.assert_called('GET', '/shares/detail?offset=50')
+
+    def test_list_with_sort_dir_verify_keys(self):
+        # Verify allowed aliases and keys
+        aliases = ['--sort_dir', '--sort-dir']
+        for alias in aliases:
+            for key in constants.SORT_DIR_VALUES:
+                for separator in self.separators:
+                    self.run_command('list ' + alias + separator + key)
+                    self.assert_called('GET', '/shares/detail?sort_dir=' + key)
+
+    def test_list_with_fake_sort_dir(self):
+        self.assertRaises(
+            ValueError,
+            self.run_command,
+            'list --sort-dir fake_sort_dir',
+        )
+
+    def test_list_with_sort_key_verify_keys(self):
+        # Verify allowed aliases and keys
+        aliases = ['--sort_key', '--sort-key']
+        for alias in aliases:
+            for key in constants.SHARE_SORT_KEY_VALUES:
+                for separator in self.separators:
+                    self.run_command('list ' + alias + separator + key)
+                    key = 'share_network_id' if key == 'share_network' else key
+                    key = 'snapshot_id' if key == 'snapshot' else key
+                    key = 'volume_type_id' if key == 'volume_type' else key
+                    self.assert_called('GET', '/shares/detail?sort_key=' + key)
+
+    def test_list_with_fake_sort_key(self):
+        self.assertRaises(
+            ValueError,
+            self.run_command,
+            'list --sort-key fake_sort_key',
+        )
+
+    def test_list_filter_by_snapshot(self):
+        fake_s = type('Empty', (object,), {'id': 'fake_snapshot_id'})
+        for separator in self.separators:
+            with mock.patch.object(cliutils, 'find_resource',
+                                   mock.Mock(return_value=fake_s)):
+                self.run_command('list --snapshot' + separator + fake_s.id)
+                self.assert_called(
+                    'GET', '/shares/detail?snapshot_id=' + fake_s.id)
+
+    def test_list_filter_by_snapshot_not_found(self):
+        self.assertRaises(
+            exceptions.CommandError,
+            self.run_command,
+            'list --snapshot not_found_expected',
+        )
+        self.assert_called('GET', '/snapshots/detail')
+
+    def test_list_filter_by_host(self):
+        for separator in self.separators:
+            self.run_command('list --host' + separator + 'fake_host')
+            self.assert_called('GET', '/shares/detail?host=fake_host')
+
+    def test_list_filter_by_share_network(self):
+        aliases = ['--share-network', '--share_network', ]
+        fake_sn = type('Empty', (object,), {'id': 'fake_share_network_id'})
+        for alias in aliases:
+            for separator in self.separators:
+                with mock.patch.object(cliutils, 'find_resource',
+                                       mock.Mock(return_value=fake_sn)):
+                    self.run_command('list ' + alias + separator + fake_sn.id)
+                    self.assert_called(
+                        'GET', '/shares/detail?share_network_id=' + fake_sn.id)
+
+    def test_list_filter_by_share_network_not_found(self):
+        self.assertRaises(
+            exceptions.CommandError,
+            self.run_command,
+            'list --share-network not_found_expected',
+        )
+        self.assert_called('GET', '/share-networks/detail')
+
+    def test_list_filter_by_project_id(self):
+        aliases = ['--project-id', '--project_id']
+        for alias in aliases:
+            for separator in self.separators:
+                self.run_command('list ' + alias + separator + 'fake_id')
+                self.assert_called('GET', '/shares/detail?project_id=fake_id')
 
     def test_show(self):
         self.run_command('show 1234')