Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#15203 closed Uncategorized (duplicate)

admin search_fields splitting query

Reported by: wkornewald 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)

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 wkornewald 5 years ago.
patch against trunk

Download all attachments as: .zip

Change History (14)

comment:1 Changed 5 years ago by wkornewald

  • Cc wkornewald@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by ramiro

  • Description modified (diff)

(WikiFormatted slightly description)

comment:3 Changed 5 years ago by kmtracey

  • Needs documentation set
  • Needs tests set
  • Resolution set to invalid
  • Status changed from new to closed

(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 5 years ago by wkornewald

  • Resolution invalid deleted
  • Status changed from closed to reopened

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 5 years ago by wkornewald

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 5 years ago by kmtracey

  • 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 5 years ago by wkornewald

patch against trunk

comment:7 Changed 5 years ago by wkornewald

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 5 years ago by anonymous

  • Triage Stage changed from Unreviewed to Accepted

comment:9 Changed 5 years ago by russellm

Oops - that was me.

comment:10 Changed 5 years ago by wkornewald

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 5 years ago by julien

  • Resolution set to duplicate
  • Severity set to Normal
  • Status changed from reopened to closed
  • Type set to Uncategorized

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

comment:12 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:13 Changed 4 years ago by wkornewald

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