Opened 6 years ago

Last modified 6 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.

Change History (0)

Note: See TracTickets for help on using tickets.
Back to Top