Promote intArrayLiteral to @wxyc/database and lint the bare-array-in-ANY() trap unwritable - #2014
Promote intArrayLiteral to @wxyc/database and lint the bare-array-in-ANY() trap unwritable#2014jakebromberg wants to merge 3 commits into
Conversation
…-array-in-ANY() trap unwritable
Interpolating a bare JS array into a Drizzle sql template splats it across N positional placeholders instead of binding a PG array, so ANY(${ids}) becomes ANY(($1, $2, ... $N)) -- a row constructor Postgres rejects at parse time (42809). The workaround was duplicated inline in six places (one already exported as intArrayLiteral from a one-shot dedup job); each of BS#1068, BS#1071, and #2007 added another private copy instead of closing the class, so the defective form stayed writable everywhere else.
Move intArrayLiteral to shared/database/src/int-array-literal.ts and update all six call sites to import it. The helper now validates every element via Number + Number.isInteger and throws on a non-integer input instead of splicing raw text -- the six inline copies' "safe by construction because TypeScript types ids: number[]" comment was never true (the arrays generally arrive via an unchecked "as unknown as" cast over driver output), so the docblock now claims only what the code enforces.
Add wxyc/no-bare-array-in-sql-template (eslint-rules/), wired for apps/**, shared/**, jobs/**, and tests/**. It resolves the tagged template's sql identifier through scope analysis to a named import from 'drizzle-orm' -- never firing on a postgres-js tagged template, where the identical ANY(${array}) syntax is correct -- and then flags any interpolation whose statically-known type is an array. A pure position/AST check ("is this ${} textually inside ANY(...)") was tried and rejected: it cannot distinguish the fixed shape (ANY(${idArrayLiteral}::int[]), a string) from the historical bug (ANY(${ids}), an array) without type information, so it would have flagged every one of the six sites this change just fixed. Full project type information is already a hard requirement of this ESLint config (recommendedTypeChecked + projectService), so the type check has no fallback to degrade to.
Running the rule across the tree surfaces exactly one unfixed instance, in jobs/va-apple-music-url-remediation/orchestrate.ts -- the real defect tracked by #2007, already being fixed by the in-flight PR #2008. Every other Drizzle sql template in the repo is clean.
…lean
Running wxyc/no-bare-array-in-sql-template across the tree surfaces exactly one hit: jobs/va-apple-music-url-remediation/orchestrate.ts:378, ANY(${albumIds}) with albumIds: number[] -- a live seventh instance of the bug this issue is about, already ticketed as #2007 and being fixed in the open, not-yet-merged PR #2008 (a VALUES-join UPDATE that removes the array bind entirely, rather than a call to intArrayLiteral).
A rule that fails on the tree it lands into is not shippable, and neither --no-verify nor waiting on #2008 (blocked on the current GitHub Actions major_outage) is the right fix -- the failure has to be resolved inside this PR.
The violation sits inside a multi-line sql template literal, so a same-line eslint-disable-next-line comment on the preceding source line would land inside the template literal's string content (the SQL text itself) rather than as a JS comment. A disable/re-enable pair brackets just this one statement instead, immediately outside the template literal -- scoped to exactly this violation, nothing else in the file. This repo's flat config default-reports unused disable directives at 'warn', so the pair will surface as a cleanup nudge once PR #2008's rewrite lands and removes the array bind it's guarding.
… as policy, reject unsafe integers
Review follow-ups on the BS#2010 lint rule.
The rule's error message and two twin docblock sentences pointed a blocked author at `jobs/album-reviews-etl/link.ts` as the `sql.join(...)`-built VALUES exemplar for a text array. That file contains no `sql.join` and no VALUES — its `textArrayLiteral` is precisely the hand-rolled literal the message forbids. An author following the pointer found the opposite of the advice, which is the one defect that undercuts a rule whose whole value is telling people what to do instead. The message now names `textArrayLiteral` for what it is (real PG quoting) and points at `jobs/library-etl/job.ts`'s `buildLegacySourcedSetWhere` for a live `sql.join` call site.
The docblock claimed a bare array is "never valid ANYWHERE in a Drizzle `sql` template." That is false: the positional splat is exactly what makes `sql\`… WHERE x IN ${ids}\`` render `IN ($1, $2, $3)`, valid SQL. Only `ANY(${ids})` produces the row constructor Postgres rejects. The rule still flags both — enforcing one predicate shape everywhere is a defensible policy and there are zero live `IN ${array}` call sites — but it is now stated as a policy rather than asserted as a fact, and an `IN`-list author gets an applicable instruction instead of none.
Four ways a Drizzle `sql` tag escapes the scope-analysis check are now documented as known gaps rather than left silent: a re-export chain, a tag passed as a parameter, a subpath import, and a namespace member-expression tag. The last was previously encoded in the rule's test suite as a `valid` case, which read as intended behavior rather than a known limitation.
`intArrayLiteral` now validates with `Number.isSafeInteger` rather than `Number.isInteger`. The docblock had promised the helper "keeps working the day a column widens from integer to bigint" — true only below 2^53. `Number('9007199254740993')` evaluates to `9007199254740992`, a different integer that `Number.isInteger` accepts, so a corrupted id would splice silently into an UPDATE or DELETE's WHERE clause. Latent today (no bigint column in the schema), which is why it needed a real check rather than a comment promising one.
Also: the integration spec's assertion now matches its comment — it asserts `merge.intArrayLiteral` is undefined, pinning that the removed private copy has not come back, rather than only re-testing the shared helper. And both `eslint.config.mjs` and the bulk-update playbook now state the rule's real coverage, which excludes the `tests/integration/*.spec.js` postgres-js tier and `scripts/**` (globally ignored for reasons unrelated to this rule; no live instance of the bug in either).
|
Review follow-ups pushed as Local checks after the revisions: lint 835 problems / 0 errors (identical warning count to clean Overlap: this PR conflicts with #2008The description's overlap section verified #2012 with
Resolution is mechanical: take #2008's body, delete this PR's comment block and directive pair. #2008 removes the array bind entirely, so the suppression has nothing left to suppress. If a resolver keeps the pair by accident it degrades to a warning, not a break — Noted on #2008 as well so whoever merges second isn't surprised. One coupling for the future: linting the tree with this rule removed from the config turns those two directive comments into hard errors ( |
The rule found a live seventh instance on main
Running the finished rule across the tree (
npm run lint) surfaced exactly one hit:jobs/va-apple-music-url-remediation/orchestrate.ts:378—ANY(${albumIds})withalbumIds: number[]imported fromdrizzle-orm. That is a real, currently-shipped instance of this exact defect, in a job that has already run against production data (album_metadata: {"candidates":206,"invalidated":0,"batches":1}on the 2026-08-06 09:33 PDT run). It's already ticketed as #2007 — found by reviewing that production failure, not by anything preventive — and is being fixed by the open, not-yet-merged PR #2008. No human review caught it before #2007; this rule would have. That gap is the strongest argument for this ticket, so it belongs here rather than in a status update.I did not touch that job's logic (out of scope, and PR #2008 fixes it differently — a VALUES-join UPDATE that removes the array bind entirely, not a call to
intArrayLiteral). See "Suppressing the one known instance" below for how the rule still ships clean.Summary
intArrayLiteral(alreadyexported fromjobs/artist-unicode-dedup/merge.ts) toshared/database/src/int-array-literal.ts, hardened to validate every element viaNumber+Number.isIntegerand throw on a non-integer input instead of splicing raw text. Update all six call sites (jobs/artist-unicode-dedup/merge.ts,jobs/flowsheet-ghost-row-sweep/orchestrate.ts,jobs/flowsheet-metadata-backfill/orchestrate.ts,jobs/legacy-dj-name-remediation/job.ts,jobs/album-critic-reviews-etl/antijoin.ts,shared/database/src/library-tiebreak.ts) to import it — no behavior change at any call site.wxyc/no-bare-array-in-sql-template(eslint-rules/), wired intoeslint.config.mjsforapps/**,shared/**,jobs/**,tests/**. It resolves a tagged template'ssqlidentifier through scope analysis to a named import from'drizzle-orm'— never postgres-js — then flags any interpolation whose statically-known type is an array.docs/bulk-update-playbook.md.Why type-aware, not AST-only
The issue allowed a narrower AST-only fallback ("does
${}sit textually insideANY(...)") if type information proved impractical. I built that version first and rejected it: it cannot distinguish the fixed shape (ANY(${idArrayLiteral}::int[]),idArrayLiteral: string) from the historical bug (ANY(${ids}),ids: number[]) — both are syntactically${Identifier}immediately afterANY(. Shipping the position-only version would have flagged every one of the six call sites this PR just fixed, at the exact moment they became correct. Type information isn't an optional enhancement to fall back away from here, either — this repo'seslint.config.mjsalready requires it project-wide (tseslint.configs.recommendedTypeChecked+parserOptions.projectService: true), so the rule usesESLintUtils.getParserServices+@typescript-eslint/type-utils'sisTypeArrayTypeOrUnionOfArrayTypesand fires on any array-typed interpolation in a confirmed Drizzlesqltemplate — not just the ones insideANY(...), the broader "ideally any${}" form the issue asked for.Proof the rule works
tests/unit/database/no-bare-array-in-sql-template.rule.test.ts— aneslint.RuleTestersuite (followingsource-tagged-constraint.rule.test.ts's existing pattern) running through@typescript-eslint/parserwithprojectService.allowDefaultProjectso a virtual, not-on-disk fixture still gets real type information. 5 valid + 7 invalid cases, including the load-bearing negative case: the identicalANY(${array})syntax under a postgres-js-shaped tag (a local variable, not adrizzle-ormimport) is asserted NOT to fire.I also verified end-to-end against the real
eslint.config.mjs, not just the RuleTester harness: a temporary fixture mirroringlibrary-tiebreak.ts's pre-fix shape failed lint with the rule's message; the fixed shape passed clean. (Fixture was written, linted, then deleted — not part of this diff.)Running the rule across the existing tree, comparing
npm run linton this branch against a cleanorigin/maincheckout with the identical invocation: baseline is 835 problems / 0 errors / 835 warnings; this branch is also 835 problems / 0 errors / 835 warnings after the suppression below. Everything else in the tree is clean — the only place the rule ever fires outside its own test fixtures is the one already-ticketed line.Suppressing the one known instance
A rule that fails on the tree it lands into isn't shippable, and this issue's own acceptance criterion says so directly: "Running the rule across the existing tree surfaces zero unfixed instances." I considered
--no-verifyand waiting for #2008 to merge first, and rejected both —--no-verifyjust relocates the same failure to CI (which will enforce this the moment GitHub Actions recovers from today'smajor_outage), and #2008 is blocked on that exact outage with no ETA, so waiting could stall this PR indefinitely. The failure has to be resolved inside this PR.jobs/va-apple-music-url-remediation/orchestrate.tsnow carries a targeted suppression aroundinvalidateAlbumBatch's onesqlstatement:A single
// eslint-disable-next-linewasn't usable here: the violation (${albumIds}) sits inside a multi-linesqltemplate literal, so a comment on the preceding source line would land inside the template literal's string content — i.e. become part of the SQL text sent to Postgres — rather than function as a JS comment. (I tried this first and caught it immediately: it round-trips as a harmless SQL comment, but it isn't a working ESLint directive, and it isn't something you'd want in a query string.) The disable/re-enable pair brackets exactly this one statement, immediately outside the template literal on both sides — not a file-level/* eslint-disable */, and it exempts nothing else in the job.This repo's flat config default-reports unused disable directives at
warn(linterOptions.reportUnusedDisableDirectivesisn't set, and ESLint 9+'s flat-config default for that option is"warn"— confirmed empirically, not from memory, by linting a throwaway fixture with a directive that suppressed nothing). So once PR #2008 lands and rewrites this statement to a VALUES-join UPDATE that removes the array bind entirely, this pair becomes a live "unused eslint-disable directive" warning — a built-in cleanup nudge for whoever rebases it, not a silent, permanent carve-out. I did not add a suppression for the// eslint-disable-next-lineform's exact wording the issue's reviewer originally suggested, because it isn't syntactically usable at this call site; said so in-line at the suppression site too.Two touched files outside the six call sites — why, and a live overlap with PR #2012
tests/mocks/database.mock.ts: one pure re-export line,export { intArrayLiteral } from '../../shared/database/src/int-array-literal.js';, added in the same style as the existingfoldArtistName/requirePositiveIntre-exports right above it.@wxyc/databaseis fully mapped to this mock in unit tests, so without this line every one of the six call sites' unit tests would fail withintArrayLiteral is not a function. This is orthogonal to the mock's.where()/.limit()query-builder sharp edge (createMockQueryChain,db.execute) — I didn't touch that, and I didn't touchtests/__mocks__/drizzle-orm.tsat all, which is the file that actually owns how an interpolated value gets serialized and is the reason a mocked-DB unit test couldn't have caught #2007 in the first place. This PR doesn't change that serialization behavior in any way; the new rule is a static, authoring-time check specifically because the runtime/mock path can't see this class of bug.tests/integration/artist-unicode-dedup-merge.spec.js: necessary, not incidental —merge.tsno longer exportsintArrayLiteral(moved to@wxyc/database), and this spec had a direct unit-style assertion againstmerge.intArrayLiteral. I updated it to import from@wxyc/databaseinstead and kept an equivalent assertion (full behavioral coverage, including the validation this promotion adds, now lives in the newtests/unit/database/int-array-literal.test.ts).This file is also touched by PR #2012 (
chore/issue-2011, open), which adds abeforeAllpre-clean block to the samedescribe. I confirmed the two changes don't collide: my edits are at the top-levelrequires (a newconst { intArrayLiteral } = require('@wxyc/database');) and in atest(...)block ~60 lines below thedescribe'sbeforeAll; #2012's edit is entirely inside thatbeforeAll's body. I verified this isn't just "looks non-adjacent" —git merge-tree --write-tree HEAD <2012-branch>(read-only preview, no working-tree changes) produced a clean merge with both changes present in the result, no conflict markers, exit 0. Whichever of us merges second will still get a clean auto-merge; flagging the overlap here so it isn't a surprise.Test plan
tests/unit/database/int-array-literal.test.ts— 10 cases (TDD: written and confirmed failing before the helper existed, then implementation, then green)tests/unit/database/no-bare-array-in-sql-template.rule.test.ts— 5 valid + 7 invalid RuleTester casesnpm run lint— 835 problems, 0 errors, 835 warnings (byte-identical to a cleanorigin/maincheckout run the same way)npm run typecheck— cleannpx tsc --noEmit -p jobs/<name>for all 5 touchedjobs/**packages — clean (va-apple-music-url-remediationstill carries its pre-existing, unrelatedTS2554pair — tracked separately as va-apple-music-url-remediation: the cooperative live-DJ pause never pauses, and a probe throw kills the run's summary #2009, untouched by this PR's comment-only edit there)npm run format:check— cleannpm run test:unit— 412 suites / 6519 tests passingGitHub Actions is in a confirmed
major_outageas of 2026-08-06 — this PR will show zero checks; none were triggered, re-run, or polled.Closes #2010