Code

Opened 3 years ago

Last modified 4 weeks ago

#17522 new Bug

ModelAdmin.ordering validation too strict

Reported by: sebastian Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: admin, validation, ordering, strict
Cc: sebastian Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

ModelAdmin.ordering checks whether all elements are the names of valid fields on the model. This is too strict as we can also define methods on the model or admin class and set admin_order_field. In fact, if such columns are used in list_display they are sortable by clicking on the corresponding column in the change list view.

The attached patch relaxes the admin validation so that the names of such methods are allowed for the default sorting. It also adds several tests to check this relaxed validation.

Attachments (1)

r17364-admin-ordering-validation.diff (6.7 KB) - added by sebastian 3 years ago.

Download all attachments as: .zip

Change History (4)

Changed 3 years ago by sebastian

comment:1 Changed 2 years ago by sebastian

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Let me add some thoughts as to why I think the current validation should be changed.

It is quite possible to add annotations to the change list queryset with queryset(). When used with admin_order_field, such columns are perfectly sortable in the change list view by clicking the corresponding column. What is not possible, however, as of r17372, is to specify these columns as default sorting for the corresponding change list view.

# models.py

class Foo(models.Model):
    pass

class Bar(models.Model):
    foo = models.ForeignKey(Foo)

# admin.py

class FooAdmin(admin.ModelAdmin):
    list_display = ('bar_count',)

    def queryset(self, request):
        return super(FooAdmin, self).queryset(request).annotate(bar_count=models.Count('bar_set'))

    def bar_count(self, foo):
        return foo.bar_count
    bar_count.admin_order_field = 'bar_count'

    # This does not work:
    #ordering = ('bar_count',)

I understand the motivation for doing some basic sanity checks on the model admin's attributes. In this case, however, I think these are too strict, i.e. checking only if a matching field exists on the model is not what should be done.

On the other hand, instead of entirely removing the checks so that we simply assume that the corresponding attribute will exist eventually at the time the query is executed, I propose this compromise: check if there is (a) an attribute, or (b) a method on either the model or admin class with admin_order_field set.

This way, we can still specify arbitrary fields, such as produced by annotations, while retaining the basic sanity check that there must be at least something declared manually, either a field or a method. This should prevent most typos while allowing the necessary freedom to add default-sortable columns based on annotation fields.

BTW, this is exactly how admin_order_field works in regular list_display in the first place. No checks for the existence of the given field are done in the validation code, for the same reason: to be able to include annotations as sortable columns.

comment:2 Changed 2 years ago by julien

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 4 weeks ago by timo

  • Patch needs improvement set

Patch no longer applies cleanly.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.