Skip to content

Commit 0e0b566

Browse files
dmealingclaude
andcommitted
fix(codegen-ts): honour an index's physical attributes, and gate the comments that delegate to a test by name
Two items, both about a guarantee that was stated but not kept. INDEX SHAPE. The two index emitters were reconciled on NAME earlier in this stream; they still disagreed on SHAPE, and the disagreement was invisible because it lived in attributes codegen never read at all. `@expr`, `@using`, `@orders` and `@where` all reach migrate and none reached this file: - an @expr-only index emitted NOTHING, because both loops required a non-empty @fields while migrate keys an expression index off @expr; - @where never arrived, so a PARTIAL unique index read as a FULLY unique one — the generated schema declares a constraint stricter than the database has, and `drizzle-kit push` proposes replacing the real index with the wrong one; - @orders was dropped, so a DESC index read as ASC. The last one had a test pinning it. `index-lookup emits .on(table.customerId, table.placedAt)` asserted the direction was ABSENT, over a fixture whose own comment says "non-unique composite on customerId + placedAt DESC" and which sets @orders ["asc","desc"]. The fixture knew the intent and the assertion pinned the defect. The two loops are now ONE function, because they were near-identical and migrate treats the two shapes identically apart from uniqueness — a rule written twice is a rule that can be forgotten twice, which is precisely how these attributes came to be honoured by neither. Migrate's exact conventions are mirrored, including the two that look like details and are not: a `btree` @using is omitted (migrate treats it as the default, so emitting it would be a spurious difference) and an all-ascending @orders serializes as no orders at all (same reason). sqlite drops @expr rather than emitting it wrong — its `.on()` takes columns, not SQL, so the only available spelling would index a COLUMN instead of the expression, which is a worse answer than an absent index. @where works on both dialects. TEST-FILE REFERENCES. Six comments across two packages delegate a guarantee to `secondary-index-name-parity.test.ts` by bare filename. Writing that is right — it swaps an assertion the reader must trust for one they can run — but nothing connected the two, so a rename would leave all six reading as live guarantees while pointing at nothing. That is worse than never promising: the comment still tells the next reader the case is covered. `scripts/check-test-references.sh` resolves every backticked `<name>.test.ts` mention by BASENAME (so moving a test between directories, which breaks no promise, does not fail it) and prints every referring site when one goes missing. Proved by renaming the file and watching it name all six, then restoring. Ten references today, all resolving. That script's first draft had the same defect as the coverage script fixed earlier in this stream, from the other direction: a quoted git pathspec matched nothing, `git grep` exits 1 on no matches, and under `set -euo pipefail` the assignment aborted — so it exited 1 having printed NOTHING. A check that fails silently is no more useful than one that passes wrongly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSCgs7z19w8aGXGiceCohC
1 parent 79a2b6b commit 0e0b566

4 files changed

Lines changed: 199 additions & 13 deletions

File tree

