[Django] #29692: Incorrect removal of order_by clause created as multiline RawSQL
Django
-------------------------------------+-------------------------------------
Reporter: Marcin | Owner: nobody
Nowak |
Type: | Status: new
Uncategorized |
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Hi.
The `SQLCompiler` is ripping off one of my "order by" clause, because he
"thinks" the clause was already "seen" (in `SQLCompiler.get_order_by()`).
I'm using expressions written as multiline `RawSQL`s, which are similar
but not the same.
The bug is located in `SQLCompiler.get_order_by()`, somewhere around line
computing part of SQL query without ordering:
{{{
without_ordering = self.ordering_parts.search(sql).group(1)
}}}
The `sql` variable contains multiline sql. As a result, the
`self.ordering_parts` regular expression is returning just a line
containing ASC or DESC words. This line is added to `seen` set, and
because my raw queries have identical last lines, only the first clasue is
returing from `SQLCompiler.get_order_by()`.
As a quick/temporal fix I can suggest making `sql` variable clean of
newline characters, like this:
{{{
sql_oneline = ' '.join(sql.split('\n'))
without_ordering = self.ordering_parts.search(sql_oneline).group(1)
}}}
Note: beware of unicode (Py2.x `u''`) and EOL dragons (`\r`).
Example of my query:
{{{
return MyModel.objects.all().order_by(
RawSQL('''
case when status in ('accepted', 'verification')
then 2 else 1 end''', []).desc(),
RawSQL('''
case when status in ('accepted', 'verification')
then (accepted_datetime, preferred_datetime)
else null end''', []).asc(),
RawSQL('''
case when status not in ('accepted', 'verification')
then (accepted_datetime, preferred_datetime, created_at)
else null end''', []).desc())
}}}
The `ordering_parts.search` is returing accordingly:
- `' then 2 else 1 end)'`
- `' else null end'`
- `' else null end'`
Second RawSQL with a ` else null end` part is removed from
query.
The fun thing is that the issue can be solved by workaround by adding a
space or any other char to the last line.
So in case of RawSQL I can just say, that current implementation of
avoiding duplicates in order by clause works only for special/rare cases
(or does not work in all cases).
The bug filed here is about wrong identification of duplicates (because it
compares only last line of SQL passed to order by clause).
Hope my notes will help you fixing the issue. Sorry for my english.
--
Ticket URL: <https://code.djangoproject.com/ticket/29692>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
Comment:
Is there a reason you can't use
[https://docs.djangoproject.com/en/dev/ref/models/conditional-
expressions/#case conditional expressions], e.g. something like:
{{{
MyModel.objects.annotate(
custom_order=Case(
When(...),
)
).order_by('custom_order')
}}}
I'm thinking that would avoid fiddly `ordering_parts` regular expression.
If there's some shortcoming to that approach, it might be easier to
address that. Allowing the ordering optimization stuff to handle arbitrary
RawSQL may be difficult.
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Marcin Nowak):
> Is there a reason you can't use conditional expressions
No, but I didn't knew about the issue, and writing raw sqls is sometimes
faster (not in this case ;)
I'm really happy having possibility to mix raw sqls with object queries.
Next time I'll use expressions, for sure.
> Allowing the ordering optimization stuff to handle arbitrary RawSQL may
be difficult.
Personally I'd like to skip RawSQL clauses in the block which is
responsible for finding duplicates. If someone is using raw sqls, he knows
the best what he is doing, IMO. And it is quite strange if Django removes
silently part of your SQL. This is very confusing. And please note that
printing a `Query` instance was generating incomplete sql, but while
checking `Query.order_by` manually, the return value was containing all
clauses. I thought that just printing is affected, but our QA dept told me
the truth ;)
I know there is no effective way to compare similarity of two raw clauses.
This may be hard for expression objects, too, but you have a possibility
to implement some `__eq__` magic (instead of comparation of generated
sqls). Unfortunately I don't know why duplicates detection was
implemented, so it's hard to tell how to improve this part.
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Patches welcome, I suppose.
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Can Sarıgöl (added)
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11001 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:4>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 0 => 1
Comment:
Is there a reason why you didn't add tests?
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Can Sarıgöl):
I was waiting for confirmation, I've added a test. Is it enough?
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
Some additional test coverage needed.
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:9>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:10>
Django
-------------------------------------+-------------------------------------
| Sarıgöl
Type: Bug | Status: assigned
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Can Sarıgöl
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:11>
Django
-------------------------------------+-------------------------------------
| Sarıgöl
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* version: 1.11 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:12>
Django
-------------------------------------+-------------------------------------
| Sarıgöl
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"567b9928a3ad37e95b9ae17ec41342daa6968739" 567b9928]:
{{{
#!CommitTicketReference repository=""
revision="567b9928a3ad37e95b9ae17ec41342daa6968739"
Fixed #29692 -- Fixed removing ordering parts for multiline RawSQL
expressions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:13>
Django
-------------------------------------+-------------------------------------
| Sarıgöl
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29692#comment:14>