Skip to content

Commit 4257e4e

Browse files
os-warrenclaude
andauthored
fix(service-datasource): stop rendering the unauthorable fields.<f>.primaryKey (#11073)
`generateObjectDraft` emitted a field-level `primaryKey: true` and `renderObjectSource` rendered it onto the field line. `primaryKey` is not a key of the spec field schema, so the `*.object.ts` the review-before-commit flow produced was refused by both instruments the file is annotated for: `tsc --noEmit` against `ServiceObject` (`TS2353`) and `ObjectSchema.safeParse` (`unrecognized_keys`). Per the maintainer ruling of 2026-08-22, the key stops being emitted and the introspected value survives as a comment above the `fields` block — information preserved for the reader, zero contract face. The comment names the column(s) the draft was given, says why it is a comment, and does not claim to be a complete key (some drivers report only the first column of a composite key). A table with no reported key renders no comment. With #10712's namespace/`sharingModel` repairs already landed, both paths -- `opts.primaryKey` set and unset -- now clear all three `os build` stages. An authorable spelling on the binding schema is deferred, not rejected; it returns as its own spec change once federated upsert has a runtime consumer. Fixes #11000 Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx Co-authored-by: Claude <noreply@anthropic.com>
1 parent 13a3dca commit 4257e4e

6 files changed

Lines changed: 480 additions & 17 deletions
Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
---
2+
"@objectstack/service-datasource": patch
3+
---
4+
5+
`os datasource introspect --primary-key` (and `POST /object-draft` with
6+
`primaryKey`) now generates an object draft that compiles and parses (#11000).
7+
8+
The generator emitted a field-level `primaryKey: true` — into the definition
9+
and onto the rendered field line. `primaryKey` is **not a key of the spec field
10+
schema**, so the `*.object.ts` the review-before-commit flow handed the user was
11+
refused by both instruments the file is annotated for:
12+
13+
- `tsc --noEmit` against `ServiceObject` — `TS2353: Object literal may only
14+
specify known properties, and 'primaryKey' does not exist in type …`;
15+
- `ObjectSchema.safeParse``unrecognized_keys` at `["fields","<f>"]`.
16+
17+
This was the last reason the `opts.primaryKey` path did not build. With #10712's
18+
namespace/`sharingModel` repairs already landed, **both** paths — `primaryKey`
19+
set and unset — now clear `defineStack()`'s namespace check, the
20+
`authoringRulesFor('build')` rule set, and `tsc --noEmit` over the rendered
21+
source.
22+
23+
The introspected key is not discarded: it is preserved as a comment above the
24+
`fields` block, naming the column(s) the draft was given as the key —
25+
26+
```ts
27+
// Remote primary key: order_id, line_no
28+
```
29+
30+
— with the reason it is a comment rather than a field key, and an explicit
31+
caveat that for a composite key some drivers report only the first column
32+
(#10997), so the list is a lower bound rather than a verified complete key. A
33+
table with no reported key gets no comment at all.
34+
35+
Per the maintainer ruling of 2026-08-22, an authorable spelling for a federated
36+
object's remote key (`external.primaryKey: string[]` on the binding schema) is
37+
**deferred, not rejected** — it returns as its own `packages/spec` change when
38+
federated upsert has a live runtime consumer to justify the surface.

.changeset/external-object-draft-passes-os-build.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ prefix — mirroring `defineStack`, which skips the check entirely rather than
2727
inventing one, and avoiding an `_customers` that would trade one invalid draft
2828
for another.
2929

30-
Note the `opts.primaryKey` path still does not build: it emits
31-
`fields.<f>.primaryKey`, which is not an authorable spec field key. That is
32-
#11000, a separate open contract question, untouched here.
30+
At the time this landed, the `opts.primaryKey` path still did not build: it
31+
emitted `fields.<f>.primaryKey`, which is not an authorable spec field key.
32+
That was #11000, and it is fixed separately in this same release — both paths
33+
build now. See that changeset for what replaced the key.

packages/services/service-datasource/src/__tests__/external-datasource-service.test.ts

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,23 @@ describe('generateObjectDraft', () => {
8989
expect(draft.name).toBe('fact_orders');
9090
expect(draft.datasource).toBe('warehouse');
9191
const fields = draft.definition.fields as Record<string, { type: string; primaryKey?: boolean }>;
92-
expect(fields.order_id).toEqual({ type: 'text', primaryKey: true });
92+
/**
93+
* `order_id` is the table's primary key, and the field carries NOTHING
94+
* about that.
95+
*
96+
* This assertion used to read `{ type: 'text', primaryKey: true }`. It was
97+
* flipped by the maintainer ruling of 2026-08-22 (「同意所有」, item 8 = D,
98+
* recorded on #11000): `fields.<f>.primaryKey` is not a key of the spec
99+
* field schema, so the draft it pinned was one `tsc --noEmit` refused
100+
* (`TS2353`) and `ObjectSchema.safeParse` refused (`unrecognized_keys`).
101+
* The pin is kept, not deleted — inverted, it is now the guard that the
102+
* unauthorable key does not come back. `toEqual` (not `toMatchObject`) is
103+
* load-bearing here: it is what makes the assertion fail on an EXTRA key.
104+
*
105+
* Where the key went instead is pinned in
106+
* `external-object-draft-primary-key.test.ts`.
107+
*/
108+
expect(fields.order_id).toEqual({ type: 'text' });
93109
expect(fields.amount.type).toBe('number');
94110
expect(fields.ordered_at.type).toBe('datetime');
95111
expect(fields.metadata.type).toBe('json');
@@ -101,7 +117,11 @@ describe('generateObjectDraft', () => {
101117
expect(draft.source).toContain("remoteName: 'fact_orders'");
102118
expect(draft.source).toContain("remoteSchema: 'mart'");
103119
expect(draft.source).toContain('REVIEW:');
104-
expect(draft.source).toContain("order_id: { type: 'text', primaryKey: true }");
120+
// Same flip, source side: the rendered field line no longer carries the
121+
// key, and the introspected key survives as the comment ruling D requires.
122+
expect(draft.source).toContain("order_id: { type: 'text' },");
123+
expect(draft.source).not.toContain('primaryKey: true');
124+
expect(draft.source).toContain('// Remote primary key: order_id');
105125
});
106126

107127
it('honours include/exclude/rename/primaryKey options', async () => {
@@ -113,6 +133,10 @@ describe('generateObjectDraft', () => {
113133
});
114134
const fields = draft.definition.fields as Record<string, unknown>;
115135
expect(Object.keys(fields)).toEqual(['order_id', 'total']);
136+
// `opts.primaryKey` still HAS an effect after ruling D — it moved from the
137+
// definition to the comment. An implementation that dropped the option
138+
// entirely would pass every other assertion in this file.
139+
expect(draft.source).toContain('// Remote primary key: order_id');
116140
});
117141

118142
it('throws when the remote table is missing', async () => {

packages/services/service-datasource/src/__tests__/external-object-draft-os-build.test.ts

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,22 @@ import {
5858
* the object NAME comes from the table name and the OWD is a constant, so a
5959
* real introspection would add cost and no coverage.
6060
*
61-
* Every column is spelled `primaryKey: false` on purpose. That keeps the whole
62-
* file on the `opts.primaryKey`-unset path, where the generator emits no
63-
* `fields.<f>.primaryKey` — the key that is NOT authorable (#11000, an open
64-
* contract question in `packages/spec`, deliberately untouched here). Pinning
65-
* these two repairs on a draft that also carries #11000's key would produce
66-
* cases that cannot go green until a card this lane does not own is decided.
61+
* Every column is spelled `primaryKey: false`, so the cases above run on the
62+
* `opts.primaryKey`-UNSET path. That was originally a workaround: #11000's
63+
* unauthorable `fields.<f>.primaryKey` made the key-set path un-buildable, and
64+
* pinning these two repairs on top of it would have produced cases that could
65+
* not go green until a card this lane did not own was decided.
66+
*
67+
* #11000 is now decided (maintainer, 2026-08-22, 「同意所有」 item 8 = D) and
68+
* fixed: the generator no longer emits that key. The fixture keeps the unset
69+
* spelling because these two defects genuinely do not read a column's PK-ness
70+
* — but the path split is no longer a limitation, and the block at the bottom
71+
* of this file re-runs the same three checks with `opts.primaryKey` SET.
72+
* ⭐ Keep them separate anyway: before the namespace/OWD repairs landed, the
73+
* key-set path failed on `unrecognized_keys` BEFORE the namespace check ran,
74+
* so on that path the namespace defect was masked rather than absent. Error
75+
* ordering hides defects in this pipeline; one path's verdict never covers the
76+
* other's.
6777
*/
6878
function remoteSchema(): IntrospectedSchema {
6979
return {
@@ -234,6 +244,48 @@ describe('an absent or blank namespace must not trade one invalid draft for anot
234244
});
235245
});
236246

247+
/**
248+
* The `opts.primaryKey`-SET path, which #11000 removed the last blocker from.
249+
*
250+
* Until ruling D, this path produced a draft that failed
251+
* `ObjectSchema.safeParse` on `unrecognized_keys` — and failed it EARLY ENOUGH
252+
* that the namespace and OWD repairs pinned above were never reached on it.
253+
* Re-running all three checks here is what makes "both paths build" a measured
254+
* claim rather than an inference from the unset path.
255+
*/
256+
describe('both paths build — the key-set path is no longer the exception', () => {
257+
/** The same service, driven with an explicit remote key. */
258+
const withKey = (ns: string | undefined = 'wh') =>
259+
serviceWith(ns).generateObjectDraft('warehouse', 'customers', { primaryKey: ['id'] });
260+
261+
it('stage 1 — the namespace prefix rule accepts the name on the key-set path too', async () => {
262+
const draft = await withKey();
263+
expect(draft.name).toBe('wh_customers');
264+
expect(validateObjectNamespacePrefix(draft.name, 'wh')).toBeNull();
265+
});
266+
267+
it('stage 2 — the definition PARSES, and carries the declared OWD', async () => {
268+
const draft = await withKey();
269+
const parsed = ObjectSchema.safeParse(draft.definition);
270+
expect(parsed.success, JSON.stringify((parsed as { error?: unknown }).error)).toBe(true);
271+
expect(draft.definition.sharingModel).toBe('private');
272+
expect(CANONICAL_OWD).toContain(draft.definition.sharingModel);
273+
});
274+
275+
it('still-generates — every field, the binding and the remote name survive the key path', async () => {
276+
const draft = await withKey();
277+
const fields = draft.definition.fields as Record<string, { type: string }>;
278+
const external = draft.definition.external as { remoteName?: string; remoteSchema?: string };
279+
280+
expect(Object.keys(fields)).toEqual(['id', 'name', 'signed_up_at']);
281+
expect(fields.signed_up_at.type).toBe('datetime');
282+
expect(external.remoteName).toBe('customers');
283+
expect(external.remoteSchema).toBe('mart');
284+
expect(draft.source).toContain("remoteSchema: 'mart', remoteName: 'customers'");
285+
expect(draft.source).toContain("sharingModel: 'private'");
286+
});
287+
});
288+
237289
describe('importObject inherits both repairs from the draft pipeline', () => {
238290
it('persists the prefixed name and the explicit OWD', async () => {
239291
const persisted: Array<{ name: string; def: Record<string, unknown> }> = [];

0 commit comments

Comments
 (0)