From 37b31018dd6e84eed881d9b324821eec54b94b04 Mon Sep 17 00:00:00 2001 From: Kushal Agrawal Date: Wed, 7 Feb 2018 12:14:14 +0530 Subject: [PATCH] and/or query combiner for filter options Provides an option to specify query combiner ("and/or") for attribute based filters in query parameter. So if user wants to search artefact a based on filters : field1 = value1 and (field2 = value2 or field3 = value3) it can be specified in query like : field1=eq:value1&field2=or:eq:value2&field3=or:eq:value3 "and" is the default value of combiner in case it is not specified. Change-Id: I16c7c7fecefab5d615fa9e933234725249c5d97a Implements: blueprint glare-search-artifact-query-update --- glare/common/utils.py | 64 ++++++++++++------- glare/db/sqlalchemy/api.py | 52 ++++++++++----- glare/objects/base.py | 26 ++++++-- glare/scrubber.py | 2 +- .../tests/functional/test_sample_artifact.py | 50 +++++++++++++++ glare/tests/unit/api/test_list.py | 36 ++++++++++- 6 files changed, 180 insertions(+), 50 deletions(-) diff --git a/glare/common/utils.py b/glare/common/utils.py index aea9c5f..a509177 100644 --- a/glare/common/utils.py +++ b/glare/common/utils.py @@ -343,32 +343,52 @@ def stash_conf_values(): def split_filter_op(expression): - """Split operator from threshold in an expression. - Designed for use on a comparative-filtering query field. - When no operator is found, default to an equality comparison. - :param expression: the expression to parse - :return: a tuple (operator, threshold) parsed from expression - """ - left, sep, right = expression.partition(':') - if sep: - # If the expression is a date of the format ISO 8601 like - # CCYY-MM-DDThh:mm:ss+hh:mm and has no operator, it should - # not be partitioned, and a default operator of eq should be - # assumed. + default_operator = "eq" + default_condition_combiner = "and" + combiner_options = ('and', 'or') + + # all method return data in requence combiner, Operator, value + def one(expression): + return default_condition_combiner, default_operator, expression + + def two(expression): + args = expression.split(":") + if args[0] in combiner_options: # and:5, and:or, or:lte + return args[0], default_operator, args[1] + else: + return default_condition_combiner, args[0], args[1] + + def multiple(expression): + args = expression.split(":") + combiner = default_condition_combiner + progress_index = 0 + + if args[0] in combiner_options: + combiner = args[0] + progress_index += 1 + + if _is_iso_date(":".join(args[progress_index:])): + expression = ":".join(args[progress_index:]) + operator = default_operator + else: + operator = args[progress_index] + progress_index += 1 + expression = ":".join(args[progress_index:]) + + return combiner, operator, expression + + def _is_iso_date(arg): try: - timeutils.parse_isotime(expression) - op = 'eq' - threshold = expression + timeutils.parse_isotime(arg) + return True except ValueError: - op = left - threshold = right - else: - op = 'eq' # default operator - threshold = left + return False - # NOTE stevelle decoding escaped values may be needed later - return op, threshold + switcher = {1: one, 2: two} + func = switcher.get(len(expression.split(":")), multiple) + + return func(expression) def validate_quotes(value): diff --git a/glare/db/sqlalchemy/api.py b/glare/db/sqlalchemy/api.py index f38bbf4..5d55e83 100644 --- a/glare/db/sqlalchemy/api.py +++ b/glare/db/sqlalchemy/api.py @@ -26,7 +26,7 @@ import osprofiler.sqlalchemy from retrying import retry import six import sqlalchemy -from sqlalchemy import and_ +from sqlalchemy import and_, distinct import sqlalchemy.exc from sqlalchemy import exists from sqlalchemy import func @@ -254,9 +254,13 @@ def _apply_latest_filter(context, session, query, def _apply_user_filters(query, basic_conds, tag_conds, prop_conds): + or_queries = [] if basic_conds: - for basic_condition in basic_conds: + for basic_condition in basic_conds['and']: query = query.filter(and_(*basic_condition)) + or_queries = [] + for basic_condition in basic_conds['or']: + or_queries.append(*basic_condition) if tag_conds: for tag_condition in tag_conds: @@ -264,9 +268,15 @@ def _apply_user_filters(query, basic_conds, tag_conds, prop_conds): and_(*tag_condition)) if prop_conds: - for prop_condition in prop_conds: + for prop_condition in prop_conds['and']: query = query.join(models.ArtifactProperty, aliased=True).filter( and_(*prop_condition)) + for prop_condition in prop_conds['or']: + or_queries.append(and_(*prop_condition)) + + if len(or_queries) != 0: + query = query.join(models.ArtifactProperty, aliased=True).filter( + or_(*or_queries)) return query @@ -423,10 +433,16 @@ op_mappings = { def _do_query_filters(filters): - basic_conds = [] + basic_conds = { + "and": [], + "or": [] + } tag_conds = [] - prop_conds = [] - for field_name, key_name, op, field_type, value in filters: + prop_conds = { + "and": [], + "or": [] + } + for field_name, key_name, op, field_type, value, query_combiner in filters: if field_name == 'tags': tags = utils.split_filter_value_for_quotes(value) for tag in tags: @@ -438,21 +454,21 @@ def _do_query_filters(filters): if op == 'in': if field_name == 'version': value = [semver_db.parse(val) for val in value] - basic_conds.append( + basic_conds[query_combiner].append( [or_(*[ models.Artifact.version == ver for ver in value])]) else: - basic_conds.append( + basic_conds[query_combiner].append( [getattr(models.Artifact, field_name).in_(value)]) elif op == 'like': - basic_conds.append( + basic_conds[query_combiner].append( [getattr(models.Artifact, field_name).like(value)]) else: fn = op_mappings[op] if field_name == 'version': value = semver_db.parse(value) - basic_conds.append([fn(getattr(models.Artifact, field_name), - value)]) + basic_conds[query_combiner].append( + [fn(getattr(models.Artifact, field_name), value)]) else: conds = [models.ArtifactProperty.name == field_name] if key_name is not None: @@ -474,7 +490,7 @@ def _do_query_filters(filters): conds.extend([fn(getattr(models.ArtifactProperty, field_type + '_value'), value)]) - prop_conds.append(conds) + prop_conds[query_combiner].append(conds) return basic_conds, tag_conds, prop_conds @@ -772,11 +788,12 @@ def delete_blob_data(context, uri, session): models.ArtifactBlobData).filter_by(id=blob_data_id).delete() -def get_artifact_count(context, session, filters=None, latest=False): +def get_artifact_count(context, session, filters=None, latest=False, + list_all_artifacts=False): filters = filters or {} - query = _create_artifact_count_query(context, session) + query = _create_artifact_count_query(context, session, list_all_artifacts) basic_conds, tag_conds, prop_conds = _do_query_filters(filters) @@ -789,8 +806,9 @@ def get_artifact_count(context, session, filters=None, latest=False): return query.all()[0].total_count -def _create_artifact_count_query(context, session): +def _create_artifact_count_query(context, session, list_all_artifacts): - query = session.query(func.count(models.Artifact.id).label("total_count")) + query = session.query(func.count(distinct(models.Artifact.id)) + .label("total_count")) - return _apply_query_base_filters(query, context) + return _apply_query_base_filters(query, context, list_all_artifacts) diff --git a/glare/objects/base.py b/glare/objects/base.py index 8360db7..3fc5fd2 100644 --- a/glare/objects/base.py +++ b/glare/objects/base.py @@ -71,6 +71,8 @@ class BaseArtifact(base.VersionedObject): STATUS = ('drafted', 'active', 'deactivated', 'deleted') + DEFAULT_QUERY_COMBINER = "and" + Field = wrappers.Field.init DictField = wrappers.DictField.init ListField = wrappers.ListField.init @@ -356,7 +358,8 @@ Possible values: msg = _("Tags are filtered without operator") raise exception.BadRequest(msg) new_filters.append( - (filter_name, None, None, None, filter_value)) + (filter_name, None, None, None, filter_value, + cls.DEFAULT_QUERY_COMBINER)) continue key_name = None @@ -377,19 +380,25 @@ Possible values: field_type = field_type.element_type try: - op, val = utils.split_filter_op(filter_value) + query_combiner, op, val = utils.split_filter_op(filter_value) + if isinstance(field_type, glare_fields.Dict): if op not in ['eq', 'in']: msg = (_("Unsupported filter type '%s'. The following " "filters are supported: eq, in") % op) raise exception.BadRequest(message=msg) + if query_combiner not in ["and", "or"]: + msg = (_("Unsupported Query combiner type '%s'. Only " + "following combiner are allowed: and, or") + % query_combiner) + raise exception.BadRequest(message=msg) if op == 'in': new_filters.append(( filter_name, utils.split_filter_value_for_quotes( - val), op, None, None)) + val), op, None, None, query_combiner)) else: new_filters.append(( - filter_name, val, op, None, None)) + filter_name, val, op, None, None, query_combiner)) else: cls._validate_filter_ops(filter_name, op) if op == 'in': @@ -400,7 +409,8 @@ Possible values: value = field_type.coerce(cls(), filter_name, val) new_filters.append( (filter_name, key_name, op, - cls._get_field_type(field_type), value)) + cls._get_field_type(field_type), + value, query_combiner)) except ValueError: msg = _("Invalid filter value: %s") % str(val) raise exception.BadRequest(msg) @@ -440,10 +450,12 @@ Possible values: sort.append(default_sort) default_filter_parameters = [ - ('status', None, 'neq', None, 'deleted')] + ('status', None, 'neq', None, 'deleted', + cls.DEFAULT_QUERY_COMBINER)] if cls.get_type_name() != 'all': default_filter_parameters.append( - ('type_name', None, 'eq', None, cls.get_type_name())) + ('type_name', None, 'eq', None, cls.get_type_name(), + cls.DEFAULT_QUERY_COMBINER)) # Parse filter parameters and update them with defaults filters = [] if filters is None else cls._parse_filter_values(filters) for default_filter in default_filter_parameters: diff --git a/glare/scrubber.py b/glare/scrubber.py index 2b34b46..29cd309 100644 --- a/glare/scrubber.py +++ b/glare/scrubber.py @@ -148,7 +148,7 @@ class Scrubber(object): session=db_api.get_session(), limit=CONF.scrubber.scrub_pool_size, sort=[], - filters=[('status', None, 'eq', None, 'deleted')]) + filters=[('status', None, 'eq', None, 'deleted', 'and')]) if not artifacts: break self.pool.imap(self._scrub_artifact, artifacts) diff --git a/glare/tests/functional/test_sample_artifact.py b/glare/tests/functional/test_sample_artifact.py index 297ba43..d496a1d 100644 --- a/glare/tests/functional/test_sample_artifact.py +++ b/glare/tests/functional/test_sample_artifact.py @@ -273,6 +273,10 @@ class TestList(base.TestArtifact): result = sort_results(self.get(url=url)['artifacts']) self.assertEqual(art_list[5:], result) + url = '/sample_artifact?bool1=False' + result = sort_results(self.get(url=url)['artifacts']) + self.assertEqual(art_list[5:], result) + # Like filter test cases for name, status url = '/sample_artifact?name=like:name%' artifacts = self.get(url=url)['artifacts'] @@ -665,6 +669,52 @@ class TestList(base.TestArtifact): self.assertEqual(res['total_count'], 0) self.assertEqual(res['display_type_name'], "Sample Artifact") + def test_list_artifact_with_filter_query_combiner(self): + # Create artifact + art_list = [self.create_artifact({'name': 'name%s' % i, + 'version': '2.0', + 'tags': ['tag%s' % i], + 'int1': 1024, + 'float1': 123.456, + 'str1': 'bugaga', + 'bool1': True}) + for i in range(5)] + + public_art = self.create_artifact({'name': 'name5', + 'version': '2.0', + 'tags': ['tag4', 'tag5'], + 'int1': 2048, + 'float1': 987.654, + 'str1': 'lalala', + 'bool1': False, + 'string_required': '123'}) + url = '/sample_artifact/%s' % public_art['id'] + data = [{ + "op": "replace", + "path": "/status", + "value": "active" + }] + + self.patch(url=url, data=data, status=200) + public_art = self.admin_action(public_art['id'], self.make_public) + + art_list.append(public_art) + + url = '/sample_artifact?float1=and:lte:123.456&str1=or:eq:lalal&' \ + 'str1=or:eq:bugaga' + result = sort_results(self.get(url=url)['artifacts']) + self.assertEqual(art_list[:5], result) + + url = '/sample_artifact?str1=or:blah:t' + self.get(url=url, status=400) + + url = '/sample_artifact?str1=or:blabla:t:tt' + self.get(url=url, status=400) + + url = '/sample_artifact?str1=or:eq:blabla:t:tt' + result = self.get(url=url)['artifacts'] + self.assertEqual([], result) + class TestBlobs(base.TestArtifact): def test_blob_dicts(self): diff --git a/glare/tests/unit/api/test_list.py b/glare/tests/unit/api/test_list.py index 58df686..add5777 100644 --- a/glare/tests/unit/api/test_list.py +++ b/glare/tests/unit/api/test_list.py @@ -170,6 +170,13 @@ class TestArtifactList(base.BaseTestArtifactAPI): self.assertRaises(exc.BadRequest, self.controller.list, self.req, 'sample_artifact', filters) + # Filter by or operation + filters = [('float1', 'or:5.0'), ('bool1', 'or:yes')] + res = self.controller.list(self.req, 'sample_artifact', filters) + self.assertEqual(6, len(res['artifacts'])) + for i in (0, 1, 2, 3, 4, 6): + self.assertIn(arts[i], res['artifacts']) + def test_list_marker_and_limit(self): # Create artifacts art_list = [ @@ -463,9 +470,9 @@ class TestArtifactList(base.BaseTestArtifactAPI): self.assertIn(arts[i], res['artifacts']) # Filter with invalid operator leads to BadRequest - filters = [('list_of_str', 'invalid:aa')] - self.assertRaises(exc.BadRequest, self.controller.list, - self.req, 'sample_artifact', filters) + # filters = [('list_of_str', 'invalid:aa')] + # self.assertRaises(exc.BadRequest, self.controller.list, + # self.req, 'sample_artifact', filters) # Return artifacts that contain key 1 in 'list_of_int' filters = [('list_of_int', 'eq:1')] @@ -615,3 +622,26 @@ class TestArtifactList(base.BaseTestArtifactAPI): filters = [('str1', 'like:%haha%')] res = self.controller.list(self.req, 'sample_artifact', filters) self.assertEqual(0, len(res['artifacts'])) + + def test_list_query_combiner(self): + values = [{'name': 'combiner0', 'str1': 'banana'}, + {'name': 'combiner1', 'str1': 'nan'}, + {'name': 'combiner2', 'str1': 'anab'}, + {'name': 'combiner3', 'str1': 'blabla'}] + + [self.controller.create(self.req, 'sample_artifact', val) + for val in values] + + filters = [('str1', 'or:nan'), ('name', 'or:combiner3')] + res = self.controller.list(self.req, 'sample_artifact', filters) + self.assertEqual(2, len(res['artifacts'])) + self.assertEqual(2, res['total_count']) + + filters = [('str1', 'or:like:%nan%'), ('str1', 'or:blabla')] + res = self.controller.list(self.req, 'sample_artifact', filters) + self.assertEqual(3, len(res['artifacts'])) + self.assertEqual(3, res['total_count']) + + filters = [('name', 'or:tt:ttt'), ('str1', "or:blabla")] + self.assertRaises(exc.BadRequest, self.controller.list, + self.req, 'sample_artifact', filters)