Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#32584 closed Uncategorized (wontfix)

OrderBy.as_sql() overwrites template, creating invalid syntax for certain database backends

Reported by: Tim Nyborg Owned by: nobody
Component: Database layer (models, ORM) Version: 3.2
Severity: Normal Keywords: order_by, nulls_first
Cc: David Beitey Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by David Beitey)

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

Change History (8)

comment:1 by Tim Nyborg, 4 years ago

Resolution: invalid
Status: newclosed

comment:2 by Tim Nyborg, 4 years ago

I had misread one of the db feature flags that was relied on

comment:3 by David Beitey, 4 years ago

Cc: David Beitey added
Description: modified (diff)
Has patch: unset
Resolution: invalid
Status: closednew
Summary: OrderBy.as_sql() overwrites templateOrderBy.as_sql() overwrites template, creating invalid syntax for certain database backends
Version: 3.13.2

comment:4 by Tim Nyborg, 4 years ago

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.

comment:5 by Mariusz Felisiak, 4 years ago

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)

comment:6 by Mariusz Felisiak, 4 years ago

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)

comment:7 by Mariusz Felisiak, 4 years ago

Resolution: wontfix
Status: newclosed

comment:8 by David Beitey, 4 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top