[Django] #17522: ModelAdmin.ordering validation too strict
Django
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
sebastian | Status: new
Type: Bug | Version: SVN
Component: | Keywords: admin, validation, ordering, strict
contrib.admin | Has patch: 1
Severity: Normal | UI/UX: 0
Triage Stage: |
Unreviewed |
Easy pickings: 0 |
-------------------------+-------------------------------------------------
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: SVN
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage:
ordering, strict | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by sebastian):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
{{{#!python
# 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:1>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: SVN
Severity: Normal | Resolution:
ordering, strict | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by julien):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:2>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Patch no longer applies cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:3>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: k@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:4>
Django
Reporter: sebastian | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by marfire):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:5>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:6>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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?
{{{#!python
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
{{{#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:7>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:8>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Javier Abadia Miranda):
Replying to [comment:7 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:9>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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:
{{{
#!python
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:
{{{
#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:10>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Sarah Boyce (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:11>
Django
Reporter: Sebastian Goll | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: admin, validation, | Triage Stage: Accepted
ordering, strict |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/17522#comment:12>