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

Fixed #26184 -- Allowed custom lookups and transforms in ModelAdmin.search_fields. #8704

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nazarewk
Contributor

nazarewk commented Jul 5, 2017

Adds ! operator to search_fields allowing for specification of custom lookups.

https://code.djangoproject.com/ticket/26184

@BertrandBordage

Not a huge fan of having yet another weird “escaping” character, but given the existing system, I guess that’s better than nothing.

Ideally, a class-based system would be better in my opinion, it would be way cleaner & more extendable. Something like:

search_fields = [Exact('first_name', case_insensitive=True), Search('username'), Startswith('group__name')]

But anyway, that pull request is fine for now, thanks for making it :)

@schinckel

This comment has been minimized.

Show comment
Hide comment
@schinckel

schinckel Jul 5, 2017

Contributor

I wonder if just returning field_name, if it contains __ (or something similar) would give satisfactory results (meaning you can use arbitrary lookups/transforms).

Contributor

schinckel commented Jul 5, 2017

I wonder if just returning field_name, if it contains __ (or something similar) would give satisfactory results (meaning you can use arbitrary lookups/transforms).

@nazarewk

This comment has been minimized.

Show comment
Hide comment
@nazarewk

nazarewk Jul 5, 2017

Contributor

@schinckel i'm not sure about others, but for me it is quite common to have stuff like event__name in search_fields, also my approach is backwards compatible.

Contributor

nazarewk commented Jul 5, 2017

@schinckel i'm not sure about others, but for me it is quite common to have stuff like event__name in search_fields, also my approach is backwards compatible.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 5, 2017

Member

I'm not sure if this completely solves the issue. In particular, there's a usage of the legacy QUERY_TERMS which seems like it may crash on custom lookups. endswith is a built-in lookup in QUERY_TERMS and thus wouldn't reveal a problem, I think.

Member

timgraham commented Jul 5, 2017

I'm not sure if this completely solves the issue. In particular, there's a usage of the legacy QUERY_TERMS which seems like it may crash on custom lookups. endswith is a built-in lookup in QUERY_TERMS and thus wouldn't reveal a problem, I think.

@timgraham timgraham changed the title from Fixed #26184 -- Unable to use custom lookups or transforms in admin search_fields to Fixed #26184 -- Allowed custom lookups and transforms in ModelAdmin.search_fields. Jul 5, 2017

Show outdated Hide outdated docs/ref/contrib/admin/index.txt Outdated
of this SQL ``WHERE`` clause::
WHERE (first_name = 'John Lennon')

This comment has been minimized.

@timgraham

timgraham Jul 5, 2017

Member

Perhaps how this allows avoiding the concerns in https://code.djangoproject.com/ticket/26001 could also be documented.

@timgraham

timgraham Jul 5, 2017

Member

Perhaps how this allows avoiding the concerns in https://code.djangoproject.com/ticket/26001 could also be documented.

This comment has been minimized.

@nazarewk

nazarewk Jul 10, 2017

Contributor

I am not 100% sure this resolves the issue, but worth giving a hint.

@nazarewk

nazarewk Jul 10, 2017

Contributor

I am not 100% sure this resolves the issue, but worth giving a hint.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 10, 2017

Member

And will you address the QUERY_TERMS comment? We need a test for that by registering a custom lookup in a test. Search with register_lookup to see existing tests that do that. Here's a draft patch for the fix:

diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py
index 5f3a832..a79b798 100644
--- a/django/contrib/admin/utils.py
+++ b/django/contrib/admin/utils.py
@@ -26,21 +26,26 @@ def lookup_needs_distinct(opts, lookup_path):
     Return True if 'distinct()' should be used to query the given lookup path.
     """
     lookup_fields = lookup_path.split(LOOKUP_SEP)
-    # Remove the last item of the lookup path if it is a query term
-    if lookup_fields[-1] in QUERY_TERMS:
-        lookup_fields = lookup_fields[:-1]
-    # Now go through the fields (following all relations) and look for an m2m
+    # Go through the fields (following all relations) and look for an m2m
+    prev_field = None
     for field_name in lookup_fields:
         if field_name == 'pk':
             field_name = opts.pk.name
-        field = opts.get_field(field_name)
-        if hasattr(field, 'get_path_info'):
-            # This field is a relation, update opts to follow the relation
-            path_info = field.get_path_info()
-            opts = path_info[-1].to_opts
-            if any(path.m2m for path in path_info):
-                # This field is a m2m relation so we know we need to call distinct
-                return True
+        try:
+            field = opts.get_field(field_name)
+        except FieldDoesNotExist:
+            # Ignore valid query lookups.
+            if prev_field and prev_field.get_lookup(field_name):
+                continue
+        else:
+            prev_field = field
+            if hasattr(field, 'get_path_info'):
+                # This field is a relation, update opts to follow the relation
+                path_info = field.get_path_info()
+                opts = path_info[-1].to_opts
+                if any(path.m2m for path in path_info):
+                    # This field is a m2m relation so we know we need to call distinct
+                    return True
     return False
 
Member

timgraham commented Jul 10, 2017

And will you address the QUERY_TERMS comment? We need a test for that by registering a custom lookup in a test. Search with register_lookup to see existing tests that do that. Here's a draft patch for the fix:

diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py
index 5f3a832..a79b798 100644
--- a/django/contrib/admin/utils.py
+++ b/django/contrib/admin/utils.py
@@ -26,21 +26,26 @@ def lookup_needs_distinct(opts, lookup_path):
     Return True if 'distinct()' should be used to query the given lookup path.
     """
     lookup_fields = lookup_path.split(LOOKUP_SEP)
