From de72cd473616bf0454749d6f105a17170cc3d5ce Mon Sep 17 00:00:00 2001 From: haixin Date: Fri, 9 Apr 2021 16:23:24 +0800 Subject: [PATCH] Optimize the query logic for share network list 1: As admin user, query share network list with specified "security_service_id" and "project_id" search opts, will get wrong result. 2: put "created_since", "created_before" search opts into database to Increase query speed Closes-Bug: #1923008 Change-Id: I49e412cb6c98fcda67531ff915b3b4c3edc64476 --- manila/api/v2/share_networks.py | 62 +++++----- manila/db/api.py | 5 + manila/db/sqlalchemy/api.py | 27 +++++ manila/tests/api/v2/test_share_networks.py | 106 ++++++++++-------- manila/tests/db/sqlalchemy/test_api.py | 48 ++++++++ ...-share-network-query-f314ec3010c06045.yaml | 6 + 6 files changed, 173 insertions(+), 81 deletions(-) create mode 100644 releasenotes/notes/bug-1923008-fix-logic-of-share-network-query-f314ec3010c06045.yaml diff --git a/manila/api/v2/share_networks.py b/manila/api/v2/share_networks.py index 3510f35aea..33a032d599 100644 --- a/manila/api/v2/share_networks.py +++ b/manila/api/v2/share_networks.py @@ -162,41 +162,39 @@ class ShareNetworkController(wsgi.Controller, wsgi.AdminActionsMixin): context = req.environ['manila.context'] search_opts = {} search_opts.update(req.GET) + filters = {} - if 'security_service_id' in search_opts: - networks = db_api.share_network_get_all_by_security_service( - context, search_opts['security_service_id']) - elif context.is_admin and 'project_id' in search_opts: - networks = db_api.share_network_get_all_by_project( - context, search_opts['project_id']) - elif context.is_admin and utils.is_all_tenants(search_opts): - networks = db_api.share_network_get_all(context) - else: - networks = db_api.share_network_get_all_by_project( - context, - context.project_id) + # if not context.is_admin, will ignore project_id and all_tenants here, + # in database will auto add context.project_id to search_opts. + if context.is_admin: + if 'project_id' in search_opts: + # if specified project_id, will not use all_tenants + filters['project_id'] = search_opts['project_id'] + elif not utils.is_all_tenants(search_opts): + # if not specified project_id and all_tenants, will get + # share networks in admin project. + filters['project_id'] = context.project_id date_parsing_error_msg = '''%s is not in yyyy-mm-dd format.''' - if 'created_since' in search_opts: - try: - created_since = timeutils.parse_strtime( - search_opts['created_since'], - fmt="%Y-%m-%d") - except ValueError: - msg = date_parsing_error_msg % search_opts['created_since'] - raise exc.HTTPBadRequest(explanation=msg) - networks = [network for network in networks - if network['created_at'] >= created_since] - if 'created_before' in search_opts: - try: - created_before = timeutils.parse_strtime( - search_opts['created_before'], - fmt="%Y-%m-%d") - except ValueError: - msg = date_parsing_error_msg % search_opts['created_before'] - raise exc.HTTPBadRequest(explanation=msg) - networks = [network for network in networks - if network['created_at'] <= created_before] + for time_comparison_filter in ['created_since', 'created_before']: + if time_comparison_filter in search_opts: + time_str = search_opts.get(time_comparison_filter) + try: + parsed_time = timeutils.parse_strtime(time_str, + fmt="%Y-%m-%d") + except ValueError: + msg = date_parsing_error_msg % time_str + raise exc.HTTPBadRequest(explanation=msg) + + filters[time_comparison_filter] = parsed_time + + if 'security_service_id' in search_opts: + filters['security_service_id'] = search_opts.get( + 'security_service_id') + + networks = db_api.share_network_get_all_by_filter(context, + filters=filters) + opts_to_remove = [ 'all_tenants', 'created_since', diff --git a/manila/db/api.py b/manila/db/api.py index d1e7b2e560..754b3f6ed1 100644 --- a/manila/db/api.py +++ b/manila/db/api.py @@ -876,6 +876,11 @@ def share_network_get(context, id): return IMPL.share_network_get(context, id) +def share_network_get_all_by_filter(context, filters=None): + """Get all share network DB records for the given filter.""" + return IMPL.share_network_get_all_by_filter(context, filters=filters) + + def share_network_get_all(context): """Get all share network DB records.""" return IMPL.share_network_get_all(context) diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index c75a61037a..5977665ad0 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -3926,6 +3926,33 @@ def share_network_get(context, id, session=None): return result +@require_context +def share_network_get_all_by_filter(context, filters=None): + model_sn = models.ShareNetwork + session = get_session() + with session.begin(): + query = _network_get_query(context, + session=session) + + legal_filter_keys = ('project_id', 'created_since', 'created_before') + + if not filters: + filters = {} + + query = exact_filter(query, model_sn, filters, legal_filter_keys) + + if 'security_service_id' in filters: + security_service_id = filters.get('security_service_id') + query = query.join( + models.ShareNetworkSecurityServiceAssociation, + models.ShareNetwork.id == models. + ShareNetworkSecurityServiceAssociation. + share_network_id).filter_by( + security_service_id=security_service_id, deleted=0) + + return query.all() + + @require_context def share_network_get_all(context): return _network_get_query(context).all() diff --git a/manila/tests/api/v2/test_share_networks.py b/manila/tests/api/v2/test_share_networks.py index c72fcd3f4b..7d00bbbd2d 100644 --- a/manila/tests/api/v2/test_share_networks.py +++ b/manila/tests/api/v2/test_share_networks.py @@ -537,14 +537,13 @@ class ShareNetworkAPITest(test.TestCase): def test_index_no_filters(self): networks = [fake_share_network] with mock.patch.object(db_api, - 'share_network_get_all_by_project', + 'share_network_get_all_by_filter', mock.Mock(return_value=networks)): result = self.controller.index(self.req) - db_api.share_network_get_all_by_project.assert_called_once_with( - self.context, - self.context.project_id) + db_api.share_network_get_all_by_filter.assert_called_once_with( + self.context, filters={}) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view_shortened( @@ -554,55 +553,56 @@ class ShareNetworkAPITest(test.TestCase): def test_index_detailed(self): networks = [fake_share_network] with mock.patch.object(db_api, - 'share_network_get_all_by_project', + 'share_network_get_all_by_filter', mock.Mock(return_value=networks)): result = self.controller.detail(self.req) - db_api.share_network_get_all_by_project.assert_called_once_with( - self.context, - self.context.project_id) + db_api.share_network_get_all_by_filter.assert_called_once_with( + self.context, filters={}) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view( result[share_networks.RESOURCES_NAME][0], fake_share_network) - @mock.patch.object(db_api, 'share_network_get_all_by_security_service', + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_filter_by_security_service(self): - db_api.share_network_get_all_by_security_service.return_value = [ + db_api.share_network_get_all_by_filter.return_value = [ fake_share_network_with_ss] req = fakes.HTTPRequest.blank( '/share_networks?security_service_id=fake-ss-id') result = self.controller.index(req) - (db_api.share_network_get_all_by_security_service. + filters = {'security_service_id': 'fake-ss-id'} + (db_api.share_network_get_all_by_filter. assert_called_once_with(req.environ['manila.context'], - 'fake-ss-id')) + filters=filters)) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view_shortened( result[share_networks.RESOURCES_NAME][0], fake_sn_with_ss_shortened) - @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_all_tenants_non_admin_context(self): req = fakes.HTTPRequest.blank( '/share_networks?all_tenants=1') fake_context = req.environ['manila.context'] - db_api.share_network_get_all_by_project.return_value = [] + db_api.share_network_get_all_by_filter.return_value = [] self.controller.index(req) - db_api.share_network_get_all_by_project.assert_called_with( - fake_context, fake_context.project_id) + db_api.share_network_get_all_by_filter.assert_called_with( + fake_context, filters={}) - @mock.patch.object(db_api, 'share_network_get_all', mock.Mock()) + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_all_tenants_admin_context(self): - db_api.share_network_get_all.return_value = [fake_share_network] + db_api.share_network_get_all_by_filter.return_value = [ + fake_share_network] req = fakes.HTTPRequest.blank( '/share_networks?all_tenants=1', use_admin_context=True) result = self.controller.index(req) - db_api.share_network_get_all.assert_called_once_with( - req.environ['manila.context']) + db_api.share_network_get_all_by_filter.assert_called_once_with( + req.environ['manila.context'], filters={}) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view_shortened( result[share_networks.RESOURCES_NAME][0], @@ -616,10 +616,9 @@ class ShareNetworkAPITest(test.TestCase): self.assertRaises(exception.InvalidInput, self.controller.index, req) - @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) - @mock.patch.object(db_api, 'share_network_get_all', mock.Mock()) + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_all_tenants_with_value_zero(self): - db_api.share_network_get_all_by_project.return_value = [ + db_api.share_network_get_all_by_filter.return_value = [ fake_share_network] req = fakes.HTTPRequest.blank( '/share_networks?all_tenants=0', @@ -631,49 +630,51 @@ class ShareNetworkAPITest(test.TestCase): self._check_share_network_view_shortened( result[share_networks.RESOURCES_NAME][0], fake_share_network_shortened) - db_api.share_network_get_all_by_project.assert_called_once_with( - req.environ['manila.context'], self.context.project_id) - db_api.share_network_get_all.assert_not_called() + filters = {'project_id': 'fake'} + db_api.share_network_get_all_by_filter.assert_called_once_with( + req.environ['manila.context'], filters=filters) - @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_filter_by_project_id_non_admin_context(self): req = fakes.HTTPRequest.blank( '/share_networks?project_id=fake project') fake_context = req.environ['manila.context'] - db_api.share_network_get_all_by_project.return_value = [] + db_api.share_network_get_all_by_filter.return_value = [] self.controller.index(req) - db_api.share_network_get_all_by_project.assert_called_with( - fake_context, fake_context.project_id) + db_api.share_network_get_all_by_filter.assert_called_with( + fake_context, filters={}) - @mock.patch.object(db_api, 'share_network_get_all_by_project', mock.Mock()) + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_filter_by_project_id_admin_context(self): - db_api.share_network_get_all_by_project.return_value = [ + db_api.share_network_get_all_by_filter.return_value = [ fake_share_network_with_ss ] req = fakes.HTTPRequest.blank( '/share_networks?project_id=fake', use_admin_context=True) result = self.controller.index(req) - db_api.share_network_get_all_by_project.assert_called_once_with( - req.environ['manila.context'], 'fake') + filters = {'project_id': 'fake'} + db_api.share_network_get_all_by_filter.assert_called_once_with( + req.environ['manila.context'], filters=filters) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view_shortened( result[share_networks.RESOURCES_NAME][0], fake_sn_with_ss_shortened) - @mock.patch.object(db_api, 'share_network_get_all_by_security_service', + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_filter_by_ss_and_project_id_admin_context(self): - db_api.share_network_get_all_by_security_service.return_value = [ + db_api.share_network_get_all_by_filter.return_value = [ fake_share_network_with_ss ] req = fakes.HTTPRequest.blank( '/share_networks?security_service_id=fake-ss-id&project_id=fake', use_admin_context=True) result = self.controller.index(req) - (db_api.share_network_get_all_by_security_service. - assert_called_once_with(req.environ['manila.context'], - 'fake-ss-id')) + filters = {'project_id': 'fake', + 'security_service_id': 'fake-ss-id'} + db_api.share_network_get_all_by_filter.assert_called_once_with( + req.environ['manila.context'], filters=filters) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view_shortened( result[share_networks.RESOURCES_NAME][0], @@ -686,20 +687,21 @@ class ShareNetworkAPITest(test.TestCase): ('name=foo&description~=ds', 1), ('name~=foo&description=ds', 1)) @ddt.unpack - @mock.patch.object(db_api, 'share_network_get_all_by_project', + @mock.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) 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 + db_api.share_network_get_all_by_filter.return_value = fake_objs req = fakes.HTTPRequest.blank( '/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_with( - req.environ['manila.context'], self.context.project_id) + filters = {'project_id': self.context.project_id} + db_api.share_network_get_all_by_filter.assert_called_with( + req.environ['manila.context'], filters=filters) self.assertEqual(share_network_number, len(result[share_networks.RESOURCES_NAME])) if share_network_number > 0: @@ -709,7 +711,7 @@ class ShareNetworkAPITest(test.TestCase): 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.patch.object(db_api, 'share_network_get_all_by_filter', mock.Mock()) def test_index_all_filter_opts(self): valid_filter_opts = { @@ -717,8 +719,7 @@ class ShareNetworkAPITest(test.TestCase): 'created_since': '1999-01-01', 'name': 'test-sn' } - db_api.share_network_get_all_by_project.return_value = [ - fake_share_network, + db_api.share_network_get_all_by_filter.return_value = [ fake_share_network_with_ss] query_string = '/share-networks?' + parse.urlencode(sorted( @@ -727,9 +728,16 @@ class ShareNetworkAPITest(test.TestCase): req = fakes.HTTPRequest.blank(query_string, use_admin_context=use_admin_context) result = self.controller.index(req) - db_api.share_network_get_all_by_project.assert_called_with( - req.environ['manila.context'], - 'fake') + parsed_created_before = timeutils.parse_strtime( + valid_filter_opts['created_before'], fmt="%Y-%m-%d") + parsed_created_since = timeutils.parse_strtime( + valid_filter_opts['created_since'], fmt="%Y-%m-%d") + filters = {'created_before': parsed_created_before, + 'created_since': parsed_created_since} + if use_admin_context: + filters['project_id'] = 'fake' + db_api.share_network_get_all_by_filter.assert_called_with( + req.environ['manila.context'], filters=filters) self.assertEqual(1, len(result[share_networks.RESOURCES_NAME])) self._check_share_network_view_shortened( result[share_networks.RESOURCES_NAME][0], diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index addaa741d2..00d1a0119e 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -2486,6 +2486,54 @@ class ShareNetworkDatabaseAPITestCase(BaseDatabaseAPITestCase): for index, net in enumerate(share_networks): self._check_fields(expected=net, actual=result[index]) + def test_get_all_by_filter_with_project_id(self): + db_api.share_network_create(self.fake_context, self.share_nw_dict) + + share_nw_dict2 = dict(self.share_nw_dict) + share_nw_dict2['id'] = 'fake share nw id2' + share_nw_dict2['project_id'] = 'fake project 2' + new_context = context.RequestContext(user_id='fake user 2', + project_id='fake project 2', + is_admin=False) + db_api.share_network_create(new_context, share_nw_dict2) + + filters = {'project_id': share_nw_dict2['project_id']} + result = db_api.share_network_get_all_by_filter( + self.fake_context.elevated(), filters=filters) + + self.assertEqual(1, len(result)) + self._check_fields(expected=share_nw_dict2, actual=result[0]) + + def test_get_all_with_created_since_or_before_filter(self): + now = timeutils.utcnow() + + share_nw1 = dict(self.share_nw_dict) + share_nw2 = dict(self.share_nw_dict) + share_nw3 = dict(self.share_nw_dict) + + share_nw1['created_at'] = (now - datetime.timedelta(seconds=1)) + share_nw2['created_at'] = (now + datetime.timedelta(seconds=1)) + share_nw3['created_at'] = (now + datetime.timedelta(seconds=2)) + + share_nw1['id'] = 'fake share nw id1' + share_nw2['id'] = 'fake share nw id2' + share_nw3['id'] = 'fake share nw id3' + + db_api.share_network_create(self.fake_context, share_nw1) + db_api.share_network_create(self.fake_context, share_nw2) + db_api.share_network_create(self.fake_context, share_nw3) + + filters1 = {'created_before': now} + filters2 = {'created_since': now} + + result1 = db_api.share_network_get_all_by_filter( + self.fake_context.elevated(), filters=filters1) + result2 = db_api.share_network_get_all_by_filter( + self.fake_context.elevated(), filters=filters2) + + self.assertEqual(1, len(result1)) + self.assertEqual(2, len(result2)) + def test_get_all_by_project(self): db_api.share_network_create(self.fake_context, self.share_nw_dict) diff --git a/releasenotes/notes/bug-1923008-fix-logic-of-share-network-query-f314ec3010c06045.yaml b/releasenotes/notes/bug-1923008-fix-logic-of-share-network-query-f314ec3010c06045.yaml new file mode 100644 index 0000000000..a730647998 --- /dev/null +++ b/releasenotes/notes/bug-1923008-fix-logic-of-share-network-query-f314ec3010c06045.yaml @@ -0,0 +1,6 @@ +--- +fixes: + - | + Fix the query logic for share network list, put "created_since", + "created_before" search opts into database to increase query speed, + integrate the database query interface.