Munchable's background jobs stopped working, and they stopped in the way that is hardest to notice: they ran on schedule, returned quickly, and did nothing. The queries inside them were failing with a syntax error, every time, for weeks.
The cause is one line of behaviour in Drizzle's sql template that is correct for the common case and silently wrong for the one next to it.
The expansion
Give Drizzle's template an array and it expands to a parenthesised list of placeholders:
sql`where slug in ${['a', 'b', 'c']}` // where slug in ($1, $2, $3)
Every rendering in this post is copied out of PgDialect().sqlToQuery(), which is also how the tests at the end of it work.
That is exactly what you want for an in list. And exactly what you do not want here:
sql`select * from unnest(${slugs}::text[])`
// select * from unnest(($1, $2, $3)::text[])
($1, $2, $3) is not an array literal. In Postgres it is a row constructor, so ($1, $2, $3)::text[] asks to cast a record to an array of text, which the planner rejects. What comes back is a complaint about a cast, which is a long way from the actual problem: the array never got serialised as an array.
The empty case is worse, because it is louder and arrives earlier:
sql`select * from unnest(${[] as string[]}::text[])`
// select * from unnest(()::text[])
() is not an expression at all. That one is a plain syntax error, raised before anything has looked at a type, and it fires on exactly the input a batch job sees most often: a run with nothing to do.
Six lines each
import { sql, type SQL } from 'drizzle-orm';
/** `$n::text[]` bound to the whole array (empty arrays included). */
export function textArray(values: readonly string[]): SQL {
return sql`${sql.param([...values])}::text[]`;
}
/** `$n::float8[]` bound to the whole array (empty arrays included). */
export function floatArray(values: readonly number[]): SQL {
return sql`${sql.param([...values])}::float8[]`;
}
sql.param is the opt-out from the expansion: it binds whatever you give it as one parameter. The driver underneath, postgres, serialises a JavaScript array into Postgres array syntax on the wire, so $1 arrives as a genuine text[] and unnest($1::text[]) is a normal function call over a normal array. An empty array arrives as an empty array, which unnest is perfectly happy to return zero rows from.
The [...values] spread is there for the type signature. Taking readonly string[] means callers can pass a frozen or readonly array without a cast, and the copy is what satisfies the mutable type the parameter binding wants. It also means the bound value cannot change under you if the caller mutates its array before the query is flushed.
What this unlocks
The reason it is worth owning these two helpers, rather than avoiding unnest and writing a loop, is that unnest is how you do a batch write in one round trip.
Updating N rows to N different values, without N statements:
await tx.execute(sql`
update catalog.taxonomy_backlog as b
set status = v.status,
resolved_at = case when v.status = 'open' then b.resolved_at else ${now.toISOString()}::timestamptz end,
contexts = case when v.status = 'open' then b.contexts else null end,
proposed = case when v.proposed = 'null' then b.proposed else v.proposed::jsonb end
from unnest(${textArray(slugs)}, ${textArray(statuses)}, ${textArray(proposals)}) as v(slug, status, proposed)
where b.slug = v.slug
`);
Three parallel arrays go in as three parameters, unnest zips them into a three-column relation, and the update ... from joins the real table against it. One statement, one round trip, however many rows. The arrays must be the same length and must stay in step, which is the one hazard of the pattern and is worth a comment wherever it appears.
The second use is a membership test on a compound key, which is the thing SQL makes you work for:
await tx
.update(catalogTaxonomyEntries)
.set({ retiredAt: now, updatedAt: now })
.where(
sql`(${catalogTaxonomyEntries.kind}, ${catalogTaxonomyEntries.key}) in (select * from unnest(${textArray(keys.map((k) => k.kind))}, ${textArray(keys.map((k) => k.key))}))`,
);
where (a, b) in (...) with a list of literal tuples means building the tuple list by string concatenation, which is where injection bugs live. Two arrays and an unnest give you the same semantics with two bound parameters.
Note also the mix: that is the Drizzle query builder for the parts it expresses well, with one sql fragment where it does not. You do not have to choose one or the other per query.
The test, and why it does not need Postgres
The thing that made this bug survive so long is that the generated SQL was never looked at. The jobs were manual before they were scheduled, the failures went to a log nobody read, and no test touched that code path because testing it looked like it needed a database.
It does not. Drizzle will render a statement for you:
import { sql } from 'drizzle-orm';
import { PgDialect } from 'drizzle-orm/pg-core';
import { floatArray, textArray } from '../db/sql';
const dialect = new PgDialect();
test('textArray binds the whole array as a single parameter, empty included', () => {
for (const values of [[], ['a'], ['a', 'b']]) {
const q = dialect.sqlToQuery(sql`select * from unnest(${textArray(values)}) t`);
assert.equal(q.sql, 'select * from unnest($1::text[]) t');
assert.deepEqual(q.params, [values]);
}
});
sqlToQuery gives you the SQL string and the parameter array, so the assertion is about the text that would be sent. Three inputs, including the empty one, all produce the identical statement with exactly one parameter. No connection, no fixtures, no container, and it runs in the same millisecond as every other unit test.
Then there is the test I like most in the file, which asserts the bug:
test('the bare template expansion is the bug this guards against', () => {
const q = dialect.sqlToQuery(sql`select unnest(${[] as string[]}::text[])`);
assert.equal(q.sql, 'select unnest(()::text[])');
});
It passes because the broken thing is still broken. If a future version of Drizzle changed the expansion, that test would fail, and the failure would be the news: the helpers might no longer be necessary. A test that pins the behaviour you are working around is how you find out when you can stop working around it, and it costs three lines.
The lesson I actually took
The template expansion is not a bug in Drizzle. in ($1, $2) is overwhelmingly what people want when they interpolate an array, and making that the default is right.
The lesson is about where the failure landed. A query that is syntactically invalid for every input is not a subtle bug. It would have been caught in five seconds by anybody running the code once. It survived because the code ran in a scheduled job, the job caught its own errors so as not to crash, and nothing compared "the job ran" against "the job did anything". Both of those are reasonable decisions in isolation and together they are a blindfold.
The fix for the queries is twelve lines. The fix for the blindfold was a test that asserts the shape of the SQL, which is the only part of this I would now write before the feature rather than after it.
The data those jobs maintain is what the public side of the site is built from: a few hundred pages like the ingredient answers and the condition guides are generated from it at build time, which is a sharp incentive to notice when the jobs behind it have quietly stopped. The homepage is the short version of what the whole thing is for.
Top comments (0)