Implement search_fields, split_words and template properties #1169

mitchelljkotler · 2020-07-16T13:43:05Z

Implements search_fields and split_words as they were in v2 and discussed in #783

Also implements simple template support, similar to v2.

Having the filtering logic be in its own method, get_search_results is important to allow for subclasses to override the filtering logic for things such as forwards, without losing the ability to pick up on model or queryset attribute from BaseListView's get_queryset method.

Close yourlabs#783

jpic · 2020-07-17T07:55:39Z

src/dal/views.py

+        elif field_name.startswith("@"):
+            return "%s__search" % field_name[1:]
+        else:
+            return "%s__icontains" % field_name


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

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.

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 ?

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?

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.

mitchelljkotler · 2020-07-17T12:21:43Z

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.

…

On Fri, Jul 17, 2020 at 3:55 AM jpic ∞ ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/dal/views.py <#1169 (comment)> : > + """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 Do we really need a code with ^, = and @ ? Otherwise, we could just let users set search_fields = foo__icontains and so on. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1169 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABBHIQINQA3L6BBF5FF5PTR377YVANCNFSM4O4I3ZZA> .

jpic · 2020-07-22T02:58:50Z

src/dal/views.py

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


Is this calling the same distinct() for every search_spec for every search_field ?

New code by Дилян Палаузов in 2017 in Django seems to perform much better.

It should not - once it finds one that requires distinct, the code breaks the loop. I have updated it to use the new code since you say it performs better.

DALv4 will be backward compatible? I am just upgrading to v3 now, I hope I will not need to do a big update again.

DAL V4 is compatible

Taken from Django code

jpic · 2020-10-12T12:07:35Z

Thank you deeply for your contribution, I'm releasing this in 3.7.0 right now, would you mind adding a note about this in the tutorial.rst file so it can be useful for others too please ?

jpic · 2020-11-10T10:16:18Z

This introduces bugs, I'll revert it unless somebody steps up to maintain it

#1190

mitchelljkotler · 2020-11-10T15:00:39Z

That looks like a bug in SuerySetSequence not properly implementing filter, unless I am missing something

jpic · 2020-11-17T16:18:01Z

Thank you for your fast response ! Do you think you can find a way to disable this new feature for QuerySetSequenceAutocompleteView ? Create a new class or something? It would be really awesome to make a bugfix release soonish ;)

jpic · 2020-11-18T14:53:14Z

Got something in #1199 but it depends on a contribution to django-querysetsequence, let's hope they release something soonish.

mitchelljkotler force-pushed the master branch from b5f051b to 12183cf Compare July 16, 2020 15:12

mitchelljkotler added 2 commits July 16, 2020 11:25

Add support for search_fields and split_words

fb50f79

Close yourlabs#783

Add support for template property

a747f0e

mitchelljkotler force-pushed the master branch from 12183cf to a747f0e Compare July 16, 2020 15:25

jpic reviewed Jul 17, 2020

View reviewed changes

jpic reviewed Jul 22, 2020

View reviewed changes

More efficient distinct checking

d1984d4

Taken from Django code

jpic merged commit 308befa into yourlabs:master Aug 12, 2020

jpic mentioned this pull request Nov 18, 2020

Support filter(*args) clokep/django-querysetsequence#76

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement search_fields, split_words and template properties #1169

Implement search_fields, split_words and template properties #1169

mitchelljkotler commented Jul 16, 2020

jpic Jul 17, 2020 •

edited

Loading

mitchelljkotler Jul 21, 2020

jpic Jul 21, 2020 •

edited

Loading

mitchelljkotler Jul 21, 2020

jpic Jul 22, 2020

mitchelljkotler commented Jul 17, 2020 via email

jpic Jul 22, 2020

mitchelljkotler Jul 23, 2020

jpic Aug 12, 2020

jpic commented Oct 12, 2020

jpic commented Nov 10, 2020

mitchelljkotler commented Nov 10, 2020

jpic commented Nov 17, 2020 via email

jpic commented Nov 18, 2020

Implement search_fields, split_words and template properties #1169

Implement search_fields, split_words and template properties #1169

Conversation

mitchelljkotler commented Jul 16, 2020

jpic Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

mitchelljkotler Jul 21, 2020

Choose a reason for hiding this comment

jpic Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

mitchelljkotler Jul 21, 2020

Choose a reason for hiding this comment

jpic Jul 22, 2020

Choose a reason for hiding this comment

mitchelljkotler commented Jul 17, 2020 via email

jpic Jul 22, 2020

Choose a reason for hiding this comment

mitchelljkotler Jul 23, 2020

Choose a reason for hiding this comment

jpic Aug 12, 2020

Choose a reason for hiding this comment

jpic commented Oct 12, 2020

jpic commented Nov 10, 2020

mitchelljkotler commented Nov 10, 2020

jpic commented Nov 17, 2020 via email

jpic commented Nov 18, 2020

jpic Jul 17, 2020 •

edited

Loading

jpic Jul 21, 2020 •

edited

Loading