fix(service-datasource): read the introspected primary key at the isPrimary/primaryKey seam (#11001)

Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

Author: os-warren

.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.<name>.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.

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<void> } }).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<void>): Promise<unknown> {
+  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<DatasourceLike> => ({ 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<Record<string, { primaryKey: boolean; nullable: boolean; sqlType: string }>> {
+  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<void> } }).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<string, { primaryKeys: string[]; columns: Record<string, unknown>[] }> };
+
+    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<void> } }).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<void> } }).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);
+  });
+});

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,14 +360,20 @@ 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,
           columns: t.columns.map((c) => ({
             name: c.name,
             sqlType: c.type,
             nullable: c.nullable,
-            primaryKey: c.primaryKey,
+            primaryKey: isPk(c),
             suggestedFieldType: suggestFieldTypeForSqlType(c.type, schema.dialect as SqlDialect),
           })),
         };