Opened 7 years ago

Last modified 15 months ago

#17522 new Bug

ModelAdmin.ordering validation too strict

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


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 Goll 7 years ago.

Download all attachments as: .zip

Change History (7)

Changed 7 years ago by Sebastian Goll

comment:1 Changed 7 years ago by Sebastian Goll

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.


class Foo(models.Model):

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


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 7 years ago by Julien Phalip

Triage Stage: UnreviewedAccepted

comment:3 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Patch no longer applies cleanly.

comment:4 Changed 4 years ago by Kevin Christopher Henry

Cc: k@… added

comment:5 Changed 4 years ago by Kevin Christopher Henry

The issue still exists. To briefly recap: the question is how to allow ordering by annotations (e.g. the Count of a related model) while still preserving static checks on the field names.

I can think of a few approaches:

  1. Leave things the way they are. The obvious downside is that it really does seem like a bug that you can view and order the change list a certain way, but you can’t specify that as the default order. (Some other downsides will become apparent below.)
  1. Get rid of the checks on ordering. This has some justification: after all, there is an inherent conflict between trying to do static, start-up-time checks on a model's fields when the design allows them to be created dynamically (via get_queryset() and annotate()). Moreover, these checks don't seem especially critical in practice—if your ordering is bad, you'll immediately see a 500 with an error message approximately as informative as the checks framework message. Still, it seems a shame to lose checks that could be helpful in the majority of cases for the sake of an edge case.
  1. Allow callables and methods marked with admin_order_field to be used. (This is the proposal made above by sebastian.) The main problem here is that it's pretty convoluted to have to add:
    def my_method(self, foo):
        return foo.bar__count
    my_method.admin_order_field = 'bar__count'

when all you really want to say is ordering = ('bar__count,'). It seems like a pretty hacky way to opt out of the checks. This also has the downside (shared with list_display) that admin_order_field isn't subject to any checks.

  1. Figure out whether or not it's possible for the model instance to have dynamic field names, and either perform or ignore the checks based on that. For example, we could ignore checks if the ModelAdmin has a get_queryset() method. This could be complicated, though, since you also have look at the default Manager. Another advantage, if this can be done reliably, is that we could start checking the value of admin_order_field.
  1. Since the purpose of all this is to allow annotations to show up on the change list page, we could just add support for that directly. For example, ordering = ('Count(bar_set)',). And similarly with list_display and admin_order_field. This would allow keeping the checks (and adding them to admin_order_field) while avoiding the pointless method declarations of 3 and skipping the need for get_queryset() when doing annotations. This is potentially the nicest design, but is probably also the most complicated solution.

comment:6 Changed 15 months ago by Andreas Backx

Could this ticket be revisited? This still applies to Django 1.11.3. It's something minor, but a patch seems to already have been made a long time ago, but I guess the ticket was forgotten.

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