From 8f5e5a8179dc2ed716c17a011e3df32ebc5a83b2 Mon Sep 17 00:00:00 2001 From: Ihor Sokhan Date: Mon, 19 May 2025 19:47:28 +0300 Subject: [PATCH 1/6] fixed FilterMixin issue with multiple values of any filter --- api/base/filters.py | 4 +-- api_tests/base/test_filters.py | 62 +++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/api/base/filters.py b/api/base/filters.py index 4074ed0374c..14a002406bf 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -292,7 +292,8 @@ def parse_query_params(self, query_params): query.get(key).update({ field_name: self._parse_date_param(field, source_field_name, op, value), }) - elif not isinstance(value, int) and source_field_name in ['_id', 'guid._id', 'journal_id', 'moderation_state']: + # elif not isinstance(value, int) and source_field_name in ['_id', 'guid._id', 'journal_id', 'moderation_state']: + elif not isinstance(value, int) and ',' in value: query.get(key).update({ field_name: { 'op': 'in', @@ -505,7 +506,6 @@ def postprocess_query_param(self, key, field_name, operation): if issubclass(self.model_class, GuidMixin) else self.model_class.primary_identifier_name ) - operation['op'] = 'in' if field_name == 'subjects': self.postprocess_subject_query_param(operation) diff --git a/api_tests/base/test_filters.py b/api_tests/base/test_filters.py index 7a61a7d7c11..9de7b642baf 100644 --- a/api_tests/base/test_filters.py +++ b/api_tests/base/test_filters.py @@ -85,6 +85,7 @@ def __init__( class FakeListView(ListFilterMixin, generics.GenericAPIView): serializer_class = FakeSerializer + model_class = FakeRecord def get_serializer_context(self): return {} @@ -378,13 +379,72 @@ def test_parse_query_params_uses_field_source_attribute(self): query_params = { 'filter[bool_field]': 'false', } - fields = self.view.parse_query_params(query_params) parsed_field = fields['filter[bool_field]']['bool_field'] assert parsed_field['source_field_name'] == 'foobar' assert parsed_field['value'] is False assert parsed_field['op'] == 'eq' + def test_multiple_values_filter(self): + user = AuthUserFactory() + node1 = NodeFactory(title='title1', creator=user) + node2 = NodeFactory(title='title2', creator=user) + node3 = NodeFactory(title='title3', creator=user) + + url = f'/{API_BASE}users/{user._id}/nodes/' + res = self.app.get(url, auth=user.auth) + assert len(res.json['data']) == 3 + + # one filter, one value + res = self.app.get(url + '?filter[title]=title2', auth=user.auth) + assert len(res.json['data']) == 1 + assert res.json['data'][0]['attributes']['title'] == 'title2' + + # one filter, two values + res = self.app.get(url + '?filter[title]=title3,title2', auth=user.auth) + assert len(res.json['data']) == 2 + for node in res.json['data']: + assert node['attributes']['title'] in ['title3', 'title2'] + + # one filter, all DB values + res = self.app.get(url + '?filter[title]=title1,title3,title2', auth=user.auth) + assert len(res.json['data']) == 3 + for node in res.json['data']: + assert node['attributes']['title'] in ['title3', 'title2', 'title1'] + + # two filters, different number of values. Filters don't intersect by nodes + res = self.app.get(url + f'?filter[title]=title3,title2&filter[id]={node1._id}', auth=user.auth) + assert len(res.json['data']) == 0 + + # two filters, different filters ordering but intersect by node2 + # node3 is not returned as filters use AND operator between themselves + res = self.app.get(url + f'?filter[id]={node2._id}&filter[title]=title3,title2', auth=user.auth) + assert len(res.json['data']) == 1 + assert res.json['data'][0]['id'] == node2._id + + # two filters, two values of the same entities + res = self.app.get(url + f'?filter[title]=title3,title1&filter[id]={node1._id},{node3._id}', auth=user.auth) + assert len(res.json['data']) == 2 + for node in res.json['data']: + assert node['id'] in [node1._id, node3._id] + assert node['attributes']['title'] in ['title3', 'title1'] + + # two filters, one of them has an unexisting value, node1 in both filters, node3 is only in the second filter + # thus shouldn't be returned + res = self.app.get(url + f'?filter[title]=wrongtitle_____,title1&filter[id]={node1._id},{node3._id}', auth=user.auth) + assert len(res.json['data']) == 1 + assert res.json['data'][0]['id'] == node1._id + assert res.json['data'][0]['attributes']['title'] == 'title1' + + # two filters whose values refer to different entities, thus no intersection and no values returned + res = self.app.get(url + f'?filter[title]=title1,title4&filter[id]={node2._id},{node3._id}', auth=user.auth) + assert len(res.json['data']) == 0 + + # two filters, all values aren't present in DB + res = self.app.get(url + '?filter[title]=wrong1,wrong2&filter[id]=guid1,guid2,guid999', auth=user.auth) + assert len(res.json['data']) == 0 + + @pytest.mark.django_db class TestOSFOrderingFilter(ApiTestCase): class query: From 8e4d6a381e88e16d8e831347718588b5862085cc Mon Sep 17 00:00:00 2001 From: Ihor Sokhan Date: Mon, 19 May 2025 19:52:03 +0300 Subject: [PATCH 2/6] remove comment --- api/base/filters.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api/base/filters.py b/api/base/filters.py index 14a002406bf..f9b5a5f322d 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -292,7 +292,6 @@ def parse_query_params(self, query_params): query.get(key).update({ field_name: self._parse_date_param(field, source_field_name, op, value), }) - # elif not isinstance(value, int) and source_field_name in ['_id', 'guid._id', 'journal_id', 'moderation_state']: elif not isinstance(value, int) and ',' in value: query.get(key).update({ field_name: { From f8083d862a72c2a3313eed3955eca59acc46b8d5 Mon Sep 17 00:00:00 2001 From: Ihor Sokhan Date: Mon, 19 May 2025 20:20:25 +0300 Subject: [PATCH 3/6] removed redundant attribute --- api_tests/base/test_filters.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api_tests/base/test_filters.py b/api_tests/base/test_filters.py index 9de7b642baf..39628d91002 100644 --- a/api_tests/base/test_filters.py +++ b/api_tests/base/test_filters.py @@ -85,7 +85,6 @@ def __init__( class FakeListView(ListFilterMixin, generics.GenericAPIView): serializer_class = FakeSerializer - model_class = FakeRecord def get_serializer_context(self): return {} From 614d56595b24fe2250b5753c134112460d7281cb Mon Sep 17 00:00:00 2001 From: Ihor Sokhan Date: Mon, 19 May 2025 20:59:11 +0300 Subject: [PATCH 4/6] limit fields that support filtering by multiple values --- api/base/filters.py | 5 +- api_tests/base/test_filters.py | 59 ------------------- .../views/test_subscriptions_list.py | 6 ++ 3 files changed, 10 insertions(+), 60 deletions(-) diff --git a/api/base/filters.py b/api/base/filters.py index f9b5a5f322d..61f0a2dd36c 100644 --- a/api/base/filters.py +++ b/api/base/filters.py @@ -160,6 +160,8 @@ class FilterMixin: LIST_FIELDS = (ser.ListField,) RELATIONSHIP_FIELDS = (RelationshipField, TargetField) + MULTIPLE_VALUES_FIELDS = ['_id', 'guid._id', 'journal_id', 'moderation_state', 'event_name'] + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) if not self.serializer_class: @@ -292,7 +294,7 @@ def parse_query_params(self, query_params): query.get(key).update({ field_name: self._parse_date_param(field, source_field_name, op, value), }) - elif not isinstance(value, int) and ',' in value: + elif not isinstance(value, int) and source_field_name in self.MULTIPLE_VALUES_FIELDS: query.get(key).update({ field_name: { 'op': 'in', @@ -505,6 +507,7 @@ def postprocess_query_param(self, key, field_name, operation): if issubclass(self.model_class, GuidMixin) else self.model_class.primary_identifier_name ) + operation['op'] = 'in' if field_name == 'subjects': self.postprocess_subject_query_param(operation) diff --git a/api_tests/base/test_filters.py b/api_tests/base/test_filters.py index 39628d91002..ef98a209503 100644 --- a/api_tests/base/test_filters.py +++ b/api_tests/base/test_filters.py @@ -384,65 +384,6 @@ def test_parse_query_params_uses_field_source_attribute(self): assert parsed_field['value'] is False assert parsed_field['op'] == 'eq' - def test_multiple_values_filter(self): - user = AuthUserFactory() - node1 = NodeFactory(title='title1', creator=user) - node2 = NodeFactory(title='title2', creator=user) - node3 = NodeFactory(title='title3', creator=user) - - url = f'/{API_BASE}users/{user._id}/nodes/' - res = self.app.get(url, auth=user.auth) - assert len(res.json['data']) == 3 - - # one filter, one value - res = self.app.get(url + '?filter[title]=title2', auth=user.auth) - assert len(res.json['data']) == 1 - assert res.json['data'][0]['attributes']['title'] == 'title2' - - # one filter, two values - res = self.app.get(url + '?filter[title]=title3,title2', auth=user.auth) - assert len(res.json['data']) == 2 - for node in res.json['data']: - assert node['attributes']['title'] in ['title3', 'title2'] - - # one filter, all DB values - res = self.app.get(url + '?filter[title]=title1,title3,title2', auth=user.auth) - assert len(res.json['data']) == 3 - for node in res.json['data']: - assert node['attributes']['title'] in ['title3', 'title2', 'title1'] - - # two filters, different number of values. Filters don't intersect by nodes - res = self.app.get(url + f'?filter[title]=title3,title2&filter[id]={node1._id}', auth=user.auth) - assert len(res.json['data']) == 0 - - # two filters, different filters ordering but intersect by node2 - # node3 is not returned as filters use AND operator between themselves - res = self.app.get(url + f'?filter[id]={node2._id}&filter[title]=title3,title2', auth=user.auth) - assert len(res.json['data']) == 1 - assert res.json['data'][0]['id'] == node2._id - - # two filters, two values of the same entities - res = self.app.get(url + f'?filter[title]=title3,title1&filter[id]={node1._id},{node3._id}', auth=user.auth) - assert len(res.json['data']) == 2 - for node in res.json['data']: - assert node['id'] in [node1._id, node3._id] - assert node['attributes']['title'] in ['title3', 'title1'] - - # two filters, one of them has an unexisting value, node1 in both filters, node3 is only in the second filter - # thus shouldn't be returned - res = self.app.get(url + f'?filter[title]=wrongtitle_____,title1&filter[id]={node1._id},{node3._id}', auth=user.auth) - assert len(res.json['data']) == 1 - assert res.json['data'][0]['id'] == node1._id - assert res.json['data'][0]['attributes']['title'] == 'title1' - - # two filters whose values refer to different entities, thus no intersection and no values returned - res = self.app.get(url + f'?filter[title]=title1,title4&filter[id]={node2._id},{node3._id}', auth=user.auth) - assert len(res.json['data']) == 0 - - # two filters, all values aren't present in DB - res = self.app.get(url + '?filter[title]=wrong1,wrong2&filter[id]=guid1,guid2,guid999', auth=user.auth) - assert len(res.json['data']) == 0 - @pytest.mark.django_db class TestOSFOrderingFilter(ApiTestCase): diff --git a/api_tests/subscriptions/views/test_subscriptions_list.py b/api_tests/subscriptions/views/test_subscriptions_list.py index cda043314b1..f1131b1fa72 100644 --- a/api_tests/subscriptions/views/test_subscriptions_list.py +++ b/api_tests/subscriptions/views/test_subscriptions_list.py @@ -54,3 +54,9 @@ def test_cannot_post_patch_put_or_delete(self, app, url, user): assert patch_res.status_code == 405 assert put_res.status_code == 405 assert delete_res.status_code == 405 + + def test_multiple_values_filter(self, app, url, global_user_notification, user): + res = app.get(url + '?filter[event_name]=comments,global', auth=user.auth) + assert len(res.json['data']) == 2 + for subscription in res.json['data']: + subscription['attributes']['event_name'] in ['global', 'comments'] From eae48c5c9c903eb1164bb45d9fda2e635c064ca8 Mon Sep 17 00:00:00 2001 From: Ihor Sokhan Date: Mon, 19 May 2025 21:00:02 +0300 Subject: [PATCH 5/6] revert --- api_tests/base/test_filters.py | 1 + 1 file changed, 1 insertion(+) diff --git a/api_tests/base/test_filters.py b/api_tests/base/test_filters.py index ef98a209503..c89771e07f9 100644 --- a/api_tests/base/test_filters.py +++ b/api_tests/base/test_filters.py @@ -378,6 +378,7 @@ def test_parse_query_params_uses_field_source_attribute(self): query_params = { 'filter[bool_field]': 'false', } + fields = self.view.parse_query_params(query_params) parsed_field = fields['filter[bool_field]']['bool_field'] assert parsed_field['source_field_name'] == 'foobar' From 318d9ef06fd180b7ea816c57987059133ae12284 Mon Sep 17 00:00:00 2001 From: Ihor Sokhan Date: Mon, 19 May 2025 21:00:25 +0300 Subject: [PATCH 6/6] revert --- api_tests/base/test_filters.py | 1 - 1 file changed, 1 deletion(-) diff --git a/api_tests/base/test_filters.py b/api_tests/base/test_filters.py index c89771e07f9..7a61a7d7c11 100644 --- a/api_tests/base/test_filters.py +++ b/api_tests/base/test_filters.py @@ -385,7 +385,6 @@ def test_parse_query_params_uses_field_source_attribute(self): assert parsed_field['value'] is False assert parsed_field['op'] == 'eq' - @pytest.mark.django_db class TestOSFOrderingFilter(ApiTestCase): class query: