Opened 10 years ago

Closed 9 years ago

#2818 closed enhancement (fixed)

[newforms-admin] Admin searches should use distinct, if query involves joins

Reported by: Andy Dustman <farcepest@…> Owned by: Adrian Holovaty
Component: contrib.admin Version: master
Severity: minor Keywords:
Cc: farcepest@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I have a model that is searchable based on fields that are several levels of indirection away, i.e.

class Course:

    # ...
    sections = ManyToManyField(Section)

    class Admin:

        search_fields = ('name', '^sections__instructors__surname',
                         '^sections__instructors__username',
                         '^sections__call_no')

i.e. Courses refers to Sections which refers to Instructors which has a couple fields to search on. Sections may belong to more than one course, and Instructors may belong to more than one section. In the current trunk, a search on surname in this scheme can return the same Course multiple times.

To fix this, I simply changed django.contrib.admin.views.main.ChangeList.get_query_set() to return a QuerySet().distinct() all the time. This doesn't seem to break anything and it fixes the duplicate results problem. However, it's possible it may have some performance impact so you might want to make it conditional on whether or not the search is across joined tables. However, it wasn't really obvious how to do this so I went for the quick-and-(clean|dirty) solution.

Attachments (2)

search_distinct.diff (520 bytes) - added by Andy Dustman <farcepest@…> 10 years ago.
Make all admin searches distinct
search_distinct.2.diff (728 bytes) - added by Andy Dustman <farcepest@…> 10 years ago.
Use distinct() only when joined search fields are present

Download all attachments as: .zip

Change History (7)

Changed 10 years ago by Andy Dustman <farcepest@…>

Attachment: search_distinct.diff added

Make all admin searches distinct

comment:1 Changed 10 years ago by Adrian Holovaty

Summary: [patch] Admin searches should use distinctAdmin searches should use distinct, if query involves joins

Yeah, I'm concerned about the performance impact that you pointed out. It should only add a DISTINCT if needed -- it should NOT always put that in.

I'm removing the [patch] designation.

Changed 10 years ago by Andy Dustman <farcepest@…>

Attachment: search_distinct.2.diff added

Use distinct() only when joined search fields are present

comment:2 Changed 10 years ago by anonymous

Summary: Admin searches should use distinct, if query involves joins[patch] Admin searches should use distinct, if query involves joins

Requested changes made: As long as at least one field contains __, the resulting QuerySet will be made distinct().

One minor concern: I am explicitly looking for '__' in the field names and I wonder if I should look for LOOKUP_SEPARATOR from django.db.models.query. However a section above this (local function construct_search()) uses an explicit '__' and LOOKUP_SEPARATOR is not currently refererenced from outside of the module where it is defined, so I'm assuming it's OK.

comment:3 Changed 9 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedReady for checkin

comment:4 Changed 9 years ago by Malcolm Tredinnick

Summary: [patch] Admin searches should use distinct, if query involves joins[newforms-admin] Admin searches should use distinct, if query involves joins

comment:5 Changed 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(In [5123]) newforms-admin: Fixed #2818 -- Filtered out duplicate results when searching in
the admin across related fields. Thanks, Andy Dustman.

Note: See TracTickets for help on using tickets.
Back to Top