Incorrect escaping when using subqueries inside datetime truncation functions

[Raphael Michel]

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.

[Raphael Michel]

Patch submitted at ​https://github.com/django/django/pull/10274

[Simon Charette]

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

[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 :(

[Tim Graham]

Yes, that's how our ​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.

[Tim Graham <timograham@…>]

In 155b31d:

Fixed #29648 -- Fixed crash when using subqueries inside datetime truncation functions.

[Raphael Michel]

Replying to Tim Graham:

There's the option create your own Trunc functions rather than patching Django's.

Thank you, I didn't think of this.