#32584: OrderBy.as_sql() overwrites template, creating invalid syntax for
certain
database backends
-------------------------------------+-------------------------------------
Reporter: Tim Nyborg | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: order_by, | Triage Stage:
nulls_first | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by David Beitey):
* status: closed => new
* cc: David Beitey (added)
* version: 3.1 => 3.2
* has_patch: 1 => 0
* resolution: invalid =>
Old description:
> A change in 3.1 has caused OrderBy.as_sql() in
> django.db.models.expressions to ignore sql templates provided by db
> backends when nulls_first or nulls_last is set:
>
> {{{
> def as_sql(self, compiler, connection, template=None, **extra_context):
> template = template or self.template
> if connection.features.supports_order_by_nulls_modifier:
> if self.nulls_last:
> template = '%s NULLS LAST' % template
> elif self.nulls_first:
> template = '%s NULLS FIRST' % template
> else:
> if self.nulls_last and not (
> self.descending and
> connection.features.order_by_nulls_first
> ):
> template = '%%(expression)s IS NULL, %s' % template
> elif self.nulls_first and not (
> not self.descending and
> connection.features.order_by_nulls_first
> ):
> template = '%%(expression)s IS NOT NULL, %s' % template
> }}}
>
> Note that template is always overwritten if nulls_first == True. In 3.0,
> the function first tested for template == None.
>
> This causes trouble for the MSSQL 3rd party driver, which provides its
> own order by functionality:
> https://github.com/microsoft/mssql-django/issues/19
>
> The following change (simply testing for template) fixed the issue for me
> (forked off 3.1 as I'm on py 3.7)
> https://github.com/timnyborg/django/commit/fd41b39ade8d37138951223eba7f2e3fb66d0d1c
New description:
A change in Django 3.1 has caused `OrderBy.as_sql()` in
`django.db.models.expressions` to incorrectly extend SQL templates
provided by db backends when `nulls_first` or `nulls_last` is set:
https://github.com/django/django/blob/ed0cc52dc3b0dfebba8a38c12b6157a007309900/django/db/models/expressions.py#L1212
{{{
def as_sql(self, compiler, connection, template=None,
**extra_context):
template = template or self.template
if connection.features.supports_order_by_nulls_modifier:
if self.nulls_last:
template = '%s NULLS LAST' % template
elif self.nulls_first:
template = '%s NULLS FIRST' % template
else:
if self.nulls_last and not (
self.descending and
connection.features.order_by_nulls_first
):
template = '%%(expression)s IS NULL, %s' % template
elif self.nulls_first and not (
not self.descending and
connection.features.order_by_nulls_first
):
template = '%%(expression)s IS NOT NULL, %s' % template
connection.ops.check_expression_support(self)
expression_sql, params = compiler.compile(self.expression)
placeholders = {
'expression': expression_sql,
'ordering': 'DESC' if self.descending else 'ASC',
**extra_context,
}
template = template or self.template
params *= template.count('%(expression)s')
return (template % placeholders).rstrip(), params
}}}
With changes from 3.1, feature flags were added along with alternative
syntax
(https://github.com/django/django/commit/7286eaf681d497167cd7dc8b70ceebfcf5cd21ad)
but the two types of syntax included only work for the given db backends.
On a backend that doesn't support either syntax (such as the MSSQL 3rd
party driver), there is currently no way of supplying its own template
with correct syntax because that template will always be modified when
`nulls_first` or `nulls_last` is set (e.g. when
descending=True/nulls_first=True or descending=False/nulls_last=True).
Can an additional database feature flag be added to Django to cover this
use case? For example `supports_order_by_nulls` and when that is `True`,
the existing code can run to modify the given template; if `False`, then
the template should not be modified.
Ref: https://github.com/microsoft/mssql-django/issues/19 &
https://github.com/microsoft/mssql-django/issues/31
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.4cdf8e8668b52a152bcd1714ebc5848a%40djangoproject.com.
