[Django] #29648: Incorrect escaping when using subqueries inside datetime truncation functions
Django
functions
-------------------------------------+-------------------------------------
Reporter: Raphael | Owner: Raphael Michel
Michel |
Type: Bug | Status: assigned
Component: Database | Version: 2.1
layer (models, ORM) |
Severity: Release | Keywords: Subqueries Datetime
blocker |
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
If you build a subquery that (a) has a datetime-typed result and (b)
contains parameterized values, like this:
{{{
inner = Fan.objects.filter(
author=OuterRef('pk'),
name__in=('Emma', 'Isabella', 'Tom')
).values('author').annotate(newest_fan=Max('fan_since')).values('newest_fan')
}}}
And then use one of the Trunc* methods on the result, like this:
{{{
outer = Author.objects.annotate(
newest_fan_year=TruncYear(Subquery(inner,
output_field=DateTimeField()))
)
}}}
Then Django will throw an `OperationalError` or `TypeError`, because it
internally builds an incorrect query:
{{{
SELECT "db_functions_author"."name", django_datetime_trunc('year', (SELECT
MAX(U0."fan_since") AS "newest_fan" FROM "db_functions_fan" U0 WHERE
(U0."author_id" = ("db_functions_author"."id") AND U0."name" IN (%%s, %%s,
%%s)) GROUP BY U0."author_id"), 'UTC') AS "newest_fan_year" FROM
"db_functions_author" ORDER BY "db_functions_author"."name" ASC
}}}
As you can see, this query contains `%%s` instead of `%s` in place of the
parameters in the subquery. The reason is that `TruncBase` makes an
incorrect assumption about the database backends:
{{{
# Escape any params because trunc_sql will format the string.
inner_sql = inner_sql.replace('%s', '%%s')
}}}
There's two things wrong with this:
* This assumption is wrong, because no database backend in core runs
string formatting on this value. Instead, they use this value as a
parameter in string formatting of another string, like `sql = "TRUNC(%s,
'Q')" % field_name`.
* A Transform should not make assumptions about implementation details of
the database backend in the first place. Some backends, like the MySQL
one, use `.format()` instead of `%` to format their strings, so the
assumption doesn't apply at all.
For reference, ``Extract`` (which is very similar to ``TruncBase`` in many
regards) doesn't have this assumption. I therefore recommend to remove the
``replace`` statement without substitution to resolve the issue.
I've prepared a patch with a regression test that I'll send as a PR as
soon as I have the ticket number of this bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/29648>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: Subqueries Datetime | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:1>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: Subqueries Datetime | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Raphael Michel):
Patch submitted at https://github.com/django/django/pull/10274
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:2>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Release blocker | Resolution:
Keywords: Subqueries Datetime | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:3>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Subqueries Datetime | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 2.1 => master
* severity: Release blocker => Normal
* stage: Unreviewed => Accepted
Comment:
Hello Raphael,
From what I can see this code was already broken on Django 2.0 which would
disqualify it for a backport to 2.1 since it isn't a regression.
Did you confirm it was working appropriately on Django 2.0 before
escalating it to a 2.1 release blocker?
https://github.com/django/django/commit/2a4af0ea43512370764303d35bc5309f8abce666
#diff-fe1badf69b7bc49eccc187edaa2d6d1dR146
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:4>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Subqueries Datetime | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Raphael Michel):
Sorry, no, you're correct, it was broken in 2.0 already.
Does that mean we can't have the fix in 2.1.1, but will need to wait for
2.2? That would be very unfortunate, since there's no workaround, and
we'd need to run a custom patched version of Django in production for more
than 6 months :(
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:5>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: Subqueries Datetime | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Yes, that's how our [https://docs.djangoproject.com/en/dev/internals
/release-process/#supported-versions supported versions policy]
works.Being conservative minimizes the risk of introducing a regression
in the stable branch.
There's the option create your own `Trunc` functions rather than patching
Django's.
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:6>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:7>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Component: Database layer | Version: master
(models, ORM) |
Keywords: Subqueries Datetime | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"155b31d4ec138664d62665eb2d8a442469045b78" 155b31d]:
{{{
#!CommitTicketReference repository=""
revision="155b31d4ec138664d62665eb2d8a442469045b78"
Fixed #29648 -- Fixed crash when using subqueries inside datetime
truncation functions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:8>
Django
functions
-------------------------------------+-------------------------------------
| Michel
Type: Bug | Status: closed
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: Subqueries Datetime | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Raphael Michel):
Replying to [comment:6 Tim Graham]:
> There's the option create your own `Trunc` functions rather than
patching Django's.
Thank you, I didn't think of this.
--
Ticket URL: <https://code.djangoproject.com/ticket/29648#comment:9>