From f906bcdd4eac47b1abeb9e6d9b23b6aeb03aef38 Mon Sep 17 00:00:00 2001 From: zhongjun Date: Tue, 18 Jul 2017 17:36:14 +0800 Subject: [PATCH] Fix the exact filter can be filter by inexact value Fix the ``exact`` filters (name, description) in ``shares``, ``snapshots``, ``share-networks`` list can be filter by ``inexact`` value. Change-Id: I51e6b754f37a09c09a60e9a7fb51d3c9721f2d1f Closes-bug: #1704971 --- manila/api/v1/share_snapshots.py | 8 ++- manila/api/v1/shares.py | 7 ++- manila/api/v2/share_networks.py | 3 +- manila/api/v2/share_snapshots.py | 2 + manila/api/v2/shares.py | 2 + manila/share/api.py | 7 ++- manila/tests/api/v2/test_share_networks.py | 33 +++++++---- manila/tests/db/sqlalchemy/test_api.py | 29 +++++++--- manila/tests/share/test_api.py | 42 +++++++++++--- .../tests/api/test_shares_actions.py | 26 ++++++++- .../tests/api/test_shares_actions_negative.py | 58 ++++++++++++++++++- ...e-description-filter-85935582e89a72a0.yaml | 12 ++++ 12 files changed, 192 insertions(+), 37 deletions(-) create mode 100644 releasenotes/notes/bug-1704971-fix-name-description-filter-85935582e89a72a0.yaml diff --git a/manila/api/v1/share_snapshots.py b/manila/api/v1/share_snapshots.py index 38a3400b26..24e24c8095 100644 --- a/manila/api/v1/share_snapshots.py +++ b/manila/api/v1/share_snapshots.py @@ -74,12 +74,14 @@ class ShareSnapshotMixin(object): """Returns a summary list of snapshots.""" req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_snapshots(req, is_detail=False) def detail(self, req): """Returns a detailed list of snapshots.""" req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_snapshots(req, is_detail=True) def _get_snapshots(self, req, is_detail): @@ -100,6 +102,9 @@ class ShareSnapshotMixin(object): # from Cinder v1 and v2 APIs. if 'name' in search_opts: search_opts['display_name'] = search_opts.pop('name') + if 'description' in search_opts: + search_opts['display_description'] = search_opts.pop( + 'description') # like filter for key, db_key in (('name~', 'display_name~'), @@ -131,7 +136,8 @@ class ShareSnapshotMixin(object): def _get_snapshots_search_options(self): """Return share snapshot search options allowed by non-admin.""" return ('display_name', 'name', 'status', 'share_id', 'size', - 'display_name~', 'display_description~') + 'display_name~', 'display_description~', 'description', + 'display_description') def update(self, req, id, body): """Update a snapshot.""" diff --git a/manila/api/v1/shares.py b/manila/api/v1/shares.py index b2dadad391..f1fb3995d2 100644 --- a/manila/api/v1/shares.py +++ b/manila/api/v1/shares.py @@ -104,6 +104,7 @@ class ShareMixin(object): req.GET.pop('export_location_path', None) req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_shares(req, is_detail=False) def detail(self, req): @@ -112,6 +113,7 @@ class ShareMixin(object): req.GET.pop('export_location_path', None) req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_shares(req, is_detail=True) def _get_shares(self, req, is_detail): @@ -139,6 +141,9 @@ class ShareMixin(object): # from Cinder v1 and v2 APIs. if 'name' in search_opts: search_opts['display_name'] = search_opts.pop('name') + if 'description' in search_opts: + search_opts['display_description'] = search_opts.pop( + 'description') # like filter for key, db_key in (('name~', 'display_name~'), @@ -176,7 +181,7 @@ class ShareMixin(object): 'is_public', 'metadata', 'extra_specs', 'sort_key', 'sort_dir', 'share_group_id', 'share_group_snapshot_id', 'export_location_id', 'export_location_path', 'display_name~', - 'display_description~' + 'display_description~', 'description', 'display_description' ) def update(self, req, id, body): diff --git a/manila/api/v2/share_networks.py b/manila/api/v2/share_networks.py index 73ba11b6e8..5089db59cf 100644 --- a/manila/api/v2/share_networks.py +++ b/manila/api/v2/share_networks.py @@ -172,7 +172,8 @@ class ShareNetworkController(wsgi.Controller): networks = [network for network in networks if network.get(key) == value or (value in network.get(key.rstrip('~')) - if network.get(key.rstrip('~')) else ())] + if key.endswith('~') and + network.get(key.rstrip('~')) else ())] else: networks = [network for network in networks if network.get(key) == value] diff --git a/manila/api/v2/share_snapshots.py b/manila/api/v2/share_snapshots.py index d36d91b3b3..a7965f67f1 100644 --- a/manila/api/v2/share_snapshots.py +++ b/manila/api/v2/share_snapshots.py @@ -289,6 +289,7 @@ class ShareSnapshotsController(share_snapshots.ShareSnapshotMixin, if req.api_version_request < api_version.APIVersionRequest("2.36"): req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_snapshots(req, is_detail=False) @wsgi.Controller.api_version("2.0") @@ -297,6 +298,7 @@ class ShareSnapshotsController(share_snapshots.ShareSnapshotMixin, if req.api_version_request < api_version.APIVersionRequest("2.36"): req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_snapshots(req, is_detail=True) diff --git a/manila/api/v2/shares.py b/manila/api/v2/shares.py index 572aee5e90..70f3884dd4 100644 --- a/manila/api/v2/shares.py +++ b/manila/api/v2/shares.py @@ -425,6 +425,7 @@ class ShareController(shares.ShareMixin, if req.api_version_request < api_version.APIVersionRequest("2.36"): req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_shares(req, is_detail=False) @@ -438,6 +439,7 @@ class ShareController(shares.ShareMixin, if req.api_version_request < api_version.APIVersionRequest("2.36"): req.GET.pop('name~', None) req.GET.pop('description~', None) + req.GET.pop('description', None) return self._get_shares(req, is_detail=True) diff --git a/manila/share/api.py b/manila/share/api.py index 40a3a8b651..8a61d3531f 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1548,7 +1548,7 @@ class API(base.Base): for s in shares: # values in search_opts can be only strings if (all(s.get(k, None) == v or (v in (s.get(k.rstrip('~')) - if s.get(k.rstrip('~')) else ())) + if k.endswith('~') and s.get(k.rstrip('~')) else ())) for k, v in search_opts.items())): results.append(s) shares = results @@ -1592,8 +1592,9 @@ class API(base.Base): results = [] not_found = object() for snapshot in snapshots: - if (all(snapshot.get(k, not_found) == v or (v in - snapshot.get(k.rstrip('~')) if + if (all(snapshot.get(k, not_found) == v or + (v in snapshot.get(k.rstrip('~')) + if k.endswith('~') and snapshot.get(k.rstrip('~')) else ()) for k, v in search_opts.items())): results.append(snapshot) diff --git a/manila/tests/api/v2/test_share_networks.py b/manila/tests/api/v2/test_share_networks.py index 9cd6227556..3f1659fb50 100644 --- a/manila/tests/api/v2/test_share_networks.py +++ b/manila/tests/api/v2/test_share_networks.py @@ -450,22 +450,35 @@ class ShareNetworkAPITest(test.TestCase): result[share_networks.RESOURCES_NAME][0], fake_sn_with_ss_shortened) + @ddt.data(('name=fo', 0), ('description=d', 0), + ('name=foo&description=d', 0), + ('name=foo', 1), ('description=ds', 1), + ('name~=foo&description~=ds', 2), + ('name=foo&description~=ds', 1), + ('name~=foo&description=ds', 1)) + @ddt.unpack @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) - def test_index_filter_by_like_filter(self): - db_api.share_network_get_all_by_project.return_value = [ - fake_share_network, - ] + def test_index_filter_by_name_and_description( + self, filter, share_network_number): + fake_objs = [{'name': 'fo2', 'description': 'd2', 'id': 'fake1'}, + {'name': 'foo', 'description': 'ds', 'id': 'fake2'}, + {'name': 'foo1', 'description': 'ds1', 'id': 'fake3'}] + db_api.share_network_get_all_by_project.return_value = fake_objs req = fakes.HTTPRequest.blank( - '/share_networks?name~=fake&description~=fake', + '/share_networks?' + filter, use_admin_context=True, version='2.36') result = self.controller.index(req) - db_api.share_network_get_all_by_project.assert_called_once_with( + db_api.share_network_get_all_by_project.assert_called_with( req.environ['manila.context'], self.context.project_id) - self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) - self._check_share_network_view_shortened( - result[share_networks.RESOURCES_NAME][0], - fake_share_network_shortened) + self.assertEqual(share_network_number, + len(result[share_networks.RESOURCES_NAME])) + if share_network_number > 0: + self._check_share_network_view_shortened( + result[share_networks.RESOURCES_NAME][0], fake_objs[1]) + if share_network_number > 1: + self._check_share_network_view_shortened( + result[share_networks.RESOURCES_NAME][1], fake_objs[2]) @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index 396c50b344..7502e3785c 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -813,18 +813,31 @@ class ShareGroupDatabaseAPITestCase(test.TestCase): self.assertDictMatch(dict(expected_group), dict(group)) self.assertEqual(fake_project, group['project_id']) - def test_share_group_get_all_by_like_filter(self): - expected_group = db_utils.create_share_group( - name='test1', description='test1') - db_utils.create_share_group(name='fake', description='fake') + @ddt.data(({'name': 'fo'}, 0), ({'description': 'd'}, 0), + ({'name': 'foo', 'description': 'd'}, 0), + ({'name': 'foo'}, 1), ({'description': 'ds'}, 1), + ({'name~': 'foo', 'description~': 'ds'}, 2), + ({'name': 'foo', 'description~': 'ds'}, 1), + ({'name~': 'foo', 'description': 'ds'}, 1)) + @ddt.unpack + def test_share_group_get_all_by_name_and_description( + self, search_opts, group_number): + db_utils.create_share_group(name='fo1', description='d1') + expected_group1 = db_utils.create_share_group(name='foo', + description='ds') + expected_group2 = db_utils.create_share_group(name='foo1', + description='ds2') groups = db_api.share_group_get_all( self.ctxt, detailed=True, - filters={'name~': 'test', 'description~': 'test'}) + filters=search_opts) - self.assertEqual(1, len(groups)) - group = groups[0] - self.assertDictMatch(dict(expected_group), dict(group)) + self.assertEqual(group_number, len(groups)) + if group_number == 1: + self.assertDictMatch(dict(expected_group1), dict(groups[0])) + elif group_number == 1: + self.assertDictMatch(dict(expected_group1), dict(groups[1])) + self.assertDictMatch(dict(expected_group2), dict(groups[0])) def test_share_group_update(self): fake_name = "my_fake_name" diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index e31b7950ee..cb0fdb5b6b 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -312,11 +312,19 @@ class ShareAPITestCase(test.TestCase): ) self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[1::2], shares) - def test_get_all_admin_filter_by_inexact_filter(self): + @ddt.data(({'name': 'fo'}, 0), ({'description': 'd'}, 0), + ({'name': 'foo', 'description': 'd'}, 0), + ({'name': 'foo'}, 1), ({'description': 'ds'}, 1), + ({'name~': 'foo', 'description~': 'ds'}, 2), + ({'name': 'foo', 'description~': 'ds'}, 1), + ({'name~': 'foo', 'description': 'ds'}, 1)) + @ddt.unpack + def test_get_all_admin_filter_by_name_and_description( + self, search_opts, get_share_number): ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True) self.mock_object(db_api, 'share_get_all_by_project', mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES)) - shares = self.api.get_all(ctx, {'name~': 'foo', 'description~': 'ds'}) + shares = self.api.get_all(ctx, search_opts) share_api.policy.check_policy.assert_has_calls([ mock.call(ctx, 'share', 'get_all'), ]) @@ -325,7 +333,11 @@ class ShareAPITestCase(test.TestCase): project_id='fake_pid_2', filters={}, is_public=False ) - self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[0::2], shares) + self.assertEqual(get_share_number, len(shares)) + if get_share_number == 2: + self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[0::2], shares) + elif get_share_number == 1: + self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[:1], shares) @ddt.data('id', 'path') def test_get_all_admin_filter_by_export_location(self, type): @@ -353,7 +365,7 @@ class ShareAPITestCase(test.TestCase): ]) db_api.share_get_all.assert_called_once_with( ctx, sort_dir='desc', sort_key='created_at', filters={}) - self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[::2], shares) + self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[:1], shares) def test_get_all_admin_filter_by_status(self): ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True) @@ -416,7 +428,7 @@ class ShareAPITestCase(test.TestCase): # one item expected, two filtered shares = self.api.get_all( - ctx, {'name': 'foo', 'status': constants.STATUS_AVAILABLE}) + ctx, {'name': 'foo1', 'status': constants.STATUS_AVAILABLE}) self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[2::4], shares) share_api.policy.check_policy.assert_has_calls([ mock.call(ctx, 'share', 'get_all'), @@ -1911,9 +1923,16 @@ class ShareAPITestCase(test.TestCase): ctx, 'fakepid', sort_dir='desc', sort_key='share_id', filters=search_opts) - def test_get_all_snapshots_not_admin_inexact_search_opts(self): - search_opts = {'name~': 'foo', 'description~': 'ds'} - fake_objs = [{'name': 'fo', 'description': 'd'}, + @ddt.data(({'name': 'fo'}, 0), ({'description': 'd'}, 0), + ({'name': 'foo', 'description': 'd'}, 0), + ({'name': 'foo'}, 1), ({'description': 'ds'}, 1), + ({'name~': 'foo', 'description~': 'ds'}, 2), + ({'name': 'foo', 'description~': 'ds'}, 1), + ({'name~': 'foo', 'description': 'ds'}, 1)) + @ddt.unpack + def test_get_all_snapshots_filter_by_name_and_description( + self, search_opts, get_snapshot_number): + fake_objs = [{'name': 'fo2', 'description': 'd2'}, {'name': 'foo', 'description': 'ds'}, {'name': 'foo1', 'description': 'ds1'}] ctx = context.RequestContext('fakeuid', 'fakepid', is_admin=False) @@ -1922,7 +1941,12 @@ class ShareAPITestCase(test.TestCase): result = self.api.get_all_snapshots(ctx, search_opts) - self.assertEqual(fake_objs[1:], result) + self.assertEqual(get_snapshot_number, len(result)) + if get_snapshot_number == 2: + self.assertEqual(fake_objs[1:], result) + elif get_snapshot_number == 1: + self.assertEqual(fake_objs[1:2], result) + share_api.policy.check_policy.assert_called_once_with( ctx, 'share_snapshot', 'get_all_snapshots') db_api.share_snapshot_get_all_by_project.assert_called_once_with( diff --git a/manila_tempest_tests/tests/api/test_shares_actions.py b/manila_tempest_tests/tests/api/test_shares_actions.py index e12990e3a3..cf179bf340 100644 --- a/manila_tempest_tests/tests/api/test_shares_actions.py +++ b/manila_tempest_tests/tests/api/test_shares_actions.py @@ -338,7 +338,15 @@ class SharesActionsTest(base.BaseSharesTest): @tc.attr(base.TAG_POSITIVE, base.TAG_API_WITH_BACKEND) @base.skip_if_microversion_lt("2.36") - def test_list_shares_with_detail_filter_by_nonexistent_name(self): + def test_list_shares_with_detail_filter_by_existed_description(self): + # list shares by description, at least one share is expected + params = {"description": self.share_desc} + shares = self.shares_v2_client.list_shares_with_detail(params) + self.assertEqual(self.share_name, shares[0]["name"]) + + @tc.attr(base.TAG_POSITIVE, base.TAG_API_WITH_BACKEND) + @base.skip_if_microversion_lt("2.36") + def test_list_shares_with_detail_filter_by_inexact_name(self): # list shares by name, at least one share is expected params = {"name~": 'tempest-share'} shares = self.shares_v2_client.list_shares_with_detail(params) @@ -560,6 +568,22 @@ class SharesActionsTest(base.BaseSharesTest): self.assertEqual(filters['status'], snap['status']) self.assertEqual(filters['name'], snap['name']) + @tc.attr(base.TAG_POSITIVE, base.TAG_API_WITH_BACKEND) + @testtools.skipUnless(CONF.share.run_snapshot_tests, + "Snapshot tests are disabled.") + @base.skip_if_microversion_not_supported("2.35") + def test_list_snapshots_with_detail_filter_by_description(self): + filters = {'description': self.snap_desc} + + # list snapshots + snaps = self.shares_client.list_snapshots_with_detail( + params=filters) + + # verify response + self.assertGreater(len(snaps), 0) + for snap in snaps: + self.assertEqual(filters['description'], snap['description']) + @tc.attr(base.TAG_POSITIVE, base.TAG_API_WITH_BACKEND) @testtools.skipUnless(CONF.share.run_snapshot_tests, "Snapshot tests are disabled.") diff --git a/manila_tempest_tests/tests/api/test_shares_actions_negative.py b/manila_tempest_tests/tests/api/test_shares_actions_negative.py index 1f3da5fe55..6d1fa6c8e5 100644 --- a/manila_tempest_tests/tests/api/test_shares_actions_negative.py +++ b/manila_tempest_tests/tests/api/test_shares_actions_negative.py @@ -15,6 +15,7 @@ import ddt from tempest import config +from tempest.lib.common.utils import data_utils from tempest.lib import exceptions as lib_exc import testtools from testtools import testcase as tc @@ -30,7 +31,18 @@ class SharesActionsNegativeTest(base.BaseSharesMixedTest): def resource_setup(cls): super(SharesActionsNegativeTest, cls).resource_setup() cls.admin_client = cls.admin_shares_v2_client - cls.share = cls.create_share() + cls.share_name = data_utils.rand_name("tempest-share-name") + cls.share_desc = data_utils.rand_name("tempest-share-description") + cls.share = cls.create_share( + name=cls.share_name, + description=cls.share_desc) + if CONF.share.run_snapshot_tests: + # create snapshot + cls.snap_name = data_utils.rand_name("tempest-snapshot-name") + cls.snap_desc = data_utils.rand_name( + "tempest-snapshot-description") + cls.snap = cls.create_snapshot_wait_for_active( + cls.share["id"], cls.snap_name, cls.snap_desc) @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) @testtools.skipUnless( @@ -167,7 +179,7 @@ class SharesActionsNegativeTest(base.BaseSharesMixedTest): self.assertEqual(0, len(shares)) @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) - @base.skip_if_microversion_not_supported("2.35") + @base.skip_if_microversion_not_supported("2.36") def test_list_shares_with_like_filter_and_invalid_version(self): # In API versions < v2.36, querying the share API by inexact # filter (name or description) should have no effect. Those @@ -182,7 +194,7 @@ class SharesActionsNegativeTest(base.BaseSharesMixedTest): self.assertGreater(len(shares), 0) @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) - @base.skip_if_microversion_not_supported("2.35") + @base.skip_if_microversion_not_supported("2.36") def test_list_shares_with_like_filter_not_exist(self): filters = { 'name~': 'fake_not_exist', @@ -191,3 +203,43 @@ class SharesActionsNegativeTest(base.BaseSharesMixedTest): shares = self.shares_v2_client.list_shares(params=filters) self.assertEqual(0, len(shares)) + + @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) + def test_list_shares_with_name_not_exist(self): + filters = { + 'name': "tempest-share", + } + shares = self.shares_v2_client.list_shares(params=filters) + + self.assertEqual(0, len(shares)) + + @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) + @base.skip_if_microversion_not_supported("2.36") + def test_list_shares_with_description_not_exist(self): + filters = { + 'description': "tempest-share", + } + shares = self.shares_v2_client.list_shares(params=filters) + + self.assertEqual(0, len(shares)) + + @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) + @base.skip_if_microversion_not_supported("2.36") + def test_list_snapshots_with_description_not_exist(self): + filters = { + 'description': "tempest-snapshot", + } + shares = self.shares_v2_client.list_snapshots_with_detail( + params=filters) + + self.assertEqual(0, len(shares)) + + @tc.attr(base.TAG_NEGATIVE, base.TAG_API_WITH_BACKEND) + def test_list_snapshots_with_name_not_exist(self): + filters = { + 'name': "tempest-snapshot", + } + shares = self.shares_v2_client.list_snapshots_with_detail( + params=filters) + + self.assertEqual(0, len(shares)) diff --git a/releasenotes/notes/bug-1704971-fix-name-description-filter-85935582e89a72a0.yaml b/releasenotes/notes/bug-1704971-fix-name-description-filter-85935582e89a72a0.yaml new file mode 100644 index 0000000000..0d03f0cc6a --- /dev/null +++ b/releasenotes/notes/bug-1704971-fix-name-description-filter-85935582e89a72a0.yaml @@ -0,0 +1,12 @@ +--- +fixes: + - Fix the ``exact`` filters (name, description) in ``shares``, + ``snapshots``, ``share-networks`` list can be filter by ``inexact`` + value. + We got the error because the ``description`` filter will be + skipped in ``shares``, ``snapshots`` list API, and we will directly + remove the ``inexact`` filter flag('~') and process the + ``exact`` filters (name, description) by ``inexact`` filter logic. + Now, we added ``description`` filter in ``shares``, ``snapshots`` list, + and check whether the filter keys has the '~' in the end in ``shares``, + ``snapshots``, ``share-networks`` list firstly.