Handle empty metadata properly in filter/list

Closes-Bug: #1782847
Change-Id: Iac33cd1295b1e7c2ff11e6d7bd003fde28785c0e

manilaclient/tests/unit/v2/fakes.py

@@ -224,12 +224,15 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
 
     def get_shares_detail(self, **kw):
         endpoint = "http://127.0.0.1:8786/v2"
-        share_id = '1234'
+        share_id1234 = '1234'
+        share_id1111 = '1111'
+        share_id2222 = '2222'
+        share_id3333 = '3333'
 
         shares = {
             'shares': [
                 {
-                    'id': share_id,
+                    'id': share_id1234,
                     'name': 'sharename',
                     'status': 'fake_status',
                     'size': 1,
@@ -237,9 +240,52 @@ class FakeHTTPClient(fakes.FakeHTTPClient):
                     'export_location': 'fake_export_location',
                     'snapshot_id': 'fake_snapshot_id',
                     'links': [
-                        {"href": endpoint + "/fake_project/shares/" + share_id,
-                         "rel": "self"},
+                        {"href": endpoint + "/fake_project/shares/"
+                         + share_id1234, "rel": "self"},
                     ],
+                    'metadata': {}
+                },
+                {
+                    'id': share_id1111,
+                    'name': 'sharename1',
+                    'status': 'fake_status1',
+                    'size': 1,
+                    'host': 'fake_host',
+                    'export_location': 'fake_export_location',
+                    'snapshot_id': 'fake_snapshot_id',
+                    'links': [
+                        {"href": endpoint + "/fake_project/shares/"
+                         + share_id1111, "rel": "self"},
+                    ],
+                    'metadata': {}
+                },
+                {
+                    'id': share_id2222,
+                    'name': 'sharename2',
+                    'status': 'fake_status2',
+                    'size': 1,
+                    'host': 'fake_host',
+                    'export_location': 'fake_export_location',
+                    'snapshot_id': 'fake_snapshot_id',
+                    'links': [
+                        {"href": endpoint + "/fake_project/shares/"
+                         + share_id2222, "rel": "self"},
+                    ],
+                    'metadata': {'key1': 'value1'}
+                },
+                {
+                    'id': share_id3333,
+                    'name': 'sharename3',
+                    'status': 'fake_status3',
+                    'size': 1,
+                    'host': 'fake_host',
+                    'export_location': 'fake_export_location',
+                    'snapshot_id': 'fake_snapshot_id',
+                    'links': [
+                        {"href": endpoint + "/fake_project/shares/"
+                         + share_id3333, "rel": "self"},
+                    ],
+                    'metadata': {'key1': 'value1', 'key2': 'value2'}
                 },
             ],
         }

manilaclient/tests/unit/v2/test_shares.py

@@ -421,7 +421,7 @@ class SharesTest(utils.TestCase):
         cs.assert_called(
             'GET', '/shares/detail?is_public=True&with_count=True')
         self.assertEqual(2, count)
-        self.assertEqual(1, len(shares))
+        self.assertEqual(4, len(shares))
 
     def test_list_shares_detailed_with_count(self):
         cs.shares.list(detailed=True)

manilaclient/tests/unit/v2/test_shell.py

@@ -231,8 +231,65 @@ class ShellTest(test_utils.TestCase):
 
     def test_list_filter_by_metadata(self):
         self.run_command('list --metadata key=value')
