[Django] #32584: OrderBy.as_sql() overwrites template
Django
-------------------------------------+-------------------------------------
Reporter: Tim | Owner: nobody
Nyborg |
Type: | Status: new
Uncategorized |
Component: Database | Version: 3.1
layer (models, ORM) | Keywords: order_by,
Severity: Normal | nulls_first
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/32584>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: closed
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: order_by, | Triage Stage:
nulls_first | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: closed
Component: Database layer | Version: 3.1
(models, ORM) |
Severity: Normal | Resolution: invalid
Keywords: order_by, | Triage Stage:
nulls_first | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timnyborg):
I had misread one of the db feature flags that was relied on
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:2>
Django
database backends
-------------------------------------+-------------------------------------
Reporter: Tim Nyborg | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution:
nulls_first | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* cc: David Beitey (added)
* version: 3.1 => 3.2
* has_patch: 1 => 0
* resolution: invalid =>
Old description:
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:
{{{
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
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
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Nyborg):
Prior to this commit:
(https://github.com/django/django/commit/7286eaf681d497167cd7dc8b70ceebfcf5cd21ad),
a supplied template would not be overridden. I think that's the correct
approach - logic to add `nulls last` or `order by <expression> is null`
should only be applied to `self.template`, if `template` is null.
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:4>
Django
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
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
David, I don't think that an additional feature flag is needed. You can
always define `as_mssql()` to override the current behavior on MSSQL. If
MSSQL doesn't support `nulls_first` and `nulls_last` I would raise an
exception, e.g.
{{{
def as_mssql(self, compiler, connection):
if self.nulls_first or self.nulls_last:
raise NotSupportedError('nulls_first/nulls_last are not supported
on MSSQL.')
return self.as_sql(compiler, connection, template=template)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:5>
Django
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
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak):
You can also disable `nulls_first` and `nulls_last` if you handled them in
your custom template, e.g.
{{{
def as_mssql(self, compiler, connection):
template = ...
copy = self.copy()
copy.nulls_first = False
copy.nulls_last = False
return copy.as_sql(compiler, connection, template=template)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:6>
Django
database backends
-------------------------------------+-------------------------------------
Reporter: Tim Nyborg | Owner: nobody
Component: Database layer | Version: 3.2
(models, ORM) |
Keywords: order_by, | Triage Stage:
nulls_first | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:7>
Django
database backends
-------------------------------------+-------------------------------------
Reporter: Tim Nyborg | Owner: nobody
Type: Uncategorized | Status: closed
Component: Database layer | Version: 3.2
(models, ORM) |
Severity: Normal | Resolution: wontfix
Keywords: order_by, | Triage Stage:
nulls_first | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by David Beitey):
@Mariusz, thanks for the suggestions - much appreciated.
I've followed your second suggestion and created
https://github.com/microsoft/mssql-django/pull/32. Relying on the
implementation details of `OrderBy.as_sql()` not to modify templates feels
slightly hacky, but in contrast, re-implementing the whole `as_mssql()`
would have involved copy-pasting most of the function out of Django's core
(e.g in order to remove the template modification code) -- and keeping
that in lock-step with Django would be a greater challenge.
--
Ticket URL: <https://code.djangoproject.com/ticket/32584#comment:8>