ModelAdmin.ordering validation too strict

[Sebastian Goll]

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.

[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.

# 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.

[Tim Graham]

Patch no longer applies cleanly.

[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.)

2. 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.

3. 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.

4. 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.

5. 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.

[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.

[Simon Charette]

#29669 is duplicate with a patch that implements the third option of comment:5

I have to say I'm not convinced this needs to be fixed anyhow. Why wouldn't the following be acceptable?

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

    def get_queryset(self, request):
        qs = super().get_queryset(request)
        return qs.annotate(
            bar_count=models.Count('bar_set')
        ).order_by('bar_count')

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

or

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

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

    def get_ordering(self, request):
        return ('bar_count',)

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

If you are dynamically defining an annotation then why are you not dynamically ordering by this annotation and expecting static options to work with it? The fact ordering is not attempted to be applied by super().queryset() is really just an implementation detail and would crash otherwise.

Given ModelAdmin.ordering now accepts expressions ordering = models.Count('bar_set'), would work as well if there isn't need to refer to it using an alias for callable field ordering references.

[Kevin Christopher Henry]

If you are dynamically defining an annotation then why are you not dynamically ordering by this annotation and expecting static options to work with it?

I'm expecting it because that's how list_display works. The purpose of admin_order_field is to allow you to statically declare how to order a dynamic piece of code. Given that this exists, it seems wrong not to respect it when it comes to the ordering attribute.

To me, the bug isn't about how much you can or can't declare statically, it's about making the API more consistent. I haven't looked at the proposed patch, but I think this general idea - considering admin_order_field when looking at ordering - will be an improvement.

[Javier Abadia Miranda]

Replying to Simon Charette:

I have to say I'm not convinced this needs to be fixed anyhow. Why wouldn't the following be acceptable?

Because the ordering tuple purpose is to specify the default ordering that the user can change afterwards by clicking on column headers. If I'm forced to embed the ordering in the get_queryset() method, then:
1) I am forced to implement a get_queryset in situations where I don't need to do it, because defining additional columns via properties with get_ methods works fine and it's convenient and consistent with list_display
2) The user can't change the ordering afterwards, or I need to put logic inside get_queryset() to include the sorting or not depending on what the user does.
The funny thing is that everything works as expected, if you can work around the too strict (in my opinion) validation.

[Sylvain Fankhauser]

For those like me wondering why the following fails with an error Cannot resolve keyword 'bar_count' into field on Django 3.2:

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

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

    def get_ordering(self, request):
        return ('bar_count',)

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

That’s because super().get_queryset(request) tries to apply the ordering on the bar_count field that’s not yet been added to the queryset. This can be fixed by changing the get_queryset method to:

def get_queryset(self, request):
    qs = self.model._default_manager.get_queryset().annotate(
        bar_count=models.Count('bar_set')
    )

    ordering = self.get_ordering(request)
    if ordering:
        qs = qs.order_by(*ordering)

    return qs