-    # Remove the last item of the lookup path if it is a query term
-    if lookup_fields[-1] in QUERY_TERMS:
-        lookup_fields = lookup_fields[:-1]
-    # Now go through the fields (following all relations) and look for an m2m
+    # Go through the fields (following all relations) and look for an m2m
+    prev_field = None
     for field_name in lookup_fields:
         if field_name == 'pk':
             field_name = opts.pk.name
-        field = opts.get_field(field_name)
-        if hasattr(field, 'get_path_info'):
-            # This field is a relation, update opts to follow the relation
-            path_info = field.get_path_info()
-            opts = path_info[-1].to_opts
-            if any(path.m2m for path in path_info):
-                # This field is a m2m relation so we know we need to call distinct
-                return True
+        try:
+            field = opts.get_field(field_name)
+        except FieldDoesNotExist:
+            # Ignore valid query lookups.
+            if prev_field and prev_field.get_lookup(field_name):
+                continue
+        else:
+            prev_field = field
+            if hasattr(field, 'get_path_info'):
+                # This field is a relation, update opts to follow the relation
+                path_info = field.get_path_info()
+                opts = path_info[-1].to_opts
+                if any(path.m2m for path in path_info):
+                    # This field is a m2m relation so we know we need to call distinct
+                    return True
     return False
 
@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 11, 2017

Member

The postgres-specifc test isn't appropriate as we're not trying to test code in contrib.postgres -- did you understand my suggestion about using with register_lookup?

Member

timgraham commented Jul 11, 2017

The postgres-specifc test isn't appropriate as we're not trying to test code in contrib.postgres -- did you understand my suggestion about using with register_lookup?

@nazarewk

This comment has been minimized.

Show comment
Hide comment
@nazarewk

nazarewk Jul 12, 2017

Contributor

Thought the postgres json lookup would be the most extreme case, but now that i think about it, it's not exactly the same.

Contributor

nazarewk commented Jul 12, 2017

Thought the postgres json lookup would be the most extreme case, but now that i think about it, it's not exactly the same.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 25, 2017

Member

Also, remove django.db.models.sql.constants.QUERY_TERMS as this is the last usage of that.

Member

timgraham commented Jul 25, 2017

Also, remove django.db.models.sql.constants.QUERY_TERMS as this is the last usage of that.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 27, 2017

Member

I think it might be possible to use the field.get_lookup() logic from lookup_needs_distinct() in construct_search() to determine if field_name ends with a valid lookup. That could eliminate the need for the ! syntax.

Member

timgraham commented Jul 27, 2017

I think it might be possible to use the field.get_lookup() logic from lookup_needs_distinct() in construct_search() to determine if field_name ends with a valid lookup. That could eliminate the need for the ! syntax.

@nazarewk

This comment has been minimized.

Show comment
Hide comment
@nazarewk

nazarewk Jul 28, 2017

