Opened 7 years ago
Last modified 7 years ago
#29648 closed Bug
Incorrect escaping when using subqueries inside datetime truncation functions — at Initial Version
| Reported by: | Raphael Michel | Owned by: | Raphael Michel |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | dev |
| Severity: | Normal | Keywords: | Subqueries Datetime |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
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.