‎scripts/check-test-references.sh‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
#!/usr/bin/env bash
2+
# A comment that names a test file by NAME must name a file that exists.
3+
#
4+
# Several load-bearing comments in this repo delegate a guarantee to a test in ANOTHER
5+
# package — "the two are now compared by codegen-ts's `secondary-index-name-parity.test.ts`
6+
# rather than by a claim in a comment" — which is exactly the right thing to write, because
7+
# it replaces an assertion a reader has to trust with one they can go run.
8+
#
9+
# The coupling is a bare filename across a package boundary, so nothing connects the two.
10+
# Rename or delete the test and every reference to it keeps reading as a live guarantee
11+
# while pointing at nothing, which is strictly worse than never having made the promise:
12+
# the comment still tells the next reader the case is covered.
13+
#
14+
# The check is deliberately narrow. It looks only at BACKTICKED `<name>.test.ts` mentions —
15+
# the convention these comments already use — so ordinary prose about testing cannot trip
16+
# it, and it resolves by BASENAME anywhere under the TS tree rather than by path, so moving
17+
# a test between directories (which breaks no promise) does not fail it. Renaming one does.
18+
set -euo pipefail
19+
cd "$(dirname "$0")/.."
20+
21+
fail=0
22+
# The pathspecs are SHELL globs, deliberately unquoted. Quoting them hands git a literal
23+
# `*` pathspec, which matches nothing here and yields an empty result — and because git grep
24+
# exits 1 on no matches, under `set -euo pipefail` that aborted the assignment and the whole
25+
# script exited 1 having printed NOTHING. A check that fails silently and a check that passes
26+
# are equally useless; `|| true` keeps "no references at all" a legitimate, reported outcome.
27+
refs=$(git grep -ohE '`[a-zA-Z0-9._-]+\.test\.ts`' -- \
28+
server/typescript/packages/*/src server/typescript/packages/*/test 2>/dev/null \
29+
| tr -d '`' | sort -u || true)
30+
31+
count=0
32+
while read -r f; do
33+
[ -z "$f" ] && continue
34+
count=$((count + 1))
35+
if [ -z "$(find server/typescript -name "$f" -print -quit)" ]; then
36+
fail=1
37+
printf ' ✖ a comment names `%s`, and no such file exists.\n' "$f" >&2
38+
printf ' Referenced from:\n' >&2
39+
git grep -n -- "$f" -- server/typescript/packages/*/src server/typescript/packages/*/test \
40+
| sed 's/^/ /' >&2 || true
41+
printf ' Restore the name, or update every reference — a comment that delegates a\n' >&2
42+
printf ' guarantee to a file that is gone still reads as a guarantee.\n' >&2
43+
fi
44+
done <<< "$refs"
45+
46+
if [ "$fail" -ne 0 ]; then exit 1; fi
47+
echo "test-file references: $count named, all resolve"

‎scripts/ci-local.sh‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,15 @@ gate_script_name_hooks() { node scripts/check-script-name-hooks.mjs; }
173173
# sentence stops being true. This checks the wiring itself.
174174
gate_no_magic_coverage() { scripts/check-no-magic-gate-coverage.sh; }
175175

176+
# ── a comment that names a test file must name one that exists ────────────────
177+
# Several load-bearing comments delegate a guarantee to a test in ANOTHER package ("the two
178+
# are now compared by codegen-ts's `secondary-index-name-parity.test.ts` rather than by a
179+
# claim in a comment"), which is the right thing to write — it replaces an assertion the
180+
# reader must trust with one they can run. The coupling is a bare filename across a package
181+
# boundary, so a rename leaves every reference reading as a live guarantee while pointing at
182+
# nothing: worse than never promising, because the comment still says the case is covered.
183+
gate_test_references() { scripts/check-test-references.sh; }
184+
176185
# ── release hygiene: no pre-release version may be committed ──────────────────
177186
# scripts/prerelease.mjs bumps every version declaration in place and restores them on
178187
# exit; a crashed run (or a hand-run sed) can leave an -rc.N behind. That is not cosmetic:
@@ -548,6 +557,7 @@ if want gates; then step "publish-set parity" gate_publish_set;
548557
if want gates; then step "no committed pre-release version" gate_no_prerelease_versions; fi
549558
if want gates; then step "script-name hook collisions" gate_script_name_hooks; fi
550559
if want gates; then step "no-magic gate wired (5 ports)" gate_no_magic_coverage; fi
560+
if want gates; then step "test-file references resolve" gate_test_references; fi
551561
if want gates; then step "metamodel-version bump" gate_metamodel_version; fi
552562
if want gates; then step_if bun "peer-range bounds" gate_peer_ranges; fi
553563
if want gates; then step_if bun "shipped doc examples load" gate_doc_examples; fi

‎server/typescript/packages/codegen-ts/src/templates/drizzle-schema.ts‎

