Opened 9 years ago

Closed 8 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
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@…> 9 years ago.
Make all admin searches distinct
search_distinct.2.diff (728 bytes) - added by Andy Dustman <farcepest@…> 9 years ago.
Use distinct() only when joined search fields are present

Download all attachments as: .zip

Change History (7)

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

Make all admin searches distinct

comment:1 Changed 9 years ago by adrian

  • Summary changed from [patch] Admin searches should use distinct to Admin 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 9 years ago by Andy Dustman <farcepest@…>

Use distinct() only when joined search fields are present

comment:2 Changed 9 years ago by anonymous

  • Summary changed from Admin searches should use distinct, if query involves joins to [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 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:4 Changed 8 years ago by mtredinnick

  • Summary changed from [patch] Admin searches should use distinct, if query involves joins to [newforms-admin] Admin searches should use distinct, if query involves joins

comment:5 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(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