Re: [Django] #33678: order_by crash when sorting by a foreign key referencing a Model with an ordering that includes expressions (was: Admin crash when sorting by a foreign key that's sorted with functions)

1 view
Skip to first unread message

Django

unread,
May 4, 2022, 12:07:34 AM5/4/22
to django-...@googlegroups.com
#33678: order_by crash when sorting by a foreign key referencing a Model with an
ordering that includes expressions
-------------------------------------+-------------------------------------
Reporter: Eduardo Rivas | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 4.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):

* type: Uncategorized => Bug
* component: contrib.admin => Database layer (models, ORM)
* stage: Unreviewed => Accepted


Comment:

Thank you for your report. I vaguely remember that this was brought up
already previously but I can't find an existing ticket on the subject.

The issue here is not the admin as you'll get the same crash if you try to
do `list(QuotePartMaterial.objects.order_by('material'))`.

What we're lacking
[https://github.com/django/django/blob/9d04711261156c487c6085171398070ea3df8122/django/db/models/sql/compiler.py#L909-L916
is a bit of logic ] that translates `order_by('material')` to
`order_by(Lower('material__code'))` instead of naively doing
`.order_by(Lower('code'))` when we're not dealing with a field inherited
through a parent link. This is something that was overlooked in #30557.

I suggest we take an approach similar to `Expression.replace_references`
in [https://github.com/django/django/pull/14625/ the ungoing work for
database constraint validation] and introduce
`Expression.prefix_references` that goes along these lines


{{{#!python
def prefix_references(self, prefix):
clone = self.copy()
clone.set_source_expressions(
[
F(f"{prefix}{expr.name}")
if isinstance(expr, F)
else expr.prefix_references(prefix)
for expr in self.get_source_expressions()
]
)
return clone
}}}

It will then be trivial to adjust `SQLCompiler.find_ordering_name` to call
`item.prefix_references(f"{field.name}{LOOKUP_SEP}")` when appropriate.

Is this something you'd be interesting in fixing and writing tests for
yourself Eduardo? This seems like a good introduction ticket to a few ORM
components if this is something you've been curious about.

--
Ticket URL: <https://code.djangoproject.com/ticket/33678#comment:1>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

Reply all
Reply to author
Forward
0 new messages