+        # /shares/detail?metadata={'key': 'value'}
         self.assert_called(
-            'GET', '/shares/detail?metadata=%7B%27key%27%3A+%27value%27%7D')
+            'GET', '/shares/detail?metadata='
+                   '%7B%27key%27%3A+%27value%27%7D')
+
+    def test_list_filter_by_metadata_with_multiple_key_values(self):
+        self.run_command('list --metadata key1=value1 '
+                         'key2=value2 key3=value3')
+        # /shares/detail?metadata={'key1': 'value1',
+        # 'key2': 'value2', 'key3': 'value3'}
+        self.assert_called(
+            'GET', '/shares/detail?metadata='
+                   '%7B%27key1%27%3A+%27value1%27%2C+%27key2%27%3A+'
+                   '%27value2%27%2C+%27key3%27%3A+%27value3%27%7D')
+
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
+    def test_list_filter_by_metadata_with_empty_metadata(self):
+        self.run_command('list --metadata \'\'')
+
+        self.assert_called(
+            'GET', '/shares/detail')
+        cliutils.print_list.assert_called()
+        args, _ = cliutils.print_list.call_args
+        shares = args[0]
+        # All 4 shares irrespective of metadata values printed
+        self.assertEqual(len(shares), 4)
+
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
+    def test_list_filter_by_metadata_set_to_None(self):
+        self.run_command('list --metadata None')
+
+        self.assert_called(
+            'GET', '/shares/detail')
+        cliutils.print_list.assert_called()
+        args, _ = cliutils.print_list.call_args
+        shares = args[0]
+        # Check that the size of shares is 2(shares with metadata={})
+        self.assertEqual(len(shares), 2)
+        for share in shares:
+            self.assertEqual(share.metadata, {})
+
+    def test_list_filter_by_metadata_with_one_empty_of_many_metadata(self):
+        self.run_command('list --metadata key=value \'\'')
+        # /shares/detail?metadata={'key': 'value'}
+        self.assert_called(
+            'GET', '/shares/detail?metadata='
+                   '%7B%27key%27%3A+%27value%27%7D')
+
+    @mock.patch.object(cliutils, 'print_list', mock.Mock())
+    def test_list_filter_by_metadata_with_no_metadata(self):
+        self.run_command('list --metadata')
+        # /shares/detail
+        self.assert_called(
+            'GET', '/shares/detail')
+        cliutils.print_list.assert_called()
+        args, _ = cliutils.print_list.call_args
+        shares = args[0]
+        # All 4 shares printed
+        self.assertEqual(len(shares), 4)
 
     def test_list_filter_by_extra_specs_and_its_aliases(self):
         aliases = ['--extra-specs', '--extra_specs', ]

manilaclient/v2/shell.py

@@ -396,8 +396,8 @@ def _translate_keys(collection, convert):
                 setattr(item, to_key, item._info[from_key])
 
 
-def _extract_metadata(args):
-    return _extract_key_value_options(args, 'metadata')
+def _extract_metadata(args, allow_empty_key=True):
+    return _extract_key_value_options(args, 'metadata', allow_empty_key)
 
 
 def _extract_extra_specs(args):
@@ -408,7 +408,7 @@ def _extract_group_specs(args):
     return _extract_key_value_options(args, 'group_specs')
 
 
-def _extract_key_value_options(args, option_name):
+def _extract_key_value_options(args, option_name, allow_empty_key=True):
     result_dict = {}
     duplicate_options = []
 
@@ -419,10 +419,12 @@ def _extract_key_value_options(args, option_name):
             # unset doesn't require a val, so we have the if/else
             if '=' in option:
                 (key, value) = option.split('=', 1)
-            else:
+            elif allow_empty_key:
                 key = option
                 value = None
-
+            else:
+                # disallow empty key
+                continue
             if key not in result_dict:
                 result_dict[key] = value
             else:
@@ -2495,7 +2497,7 @@ def do_list(cs, args):
         'share_network_id': share_network.id,
         'snapshot_id': snapshot.id,
         'share_type_id': share_type.id,
-        'metadata': _extract_metadata(args),
+        'metadata': _extract_metadata(args, allow_empty_key=False),
         'extra_specs': _extract_extra_specs(args),
         'share_server_id': args.share_server_id,
         'project_id': args.project_id,
@@ -2551,6 +2553,10 @@ def do_list(cs, args):
             search_opts=search_opts, sort_key=args.sort_key,
             sort_dir=args.sort_dir
         )
+    # When shell input is "--metadata None", filter metadata={}
+    if shares and args.metadata:
+        if "None" in args.metadata:
+            shares = [share for share in shares if share.metadata == {}]
     # NOTE(vponomaryov): usage of 'export_location' and
     # 'export_locations' columns may cause scaling issue using API 2.9+ and
     # when lots of shares are returned.

releasenotes/notes/bug-1782847-filter-list-resources-with-empty-metadata-key-value-cc6f858994f6e9ee.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    The --metadata command should handle empty metadata in proper way.
+    Introduced "None" metadata tag to filter list which have empty.
+    Example: "list --metadata None" will return only elements with metadata={}
+