Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#15203 closed Uncategorized (duplicate)

admin search_fields splitting query

Reported by: Waldemar Kornewald Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Ramiro Morales)

When you specify a ModelAdmin search_field as exact (with "=" prefix) the user's search query is split() and then multiple __iexact filters are applied, one for each word. This behavior doesn't make sense with a CharField because we're doing an iexact match (there's no way two distinct filters can match at the same time, except if we're doing a JOIN). It should just take the whole query and match against that.

The same problem probably also affects __search queries (with an "@" prefix). The string should probably also be passed directly to the DB.

Attachments (1)

admin-search.diff (1.5 KB) - added by Waldemar Kornewald 6 years ago.
patch against trunk

Download all attachments as: .zip

Change History (14)

comment:1 Changed 6 years ago by Waldemar Kornewald

Cc: wkornewald@… added

comment:2 Changed 6 years ago by Ramiro Morales

Description: modified (diff)

(WikiFormatted slightly description)

comment:3 Changed 6 years ago by Karen Tracey

Needs documentation: set
Needs tests: set
Resolution: invalid
Status: newclosed

(When formatting only one inline backtick is needed; doubling them effectively removes them.)

I think you have overlooked the case of multiple fields listed in search_fields. I have used fields prefixed with equal in search_fields and I currently do get the results I expect, based on the existing doc, when entering multiple words in the search field. (One word entered can match exactly the equal-preceded field in search_fields and another can match some other field in search fields.) The doc already notes that it's not possible to search for an exact match on a value that contains a space. As far as I can see current behavior is easily understandable and documented, even if it is restrictive on what exactly you can search for. I think the change you propose would break the case where there are multiple fields in search_fields.

comment:4 Changed 6 years ago by Waldemar Kornewald

Resolution: invalid
Status: closedreopened

Right, I've overlooked that. But I don't agree that this ticket is invalid. Django's current behavior is not right. Splitting the string is very confusing and I'm pretty sure it's unnecessary in the case of __search. I'll fix the patch.

comment:5 Changed 6 years ago by Waldemar Kornewald

OK, I've updated the patch. The query string must still be split in case there are multiple fields, but additionally, exact matches on the whole query string should also be taken into account. In case of just a single search field the query should not be split.

The patch doesn't tread __search differently, anymore, since splitting is still necessary for multiple search fields, anyway.

The patch still doesn't have any documentation and unit tests. I'll try to push a complete patch in the next few weeks, but that'll probably be too late for the 1.3 release.

comment:6 Changed 6 years ago by Karen Tracey

Patch needs improvement: set

The updated patch still breaks the case I was concerned about above. When I've got multiple fields in search_fields, say:

    search_fields = ('=name', '=adopted_name', 'markings', 'adopter__name',)

(Happens to be a DB tracking cat adoptions for a rescue group.) Anyway, with current trunk code, if I enter "Lucky tabby" in the search box I get hits: cats with an (exact) name or adopted name of Lucky, and tabby somewhere in the markings field. This result I believe is correct, it matches my expectations and the documentation.

With the (updated) patch, I get no hits, which I don't believe is the correct result for this case.

Changed 6 years ago by Waldemar Kornewald

Attachment: admin-search.diff added

patch against trunk

comment:7 Changed 6 years ago by Waldemar Kornewald

Thanks for the note. That was caused by a typo. The new patch should work correctly, but I should really better write a few unit tests which check all important cases. ;)

comment:8 Changed 6 years ago by anonymous

Triage Stage: UnreviewedAccepted

comment:9 Changed 6 years ago by Russell Keith-Magee

Oops - that was me.

comment:10 Changed 6 years ago by Waldemar Kornewald

I was already wondering if the triage state needs to be reverted. :) Thanks for the note. FYI, I still haven't found the time to write the unit tests and update the documentation. Hopefully I'll finish this before you release 1.3 stable. :(

comment:11 Changed 6 years ago by Julien Phalip

Resolution: duplicate
Severity: Normal
Status: reopenedclosed
Type: Uncategorized

I think this is the same issue as #6933, which reports the problem when the field name starts with ^.

comment:12 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:13 Changed 5 years ago by Waldemar Kornewald

Cc: wkornewald@… removed
Easy pickings: unset
UI/UX: unset
Note: See TracTickets for help on using tickets.
Back to Top