-
Notifications
You must be signed in to change notification settings - Fork 345
[ENG-8052] Fixed FilterMixin issue with multiple values of any filter #11150
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ENG-8052] Fixed FilterMixin issue with multiple values of any filter #11150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly just concerned about database performance.
api/base/filters.py
Outdated
@@ -292,7 +292,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 source_field_name in ['_id', 'guid._id', 'journal_id', 'moderation_state']: | |||
elif not isinstance(value, int) and ',' in value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned on Slack, let's just add the field we want to explicitly filter on here rather than opening it up to all fields. Too open may cause people to be able to easily construct complex queries that would cause us database problems without having a clear use case for us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, okay
api/base/filters.py
Outdated
@@ -505,7 +505,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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, you mentioned that in your Changes section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it was broking one value filtering
api_tests/base/test_filters.py
Outdated
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably you won't need these specific tests with the change I requested, but you might add one for the notification subscriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good when CI passes.
883b530
into
CenterForOpenScience:feature/pbs-25-10
…cience/osf.io into refactor-notifications * 'feature/pbs-25-10' of https://github.com/CenterForOpenScience/osf.io: fix issue where trying another already confirmed email threw an uncaught exception (CenterForOpenScience#11161) [ENG-8148] Add ArtifactOutcome in annotations to linked nodes (CenterForOpenScience#11158) [ENG-7966] Add "collected-in" relationship for Nodes (CenterForOpenScience#11140) fix issue where not having any external identities caused a 500 [ENG-7965] Add v2 email token confirmation endpoints (CenterForOpenScience#11139) [ENG-8052] Fixed FilterMixin issue with multiple values of notification subscription field (CenterForOpenScience#11150) support related_counts for view_only links (CenterForOpenScience#11148) allow admins change registration providers (CenterForOpenScience#11145) [ENG-7927] Improved logging for embargo termination (CenterForOpenScience#11137) [ENG-7873] CLONE - SPAM - When Hamming a Spammed user, preprints and registrations remain private (CenterForOpenScience#11125) Update changelog and package.json fix TypeError when check stucked registration revert async email sending (CenterForOpenScience#11134) [ENG-7921] Add scopes for applications to full_read and full_write scopes (CenterForOpenScience#11126) # Conflicts: # api_tests/nodes/views/test_node_detail.py # api_tests/nodes/views/test_node_linked_registrations.py # framework/auth/oauth_scopes.py # tests/test_registrations/test_retractions.py
…on subscription field (CenterForOpenScience#11150)
Purpose
Query params filters were limited to handle multiple values starting from this commit 6fd1968 for these fields only:
_id
,guid._id
,journal_id
,moderation_state
. Over time this list have been updated to these four values. Now all filter classes that inherit fromFilterMixin
support this feature.In order to filter by multiple values, FE should use the following syntax:
?filter[title]=text1,text2
Filter uses OR operator for its values while filters use AND operator between themselves
Changes
id
field inListFilterMixin.postprocess_query_param
so thatin
lookup is not used for oneid
that is actually string.Concerns
__in
lookup for fields in these filters instead of__icontains
(which was wrong astitle1,title2
was passed as one value). It doesn't impact any filtering that FE does using internal values they get from BE, so for the case in the ticket it'll work. But do we have filtering on the platform where user can enter text manually that will be passed as a query parameter together with some filter? If user enters "Dev" and FE filters like?filter['title']=Dev
and it should find all records that contain this word, I assume it won't work. It'll actually require refactoring filters.Ticket
https://openscience.atlassian.net/jira/software/c/projects/ENG/boards/145?selectedIssue=ENG-8052