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

changed behaviour for __contains queries and multiple words in search-term #39

Closed
syphar opened this issue Feb 23, 2021 · 8 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@syphar
Copy link

syphar commented Feb 23, 2021

I'm creating this as a bug, while of course this could be also an intentional (breaking) change to the library. If that's the case I would work around it.

Beginning with version 7.6.0 there was a change in search behaviour, originating in in this commit.

The example is

  • search_fields = {'title__icontains'} and
  • using ModelSelect2Widget as base for the widget

Now I'm searching for two separate words.

The old behavior (7.5.0 and earlier) was that both words needed to be in the title, so the term was split into the words, while combining the resulting two __icontains searches with AND. So in the result for the selectbox we only get the records which contain both words in the title.

New behaviour since 7.6.0 is different, here we also split, but the searches are combined with OR.
So in the result we have all records that have either one OR the other word in the title

Expected behavior
I would have expected the search-results to be the same. While I understand the breaking change in the release, I don't see why this specific behaviour was changed.

When we take the default django-admin-search as an example (also the base for autocomplete_fields), we find:

https://github.com/django/django/blob/3668da8de821fcb4ddd83fb50a1f04aa02600894/django/contrib/admin/options.py#L1020-L1030

        use_distinct = False
        search_fields = self.get_search_fields(request)
        if search_fields and search_term:
            orm_lookups = [construct_search(str(search_field))
                           for search_field in search_fields]
            for bit in search_term.split():
                or_queries = [models.Q(**{orm_lookup: bit})
                              for orm_lookup in orm_lookups]
                queryset = queryset.filter(reduce(operator.or_, or_queries))
            use_distinct |= any(lookup_needs_distinct(self.opts, search_spec) for search_spec in orm_lookups)

Which means, django admin also does:

  • use OR for connecting the searches by field,
  • but is uses AND for the search for multiple words in the search-term.

possible solution

If you see this also as a bug, I'm happy to work on a fix for it. Otherwise I would try to work around this behaviour.

@syphar syphar added the bug Something isn't working label Feb 23, 2021
@syphar syphar changed the title changed behaviour for __contains queries and multiple words in search-term changed behaviour for __contains queries and multiple words in search-term Feb 23, 2021
@syphar syphar changed the title changed behaviour for __contains queries and multiple words in search-term changed behaviour for __contains queries and multiple words in search-term Feb 23, 2021
@syphar
Copy link
Author

syphar commented Mar 2, 2021

@codingjoe could you make a call on which direction you do want to go?

I'm happy to provide a PR, if you also see this as bug

@codingjoe
Copy link
Owner

OK, how about we replicate the same behavior that search_fields in Django admin provides? I would certainly welcome a PR from you.

@syphar
Copy link
Author

syphar commented Mar 4, 2021

@codingjoe one question, thinking about this:

So, when the goal is to be more similar to the django admin search, I would have to revert the change in 07054b2.

Or should I only fix the AND/OR part for contains queries, and leave the difference to admin as is for startswith and endswith?

@codingjoe
Copy link
Owner

I don't want to just revert the commit, since it added tests, which are great even if they test for the wrong thing. I will create a patch really quick.

codingjoe added a commit that referenced this issue Mar 6, 2021
Different to before we now match search terms only if all
bits match or the entire statement. The last part differs
from the behavior in Django admin, which does not inlcude
exact machtes of the entire search term inlcude spaces.

Partially revert 07054b2
codingjoe added a commit that referenced this issue Mar 6, 2021
Different to before we now match search terms only if all
bits match or the entire statement. The last part differs
from the behavior in Django admin, which does not inlcude
exact machtes of the entire search term inlcude spaces.

Partially revert 07054b2
@syphar
Copy link
Author

syphar commented Mar 6, 2021

@codingjoe I'm happy to help if there is anything I can do

@codingjoe
Copy link
Owner

You can review #40

@syphar
Copy link
Author

syphar commented Mar 8, 2021

@codingjoe you mean #43 ?

@codingjoe
Copy link
Owner

yes ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants