From 57e6a7ccbf0f80bdee4a355cb2247959f5ee8dc1 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 23 Aug 2026 02:50:12 +0000 Subject: [PATCH] docs(service-datasource): make the primaryKeyReader docblock true again after the driver aligned to spec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `primaryKeyReader` docblock is the one place this seam's reasoning is written down, and two of its statements went false when the driver was aligned to the `packages/spec` introspection contract (`95437e7d2d`, #10676/#10998) and when the SQLite composite-key truncation was repaired (#10997, PR #11104). Comment-only. No executable line changes. - The producer table said `SqlDriver` spells the per-column signal `isPrimary`. It emits `primaryKey` and no `isPrimary` at all. - The `#10997` note said the SQLite composite-key truncation "is upstream of this seam and is not repaired here". It was repaired; `introspectPrimaryKeys` now reports every member of a composite key in declared key order. - The closing note said reading the extra spellings structurally was needed "because reconciling `objectql/src/util.ts` with the spec contract is a spec-owned change". That reconciliation has happened. - The `refreshCatalog` seam comment repeated the stale claim that "a real driver spells this `isPrimary` / `primaryKeys`, never `primaryKey`". - The seam suite's second `describe` said `SqlDriver` derives `isPrimary` FROM `primaryKeys`; it derives `primaryKey`. The union read is UNCHANGED — all three arms stay. `table.primaryKeys` is live and independent: after #10997 it is the only signal here that carries a composite key in declared key order, which a per-column boolean cannot express. The `isPrimary` arm is now recorded as a deliberate compatibility belt rather than a bridge to a live producer, with the measurement that decided it, so the next reader does not re-derive it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 --- .../external-introspection-seam.test.ts | 32 +++-- .../src/external-datasource-service.ts | 111 +++++++++++------- 2 files changed, 91 insertions(+), 52 deletions(-) diff --git a/packages/services/service-datasource/src/__tests__/external-introspection-seam.test.ts b/packages/services/service-datasource/src/__tests__/external-introspection-seam.test.ts index 0ac0a62803..d261d5ff89 100644 --- a/packages/services/service-datasource/src/__tests__/external-introspection-seam.test.ts +++ b/packages/services/service-datasource/src/__tests__/external-introspection-seam.test.ts @@ -101,10 +101,15 @@ describe('the introspection seam, as a real SqlDriver actually spells it', () => // so that it would redden the moment the driver was aligned. That flip has // now happened (#10676/#10998), and it is pinned here in its new // direction: the driver emits the spec spelling and no longer emits the - // retired one. The union read in `primaryKeyReader` below therefore no - // longer has an in-tree producer needing its `isPrimary` arm; collapsing - // it is this lane's call to make, deliberately not made from the driver - // change, and until then this file keeps both arms pinned. + // retired one. The union read in `primaryKeyReader` therefore has no + // in-tree producer left for its `isPrimary` arm — and that arm stays + // anyway. The services lane made that call in #11123: the seam's producer + // population is open by design (a host builds the driver, and the handle + // types introspection as `Promise`), so the compiler channel the + // retirement relies on cannot reach a host-built driver still emitting the + // old spelling. Dropping the arm is a narrowing of accepted input, not a + // dead-code deletion. `primaryKeyReader`'s docblock carries the full + // measurement; this file keeps all the arms pinned. expect(table.primaryKeys).toEqual(['id']); expect(id.primaryKey).toBe(true); expect(id.isPrimary).toBeUndefined(); @@ -154,13 +159,18 @@ describe('the introspection seam, as a real SqlDriver actually spells it', () => describe('the seam read, where the two spellings disagree', () => { /** * No in-tree driver produces a disagreement — `SqlDriver` derives - * `isPrimary` FROM `primaryKeys`, so the two always agree. This case is - * therefore hand-built ON PURPOSE, and it is the one place in this file - * where that is the right instrument: it fixes the behaviour under a - * disagreement no live database can currently stage, so a future producer - * that fills only one of the two signals cannot silently lose half a - * composite key. The spelling seam itself is pinned above, against a real - * driver, where a fake would have been blind. + * `primaryKey` FROM `primaryKeys`, so its two signals always agree, and + * since #10676/#10998 it does not emit `isPrimary` at all. These two cases + * are therefore hand-built ON PURPOSE, and it is the one place in this file + * where that is the right instrument: no live in-tree database can stage + * this disagreement, and feeding the retired spelling is the ONLY exercise + * the union's `isPrimary` arm now has anywhere in the tree. They pin the + * behaviour a host-built driver still emitting that spelling depends on — + * see `primaryKeyReader`'s docblock for why the arm is kept rather than + * collapsed (#11123) — and they pin that a producer filling only one of the + * two signals cannot silently lose half a composite key. If the arm is ever + * retired, these two cases go with it. The spelling seam itself is pinned + * above, against a real driver, where a fake would have been blind. */ function serviceOverRaw(table: unknown): ExternalDatasourceService { return serviceOver({ tables: { order_lines: table } }); diff --git a/packages/services/service-datasource/src/external-datasource-service.ts b/packages/services/service-datasource/src/external-datasource-service.ts index 942f2c6ea2..544afe67b0 100644 --- a/packages/services/service-datasource/src/external-datasource-service.ts +++ b/packages/services/service-datasource/src/external-datasource-service.ts @@ -118,50 +118,77 @@ export interface ExternalDatasourceServiceConfig { const BUILTIN_COLUMNS = new Set(['id', 'created_at', 'updated_at']); /** - * Read "is this column part of the remote primary key" across the TWO - * introspection contracts that meet at this service. + * Read "is this column part of the remote primary key" across the + * introspection spellings that can arrive at this service. * * `plugin.ts` hands the driver's `introspectSchema()` result to this service - * unmodified, and the driver does not speak the contract this file is typed - * against: + * unmodified. There is now ONE declared contract on both sides of that + * handoff — `packages/spec`'s `IntrospectedColumn` — and the in-tree driver + * speaks it: * - * | producer | per-column | table-level | - * | ---------------------------------------------- | -------------- | -------------- | - * | `SqlDriver` (+ `SqliteWasmDriver`, which extends it) | `isPrimary` | `primaryKeys` | - * | `packages/spec` `IntrospectedColumn` (what this file's types say) | `primaryKey` | — | + * | producer | per-column | table-level | + * | --------------------------------------------------------------------- | ------------ | ------------- | + * | `SqlDriver` (+ `SqliteWasmDriver` / `TursoDriver`, which extend it) | `primaryKey` | `primaryKeys` | + * | `packages/spec` `IntrospectedColumn` (what this file's types say) | `primaryKey` | — | * - * Measured against a live SQLite database at `368e7a06f`: the driver's column - * for a `primary key (id)` table carries `isPrimary: true` and the table - * carries `primaryKeys: ['id']`, while `primaryKey` is `undefined`. Reading - * only `col.primaryKey` therefore reads a key no in-tree driver ever sets, and - * the remote key is silently lost. + * That agreement is NEW, and this reader predates it. Measured against a live + * SQLite database at `368e7a06f`, the driver's column for a `primary key (id)` + * table carried `isPrimary: true` and no `primaryKey` key at all, so reading + * only `col.primaryKey` lost the remote key — the defect this reader was + * written for. `95437e7d2d` (#10676 / #10998) retired that spelling at the + * producer: `introspectSchema` now derives `col.primaryKey` FROM `primaryKeys` + * and emits no `isPrimary`. * - * This reads the UNION of all three signals rather than picking one: + * This still reads the UNION of three signals rather than picking one, and + * each arm is here for its own reason: * - * - No in-tree producer uses `primaryKey: false` / `isPrimary: false` to - * NEGATE a key another signal asserts — the falses are just "not a key", - * written by producers that fill exactly one of the three. A precedence - * chain would therefore drop a real key whenever the winning signal is the - * one its producer left blank, which is the defect being repaired here. - * - A producer that fills only `table.primaryKeys` (the shape a table-level - * reader would naturally emit) is covered without needing a per-column flag, - * and vice versa. + * - `col.primaryKey` — the spec spelling, what every in-tree producer writes. + * - `table.primaryKeys` — LIVE and INDEPENDENT, not a legacy arm. A + * per-column boolean cannot express key ORDER, and since #10997 (PR #11104) + * this list reports every member of a COMPOSITE key in declared key order. + * It is the only signal here that carries one. ⛔ Collapsing it away drops + * composite-key handling at this seam. + * - `col.isPrimary` — a RETIRED spelling, kept as a compatibility belt. See + * the note below before touching it. * - * When the per-column flag and the table-level list DISAGREE, the union takes - * both. That is deliberate: for a federated table, under-reporting the key - * costs the caller its addressing key, and no in-tree consumer treats a - * column's PK-ness as an exclusive claim. Note that no in-tree driver produces - * such a disagreement today — `SqlDriver` derives `isPrimary` FROM - * `primaryKeys`, so the two always agree, including where both are wrong (a - * SQLite composite key reports only its first column, because - * `introspectPrimaryKeys` filters `PRAGMA table_info` on `pk === 1` while - * SQLite numbers composite members `1, 2, ...`). That truncation is upstream - * of this seam and is not repaired here. + * No producer uses `primaryKey: false` / `isPrimary: false` to NEGATE a key + * another signal asserts — the falses are just "not a key", written by + * producers that fill one signal and leave the others blank. A precedence + * chain would therefore drop a real key whenever the winning signal is the one + * its producer left blank, which is the defect being repaired here. When the + * per-column flag and the table-level list DISAGREE the union takes both: for + * a federated table, under-reporting the key costs the caller its addressing + * key, and no in-tree consumer treats a column's PK-ness as an exclusive + * claim. (The SQLite composite-key truncation this note used to disclaim is + * gone — #10997 repaired `introspectPrimaryKeys` upstream, so the driver's two + * signals now agree on the WHOLE key rather than agreeing on a truncated one.) * - * Deliberately structural: the extra spellings are read off the value without - * widening any declared contract, because reconciling - * `packages/objectql/src/util.ts` with - * `packages/spec/src/contracts/schema-diff-service.ts` is a spec-owned change. + * WHY THE `isPrimary` ARM STAYS, with no producer left in this tree — measured + * for #11123 on `52a41b72ee`, so the next reader need not re-derive it: + * + * - Nothing in-tree writes it. Every surviving whole-identifier hit is prose, + * and `objectql`'s `isPrimaryKeyField` merely CONTAINS the substring. + * - It is still not dead code, because the producer population here is open + * by design. `contracts/datasource-driver-factory.ts` says the framework + * "ships no universal driver-by-id registry" — concrete drivers are built + * by the HOST — and types the handle as `introspectSchema?(): Promise`. + * The retirement shipped as a BREAKING change whose stated migration + * channel is the compiler, "precisely and at every site"; against an + * `unknown` result that channel never fires, so a host-built driver still + * emitting the old spelling is reached by nothing and would silently lose + * its key here. + * - The belt's clock has not run either: the union (#11001) and the + * retirement (#11124) are BOTH still unconsumed changesets at `17.1.0`, so + * no released version has ever emitted `primaryKey` from this driver. + * + * Dropping the arm is therefore a NARROWING OF ACCEPTED INPUT rather than a + * dead-code deletion, and wants the contract-review gate. Its only exercise is + * the staged-disagreement pair in + * `__tests__/external-introspection-seam.test.ts`; those cases go with it. + * + * Deliberately structural: the retired spelling is read off the value without + * widening any declared contract — no declared type carries it any more, so + * there is nothing left to read it through. */ function primaryKeyReader(table: IntrospectedTable): (col: IntrospectedColumn) => boolean { const declared = (table as unknown as { primaryKeys?: unknown }).primaryKeys; @@ -481,11 +508,13 @@ export class ExternalDatasourceService implements IExternalDatasourceService { dialect: schema.dialect, tables: Object.values(schema.tables).map((t) => { const { schema: s, name } = parseQualified(t.name); - // The introspection seam: a real driver spells this `isPrimary` / - // `primaryKeys`, never `primaryKey`. `ExternalCatalogSchema` defaults - // the key to `false`, so reading only `c.primaryKey` persisted a - // catalog in which EVERY column claimed not to be part of the remote - // key — including the ones that are. + // The introspection seam. `ExternalCatalogSchema` defaults this key to + // `false`, so when the in-tree driver still spelled it `isPrimary`, + // reading only `c.primaryKey` persisted a catalog in which EVERY column + // claimed not to be part of the remote key — including the ones that + // are. The driver has since been aligned to the spec spelling + // (`95437e7d2d`), but this must stay a `primaryKeyReader` call: see its + // docblock for the two arms that are still load-bearing. const isPk = primaryKeyReader(t); return { remoteSchema: s,