Skip to content
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

Let SearchFilter subclasses dynamically set search fields #6279

Merged
merged 1 commit into from Feb 19, 2019

Conversation

Projects
None yet
6 participants
@allanbreyes
Copy link
Contributor

allanbreyes commented Oct 25, 2018

Description

This pull request refactors #filter_queryset and extracts a #get_search_fields method, passing in the request in as a second argument (currently unused). This allows subclasses to override this method and dynamically set search fields (vs. overriding the entire #filter_queryset method). A few example use cases:

  1. Suppose you have 5 search fields, with email being one of them. If you run the search terms through a regex and recognize an email, you can dynamically set the search fields only to email. (This avoids wasteful searches on the other 4 fields.)

  2. Suppose the default search fields are ("^book_title",). In combination with overriding the #get_search_terms method, a subclass might implement a light lexer/parser to recognize when a search term is encased in quotes, e.g. "Green Eggs and Ham" to translate to an exact search of that string on book_title. This is how Google searches (roughly) work. Otherwise, the existing behavior would do a prefix search on ['"Green", 'Eggs', 'and', 'Ham"'].

  3. Handling more complex search configurations, e.g. Google advanced search.

Thanks! ❤️ DRF!

@carltongibson
Copy link
Collaborator

carltongibson left a comment

There's the obvious "docs & tests" but this seems like a nice/small enough adjustment.

@allanbreyes

This comment has been minimized.

Copy link
Contributor Author

allanbreyes commented Oct 25, 2018

👍 Happy to add any unit tests to ensure forward compatibility for consumers who might subclass it.

@allanbreyes

This comment has been minimized.

Copy link
Contributor Author

allanbreyes commented Oct 26, 2018

I added a test that ensures that subclasses can properly inherit from both get_search_fields and get_search_terms. That would cover really complex cases (i.e. example 2) where a consumer would want to hook into both methods.

Show resolved Hide resolved tests/test_filters.py Outdated
@allanbreyes

This comment has been minimized.

Copy link
Contributor Author

allanbreyes commented Nov 21, 2018

@carltongibson @rpkilby any more thoughts on this? Thank you!

@rpkilby
Copy link
Member

rpkilby left a comment

A couple of minor nitpicks, but this otherwise looks good to me 👍

Show resolved Hide resolved rest_framework/filters.py
Show resolved Hide resolved tests/test_filters.py Outdated

@rpkilby rpkilby added this to the 3.9.1 Release milestone Nov 21, 2018

allanbreyes added a commit to allanbreyes/django-rest-framework that referenced this pull request Dec 1, 2018

Address encode#6279 code review comments
- Makes get_search_fields' request parameter required (removes default)
- Cleans up some formatting
@allanbreyes

This comment has been minimized.

Copy link
Contributor Author

allanbreyes commented Dec 1, 2018

Apologies for the delay; been a bit busy at work. Thanks for reviewing, @rpkilby!

@rpkilby

This comment has been minimized.

Copy link
Member

rpkilby commented Dec 3, 2018

Thanks!

@auvipy

auvipy approved these changes Feb 14, 2019

@carltongibson carltongibson force-pushed the allanbreyes:get_search_fields branch from d636c64 to 210240d Feb 19, 2019

@carltongibson
Copy link
Collaborator

carltongibson left a comment

Hi @allanbreyes.

Super. Thank you. Welcome aboard! 🎁

@carltongibson carltongibson merged commit d110454 into encode:master Feb 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@allanbreyes allanbreyes deleted the allanbreyes:get_search_fields branch Feb 19, 2019

@@ -218,6 +218,13 @@ For example:

By default, the search parameter is named `'search`', but this may be overridden with the `SEARCH_PARAM` setting.

To dynamically change search fields based on request content, it's possible to subclass the `SearchFilter` and override the `get_search_fields()` function. For example, the following subclass will only search on `title` if the query parameter `title_only` is in the request:

class CustomSearchFilter(self, view, request):

This comment has been minimized.

@merwok

merwok Mar 5, 2019

Contributor

Did you mean:

class CustomSearchFilter(SearchFilter):
    def get_search_fields(self, view, request):
      ...

This comment has been minimized.

@rpkilby

rpkilby Mar 6, 2019

Member

Thanks, someone else noticed this in #6487.

This comment has been minimized.

@allanbreyes

allanbreyes Mar 7, 2019

Author Contributor

Thanks for catching! Apologies for the mistake :(

search_fields = ('$title', '$text')

view = SearchListView.as_view()
request = factory.get('/', {'search': '^\w{3}$'})

This comment has been minimized.

@merwok

merwok Mar 5, 2019

Contributor

Use raw strings to avoid Python warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.