Contributor
  1. @timgraham what do you think about my proposal?
  2. Maybe drop default icontains alltogether, it will break backwards compatibility though.
  3. Maybe I should add some better split() logic (like grouping multiple words with ")?

PS: I am going on holidays, will be back at 7th August, can write better documentation and work on it then.

Contributor

nazarewk commented Jul 28, 2017

  1. @timgraham what do you think about my proposal?
  2. Maybe drop default icontains alltogether, it will break backwards compatibility though.
  3. Maybe I should add some better split() logic (like grouping multiple words with ")?

PS: I am going on holidays, will be back at 7th August, can write better documentation and work on it then.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Sep 4, 2017

Member

That last commit seems complicated and there's some unrelated refactoring which makes it difficult to follow. My idea was along the lines of this (I ignored your last commit and wrote this patch on top of the commit before):

diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index cfd9ee0..a880477 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -891,9 +891,33 @@ class ModelAdmin(BaseModelAdmin):
         """
         # Apply keyword searches.
         def construct_search(field_name):
-            if field_name.startswith('!'):
-                return field_name[1:]
-            elif field_name.startswith('^'):
+            opts = queryset.model._meta
+            lookup_fields = field_name.split(LOOKUP_SEP)
+            # Go through the fields (following all relations) and look for an m2m
+            prev_field = None
+            for path_part in lookup_fields:
+                if path_part == 'pk':
+                    path_part = opts.pk.name
+                try:
+                    field = opts.get_field(path_part)
+                except FieldDoesNotExist:
+                    # Use valid query lookups.
+                    if prev_field and prev_field.get_lookup(path_part):
+                        return field_name
+                else:
+                    prev_field = field
+                    # The rest of the logic from lookup_needs_distinct() --
+                    # need a test (that spans more relations?) to show if it's
+                    # needed.
+                    # if hasattr(field, 'get_path_info'):
+                        # This field is a relation, update opts to follow the relation
+                        # path_info = field.get_path_info()
+                        # opts = path_info[-1].to_opts
+                        # if any(path.m2m for path in path_info):
+                        #    # This field is a m2m relation so we know we need to call distinct
+                        #    return True
+
+            if field_name.startswith('^'):
                 return "%s__istartswith" % field_name[1:]
             elif field_name.startswith('='):
                 return "%s__iexact" % field_name[1:]
diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py
index 6a9d8aa..1dd529e 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -436,7 +436,7 @@ class ChangeListTests(TestCase):
         Concert.objects.create(name='Woodstock', group=band)
 
         m = ConcertAdmin(Concert, custom_site)
-        m.search_fields = ['!group__name__cc']
+        m.search_fields = ['group__name__cc']
 
         try:
             Field.register_lookup(Contains, 'cc')

This borrows logic from lookup_needs_distinct(). Maybe there's a chance to refactor it into something shared, but I think it would be easier to follow the patch without that refactoring in this PR.

Member

timgraham commented Sep 4, 2017

That last commit seems complicated and there's some unrelated refactoring which makes it difficult to follow. My idea was along the lines of this (I ignored your last commit and wrote this patch on top of the commit before):

diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py
index cfd9ee0..a880477 100644
--- a/django/contrib/admin/options.py
+++ b/django/contrib/admin/options.py
@@ -891,9 +891,33 @@ class ModelAdmin(BaseModelAdmin):
         """
         # Apply keyword searches.
         def construct_search(field_name):
-            if field_name.startswith('!'):
-                return field_name[1:]
-            elif field_name.startswith('^'):
+            opts = queryset.model._meta
+            lookup_fields = field_name.split(LOOKUP_SEP)
+            # Go through the fields (following all relations) and look for an m2m
+            prev_field = None
+            for path_part in lookup_fields:
+                if path_part == 'pk':
+                    path_part = opts.pk.name
+                try:
+                    field = opts.get_field(path_part)
+                except FieldDoesNotExist:
+                    # Use valid query lookups.
+                    if prev_field and prev_field.get_lookup(path_part):
+                        return field_name
+                else:
+                    prev_field = field
+                    # The rest of the logic from lookup_needs_distinct() --
+                    # need a test (that spans more relations?) to show if it's
+                    # needed.
+                    # if hasattr(field, 'get_path_info'):
+                        # This field is a relation, update opts to follow the relation
+                        # path_info = field.get_path_info()
+                        # opts = path_info[-1].to_opts
+                        # if any(path.m2m for path in path_info):
+                        #    # This field is a m2m relation so we know we need to call distinct
+                        #    return True
+
+            if field_name.startswith('^'):
                 return "%s__istartswith" % field_name[1:]
             elif field_name.startswith('='):
                 return "%s__iexact" % field_name[1:]
diff --git a/tests/admin_changelist/tests.py b/tests/admin_changelist/tests.py
index 6a9d8aa..1dd529e 100644
--- a/tests/admin_changelist/tests.py
+++ b/tests/admin_changelist/tests.py
@@ -436,7 +436,7 @@ class ChangeListTests(TestCase):
         Concert.objects.create(name='Woodstock', group=band)
 
         m = ConcertAdmin(Concert, custom_site)
-        m.search_fields = ['!group__name__cc']
+        m.search_fields = ['group__name__cc']
 
         try:
             Field.register_lookup(Contains, 'cc')

This borrows logic from lookup_needs_distinct(). Maybe there's a chance to refactor it into something shared, but I think it would be easier to follow the patch without that refactoring in this PR.

m = BandAdmin(Band, custom_site)
m.search_fields = [
('nr_of_members__cgt', int)

This comment has been minimized.

@timgraham

timgraham Sep 4, 2017

Member

I'm not sure if there are lookups where it's not the case, but comparisons such as Choice.objects.filter(votes__gte='2') seem to work fine with the value as a string so the "transform" stuff seems unnecessary, at this for the first version of this.

@timgraham

timgraham Sep 4, 2017

Member

I'm not sure if there are lookups where it's not the case, but comparisons such as Choice.objects.filter(votes__gte='2') seem to work fine with the value as a string so the "transform" stuff seems unnecessary, at this for the first version of this.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Nov 15, 2017

Member

I pushed my proposal in #9357. There might be some refactoring to do as you've proposed but it would be more clear to do that in a separate PR, I think. Thanks for the initial patch and please review my PR if you'd like.

Member

timgraham commented Nov 15, 2017

I pushed my proposal in #9357. There might be some refactoring to do as you've proposed but it would be more clear to do that in a separate PR, I think. Thanks for the initial patch and please review my PR if you'd like.

@timgraham timgraham closed this Nov 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment