Align SearchFilter behaviour to django.contrib.admin search #9017

sevdog · 2023-06-22T09:59:24Z

Description

Make SearchFilter behave more like django.contrib.admin search (which is already stated in docs).

Allow use any lookup by appending it the fields in search_fields
Perform a query-only distinct filtering
Handle quotes surrounded terms with escapes (ie: "the text", "the \"text\"")
Split by spaces only by spaces, considering quoteing (breaking change)
Raise a validation error if any null-character is provided in search (breaking change)

See #8205

auvipy · 2023-06-23T02:54:37Z

looks like a nice improvment

tests/test_filters.py

auvipy · 2023-06-23T03:05:56Z

can we avoid at least one breaking change

Split by spaces only by spaces, considering quoteing (breaking change) --- and keep both? if it is not too hard?

sevdog · 2023-06-23T07:12:52Z

can we avoid at least one breaking change

Split by spaces only by spaces, considering quoteing (breaking change) --- and keep both? if it is not too hard?

This could be interesting, however it should be defined the order of split logics due to how smart_split works and which kind of user experience is expected. Just to give an example, consider the following search:

'"hello, world" can I,help you'

With only smart_split this becomes

['hello, world', 'can', 'I,help', 'you']

Maybe a good compromise would be:

if comma is inside quotes, do not split
otherwhise split

With this logic the first search whould become

['hello, world', 'can', 'I' , 'help', 'you']

There is an edge case: what to do if there are one or more commas before or after a quoted sentence?

',"hello, world", what to,,do,?'

I belive that in this case it would be nice to strip commas from tokens extracted by smart_split and then yield quoted sentences as the are and split other by comma.

The generator to handle this iteration whould be something like:

def search_smart_split(search_terms):
    for term in smart_split(search_terms):
        term = term.strip(',')
        if term.startswith(('"', "'")) and term[0] == term[-1]:
            yield unescape_string_literal(term)
        else:
            yield from (sub_term for sub_term in term.split(',') if sub_term)

This should parse the latter term into:

['hello, world', 'what', 'to', 'do', '?']

sevdog · 2023-06-23T07:37:34Z

Testing the previous approach I encountered an other case which I did not considered before. What to do when there is a quoted phrase followed by other characters:

'"hello, world",found'

The smart_split keeps this phrase togheter, so splitting by comma results in

['"hello', ' world"', 'found']

The whitespace is easy and intuitive to remove (IMO), but I am not sure if in this case also the quotes should be removed (thus using .split(' \'"') instead of .split()).

auvipy · 2023-06-24T06:07:05Z

are we going to preserve the old way or going to have option for both new and old ways?

sevdog · 2023-06-28T06:18:03Z

are we going to preserve the old way or going to have option for both new and old ways?

It depends on which mechanism are we willing to provide to select this behaviour. I can think these:

an attribute on filter class
an attribute on view
a settings (which would be global for every view/filter)

auvipy · 2023-07-13T05:31:08Z

from the tests it seems that both type of search is supported in this PR, right? I would like to consider this improvement for 3.15 release

sevdog · 2023-07-13T06:55:30Z

from the tests it seems that both type of search is supported in this PR, right? I would like to consider this improvement for 3.15 release

Yes, search_smart_split does first the split like django.contrib.admin and then splits by comma. The only case in which this would give different results is where the search input is a mixed combination of quoted strings and commas. A simple CSV vaue (ie: an,example,search) would be splitted like it was before.

auvipy · 2023-07-16T06:02:48Z

rest_framework/compat.py

@@ -14,13 +13,6 @@ def unicode_http_header(value):
    return value


-def distinct(queryset, base):


can we change this in a separate PR? or it is part of the change in this PR?

It could be splitted, however this method was introduced in b4b2dc1 only for the search class and putted into compact package in the hope of being re-used.

IMO this should be kept togheter with the rest of the PR since this change goes in the same direction of "align the behaviour with django".

I just noticed that the code I used (which was introduced in django/django@1871182) has just been removed from the main branch in django/django@d569c1d. So the whole thing may just be changed to use .distinct() like django is going to do.

PS: I really do not understand why 8 years ago there was the need to have a different implementation of distinct only for Oracle engine and why this was implemented here.

PS: I really do not understand why 8 years ago there was the need to have a different implementation of distinct only for Oracle engine and why this was implemented here.

at that time django oracle back end was not properly maintained. but now it is

auvipy · 2023-07-16T06:03:12Z

docs/api-guide/filtering.md

 For [JSONField][JSONField] and [HStoreField][HStoreField] fields you can filter based on nested values within the data structure using the same double-underscore notation:

    search_fields = ['data__breed', 'data__owner__other_pets__0__name']

-By default, searches will use case-insensitive partial matches.  The search parameter may contain multiple search terms, which should be whitespace and/or comma separated.  If multiple search terms are used then objects will be returned in the list only if all the provided terms are matched.
+By default, searches will use case-insensitive partial matches.  The search parameter may contain multiple search terms, which should be whitespace separated.  If multiple search terms are used then objects will be returned in the list only if all the provided terms are matched.


do we need further enhancement of the docs?

I will add some examples about search term and how it will get parsed.

docs/api-guide/filtering.md

auvipy · 2023-07-25T07:42:01Z

do you intend to add anything else here or improve anything else?

sevdog · 2023-07-25T10:42:12Z

As of now I belive that this should be fine.

I tried to figure out what could have been added to the docs, but I felt that it was just copy-pasting fron django admin docs without adding any value.

) * Use subquery to remove duplicates in SearchFilter * Align SearchFilter behaviour to django.contrib.admin * Add compatibility with older django/python versions * Allow search to split also by comma after smart split * Use generator to build search conditions to reduce iterations * Improve search documentation * Update docs/api-guide/filtering.md --------- Co-authored-by: Asif Saif Uddin <[email protected]>

tomchristie · 2024-03-22T12:21:23Z

Really nice bit of functionality, thanks... have followed up with #9338.

sevdog force-pushed the search-filter branch from 7736f40 to 1f0f0ad Compare June 22, 2023 10:07

auvipy self-requested a review June 23, 2023 02:51

auvipy requested changes Jun 23, 2023

View reviewed changes

tests/test_filters.py Outdated Show resolved Hide resolved

sevdog force-pushed the search-filter branch from d69551a to a74ed9f Compare June 23, 2023 06:49

auvipy requested a review from a team June 24, 2023 06:05

auvipy mentioned this pull request Jun 27, 2023

Fix: SearchFilter renders search field with invalid value #9023

Merged

sevdog added 4 commits July 3, 2023 11:26

Use subquery to remove duplicates in SearchFilter

dee83ce

Align SearchFilter behaviour to django.contrib.admin

ad96159

Add compatibility with older django/python versions

82c42dc

Allow search to split also by comma after smart split

1ce85d0

sevdog force-pushed the search-filter branch from 9d0858c to 1ce85d0 Compare July 3, 2023 16:19

Use generator to build search conditions to reduce iterations

a7bafb7

auvipy added this to the 3.15 milestone Jul 13, 2023

auvipy requested changes Jul 16, 2023

View reviewed changes

Improve search documentation

67cd9c2

auvipy requested changes Jul 25, 2023

View reviewed changes

docs/api-guide/filtering.md Outdated Show resolved Hide resolved

Update docs/api-guide/filtering.md

140b398

auvipy self-requested a review July 25, 2023 07:37

auvipy approved these changes Jul 25, 2023

View reviewed changes

auvipy added the Enhancement label Jul 25, 2023

auvipy merged commit b99df0c into encode:master Jul 25, 2023

mjsir911 mentioned this pull request Feb 6, 2024

Initial Release Notes for 3.15 #8794

Closed

This was referenced Mar 21, 2024

3.15 backward compatibility issue with 3.14 - rest_framework.filters.SearchFilter.get_search_terms returns str instead of list #9308

Closed

SearchFilter.get_search_terms returns list. #9338

Merged

sevdog deleted the search-filter branch March 22, 2024 13:58

This was referenced Apr 13, 2024

[3.15] Add construct_search function queryset argument for SearchFilter typeddjango/djangorestframework-stubs#574

Merged

[3.15] Remove compat.distinct function typeddjango/djangorestframework-stubs#579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align SearchFilter behaviour to django.contrib.admin search #9017

Align SearchFilter behaviour to django.contrib.admin search #9017

sevdog commented Jun 22, 2023

auvipy commented Jun 23, 2023

auvipy commented Jun 23, 2023

sevdog commented Jun 23, 2023 •

edited

Loading

sevdog commented Jun 23, 2023

auvipy commented Jun 24, 2023

sevdog commented Jun 28, 2023

auvipy commented Jul 13, 2023

sevdog commented Jul 13, 2023

auvipy Jul 16, 2023

sevdog Jul 17, 2023

auvipy Jul 25, 2023

auvipy Jul 16, 2023

sevdog Jul 17, 2023

auvipy commented Jul 25, 2023

sevdog commented Jul 25, 2023

tomchristie commented Mar 22, 2024

		@@ -14,13 +13,6 @@ def unicode_http_header(value):
		return value


		def distinct(queryset, base):

Align SearchFilter behaviour to django.contrib.admin search #9017

Align SearchFilter behaviour to django.contrib.admin search #9017

Conversation

sevdog commented Jun 22, 2023

Description

auvipy commented Jun 23, 2023

auvipy commented Jun 23, 2023

sevdog commented Jun 23, 2023 • edited Loading

sevdog commented Jun 23, 2023

auvipy commented Jun 24, 2023

sevdog commented Jun 28, 2023

auvipy commented Jul 13, 2023

sevdog commented Jul 13, 2023

auvipy Jul 16, 2023

Choose a reason for hiding this comment

sevdog Jul 17, 2023

Choose a reason for hiding this comment

auvipy Jul 25, 2023

Choose a reason for hiding this comment

auvipy Jul 16, 2023

Choose a reason for hiding this comment

sevdog Jul 17, 2023

Choose a reason for hiding this comment

auvipy commented Jul 25, 2023

sevdog commented Jul 25, 2023

tomchristie commented Mar 22, 2024

sevdog commented Jun 23, 2023 •

edited

Loading