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

@@ -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:
+        # if not context.is_admin, will ignore project_id and all_tenants here,
-            networks = db_api.share_network_get_all_by_security_service(
+        # in database will auto add context.project_id to search_opts.
-                context, search_opts['security_service_id'])
+        if context.is_admin:
-        elif context.is_admin and 'project_id' in search_opts:
+            if 'project_id' in search_opts:
-            networks = db_api.share_network_get_all_by_project(
+                # if specified project_id, will not use all_tenants
-                context, search_opts['project_id'])
+                filters['project_id'] = search_opts['project_id']
-        elif context.is_admin and utils.is_all_tenants(search_opts):
+            elif not utils.is_all_tenants(search_opts):
-            networks = db_api.share_network_get_all(context)
+                # if not specified project_id and all_tenants, will get
-        else:
+                # share networks in admin project.
-            networks = db_api.share_network_get_all_by_project(
+                filters['project_id'] = context.project_id
-                context,
-                context.project_id)
 
         date_parsing_error_msg = '''%s is not in yyyy-mm-dd format.'''
-        if 'created_since' in search_opts:
+        for time_comparison_filter in ['created_since', 'created_before']:
-            try:
+            if time_comparison_filter in search_opts:
-                created_since = timeutils.parse_strtime(
+                time_str = search_opts.get(time_comparison_filter)
-                    search_opts['created_since'],
+                try:
-                    fmt="%Y-%m-%d")
+                    parsed_time = timeutils.parse_strtime(time_str,
-            except ValueError:
+                                                          fmt="%Y-%m-%d")
-                msg = date_parsing_error_msg % search_opts['created_since']
+                except ValueError:
-                raise exc.HTTPBadRequest(explanation=msg)
+                    msg = date_parsing_error_msg % time_str
-            networks = [network for network in networks
+                    raise exc.HTTPBadRequest(explanation=msg)
-                        if network['created_at'] >= created_since]
+
-        if 'created_before' in search_opts:
+                filters[time_comparison_filter] = parsed_time
-            try:
+
-                created_before = timeutils.parse_strtime(
+        if 'security_service_id' in search_opts:
-                    search_opts['created_before'],
+            filters['security_service_id'] = search_opts.get(
-                    fmt="%Y-%m-%d")
+                'security_service_id')
-            except ValueError:
+
-                msg = date_parsing_error_msg % search_opts['created_before']
+        networks = db_api.share_network_get_all_by_filter(context,
-                raise exc.HTTPBadRequest(explanation=msg)
+                                                          filters=filters)
-            networks = [network for network in networks
+
-                        if network['created_at'] <= created_before]
         opts_to_remove = [
             'all_tenants',
             'created_since',

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)

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()

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(
+            db_api.share_network_get_all_by_filter.assert_called_once_with(
-                self.context,
+                self.context, filters={})
-                self.context.project_id)
 
             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(
+            db_api.share_network_get_all_by_filter.assert_called_once_with(
-                self.context,
+                self.context, filters={})
-                self.context.project_id)
 
             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(
+        db_api.share_network_get_all_by_filter.assert_called_with(
-            fake_context, fake_context.project_id)
+            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(
+        db_api.share_network_get_all_by_filter.assert_called_once_with(
-            req.environ['manila.context'])
+            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_by_filter', mock.Mock())
-    @mock.patch.object(db_api, 'share_network_get_all', 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(
+        filters = {'project_id': 'fake'}
-            req.environ['manila.context'], self.context.project_id)
+        db_api.share_network_get_all_by_filter.assert_called_once_with(
-        db_api.share_network_get_all.assert_not_called()
+            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(
+        db_api.share_network_get_all_by_filter.assert_called_with(
-            fake_context, fake_context.project_id)
+            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(
+        filters = {'project_id': 'fake'}
-            req.environ['manila.context'], '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.
+        filters = {'project_id': 'fake',
-            assert_called_once_with(req.environ['manila.context'],
+                   'security_service_id': 'fake-ss-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(
+        filters = {'project_id': self.context.project_id}
-            req.environ['manila.context'], 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 = [
+        db_api.share_network_get_all_by_filter.return_value = [
-            fake_share_network,
             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(
+            parsed_created_before = timeutils.parse_strtime(
-                req.environ['manila.context'],
+                valid_filter_opts['created_before'], fmt="%Y-%m-%d")
-                'fake')
+            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],

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)
 

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.