1. Michael Manfre
  2. django-mssql
  3. Issues


Issue #40 resolved

Top-level multi-parethesis expressions throw the SQL compiler

Martijn Pieters
created an issue

We ran into an issue where the SQLCompiler._alias_columns would not handle an custom aggregate expression correctly.

Through a custom Aggregate object, the query would include the expression:

COUNT([devices_devicemachine].[id])-COUNT([devices_devicemachine].[deleted_at]) AS [aggregate]

which, once through the replace-parens-with-placeholders loop would come out the other end as:

COUNT({_placeholder_1}) - COUNT({_placeholder_2}) AS [aggregate]

and the _replace_sub(col) function would fail to handle that as there are two, not one placeholders in that expression.

I've struggled a little to write a testcase for this that doesn't involve including the full aggregate code. The work-around is to generate an expression with outer parenthesis:

(COUNT([devices_devicemachine].[id])-COUNT([devices_devicemachine].[deleted_at])) AS [aggregate]

as nested expressions are handled by .format(**parens) instead.

That does still leave the _replace_sub() function as fragile and error-prone; the blanket except: in the function is even dangerous as it'll catch MemoryError and KeyboardInterrupt exceptions too.

str.format() will always throw an exception if any placeholder doesn't have a corresponding value. I would like to propose the following implementation instead:

def _replace_sub(col):
    """Replace all placeholders with expanded values"""
    while _re_col_placeholder.search(col):
        col = col.format(**parens)
    return col

This change will recursively replace placeholders, fixes the multiple-placeholders issue we encountered, and passes all django-mssql tests.

Comments (3)

  1. Michael Manfre repo owner

    As you noted, the SQL mangling to support limit/offset is fragile and falls down when stressed with custom behaviors. Due to its fragile nature, changes to it need tests. How does this change fare against the django test suite?

  2. Martijn Pieters reporter

    I've added a test anyway, see pull request #10.

    I've tried running the django test suite against django-mssql, but I am having some trouble getting it to run against the project.

    Running the 1.5 django branch, I see a lot of failures and errors, just running this against django-mssql default (with none of my still-open branches). The tests are also very slow to run on my VM. I'll play with them some more, see if I can get the tests running properly against django-mssql default, then run them again against the PR 10 changes.

  3. Log in to comment