diff --git a/manila/db/sqlalchemy/api.py b/manila/db/sqlalchemy/api.py index 84215f4ecb..3e7a3dfeed 100644 --- a/manila/db/sqlalchemy/api.py +++ b/manila/db/sqlalchemy/api.py @@ -205,9 +205,19 @@ def apply_sorting(model, query, sort_key, sort_dir): "sort_key": sort_key, "sort_dir": sort_dir} raise exception.InvalidInput(reason=msg) - sort_attr = getattr(model, sort_key) - sort_method = getattr(sort_attr, sort_dir.lower()) - return query.order_by(sort_method()) + # NOTE(maaoyu): We add the additional sort by ID in this case to + # get deterministic results. Without the ordering by ID this could + # lead to flapping return lists. + sort_keys = [sort_key] + if sort_key != 'id': + sort_keys.append('id') + + for sort_key in sort_keys: + sort_attr = getattr(model, sort_key) + sort_method = getattr(sort_attr, sort_dir.lower()) + query = query.order_by(sort_method()) + + return query def handle_db_data_error(f): @@ -1743,6 +1753,7 @@ def share_replica_delete(context, share_replica_id, session=None): ################ +@require_context def _share_get_query(context, session=None): if session is None: session = get_session() @@ -1750,6 +1761,96 @@ def _share_get_query(context, session=None): options(joinedload('share_metadata'))) +def _process_share_filters(query, filters, project_id=None, is_public=False): + if filters is None: + filters = {} + + share_filter_keys = ['share_group_id', 'snapshot_id'] + instance_filter_keys = ['share_server_id', 'status', 'share_type_id', + 'host', 'share_network_id'] + share_filters = {} + instance_filters = {} + + for k, v in filters.items(): + share_filters.update({k: v}) if k in share_filter_keys else None + instance_filters.update({k: v}) if k in instance_filter_keys else None + + no_key = 'key_is_absent' + + def _filter_data(query, model, desired_filters): + for key, value in desired_filters.items(): + filter_attr = getattr(model, key, no_key) + if filter_attr == no_key: + pass + query = query.filter(filter_attr == value) + return query + + if share_filters: + query = _filter_data(query, models.Share, share_filters) + if instance_filters: + query = _filter_data(query, models.ShareInstance, instance_filters) + + if project_id: + if is_public: + query = query.filter(or_(models.Share.project_id == project_id, + models.Share.is_public)) + else: + query = query.filter(models.Share.project_id == project_id) + + display_name = filters.get('display_name') + if display_name: + query = query.filter( + models.Share.display_name == display_name) + else: + display_name = filters.get('display_name~') + if display_name: + query = query.filter(models.Share.display_name.op('LIKE')( + u'%' + display_name + u'%')) + + display_description = filters.get('display_description') + if display_description: + query = query.filter( + models.Share.display_description == display_description) + else: + display_description = filters.get('display_description~') + if display_description: + query = query.filter(models.Share.display_description.op('LIKE')( + u'%' + display_description + u'%')) + + export_location_id = filters.pop('export_location_id', None) + export_location_path = filters.pop('export_location_path', None) + if export_location_id or export_location_path: + query = query.join( + models.ShareInstanceExportLocations, + models.ShareInstanceExportLocations.share_instance_id == + models.ShareInstance.id) + if export_location_path: + query = query.filter( + models.ShareInstanceExportLocations.path == + export_location_path) + if export_location_id: + query = query.filter( + models.ShareInstanceExportLocations.uuid == + export_location_id) + + if 'metadata' in filters: + for k, v in filters['metadata'].items(): + # pylint: disable=no-member + query = query.filter( + or_(models.Share.share_metadata.any( + key=k, value=v))) + if 'extra_specs' in filters: + query = query.join( + models.ShareTypeExtraSpecs, + models.ShareTypeExtraSpecs.share_type_id == + models.ShareInstance.share_type_id) + for k, v in filters['extra_specs'].items(): + query = query.filter(or_(models.ShareTypeExtraSpecs.key == k, + models.ShareTypeExtraSpecs.value == v)) + + return query + + def _metadata_refs(metadata_dict, meta_class): metadata_refs = [] if metadata_dict: @@ -1854,6 +1955,9 @@ def _share_get_all_with_filters(context, project_id=None, share_server_id=None, :returns: list -- models.Share :raises: exception.InvalidInput """ + if filters is None: + filters = {} + if not sort_key: sort_key = 'created_at' if not sort_dir: @@ -1865,54 +1969,13 @@ def _share_get_all_with_filters(context, project_id=None, share_server_id=None, ) ) - if project_id: - if is_public: - query = query.filter(or_(models.Share.project_id == project_id, - models.Share.is_public)) - else: - query = query.filter(models.Share.project_id == project_id) - if share_server_id: - query = query.filter( - models.ShareInstance.share_server_id == share_server_id) - if share_group_id: - query = query.filter( - models.Share.share_group_id == share_group_id) + filters['share_group_id'] = share_group_id + if share_server_id: + filters['share_server_id'] = share_server_id - # Apply filters - if not filters: - filters = {} - - export_location_id = filters.get('export_location_id') - export_location_path = filters.get('export_location_path') - if export_location_id or export_location_path: - query = query.join( - models.ShareInstanceExportLocations, - models.ShareInstanceExportLocations.share_instance_id == - models.ShareInstance.id) - if export_location_path: - query = query.filter( - models.ShareInstanceExportLocations.path == - export_location_path) - if export_location_id: - query = query.filter( - models.ShareInstanceExportLocations.uuid == - export_location_id) - - if 'metadata' in filters: - for k, v in filters['metadata'].items(): - # pylint: disable=no-member - query = query.filter( - or_(models.Share.share_metadata.any( - key=k, value=v))) - if 'extra_specs' in filters: - query = query.join( - models.ShareTypeExtraSpecs, - models.ShareTypeExtraSpecs.share_type_id == - models.ShareInstance.share_type_id) - for k, v in filters['extra_specs'].items(): - query = query.filter(or_(models.ShareTypeExtraSpecs.key == k, - models.ShareTypeExtraSpecs.value == v)) + query = _process_share_filters( + query, filters, project_id, is_public=is_public) try: query = apply_sorting(models.Share, query, sort_key, sort_dir) @@ -1935,8 +1998,12 @@ def _share_get_all_with_filters(context, project_id=None, share_server_id=None, @require_admin_context def share_get_all(context, filters=None, sort_key=None, sort_dir=None): + project_id = filters.pop('project_id', None) if filters else None query = _share_get_all_with_filters( - context, filters=filters, sort_key=sort_key, sort_dir=sort_dir) + context, + project_id=project_id, + filters=filters, sort_key=sort_key, sort_dir=sort_dir) + return query diff --git a/manila/share/api.py b/manila/share/api.py index 81708d80c4..d148129cd4 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -1654,12 +1654,18 @@ class API(base.Base): # Prepare filters filters = {} - if 'export_location_id' in search_opts: - filters['export_location_id'] = search_opts.pop( - 'export_location_id') - if 'export_location_path' in search_opts: - filters['export_location_path'] = search_opts.pop( - 'export_location_path') + + filter_keys = [ + 'display_name', 'share_group_id', 'display_name~', + 'display_description', 'display_description~', 'snapshot_id', + 'status', 'share_type_id', 'project_id', 'export_location_id', + 'export_location_path', 'limit', 'offset', 'host', + 'share_network_id'] + + for key in filter_keys: + if key in search_opts: + filters[key] = search_opts.pop(key) + if 'metadata' in search_opts: filters['metadata'] = search_opts.pop('metadata') if not isinstance(filters['metadata'], dict): @@ -1674,10 +1680,7 @@ class API(base.Base): msg = _("Wrong extra specs filter provided: " "%s.") % six.text_type(filters['extra_specs']) raise exception.InvalidInput(reason=msg) - if 'limit' in search_opts: - filters['limit'] = search_opts.pop('limit') - if 'offset' in search_opts: - filters['offset'] = search_opts.pop('offset') + if not (isinstance(sort_key, six.string_types) and sort_key): msg = _("Wrong sort_key filter provided: " "'%s'.") % six.text_type(sort_key) @@ -1691,7 +1694,7 @@ class API(base.Base): is_public = strutils.bool_from_string(is_public, strict=True) # Get filtered list of shares - if 'host' in search_opts: + if 'host' in filters: policy.check_policy(context, 'share', 'list_by_host') if 'share_server_id' in search_opts: # NOTE(vponomaryov): this is project_id independent diff --git a/manila/tests/db/sqlalchemy/test_api.py b/manila/tests/db/sqlalchemy/test_api.py index f8756344b9..90dadc7dcb 100644 --- a/manila/tests/db/sqlalchemy/test_api.py +++ b/manila/tests/db/sqlalchemy/test_api.py @@ -565,6 +565,57 @@ class ShareDatabaseAPITestCase(test.TestCase): self.assertEqual(0, len( set(shares_requested_ids) & set(shares_not_requested_ids))) + @ddt.data( + ({'status': constants.STATUS_AVAILABLE}, 'status', + [constants.STATUS_AVAILABLE, constants.STATUS_ERROR]), + ({'share_group_id': 'fake_group_id'}, 'share_group_id', + ['fake_group_id', 'group_id']), + ({'snapshot_id': 'fake_snapshot_id'}, 'snapshot_id', + ['fake_snapshot_id', 'snapshot_id']), + ({'share_type_id': 'fake_type_id'}, 'share_type_id', + ['fake_type_id', 'type_id']), + ({'host': 'fakehost@fakebackend#fakepool'}, 'host', + ['fakehost@fakebackend#fakepool', 'foo@bar#test']), + ({'share_network_id': 'fake_net_id'}, 'share_network_id', + ['fake_net_id', 'net_id']), + ({'display_name': 'fake_share_name'}, 'display_name', + ['fake_share_name', 'share_name']), + ({'display_description': 'fake description'}, 'display_description', + ['fake description', 'description']) + ) + @ddt.unpack + def test_share_get_all_with_filters(self, filters, key, share_values): + for value in share_values: + kwargs = {key: value} + db_utils.create_share(**kwargs) + + results = db_api.share_get_all(self.ctxt, filters=filters) + + for share in results: + self.assertEqual(share[key], filters[key]) + + @ddt.data( + ('display_name~', 'display_name', + ['fake_name_1', 'fake_name_2', 'fake_name_3'], 'fake_name'), + ('display_description~', 'display_description', + ['fake desc 1', 'fake desc 2', 'fake desc 3'], 'fake desc') + ) + @ddt.unpack + def test_share_get_all_like_filters( + self, filter_name, key, share_values, like_value): + for value in share_values: + kwargs = {key: value} + db_utils.create_share(**kwargs) + db_utils.create_share( + display_name='irrelevant_name', + display_description='should not be queried') + + filters = {filter_name: like_value} + + results = db_api.share_get_all(self.ctxt, filters=filters) + + self.assertEqual(len(share_values), len(results)) + @ddt.data(None, 'writable') def test_share_get_has_replicas_field(self, replication_type): share = db_utils.create_share(replication_type=replication_type) diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index 7c173a6764..469e55c2ee 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -397,29 +397,35 @@ class ShareAPITestCase(test.TestCase): def test_get_all_admin_filter_by_status(self): 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[1:])) + expected_filter = {'status': constants.STATUS_AVAILABLE} + self.mock_object( + db_api, 'share_get_all_by_project', + mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[0::2])) + shares = self.api.get_all(ctx, {'status': constants.STATUS_AVAILABLE}) share_api.policy.check_policy.assert_has_calls([ mock.call(ctx, 'share', 'get_all'), ]) db_api.share_get_all_by_project.assert_called_once_with( ctx, sort_dir='desc', sort_key='created_at', - project_id='fake_pid_2', filters={}, is_public=False + project_id='fake_pid_2', filters=expected_filter, is_public=False ) - self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[2::4], shares) + self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[0::2], shares) def test_get_all_admin_filter_by_status_and_all_tenants(self): ctx = context.RequestContext('fake_uid', 'fake_pid_2', is_admin=True) - self.mock_object(db_api, 'share_get_all', - mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES)) + self.mock_object( + db_api, 'share_get_all', + mock.Mock(return_value=_FAKE_LIST_OF_ALL_SHARES[1::2])) + expected_filter = {'status': constants.STATUS_ERROR} shares = self.api.get_all( ctx, {'status': constants.STATUS_ERROR, 'all_tenants': 1}) share_api.policy.check_policy.assert_has_calls([ mock.call(ctx, 'share', 'get_all'), ]) db_api.share_get_all.assert_called_once_with( - ctx, sort_dir='desc', sort_key='created_at', filters={}) + ctx, sort_dir='desc', sort_key='created_at', + filters=expected_filter) self.assertEqual(_FAKE_LIST_OF_ALL_SHARES[1::2], shares) def test_get_all_non_admin_filter_by_all_tenants(self): @@ -446,9 +452,12 @@ class ShareAPITestCase(test.TestCase): share_api.policy.check_policy.assert_has_calls([ mock.call(ctx, 'share', 'get_all'), ]) + expected_filter_1 = {'status': constants.STATUS_ERROR} + expected_filter_2 = {'status': constants.STATUS_AVAILABLE} + db_api.share_get_all_by_project.assert_called_once_with( ctx, sort_dir='desc', sort_key='created_at', - project_id='fake_pid_2', filters={}, is_public=False + project_id='fake_pid_2', filters=expected_filter_1, is_public=False ) # two items expected, one filtered @@ -463,10 +472,14 @@ class ShareAPITestCase(test.TestCase): mock.call(ctx, 'share', 'get_all'), ]) db_api.share_get_all_by_project.assert_has_calls([ - mock.call(ctx, sort_dir='desc', sort_key='created_at', - project_id='fake_pid_2', filters={}, is_public=False), - mock.call(ctx, sort_dir='desc', sort_key='created_at', - project_id='fake_pid_2', filters={}, is_public=False), + mock.call( + ctx, sort_dir='desc', sort_key='created_at', + project_id='fake_pid_2', filters=expected_filter_1, + is_public=False), + mock.call( + ctx, sort_dir='desc', sort_key='created_at', + project_id='fake_pid_2', filters=expected_filter_2, + is_public=False), ]) @ddt.data('True', 'true', '1', 'yes', 'y', 'on', 't', True)