Opened 4 years ago
Last modified 4 years ago
#32584 closed Uncategorized
OrderBy.as_sql() overwrites template, creating invalid syntax for certain database backends — at Version 3
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 )
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
Change History (3)
comment:1 by , 4 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:2 by , 4 years ago
comment:3 by , 4 years ago
Cc: | added |
---|---|
Description: | modified (diff) |
Has patch: | unset |
Resolution: | invalid |
Status: | closed → new |
Summary: | OrderBy.as_sql() overwrites template → OrderBy.as_sql() overwrites template, creating invalid syntax for certain database backends |
Version: | 3.1 → 3.2 |
I had misread one of the db feature flags that was relied on