Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#10348 closed (fixed)

contrib.admin: select_related overwritten by django.contrib.admin.views.ChangeList.get_query_set

Reported by: erny Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords: efficient-admin
Cc: matthew@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

In trunk and 1.0.x, although I define (see #8213, see also #9554, and #9849 marked as dupe)

class MyModelAdmin(ModelAdmin):
    def queryset(self):
        return super(MyModelAdmin, self).queryset().select_related('foo__bar__baz', depth=2)

it is completely overwritten by the following code in `django/contrib/admin/views/main.py near line 83:

        # Use select_related() if one of the list_display options is a field
        # with a relationship.
        if self.list_select_related:
            qs = qs.select_related()
        else:
            for field_name in self.list_display:
                try:
                    f = self.lookup_opts.get_field(field_name)
                except models.FieldDoesNotExist:
                    pass
                else:
                    if isinstance(f.rel, models.ManyToOneRel):
                        qs = qs.select_related()
                        break

        # Set ordering.
        if self.order_field:
            qs = qs.order_by('%s%s' % ((self.order_type == 'desc' and '-' or ''), self.order_field))

This should definitively be an option of ModelAdmin.

Attachments (3)

admin_select_related.diff (1.5 KB) - added by erny 6 years ago.
new version of patch
10348_with-none-and-explicit-select-related.diff (2.1 KB) - added by mrts 6 years ago.
Add None and explicitly select the related fields when False
10348-introspect-qs.diff (3.8 KB) - added by mmarshall 6 years ago.
Don't change select_related if it's already truthy. (with tests)

Download all attachments as: .zip

Change History (13)

comment:1 Changed 6 years ago by erny

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by erny

The patch has a small error: s/selected/select

comment:3 Changed 6 years ago by erny

just found another error, will attach new patch.

Changed 6 years ago by erny

new version of patch

comment:4 Changed 6 years ago by jacob

  • milestone set to 1.1
  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 6 years ago by mrts

#10742 was a duplicate and proposed to use three-valued logic, leaving existing behaviour as-is and adding None:

  • True -- always call select_related(),
  • False (the default) -- call select_related() if any of the fields in list_display is a foreign key,
  • None (added) -- leave the queryset alone.

It has a working patch.

comment:6 Changed 6 years ago by mrts

  • Keywords efficient-admin added
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

Changed 6 years ago by mrts

Add None and explicitly select the related fields when False

comment:7 Changed 6 years ago by mmarshall

  • Cc matthew@… added

Distinguishing between False and None feels hacky to me.

Would it work to have ChangeList.get_query_set only call select_related if qs.query.select_related is False?

Changed 6 years ago by mmarshall

Don't change select_related if it's already truthy. (with tests)

comment:8 Changed 6 years ago by jacob

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

(In [10782]) Fixed #10348: ChangeList no longer overwrites a select_related provided by ModelAdmin.queryset().

comment:9 Changed 6 years ago by jacob

(In [10783]) [1.0.X] Fixed #10348: ChangeList no longer overwrites a select_related provided by ModelAdmin.queryset(). Backport of [10782] from trunk.

comment:10 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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