Lines changed: 75 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import { MetaObject, MetaField, MetaIndex, isMetaObject, stripPackage } from "@m
77
import {
88
FIELD_SUBTYPE_LONG,
99
IDENTITY_ATTR_FIELDS, IDENTITY_ATTR_GENERATION,
10+
IDENTITY_ATTR_EXPR, IDENTITY_ATTR_USING, IDENTITY_ATTR_ORDERS, IDENTITY_ATTR_WHERE,
1011
GENERATION_INCREMENT, GENERATION_UUID,
1112
FIELD_ATTR_AUTO_SET,
1213
FIELD_ATTR_OBJECT_REF,
@@ -235,10 +236,78 @@ export function renderDrizzleSchema(obj: MetaObject, ctx: RenderContext): Code {
235236
callbackEntries.push(buildCompositeKeyCallback(pkFieldNames, importModule));
236237
}
237238

239+
/**
240+
* One index callback entry, for BOTH `identity.secondary` (unique) and `index.lookup`
241+
* (non-unique). The two shapes differ only in that boolean, and migrate treats their
242+
* physical attributes identically — `@expr`/`@using`/`@orders`/`@where` are registered on
243+
* both by the same provider — so they are emitted by ONE function here. They used to be
244+
* two near-identical loops, which is how the physical attributes came to be honoured by
245+
* neither: a rule written twice is a rule that can be forgotten twice.
246+
*
247+
* What was missing is a SHAPE divergence, not a naming one (the names were reconciled
248+
* separately). An `@expr`-only index emitted NOTHING at all here, because both loops
249+
* required a non-empty `@fields` — while migrate emits it, keying off `@expr`. And
250+
* `@where` never reached this file, so a PARTIAL unique index read as a FULLY unique one
251+
* in the generated schema: `drizzle-kit push` would propose replacing the database's
252+
* partial index with a total one, and any diff written from generated source disagreed
253+
* with the database over a difference that was not real. Same class as the sqlite
254+
* emitter dropping these in 0.15.21.
255+
*/
256+
const indexEntry = (
257+
node: { name: string; attr: (n: string) => unknown },
258+
fieldNames: string[],
259+
unique: boolean,
260+
): Code | undefined => {
261+
const str = (n: string): string | undefined => {
262+
const raw = node.attr(n);
263+
return typeof raw === "string" && raw.trim().length > 0 ? raw.trim() : undefined;
264+
};
265+
const expr = str(IDENTITY_ATTR_EXPR);
266+
// An expression index keys off @expr; a plain one needs @fields. Mirrors migrate's
267+
// `if (fieldNames.length === 0 && expr === undefined) continue`.
268+
if (fieldNames.length === 0 && expr === undefined) return undefined;
269+
270+
const sqlSym = imp("sql@drizzle-orm");
271+
const indexSym = imp(`${unique ? "uniqueIndex" : "index"}@${importModule}`);
272+
273+
// @orders is positional against @fields; migrate only serializes it when at least one
274+
// entry is `desc`, because an all-ascending array must be indistinguishable from no
275+
// orders at all for diff stability. Match that exactly or the two disagree on a
276+
// no-op.
277+
const ordersRaw = node.attr(IDENTITY_ATTR_ORDERS);
278+
const orders = Array.isArray(ordersRaw)
279+
? fieldNames.map((_, i) => (ordersRaw[i] === "desc" ? "desc" : "asc"))
280+
: [];
281+
const anyDesc = orders.some((o) => o === "desc");
282+
const colRefs = fieldNames.map((f, i) =>
283+
anyDesc ? `table.${f}.${orders[i] === "desc" ? "desc" : "asc"}()` : `table.${f}`);
284+
285+
// sqlite's `.on()` accepts only columns — no SQL — so an expression index is not
286+
// expressible there. It is skipped rather than emitted wrong; migrate still creates it,
287+
// and a Drizzle declaration that silently indexed a COLUMN instead of the expression
288+
// would be a worse answer than an absent one.
289+
if (expr !== undefined && dialect === "sqlite") return undefined;
290+
291+
// @using selects the access method and takes the columns itself, so it REPLACES .on().
292+
// Postgres-only: SQLite has no USING clause. migrate omits a `btree` value as the
293+
// default, and so does this — emitting it would be a spurious difference.
294+
const using = dialect === "sqlite" ? undefined : str(IDENTITY_ATTR_USING);
295+
const target: Code =
296+
expr !== undefined ? code`${sqlSym}\`${expr}\``
297+
: code`${colRefs.join(", ")}`;
298+
const head: Code =
299+
using !== undefined && using !== "btree"
300+
? code`${indexSym}(${JSON.stringify(node.name)}).using(${JSON.stringify(using)}, ${target})`
301+
: code`${indexSym}(${JSON.stringify(node.name)}).on(${target})`;
302+
303+
// @where — the partial-index predicate, available on both dialects.
304+
const where = str(IDENTITY_ATTR_WHERE);
305+
return where === undefined ? head : code`${head}.where(${sqlSym}\`${where}\`)`;
306+
};
307+
238308
// identity.secondary — always unique (uniqueness is in the type, no boolean).
239309
for (const sec of secondaryIdentities) {
240310
const fields = sec.attr(IDENTITY_ATTR_FIELDS) as string[] | undefined;
241-
if (!Array.isArray(fields) || fields.length === 0) continue;
242311
// The identity's OWN name, which is what migrate puts in the database
243312
// (`expected-schema.ts`, secondary-identity pass) and what `index.lookup` three
244313
// blocks below already emits. This used to compose `idx_<table>_<col>...` by running
@@ -248,21 +317,16 @@ export function renderDrizzleSchema(obj: MetaObject, ctx: RenderContext): Code {
248317
// over a field NAME cannot see `@column`, so a declared physical column was invisible
249318
// to it. Gated by `secondary-index-name-parity.test.ts`, whose fixture is de-blinded
250319
// against exactly that second defect.
251-
const indexName = sec.name;
252-
const indexSym = imp(`uniqueIndex@${importModule}`);
253-
// Use the callback param `table` (not the outer varName) so TS doesn't see
320+
// Column refs use the callback param `table` (not the outer varName) so TS does not see
254321
// the table referencing itself inside its own initializer (TS7022/TS7024).
255-
const cols = fields.map((f) => `table.${f}`).join(", ");
256-
callbackEntries.push(code`${indexSym}(${JSON.stringify(indexName)}).on(${cols})`);
322+
const entry = indexEntry(sec, Array.isArray(fields) ? fields : [], /* unique */ true);
323+
if (entry !== undefined) callbackEntries.push(entry);
257324
}
258325

259326
// index.lookup — always non-unique. Use MetaIndex.fields() per ADR-0039.
260327
for (const lookup of obj.lookupIndexes()) {
261-
const fields = lookup.fields();
262-
if (fields.length === 0) continue;
263-
const indexSym = imp(`index@${importModule}`);
264-
const cols = fields.map((f) => `table.${f}`).join(", ");
265-
callbackEntries.push(code`${indexSym}(${JSON.stringify(lookup.name)}).on(${cols})`);
328+
const entry = indexEntry(lookup, lookup.fields(), /* unique */ false);
329+
if (entry !== undefined) callbackEntries.push(entry);
266330
}
267331

268332
// Emit table-level CHECK constraints for enum fields. Both halves are composed by

‎server/typescript/packages/codegen-ts/test/index-lookup-codegen.test.ts‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ describe("renderDrizzleSchema — index.lookup emits non-unique index", () => {
9898
expect(out).toContain('"orders_customer_placed_idx"');
9999
});
100100

101-
test("index.lookup emits .on(table.customerId, table.placedAt)", () => {
101+
test("index.lookup emits .on(table.customerId.asc(), table.placedAt.desc()) — @orders honoured", () => {
102102
const order = makeOrderWithLookupIndex();
103103
const root = makeRoot([order]);
104104
const ctx = makeRenderContext({
@@ -110,7 +110,72 @@ describe("renderDrizzleSchema — index.lookup emits non-unique index", () => {
110110
relationMap: buildRelationMap(root),
111111
});
112112
const out = renderDrizzleSchema(root.findObject("Order")!, ctx).toString();
113-
expect(out).toMatch(/\.on\(table\.customerId,\s*table\.placedAt\)/);
113+
// The fixture's own comment says "customerId + placedAt DESC" and it sets
114+
// @orders ["asc","desc"], yet this assertion used to require the direction to be
115+
// ABSENT — pinning the defect rather than the intent. migrate emits the DESC, so the
116+
// generated schema declared a differently-ordered index than the database has.
117+
expect(out).toMatch(/\.on\(table\.customerId\.asc\(\),\s*table\.placedAt\.desc\(\)\)/);
118+
});
119+
120+
// The physical escapes (@expr / @where / @using) reached migrate and never reached this
121+
// file at all, which is a SHAPE divergence rather than a naming one: a PARTIAL unique index
122+
// read as a fully unique one in the generated schema, and an @expr-only index emitted
123+
// nothing whatsoever because both loops required a non-empty @fields. `drizzle-kit push`
124+
// would then propose replacing the database's index with the wrong one.
125+
test("@where emits a partial index — a partial UNIQUE must not read as a total one", () => {
126+
const user = makeUserWithSecondary();
127+
const sec = user.children().find((c) => c.name === "idx_email")!;
128+
sec.setAttr("where", "deleted_at IS NULL");
129+
const root = makeRoot([user]);
130+
const ctx = makeRenderContext({
131+
dialect: "postgres", loadedRoot: root, outDir: "/x", dbImport: "~/db",
132+
pkMap: buildPkMap(root), relationMap: buildRelationMap(root),
133+
});
134+
const out = renderDrizzleSchema(root.findObject("User")!, ctx).toString();
135+
expect(out).toMatch(/\.where\(sql`deleted_at IS NULL`\)/);
136+
});
137+
138+
test("@expr emits an expression index, which used to emit NOTHING", () => {
139+
const user = makeUserWithSecondary();
140+
const sec = user.children().find((c) => c.name === "idx_email")!;
141+
// An expression index keys off @expr, not @fields — migrate emits it on @expr alone.
142+
sec.setAttr("fields", []);
143+
sec.setAttr("expr", "lower(email)");
144+
const root = makeRoot([user]);
145+
const ctx = makeRenderContext({
146+
dialect: "postgres", loadedRoot: root, outDir: "/x", dbImport: "~/db",
147+
pkMap: buildPkMap(root), relationMap: buildRelationMap(root),
148+
});
149+
const out = renderDrizzleSchema(root.findObject("User")!, ctx).toString();
150+
expect(out).toContain('uniqueIndex("idx_email")');
151+
expect(out).toMatch(/\.on\(sql`lower\(email\)`\)/);
152+
});
153+
154+
test("@using selects the access method and replaces .on()", () => {
155+
const user = makeUserWithSecondary();
156+
const sec = user.children().find((c) => c.name === "idx_email")!;
157+
sec.setAttr("using", "gin");
158+
const root = makeRoot([user]);
159+
const ctx = makeRenderContext({
160+
dialect: "postgres", loadedRoot: root, outDir: "/x", dbImport: "~/db",
161+
pkMap: buildPkMap(root), relationMap: buildRelationMap(root),
162+
});
163+
const out = renderDrizzleSchema(root.findObject("User")!, ctx).toString();
164+
expect(out).toMatch(/\.using\("gin", table\.email\)/);
165+
});
166+
167+
test("@using btree is omitted — migrate treats it as the default, so emitting it would differ", () => {
168+
const user = makeUserWithSecondary();
169+
const sec = user.children().find((c) => c.name === "idx_email")!;
170+
sec.setAttr("using", "btree");
171+
const root = makeRoot([user]);
172+
const ctx = makeRenderContext({
173+
dialect: "postgres", loadedRoot: root, outDir: "/x", dbImport: "~/db",
174+
pkMap: buildPkMap(root), relationMap: buildRelationMap(root),
175+
});
176+
const out = renderDrizzleSchema(root.findObject("User")!, ctx).toString();
177+
expect(out).not.toContain(".using(");
178+
expect(out).toMatch(/\.on\(table\.email\)/);
114179
});
115180

116181
test("index.lookup does NOT add .unique() to the indexed columns", () => {

0 commit comments

Comments
 (0)