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

Implement search_fields, split_words and template properties #1169

Merged
merged 3 commits into from
Aug 12, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 59 additions & 3 deletions src/dal/views.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,17 @@
"""Base views for autocomplete widgets."""

import json
import operator
from functools import reduce

import django
from django import http
from django.contrib.admin.utils import lookup_needs_distinct
from django.contrib.auth import get_permission_codename
from django.core.exceptions import ImproperlyConfigured
from django.db.models import Q
from django.http import HttpResponseBadRequest, HttpResponseNotAllowed
from django.template.loader import render_to_string
from django.views.generic.list import BaseListView

import six
Expand Down Expand Up @@ -71,6 +76,9 @@ class BaseQuerySetView(ViewMixin, BaseListView):
context_object_name = 'results'
model_field_name = 'name'
create_field = None
search_fields = []
split_words = None
template = None

def has_more(self, context):
"""For widgets that have infinite-scroll feature."""
Expand All @@ -82,7 +90,10 @@ def get_result_value(self, result):

def get_result_label(self, result):
"""Return the label of a result."""
return six.text_type(result)
if self.template:
return render_to_string(self.template, {"result": result})
else:
return six.text_type(result)

def get_selected_result_label(self, result):
"""Return the label of a selected result."""
Expand All @@ -92,11 +103,56 @@ def get_queryset(self):
"""Filter the queryset with GET['q']."""
qs = super(BaseQuerySetView, self).get_queryset()

if self.q:
qs = qs.filter(**{'%s__icontains' % self.model_field_name: self.q})
qs = self.get_search_results(qs, self.q)

return qs

def get_search_fields(self):
"""Get the fields to search over."""
if self.search_fields:
return self.search_fields
else:
return [self.model_field_name]

def _construct_search(self, field_name):
"""Apply keyword searches."""
if field_name.startswith("^"):
return "%s__istartswith" % field_name[1:]
elif field_name.startswith("="):
return "%s__iexact" % field_name[1:]
elif field_name.startswith("@"):
return "%s__search" % field_name[1:]
else:
return "%s__icontains" % field_name
Copy link
Member

@jpic jpic Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need a code with ^, = and @ ?
Otherwise, we could just let users set search_fields = ['foo__icontains'] and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking in if you would like me to make this change so this can get merged in

I do make use of them in my application and they are supported in the
Django admin. If you prefer they be kept out of the base library, I will
amend the PR.

Copy link
Member

@jpic jpic Jul 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose both work in this case so it'd be fine except that there is no test in this patch so if you depend on it then keep in mind another contributor might break it in the future.

Are you ok taking that risk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the existing tests, they all seem to be selenium based? I am not familiar with these types of tests - is their any set up for writing some simple unit tests for the changes I made?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this code comes from django and has also had some updates:

470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  987)     def get_search_results(self, request, queryset, search_term):
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  988)         """
5411821e3b8 (Anton Samarchyan         2017-01-24 15:31:57 -0500  989)         Return a tuple containing a queryset to implement the search
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  990)         and a boolean indicating if the results may contain duplicates.
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  991)         """
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  992)         # Apply keyword searches.
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  993)         def construct_search(field_name):
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  994)             if field_name.startswith('^'):
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  995)                 return "%s__istartswith" % field_name[1:]
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  996)             elif field_name.startswith('='):
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  997)                 return "%s__iexact" % field_name[1:]
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  998)             elif field_name.startswith('@'):
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700  999)                 return "%s__search" % field_name[1:]
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1000)             # Use field_name if it includes a lookup.
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1001)             opts = queryset.model._meta
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1002)             lookup_fields = field_name.split(LOOKUP_SEP)
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1003)             # Go through the fields, following all relations.
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1004)             prev_field = None
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1005)             for path_part in lookup_fields:
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1006)                 if path_part == 'pk':
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1007)                     path_part = opts.pk.name
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1008)                 try:
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1009)                     field = opts.get_field(path_part)
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1010)                 except FieldDoesNotExist:
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1011)                     # Use valid query lookups.
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1012)                     if prev_field and prev_field.get_lookup(path_part):
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1013)                         return field_name
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1014)                 else:
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1015)                     prev_field = field
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1016)                     if hasattr(field, 'get_path_info'):
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1017)                         # Update opts to follow the relation.
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1018)                         opts = field.get_path_info()[-1].to_opts
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1019)             # Otherwise, use the field with icontains.
244cc401559 (Krzysztof Nazarewski     2017-07-05 13:00:10 +0200 1020)             return "%s__icontains" % field_name
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1021)
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1022)         use_distinct = False
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1023)         search_fields = self.get_search_fields(request)
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1024)         if search_fields and search_term:
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1025)             orm_lookups = [construct_search(str(search_field))
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1026)                            for search_field in search_fields]
26a413507ab (Alix                     2020-05-26 17:53:18 +0200 1027)             for bit in smart_split(search_term):
26a413507ab (Alix                     2020-05-26 17:53:18 +0200 1028)                 if bit.startswith(('"', "'")):
26a413507ab (Alix                     2020-05-26 17:53:18 +0200 1029)                     bit = unescape_string_literal(bit)
a8df8e34f9f (Simon Charette           2013-10-07 13:38:15 -0400 1030)                 or_queries = [models.Q(**{orm_lookup: bit})
a8df8e34f9f (Simon Charette           2013-10-07 13:38:15 -0400 1031)                               for orm_lookup in orm_lookups]
a8df8e34f9f (Simon Charette           2013-10-07 13:38:15 -0400 1032)                 queryset = queryset.filter(reduce(operator.or_, or_queries))
4c599ece57f (Дилян Палаузов           2017-12-27 00:14:12 +0530 1033)             use_distinct |= any(lookup_needs_distinct(self.opts, search_spec) for search_spec in orm_lookups)
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1034)
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1035)         return queryset, use_distinct
470a9bb22d4 (Loic Bistuer             2013-08-04 17:18:17 +0700 1036)

So maybe no need for unit testing the queryset magic because that's done in django, but worth checking if there's any update that's relevant to this patch.

Still not sure how this will play out with #1170, but it seems inheriting from the same class, it's just that apparently the view gets a declaration per-field in DAL 4.0 so either both v3 and v4 declarations will be supported either only v4, but I'd tend to go with the former and maintain BC.


def get_search_results(self, queryset, search_term):
"""Filter the results based on the query."""
search_fields = self.get_search_fields()
if search_fields and search_term:
orm_lookups = [
self._construct_search(search_field) for search_field in search_fields
]
if self.split_words is not None:
word_conditions = []
for word in search_term.split():
or_queries = [Q(**{orm_lookup: word}) for orm_lookup in orm_lookups]
word_conditions.append(reduce(operator.or_, or_queries))
op_ = operator.or_ if self.split_words == "or" else operator.and_
queryset = queryset.filter(reduce(op_, word_conditions))
else:
or_queries = [
Q(**{orm_lookup: search_term}) for orm_lookup in orm_lookups
]
queryset = queryset.filter(reduce(operator.or_, or_queries))

if any(
lookup_needs_distinct(queryset.model._meta, search_spec)
for search_spec in orm_lookups
):
queryset = queryset.distinct()

return queryset

def create_object(self, text):
"""Create an object given a text."""
return self.get_queryset().get_or_create(
Expand Down