From d24527a7f476a439a643dc972ff94a59bc11e395 Mon Sep 17 00:00:00 2001 From: tspyderboy Date: Wed, 27 Mar 2024 18:02:49 +0530 Subject: [PATCH] Handle empty metadata properly in filter/list Introduced "None" tag for metadata command. "list --metadata None" command will filter out shares that has empty metadata, i.e., metadata={} Closes-Bug: #1782847 Change-Id: Iac33cd1295b1e7c2ff11e6d7bd003fde28785c0e --- manilaclient/tests/unit/v2/fakes.py | 54 +++++++++++++++-- manilaclient/tests/unit/v2/test_shares.py | 2 +- manilaclient/tests/unit/v2/test_shell.py | 59 ++++++++++++++++++- manilaclient/v2/shell.py | 18 ++++-- ...y-metadata-key-value-cc6f858994f6e9ee.yaml | 6 ++ 5 files changed, 127 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bug-1782847-filter-list-resources-with-empty-metadata-key-value-cc6f858994f6e9ee.yaml diff --git a/manilaclient/tests/unit/v2/fakes.py b/manilaclient/tests/unit/v2/fakes.py index d835a9c45..cf845291d 100644 --- a/manilaclient/tests/unit/v2/fakes.py +++ b/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'} }, ], } diff --git a/manilaclient/tests/unit/v2/test_shares.py b/manilaclient/tests/unit/v2/test_shares.py index ecbab2388..e5ce890c0 100644 --- a/manilaclient/tests/unit/v2/test_shares.py +++ b/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) diff --git a/manilaclient/tests/unit/v2/test_shell.py b/manilaclient/tests/unit/v2/test_shell.py index 8134c082b..99069c446 100644 --- a/manilaclient/tests/unit/v2/test_shell.py +++ b/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', ] diff --git a/manilaclient/v2/shell.py b/manilaclient/v2/shell.py index 15df00ddd..ab95fbf4f 100644 --- a/manilaclient/v2/shell.py +++ b/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: @@ -2502,7 +2504,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, @@ -2558,6 +2560,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. diff --git a/releasenotes/notes/bug-1782847-filter-list-resources-with-empty-metadata-key-value-cc6f858994f6e9ee.yaml b/releasenotes/notes/bug-1782847-filter-list-resources-with-empty-metadata-key-value-cc6f858994f6e9ee.yaml new file mode 100644 index 000000000..161f2d28a --- /dev/null +++ b/releasenotes/notes/bug-1782847-filter-list-resources-with-empty-metadata-key-value-cc6f858994f6e9ee.yaml @@ -0,0 +1,6 @@ +--- +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={}