diff --git a/.changeset/external-catalog-introspected-primary-key.md b/.changeset/external-catalog-introspected-primary-key.md new file mode 100644 index 0000000000..3b76785cec --- /dev/null +++ b/.changeset/external-catalog-introspected-primary-key.md @@ -0,0 +1,39 @@ +--- +"@objectstack/service-datasource": patch +--- + +Restore the introspected primary key in the persisted `external_catalog` +(#10676). `ExternalDatasourceService` reads `column.primaryKey` — the +`packages/spec` `IntrospectedColumn` spelling — but `plugin.ts` hands it the +driver's `introspectSchema()` result unmodified, and `SqlDriver` (and +`SqliteWasmDriver`, which extends it) speaks the other `IntrospectedColumn` +contract, from `packages/objectql/src/util.ts`: it sets `column.isPrimary` and +fills `table.primaryKeys`, never `column.primaryKey`. + +Measured against a live SQLite database: for a table declared +`primary key (id)`, the driver's `id` column carries `isPrimary: true` and the +table carries `primaryKeys: ['id']`, while `primaryKey` is `undefined`. Because +`ExternalCatalogSchema` defaults `primaryKey` to `false`, `refreshCatalog` +persisted a catalog in which **every** column of **every** remote table claimed +not to be part of the remote key — so Studio's schema browser and the boot gate +read a catalog that shows no primary keys at all. + +The seam now reads the union of all three signals (`primaryKey`, `isPrimary`, +`table.primaryKeys`) rather than any one of them. No in-tree producer uses a +`false` to negate a key another signal asserts, and taking the union means a +producer that fills only the table-level list — or only the per-column flag — +cannot lose half a composite key. No response or record shape changes: a field +that should always have carried the introspected value starts carrying it. + +The regression pin drives the service off a **real** `SqlDriver.introspectSchema()` +result rather than a hand-written fixture. The pre-existing suite could not see +this defect precisely because it hand-wrote its fixture in the spec spelling, so +no test ever fed the service what a driver actually emits. + +Not fixed here: `generateObjectDraft` still drops the key from the generated +object definition. Its destination is an open contract question rather than a +missing read — `fields..primaryKey` is **not** an authorable spec field +key (an object literal carrying it fails `tsc` against `ServiceObject` with +TS2353, and `ObjectSchema.safeParse` with `unrecognized_keys`), and there is no +key on `ObjectExternalBindingSchema` to hold a remote primary key either. See +#10676 for the routing decision. 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 new file mode 100644 index 0000000000..28cb5363f8 --- /dev/null +++ b/packages/services/service-datasource/src/__tests__/external-introspection-seam.test.ts @@ -0,0 +1,206 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The pin the original suite could not be: this service driven off a REAL + * `SqlDriver.introspectSchema()` result, not a hand-written fake. + * + * `external-datasource-service.test.ts` hand-writes its fixture with + * `primaryKey: true` — the `packages/spec` contract spelling + * (`contracts/schema-diff-service.ts`). The driver emits the OTHER spelling: + * `SqlDriver.introspectSchema` sets `col.isPrimary` and fills + * `table.primaryKeys` (the `packages/objectql/src/util.ts` shape). `plugin.ts` + * hands the driver's result to this service unmodified, so the two contracts + * meet — and disagree — exactly here. A fixture written in EITHER spelling is + * blind to that; only a live introspection can see it, so every case below + * introspects a real in-memory SQLite database rather than describing one. + * + * Both directions are pinned deliberately: an implementation that stamped the + * key onto the first column would satisfy a positive-only suite. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { SqlDriver } from '@objectstack/driver-sql'; +import type { IntrospectedSchema } from '@objectstack/spec/contracts'; +import { + ExternalDatasourceService, + type DatasourceLike, +} from '../external-datasource-service.js'; + +const opened: SqlDriver[] = []; + +afterEach(async () => { + while (opened.length) { + const d = opened.pop()!; + try { + await (d as unknown as { knex?: { destroy(): Promise } }).knex?.destroy(); + } catch { + /* the pool may never have opened */ + } + } +}); + +/** + * A live in-memory SQLite database, introspected by the real driver. Returns + * the driver's own result, deliberately NOT reshaped — anything this service + * needs, it must read from the bytes the driver actually produces. + */ +async function introspectReal(ddl: (knex: never) => Promise): Promise { + const driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + } as never); + opened.push(driver); + await ddl((driver as unknown as { knex: never }).knex); + return driver.introspectSchema(); +} + +/** Wire the service to a fixed introspection result, exactly as `plugin.ts` does. */ +function serviceOver(schema: unknown): ExternalDatasourceService { + return new ExternalDatasourceService({ + introspect: async () => schema as IntrospectedSchema, + getDatasource: async (name): Promise => ({ name, schemaMode: 'external' }), + getObject: async () => undefined, + listObjects: async () => [], + }); +} + +/** The catalog columns for one remote table, keyed by column name. */ +async function catalogColumns( + schema: unknown, + remoteName: string, +): Promise> { + const catalog = await serviceOver(schema).refreshCatalog('showcase_external'); + const table = catalog.tables.find((t) => t.remoteName === remoteName); + expect(table, `no '${remoteName}' in the refreshed catalog`).toBeDefined(); + return Object.fromEntries(table!.columns.map((c) => [c.name, c])) as never; +} + +describe('the introspection seam, as a real SqlDriver actually spells it', () => { + it('the driver speaks isPrimary/primaryKeys and never the spec spelling', async () => { + const schema = (await introspectReal(async (knex: never) => { + await (knex as { schema: { createTable(n: string, cb: (t: never) => void): Promise } }).schema.createTable( + 'customers', + (t: never) => { + const b = t as unknown as { string(n: string): { primary(): void }; integer(n: string): void }; + b.string('id').primary(); + b.string('name'); + b.integer('age'); + }, + ); + })) as { tables: Record[] }> }; + + const table = schema.tables.customers; + const id = table.columns.find((c) => c.name === 'id')!; + + // This is the whole defect, stated as an assertion on the producer's own + // output. If it ever flips — the driver starting to emit the spec + // spelling, or the two contracts being reconciled upstream — the union + // read in `primaryKeyReader` stops being load-bearing and should be + // re-derived rather than quietly relaxed. + expect(table.primaryKeys).toEqual(['id']); + expect(id.isPrimary).toBe(true); + expect(id.primaryKey).toBeUndefined(); + }); + + it('refreshCatalog carries the introspected key onto the right column only', async () => { + const schema = await introspectReal(async (knex: never) => { + await (knex as { schema: { createTable(n: string, cb: (t: never) => void): Promise } }).schema.createTable( + 'customers', + (t: never) => { + const b = t as unknown as { string(n: string): { primary(): void }; integer(n: string): void }; + b.string('id').primary(); + b.string('name'); + b.integer('age'); + }, + ); + }); + + const cols = await catalogColumns(schema, 'customers'); + expect(cols.id.primaryKey).toBe(true); + // …and nothing else was promoted. + expect(cols.name.primaryKey).toBe(false); + expect(cols.age.primaryKey).toBe(false); + }); + + it('refreshCatalog invents no key for a table that declares none', async () => { + const schema = await introspectReal(async (knex: never) => { + await (knex as { schema: { createTable(n: string, cb: (t: never) => void): Promise } }).schema.createTable( + 'events', + (t: never) => { + const b = t as unknown as { string(n: string): unknown }; + b.string('label'); + b.string('payload'); + }, + ); + }); + + const cols = await catalogColumns(schema, 'events'); + // Still a usable catalog entry… + expect(Object.keys(cols)).toEqual(['label', 'payload']); + // …and no column was promoted, least of all the first one. + expect(cols.label.primaryKey).toBe(false); + expect(cols.payload.primaryKey).toBe(false); + }); +}); + +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. + */ + function serviceOverRaw(table: unknown): ExternalDatasourceService { + return serviceOver({ tables: { order_lines: table } }); + } + + const cols = [ + { name: 'order_id', type: 'varchar', nullable: false }, + { name: 'line_no', type: 'varchar', nullable: false }, + { name: 'sku', type: 'varchar', nullable: true }, + ]; + + it('takes the union when the table-level list is wider than the per-column flag', async () => { + const catalog = await serviceOverRaw({ + name: 'order_lines', + primaryKeys: ['order_id', 'line_no'], + columns: cols.map((c) => ({ ...c, isPrimary: c.name === 'order_id' })), + }).refreshCatalog('showcase_external'); + + const byName = Object.fromEntries(catalog.tables[0].columns.map((c) => [c.name, c])); + expect(byName.order_id.primaryKey).toBe(true); + expect(byName.line_no.primaryKey).toBe(true); + expect(byName.sku.primaryKey).toBe(false); + }); + + it('takes the union when the per-column flag is wider than the table-level list', async () => { + const catalog = await serviceOverRaw({ + name: 'order_lines', + primaryKeys: ['order_id'], + columns: cols.map((c) => ({ ...c, isPrimary: c.name !== 'sku' })), + }).refreshCatalog('showcase_external'); + + const byName = Object.fromEntries(catalog.tables[0].columns.map((c) => [c.name, c])); + expect(byName.order_id.primaryKey).toBe(true); + expect(byName.line_no.primaryKey).toBe(true); + expect(byName.sku.primaryKey).toBe(false); + }); + + it('still honours the spec spelling on its own — the pre-existing fixtures keep working', async () => { + const catalog = await serviceOverRaw({ + name: 'order_lines', + // No `primaryKeys`, no `isPrimary` — the hand-written shape the rest of + // this package's suite feeds in. + columns: cols.map((c) => ({ ...c, primaryKey: c.name === 'order_id' })), + }).refreshCatalog('showcase_external'); + + const byName = Object.fromEntries(catalog.tables[0].columns.map((c) => [c.name, c])); + expect(byName.order_id.primaryKey).toBe(true); + expect(byName.line_no.primaryKey).toBe(false); + }); +}); diff --git a/packages/services/service-datasource/src/external-datasource-service.ts b/packages/services/service-datasource/src/external-datasource-service.ts index 48a5aa0384..5707f16a98 100644 --- a/packages/services/service-datasource/src/external-datasource-service.ts +++ b/packages/services/service-datasource/src/external-datasource-service.ts @@ -22,6 +22,7 @@ import type { SchemaValidationReport, IntrospectedSchema, IntrospectedTable, + IntrospectedColumn, } from '@objectstack/spec/contracts'; import type { SchemaDiffEntry } from '@objectstack/spec/shared'; import { @@ -93,6 +94,63 @@ export interface ExternalDatasourceServiceConfig { /** Columns ObjectStack manages itself — never validated against the remote. */ 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. + * + * `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: + * + * | producer | per-column | table-level | + * | ---------------------------------------------- | -------------- | -------------- | + * | `SqlDriver` (+ `SqliteWasmDriver`, which extends it) | `isPrimary` | `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. + * + * This reads the UNION of all three signals rather than picking one: + * + * - 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. + * + * 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. + * + * 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. + */ +function primaryKeyReader(table: IntrospectedTable): (col: IntrospectedColumn) => boolean { + const declared = (table as unknown as { primaryKeys?: unknown }).primaryKeys; + const listed = new Set( + Array.isArray(declared) ? declared.filter((n): n is string => typeof n === 'string') : [], + ); + return (col) => + col.primaryKey === true || + (col as { isPrimary?: unknown }).isPrimary === true || + listed.has(col.name); +} + /** Split a possibly schema-qualified name (`mart.fact_orders`). */ function parseQualified(raw: string): { schema?: string; name: string } { const idx = raw.indexOf('.'); @@ -302,6 +360,12 @@ 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. + const isPk = primaryKeyReader(t); return { remoteSchema: s, remoteName: name, @@ -309,7 +373,7 @@ export class ExternalDatasourceService implements IExternalDatasourceService { name: c.name, sqlType: c.type, nullable: c.nullable, - primaryKey: c.primaryKey, + primaryKey: isPk(c), suggestedFieldType: suggestFieldTypeForSqlType(c.type, schema.dialect as SqlDialect), })), };