Skip to content

Commit b72226f

Browse files
os-muskclaude
andauthored
fix(driver-sql): declare the indexes key initObjects / registerObjectMetadata already read (#16710)
* fix(driver-sql): declare the `indexes` key `initObjects` / `registerObjectMetadata` already read Both entry points took `Array<{ name; fields?; tenancy? }>` with no `indexes`, while `registerManagedObjectMetadata` read the key out of those very objects through an `(obj as any)` cast and filled `managedObjectIndexes` from it — the map `syncDeclaredIndexes` renders every declared UNIQUE from. The sibling `detectManagedDrift` on the same class already declared `indexes?: any[]`, so the two halves of one class disagreed about the shape of the same input. This is the shape #4311 fixed for `tenancy`, one key over. Nothing tripped over it because TypeScript's excess-property check fires on a fresh object literal and not on one bound to a variable first, and every caller in the package happened to bind first — a green that held for a reason unrelated to correctness. - Add `indexes?: any[]` to `registerObjectMetadata`, `initObjects` and the shared `registerManagedObjectMetadata` helper, spelled as `detectManagedDrift` spells it. - Delete the `as any` at the `managedObjectIndexes.set` read site: the cast was the evidence that the declaration and the read disagreed. - Pin the fresh-object-literal form, which is the only form that can go red on this defect; a variable-bound call measures nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg * fix(driver-sql): drop the two now-dead `as any` casts on the `indexes` parameter Contract-review finding 4. Both sites read `indexes` off the same `initObjects` parameter this change types, so once the signature declares the key the casts state a disagreement that no longer exists — and the changeset's "path closed" sentence is only true with them gone. Both lines sit inside this card's declared region and no other in-flight claim holds them. The remaining casts are deliberate: `ensureShardTable` reads through its own narrow parameter type outside the region, and the `lifecycle` read is the third-key class carried by a separate card. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg * chore(changeset): grade the driver-sql accept-set widening `minor`, and state the narrowing Contract-review findings 1, 2 and 7. Level: the governing text is the WHICH LEVEL maintainer ruling in .github/workflows/pr-automation.yml — "A purely additive widening of a published package's public surface (a new exported symbol on an index, a new accepted key or value) takes at least `minor` ... a `fix(` that widens an index is therefore `minor`" — mechanized by check-changeset-no-major.mjs, which reads a declared clause-② plus a `patch` grade as a self-contradiction inside one PR. The AGENTS.md sentence previously cited is the floor against `none`, not a ceiling, and the historical `patch` precedents are named pre-rule by that same ruling. Text: the previous "no call that compiles today stops compiling" was measured false. The accept set moves both ways — widened for fresh object literals, narrowed for variable-bound `indexes` spelled as a record, a readonly tuple or null, all three of which compiled before and now fail TS2322. Measured in both directions on this package's own tsc. No migration is owed: no caller here is affected, and every newly rejected shape was already discarded at run time by the Array.isArray guard. Scope: names @objectstack/driver-sqlite-wasm, whose SqliteWasmDriver extends SqlDriver and overrides neither method, so both signatures land in its published .d.ts. Same fixed version group, so this is a CHANGELOG effect only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg * docs(changeset): correct the run-time claim — an `as const` tuple compiled AND worked Re-review F1. The changeset said every shape newly rejected at compile time was already discarded at run time by the `Array.isArray` guard. That is false for one of the three shapes it names: `as const` is type-only, so at run time the value is a plain array, `Array.isArray` returns true, and the index WAS synced. Measured against the real driver rather than reasoned about: all three shapes passed through `registerObjectMetadata` with the argument cast to `any`, so the value reaching the driver is exactly what a pre-fix caller passed (the read path is unchanged by this PR — the cast removal is type-only). Result: the `as const` tuple is recorded in `managedObjectIndexes`; the record and `null` are deleted. So the three are now stated apart: a record and `null` never survived the guard and got no index and no diagnostic, while the `readonly` tuple compiled and worked and is now rejected on the type surface alone. No banner and no ADR-0087 disposition are owed, on the two grounds that survive the correction: zero affected callers, and `any[]` already published on `detectManagedDrift` for the same key on the same class. Also pins the narrowing axis, which nothing on the tree covered: two `@ts-expect-error` lines in the existing pin file. They are the assertion — if either stops erroring, tsc fails it as TS2578. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ADLdAs2pVcH17h9tZKWMBg * docs(changeset): state the readonly-tuple narrowing truthfully (#16570) Re-review F1. The changeset's run-time sentence was false for one of the three shapes it names: `as const` is type-only, so the value is a plain array at run time, `Array.isArray` returns true, and the index WAS synced. The three shapes are stated apart — a record and `null` were discarded at run time by the guard and got no index and no diagnostic; the `readonly` tuple compiled and worked and is now rejected on the type surface alone (`TS2322`), with no in-repo caller and no way to have reached `detectManagedDrift` on the same class, which already publishes `any[]` for the same key. This commit records the seat's disposition on that corrected ground, which the restatement had left unstated: no `BREAKING` banner and no ADR-0087 disposition, resting on grounds (i) zero affected callers and (iii) the sibling's already published `any[]` spelling — never on the run-time ground, which is false for the `readonly` tuple. No code change. Gates at this head, verbatim: node scripts/check-changeset-no-major.mjs --base origin/main EXIT=0 node scripts/check-adr-0087-registration.mjs --base origin/main EXIT=0 node scripts/check-empty-changeset.mjs --base origin/main EXIT=0 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TezFG8ZMrNH6n5VTNpPpdH --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent b38821d commit b72226f

3 files changed

Lines changed: 205 additions & 7 deletions

File tree

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
---
2+
"@objectstack/driver-sql": minor
3+
"@objectstack/driver-sqlite-wasm": minor
4+
---
5+
6+
`SqlDriver.initObjects()` and `SqlDriver.registerObjectMetadata()` now declare the `indexes` key they have always read.
7+
8+
Both entry points took `Array<{ name; fields?; tenancy? }>`, with no `indexes` in the type. The key was read out of those very objects one call deep anyway, through an `as any`, in `registerManagedObjectMetadata` — and the map it fills, `managedObjectIndexes`, is what `syncDeclaredIndexes` renders every declared UNIQUE from. So the driver's whole index-sync path was driven by a key its own signature said did not exist, while the sibling `detectManagedDrift` on the same class had always declared `indexes?: any[]`: the two halves of one class disagreed about the shape of the same input.
9+
10+
That is the shape #4311 already addressed for `tenancy`, one key over, and the comment it left above `initObjects` described `indexes` word for word.
11+
12+
**Why nothing tripped over it.** TypeScript's excess-property check fires on a fresh object literal and not on one bound to a variable first, so the same object was accepted or rejected by nothing but where it was spelled — `await driver.initObjects([{ ...bare, indexes: [] }])` was rejected with TS2353, `const o = { ...bare, indexes: [] }; await driver.initObjects([o])` was accepted, and the index was synced either way. Every caller happened to bind first, so the package typechecked green for a reason unrelated to correctness.
13+
14+
**Why this matters beyond a compile error.** The loud symptom was a rejected correct call. The quiet one is the reachable branch: an author — or an AI — reading the signature concludes `indexes` is not accepted and drops the key, and a declared UNIQUE is then never synced, with no error at authoring time and no error at boot. The schema says those rows cannot collide; they can.
15+
16+
What changed, all inside `SqlDriver`:
17+
18+
- `registerObjectMetadata(objects)`, `initObjects(objects)` and the shared `registerManagedObjectMetadata(obj)` helper each gained `indexes?: any[]`, spelled exactly as `detectManagedDrift` already spells it.
19+
- Every `(obj as any)` cast reading `indexes` off those parameters is gone — the one at the `managedObjectIndexes.set` site and the two inside `initObjects`' own create/alter path. The cast was the evidence that the declaration and the read disagreed; leaving any of them would have fixed the signature while keeping the "the type does not admit me but I read it anyway" path alive. That path is now closed on this parameter.
20+
21+
**What the accept set does, precisely — it moves in both directions.** For a **fresh object literal**, which is what an author writes and what the excess-property check judges, this is purely a widening: `{ ...bare, indexes: [...] }` was rejected and is now accepted. For a **variable-bound** argument, which bypasses that check and is judged by ordinary assignability, it is a narrowing: `indexes` spelled as a record, as a `readonly` tuple (`as const`), or as `null` compiled under the previous signatures and is now rejected with TS2322. Measured in both directions, all three shapes, on this package's own `tsc`.
22+
23+
That narrowing is deliberate, and the three shapes did **not** all behave the same way before it — the difference is worth stating exactly, because only one of them ever worked:
24+
25+
- A **record** and **`null`** never survived the `Array.isArray(obj.indexes)` guard the driver has always applied. That author got no index and no diagnostic — silently, at run time. Rejecting those two at compile time is precisely the failure this change exists to make impossible.
26+
- A **`readonly` tuple (`as const`)** is a different case, and the only one with anything to lose. `as const` is type-only: at run time the value is a plain array, `Array.isArray` returns `true`, and the index **was** synced. That caller compiled and worked, and is now rejected at compile time. Nothing about its run-time behaviour changed — the rejection is entirely on the type surface.
27+
28+
No migration is owed even so. No caller in this repository is affected, and the shape could never have reached `detectManagedDrift` on the same class either, which publishes the very same `any[]` spelling for the very same key — so a `readonly` caller was already unable to use half of this driver's declared-index surface. A caller in that position spells the array without `as const`, or widens it at the call site.
29+
30+
The disposition on that corrected ground, recorded here because the ground itself moved: **no `BREAKING` banner and no ADR-0087 disposition**, resting on grounds (i) and (iii) alone — zero affected callers, and the `any[]` spelling already published on `detectManagedDrift` for the same key on the same class. The ground that every newly rejected shape had already been discarded at run time is **not** among them: it is false for the `readonly` tuple, and nothing here leans on it.
31+
32+
`@objectstack/driver-sqlite-wasm` is named because `SqliteWasmDriver extends SqlDriver` and overrides neither method, so both widened signatures land in its own published `.d.ts` and its consumers see the identical change. The two packages are in the same fixed version group, so this is a CHANGELOG effect rather than a version one.
33+
34+
The `IDataDriver` contract itself did not move: `registerObjectMetadata?(schemas: unknown[])` in `@objectstack/spec` already accepted `unknown[]`, and `SqlDriver` narrowed it on its own. What grew is `SqlDriver`'s own published accept set.
Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#16570] `initObjects` and `registerObjectMetadata` accept `indexes` — the
5+
* key they have always READ — spelled as a **fresh object literal**.
6+
*
7+
* ## The defect this pins
8+
*
9+
* Both entry points declared `Array<{ name; fields?; tenancy? }>`, with no
10+
* `indexes`. The key was read one call deep anyway, through an `as any`, in
11+
* `registerManagedObjectMetadata`:
12+
*
13+
* ```ts
14+
* this.managedObjectIndexes.set(tableName, (obj as any).indexes);
15+
* ```
16+
*
17+
* and `managedObjectIndexes` is what `syncDeclaredIndexes` renders every
18+
* declared UNIQUE from — so the driver's whole index-sync path was driven by a
19+
* key its own signature said did not exist. `detectManagedDrift`, on the same
20+
* class, had always declared `indexes?: any[]`: the two halves of one class
21+
* disagreed about the shape of the same input. That is the shape #4311 already
22+
* fixed for `tenancy`, and the comment it left above `initObjects` described
23+
* `indexes` word for word.
24+
*
25+
* ## Why the FORM of this pin is the whole point
26+
*
27+
* TypeScript's excess-property check fires on a **fresh object literal** and
28+
* not on one bound to a variable first, so the same object was accepted or
29+
* rejected by where it was spelled:
30+
*
31+
* ```ts
32+
* await driver.initObjects([{ ...bare, indexes: [] }]); // TS2353
33+
* const withoutIndex = { ...bare, indexes: [] };
34+
* await driver.initObjects([withoutIndex]); // accepted
35+
* ```
36+
*
37+
* Every existing caller in this package happened to bind first — one of them
38+
* (`sql-driver-11794-richtext-text-family.test.ts`) even wrote the workaround
39+
* down: *"Hoisted (not an inline literal) … `indexes` rides through
40+
* `initObjects` beyond its narrow parameter type"*. So the package typechecked
41+
* green for a reason unrelated to correctness, and a **variable-bound pin
42+
* cannot go red on this defect** — it measures nothing. Every call below is
43+
* therefore an inline literal in argument position, which is what makes
44+
* `tsc --noEmit` (this package's `typecheck` script) the instrument that
45+
* measures it: revert either signature and these lines stop compiling with
46+
*
47+
* TS2353: Object literal may only specify known properties, and 'indexes'
48+
* does not exist in type '{ name: string; fields?: Record<string, any>
49+
* | undefined; tenancy?: any; }'.
50+
*
51+
* The runtime assertions are the other half: they prove the key is not merely
52+
* *admitted* by the type but still *read* — recorded in `managedObjectIndexes`
53+
* (§1), rendered into a physical UNIQUE (§2), and cleared when withdrawn (§3).
54+
* A signature relaxation that quietly stopped reading the key would pass the
55+
* compile leg alone.
56+
*
57+
* Runs on the always-available in-memory SQLite cell: the defect is in a
58+
* parameter type and in the registry it feeds, neither of which is
59+
* dialect-specific.
60+
*/
61+
62+
import { describe, it, expect, afterEach } from 'vitest';
63+
import { SqlDriver } from './sql-driver.js';
64+
import { dialectCell } from './live-dialect-matrix.testkit.js';
65+
66+
const SQLITE = dialectCell('sqlite');
67+
68+
/**
69+
* The un-indexed base object, deliberately WITHOUT `indexes` — every call site
70+
* below spreads it and writes `indexes` inline, so the literal being checked is
71+
* fresh in argument position. `tenancy: { enabled: false }` keeps the declared
72+
* UNIQUE on the plain (non-tenant-scoped) path, and the bounded `maxLength`
73+
* keeps `v` a keyable varchar rather than an unbounded TEXT.
74+
*/
75+
const bareObject = (name: string) => ({
76+
name,
77+
tenancy: { enabled: false },
78+
fields: { v: { type: 'text', maxLength: 64 } },
79+
});
80+
81+
/** What the driver recorded for `table`, read off the protected registry. */
82+
const recordedIndexes = (driver: SqlDriver, table: string): unknown =>
83+
(driver as unknown as { managedObjectIndexes: Map<string, unknown> }).managedObjectIndexes.get(table);
84+
85+
describe('initObjects / registerObjectMetadata accept `indexes` as a fresh object literal (#16570)', () => {
86+
let driver: SqlDriver | undefined;
87+
afterEach(async () => {
88+
await driver?.disconnect().catch(() => {});
89+
driver = undefined;
90+
});
91+
92+
it('§1 registerObjectMetadata: the inline literal compiles AND the key is recorded', async () => {
93+
const T = 'os16570_register';
94+
driver = new SqlDriver(SQLITE.config());
95+
const declared = [{ fields: ['v'], unique: true as const, name: `uniq_${T}_v` }];
96+
97+
// Fresh literal in argument position — not hoisted to a variable first.
98+
driver.registerObjectMetadata([{ ...bareObject(T), indexes: declared }]);
99+
100+
expect(recordedIndexes(driver, T)).toEqual(declared);
101+
});
102+
103+
it('§2 initObjects: the inline literal compiles AND the declared UNIQUE is physically synced', async () => {
104+
const T = 'os16570_init';
105+
driver = new SqlDriver(SQLITE.config());
106+
107+
// Fresh literal in argument position.
108+
await driver.initObjects([
109+
{ ...bareObject(T), indexes: [{ fields: ['v'], unique: true as const, name: `uniq_${T}_v` }] },
110+
]);
111+
112+
const knex = (driver as unknown as { knex: (t: string) => any }).knex;
113+
await knex(T).insert({ id: 'a', v: 'same' });
114+
// If `indexes` had been dropped at authoring time — the silent failure mode
115+
// this card is about — this second row would be accepted.
116+
await expect(knex(T).insert({ id: 'b', v: 'same' })).rejects.toThrow();
117+
});
118+
119+
it('§3 initObjects: the exact `{ ...bare, indexes: [] }` spelling from the card compiles, and withdraws the entry', async () => {
120+
const T = 'os16570_withdraw';
121+
driver = new SqlDriver(SQLITE.config());
122+
const bare = bareObject(T);
123+
124+
await driver.initObjects([{ ...bare, indexes: [{ fields: ['v'], unique: true as const, name: `uniq_${T}_v` }] }]);
125+
expect(recordedIndexes(driver, T)).toHaveLength(1);
126+
127+
// The spelling named in the card, verbatim: an empty array must CLEAR the
128+
// entry, not leave the previous one standing.
129+
await driver.initObjects([{ ...bare, indexes: [] }]);
130+
expect(recordedIndexes(driver, T)).toEqual([]);
131+
});
132+
});
133+
134+
/**
135+
* The other half of the accept set. A variable-bound argument bypasses the
136+
* excess-property check and is judged by ordinary assignability, so `indexes`
137+
* must be an array — the two `@ts-expect-error`s ARE the assertion here: if
138+
* either line stops erroring, `tsc` fails it as TS2578. Compile-time only,
139+
* deliberately never called.
140+
*/
141+
export async function pinsTheNarrowingAxis(driver: SqlDriver): Promise<void> {
142+
const asRecord = { ...bareObject('os16570_narrow'), indexes: { uniq_v: { fields: ['v'] } } };
143+
// @ts-expect-error TS2322 — a record is not `any[]`, and never synced an index at run time.
144+
await driver.initObjects([asRecord]);
145+
146+
const asNull = { ...bareObject('os16570_narrow'), indexes: null };
147+
// @ts-expect-error TS2322 — `null` is not `any[]`, and never synced an index at run time.
148+
await driver.initObjects([asNull]);
149+
}

packages/drivers/driver-sql/src/sql-driver.ts

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9735,7 +9735,7 @@ export class SqlDriver implements IDataDriver {
97359735
* which `initObjects` goes on to use for its DDL.
97369736
*/
97379737
protected registerManagedObjectMetadata(
9738-
obj: { name: string; fields?: Record<string, any>; tenancy?: any },
9738+
obj: { name: string; fields?: Record<string, any>; tenancy?: any; indexes?: any[] },
97399739
): { tableName: string; tenantField: string | null } {
97409740
const tableName = StorageNameMapping.resolveTableName(obj);
97419741
// #2186: remember the authoritative metadata field set for this table so
@@ -9744,8 +9744,8 @@ export class SqlDriver implements IDataDriver {
97449744
// Always overwrite — a metadata change that REMOVES `indexes` must clear
97459745
// the previous entry, or drift detection keeps expecting an index nobody
97469746
// declares any more (and never reports it as orphaned).
9747-
if (Array.isArray((obj as any).indexes)) {
9748-
this.managedObjectIndexes.set(tableName, (obj as any).indexes);
9747+
if (Array.isArray(obj.indexes)) {
9748+
this.managedObjectIndexes.set(tableName, obj.indexes);
97499749
} else {
97509750
this.managedObjectIndexes.delete(tableName);
97519751
}
@@ -9865,7 +9865,7 @@ export class SqlDriver implements IDataDriver {
98659865
* Idempotent: pure metadata assignment, safe to re-drive on every reload.
98669866
*/
98679867
registerObjectMetadata(
9868-
objects: Array<{ name: string; fields?: Record<string, any>; tenancy?: any }>,
9868+
objects: Array<{ name: string; fields?: Record<string, any>; tenancy?: any; indexes?: any[] }>,
98699869
): void {
98709870
for (const obj of objects) this.registerManagedObjectMetadata(obj);
98719871
}
@@ -9876,7 +9876,22 @@ export class SqlDriver implements IDataDriver {
98769876
// undeclared here until #4311 (`registerExternalObject` and
98779877
// `computeAndRecordTenantField` both had it), so a caller spelling the key
98789878
// correctly was rejected by the type while the driver read it regardless.
9879-
async initObjects(objects: Array<{ name: string; fields?: Record<string, any>; tenancy?: any }>): Promise<void> {
9879+
//
9880+
// `indexes` is the same story, one key over, and it went undeclared here for
9881+
// longer: `registerManagedObjectMetadata` fills `managedObjectIndexes` from
9882+
// it, and that map is what `syncDeclaredIndexes` renders every declared
9883+
// UNIQUE from — so the whole index-sync path was driven by a key this
9884+
// signature said did not exist, reached through an `as any`. The sibling
9885+
// `detectManagedDrift` on this class had always declared it, so the two
9886+
// halves disagreed about the shape of the same input. Nothing tripped over
9887+
// it because TypeScript's excess-property check fires on a FRESH object
9888+
// literal and not on one bound to a variable first, and every caller here
9889+
// happened to bind first — a green that held for a reason unrelated to
9890+
// correctness. `src/sql-driver-16570-init-objects-indexes-param.test.ts`
9891+
// pins the fresh-literal form so it cannot silently go back.
9892+
async initObjects(
9893+
objects: Array<{ name: string; fields?: Record<string, any>; tenancy?: any; indexes?: any[] }>,
9894+
): Promise<void> {
98809895
// In-memory registration FIRST, and deliberately ahead of the DDL gate
98819896
// below: being refused permission to alter a schema is not a reason to stay
98829897
// ignorant of the objects we were just told about. On a datasource we are a
@@ -9974,7 +9989,7 @@ export class SqlDriver implements IDataDriver {
99749989
table: tableName,
99759990
fields: obj.fields ?? {},
99769991
tenantField,
9977-
declaredIndexes: (obj as any).indexes,
9992+
declaredIndexes: obj.indexes,
99789993
});
99799994

99809995
if (!exists) {
@@ -10049,7 +10064,7 @@ export class SqlDriver implements IDataDriver {
1004910064
// referenced column physically exists — which is also why field-level
1005010065
// `unique` can no longer be emitted inline by `createColumn`: a composite
1005110066
// needs the tenant column to already be there.
10052-
const declaredIndexes = (obj as any).indexes;
10067+
const declaredIndexes = obj.indexes;
1005310068
const uniqueFields = Object.values<any>(obj.fields ?? {}).some((f) =>
1005410069
isUniqueScopeDeclared(f?.unique),
1005510070
);

0 commit comments

Comments
 (0)