Changes between Initial Version and Version 3 of Ticket #32584


Ignore:
Timestamp:
Apr 19, 2021, 2:26:00 AM (3 years ago)
Author:
David Beitey
Comment:

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #32584

    • Property Cc David Beitey added
    • Property Has patch unset
    • Property Summary OrderBy.as_sql() overwrites template → OrderBy.as_sql() overwrites template, creating invalid syntax for certain database backends
    • Property Version 3.1 → 3.2

  • Ticket #32584 – Description

    initial v3  
    1 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:
     1A 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:
    22
    3  
     3https://github.com/django/django/blob/ed0cc52dc3b0dfebba8a38c12b6157a007309900/django/db/models/expressions.py#L1212
     4
    45{{{
    5   def as_sql(self, compiler, connection, template=None, **extra_context):
     6    def as_sql(self, compiler, connection, template=None, **extra_context):
    67        template = template or self.template
    78        if connection.features.supports_order_by_nulls_modifier:
    … …  
    1920            ):
    2021                template = '%%(expression)s IS NOT NULL, %s' % template
     22        connection.ops.check_expression_support(self)
     23        expression_sql, params = compiler.compile(self.expression)
     24        placeholders = {
     25            'expression': expression_sql,
     26            'ordering': 'DESC' if self.descending else 'ASC',
     27            **extra_context,
     28        }
     29        template = template or self.template
     30        params *= template.count('%(expression)s')
     31        return (template % placeholders).rstrip(), params
    2132}}}
    2233
    23 Note that template is always overwritten if nulls_first == True.  In 3.0, the function first tested for template == None.
     34With 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).
    2435
    25 This causes trouble for the MSSQL 3rd party driver, which provides its own order by functionality:
    26 https://github.com/microsoft/mssql-django/issues/19
     36Can 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.
    2737
    28 The following change (simply testing for template) fixed the issue for me (forked off 3.1 as I'm on py 3.7)
    29 https://github.com/timnyborg/django/commit/fd41b39ade8d37138951223eba7f2e3fb66d0d1c
     38Ref: https://github.com/microsoft/mssql-django/issues/19 & https://github.com/microsoft/mssql-django/issues/31
Back to Top