Skip to content

Commit a97489b

Browse files
nizzlenitzclaude
andcommitted
Merge base #1921: route the union translation through the shared emitter
The base branch added `types` (the source union) and translated it per surface at each call site. This branch replaced those call sites with one shared emitter, so the translation moves into it: `attributeToSchema` emits the union verbatim for MCP and `oneOf` for OpenAPI 3.0, which also makes it work at every nesting level rather than only the top one. Also expresses a `null`-only declaration as `{ nullable: true, enum: [null] }` rather than dropping it — 3.0 has no `null` type, but it can say "only null". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0188G62J9fZQg4J9rVuqLzjy
2 parents 61d2773 + 2039e4d commit a97489b

6 files changed

Lines changed: 161 additions & 12 deletions

File tree

components/mcp/tools/schemas/derive.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@ export interface HarperAttribute {
2525
description?: string;
2626
hidden?: boolean;
2727
nullable?: boolean;
28+
/** Source JSON-Schema type union from `static properties`; MCP accepts type arrays, so it passes through. */
29+
types?: readonly string[];
2830
isPrimaryKey?: boolean;
2931
properties?: HarperAttribute[];
3032
elements?: HarperAttribute;

resources/jsonSchemaTypes.ts

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ export interface JsonSchemaFragment {
3232
const?: unknown;
3333
/** Binary encoding of a string-typed value. Emitted on the MCP surface only — not a 3.0.3 keyword. */
3434
contentEncoding?: string;
35+
/** Emitted by the OpenAPI 3.0 projection for a genuine multi-type union; not authored directly. */
36+
oneOf?: JsonSchemaFragment[];
3537
}
3638

3739
/**
@@ -75,6 +77,12 @@ export interface AttributeLike {
7577
assignCreatedTime?: boolean;
7678
assignUpdatedTime?: boolean;
7779
nullable?: boolean;
80+
/**
81+
* The source JSON-Schema type union, verbatim, when `static properties` declared one. `type` holds
82+
* the first non-null member so single-type consumers keep working; surfaces that can express a
83+
* union (MCP passes it through, OpenAPI 3.0 translates it to `oneOf`) read this instead.
84+
*/
85+
types?: readonly string[];
7886
elements?: AttributeLike;
7987
/** Sub-attributes of a nested object field (the same array form `Table.validate` iterates). */
8088
properties?: AttributeLike[];
@@ -111,6 +119,10 @@ export function attributeToFragment(attr: AttributeLike): JsonSchemaFragment {
111119
} else if (attr.type === 'array' && attr.elements) {
112120
fragment.type = 'array';
113121
fragment.items = attributeToFragment(attr.elements);
122+
} else if (attr.types) {
123+
// A declared union round-trips verbatim; collapsing it to `attr.type` here would make the
124+
// canonical `Table.properties` disagree with what the author wrote.
125+
fragment.type = [...attr.types] as JsonSchemaType[];
114126
} else {
115127
const jsonType = attr.type ? DATA_TYPES[attr.type] : undefined;
116128
if (jsonType) fragment.type = jsonType;
@@ -161,9 +173,11 @@ function fragmentToAttribute(name: string, fragment: JsonSchemaFragment): Attrib
161173
// than misleadingly reusing the array field's own name.
162174
attr.elements = fragmentToAttribute('', fragment.items);
163175
} else if (Array.isArray(fragment.type)) {
164-
// JSON-Schema union type. Fold a `'null'` member into `nullable` (the OpenAPI-expressible form)
165-
// and keep the remaining member. A single non-null member is the common `['T','null']` case; a
166-
// genuine multi-type union isn't expressible on an attribute, so the first member is kept.
176+
// JSON-Schema union type. Keep the source union on `types` so surfaces that can express one
177+
// (MCP natively, OpenAPI 3.0 via `oneOf`) don't have to reconstruct it, and fold a `'null'`
178+
// member into `nullable` as well since that is the form OpenAPI needs. `type` carries the first
179+
// non-null member for the single-type consumers (validation, query coercion) that read it.
180+
attr.types = fragment.type;
167181
const members = fragment.type.filter((t) => t !== 'null');
168182
if (members.length !== fragment.type.length) attr.nullable = true;
169183
if (members.length > 0) attr.type = members[0];
@@ -251,6 +265,17 @@ export function resolveDeclaredType(type: string | undefined, context?: string):
251265
return undefined;
252266
}
253267

268+
/**
269+
* The non-null members of an attribute's declared type union, but only when there is more than one —
270+
* `['string','null']` is nullability, not a union, and `type` already carries its single member.
271+
* Returns undefined when the attribute has no union to translate.
272+
*/
273+
export function unionMembers(attr: { types?: readonly string[] }): string[] | undefined {
274+
if (!attr.types) return undefined;
275+
const members = attr.types.filter((member) => member !== 'null');
276+
return members.length > 1 ? members : undefined;
277+
}
278+
254279
/** Test hook: the unknown-type warning is once-per-process, which would leak across test cases. */
255280
export function _resetUnknownTypeWarningsForTest(): void {
256281
warnedUnknownTypes.clear();
@@ -303,6 +328,14 @@ export function attributeToSchema(attr: AttributeLike, options: SchemaEmitOption
303328
const items = attributeToSchema(attr.elements, childOptions);
304329
if (items) fragment.items = items;
305330
}
331+
} else if (attr.types && options.dialect === 'json-schema') {
332+
// JSON Schema has type unions — emit the author's declaration as written.
333+
fragment.type = [...attr.types] as JsonSchemaType[];
334+
} else if (unionMembers(attr)) {
335+
// 3.0.3 has neither type arrays nor a `null` type, so a genuine union becomes `oneOf`. Each
336+
// member still goes through the surface's own primitive mapping, so it is described exactly as
337+
// the same type would be on its own.
338+
fragment.oneOf = unionMembers(attr).map((member) => options.mapPrimitive(member, attr));
306339
} else {
307340
// Copy the mapper's result field by field rather than merging it wholesale: the set of keys a
308341
// surface may contribute to a leaf schema is fixed, and spreading an arbitrary object here would
@@ -312,6 +345,8 @@ export function attributeToSchema(attr: AttributeLike, options: SchemaEmitOption
312345
if (primitive.description !== undefined) fragment.description = primitive.description;
313346
if (primitive.format !== undefined) fragment.format = primitive.format;
314347
if (primitive.contentEncoding !== undefined) fragment.contentEncoding = primitive.contentEncoding;
348+
if (primitive.nullable !== undefined) fragment.nullable = primitive.nullable;
349+
if (primitive.enum !== undefined) fragment.enum = primitive.enum;
315350
}
316351

317352
if (attr.nullable) applyNullability(fragment, options.dialect);
@@ -347,9 +382,15 @@ export function attributeToSchema(attr: AttributeLike, options: SchemaEmitOption
347382
}
348383

349384
function applyNullability(fragment: JsonSchemaFragment, dialect: SchemaDialect): void {
350-
// Both dialects express nullability as a modification of `type`, so neither has anything to say
351-
// about a fragment that never resolved one.
352-
if (!('type' in fragment) || fragment.type === undefined) return;
385+
// Nullability qualifies a schema that says something; neither dialect has anything to add to a
386+
// fragment that resolved neither a `type` nor a `oneOf`.
387+
if ((!('type' in fragment) || fragment.type === undefined) && fragment.oneOf === undefined) return;
388+
if (fragment.type === undefined) {
389+
// A `oneOf` union. 3.0 takes `nullable` alongside it; JSON Schema takes a `null` branch.
390+
if (dialect === 'openapi-3.0.3') fragment.nullable = true;
391+
else fragment.oneOf = [...fragment.oneOf, { type: 'null' }];
392+
return;
393+
}
353394
if (dialect === 'openapi-3.0.3') {
354395
// OpenAPI 3.0.3 has no union types; `nullable` is the spec-provided expression.
355396
fragment.nullable = true;

resources/openApi.ts

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import {
88
attributeToSchema,
99
projectPropertiesToAttributes,
1010
resolveDeclaredType,
11+
unionMembers,
1112
} from './jsonSchemaTypes.ts';
1213

1314
const OPENAPI_VERSION = '3.0.3';
@@ -34,7 +35,10 @@ function openApiPrimitive(type: string | undefined, attributeName: string | unde
3435
if (!resolved) return {};
3536
// 3.0.x has no `'null'` type — nullability is the `nullable` keyword, so a bare `type: 'null'`
3637
// becomes an untyped nullable schema rather than a type the dialect can't express.
37-
if (resolved === 'null') return {}; // 3.0 has no null type; a bare `nullable` on an untyped schema says nothing
38+
// 3.0 has no `'null'` type. A bare `{ nullable: true }` constrains nothing, so express "only null"
39+
// the one way the dialect can: a `null`-only enum. Dropping the declaration outright (#1921 review)
40+
// left the document silent about a field the author did describe.
41+
if (resolved === 'null') return { nullable: true, enum: [null] };
3842
// Preserve the Harper type name as `format` for the types where it adds information, matching the
3943
// top-level `Type()` emitter.
4044
return Object.hasOwn(DATA_TYPES, type) ? (new Type(resolved, type) as JsonSchemaFragment) : { type: resolved };
@@ -168,6 +172,7 @@ export function generateJsonApi(resources: Resources, serverHttpURL: string) {
168172
if (attributes) {
169173
for (const attr of attributes) {
170174
const { type, name, elements, relationship, definition, nullable, description, hidden } = attr;
175+
const union = unionMembers(attr);
171176
// @hidden field-level: suppress the attribute from props, query params, and required.
172177
if (hidden) continue;
173178
const def = definition ?? elements?.definition;
@@ -196,6 +201,10 @@ export function generateJsonApi(resources: Resources, serverHttpURL: string) {
196201
// shared emitter (sub-properties recursed, @hidden suppressed at every level, hints
197202
// carried); OpenAPI's table path uses $refs instead.
198203
props[name] = attributeToOpenApiSchema(attr) ?? {};
204+
} else if (union) {
205+
// A genuine multi-type union (`['string','number']`). 3.0 has no type arrays, so the
206+
// equivalent is `oneOf`; `attr.type` alone would drop every member but the first.
207+
props[name] = { oneOf: union.map((member) => openApiPrimitive(member, name)) };
199208
} else if (type === 'array') {
200209
if (!elements) {
201210
// `{ type: 'array' }` with no items — valid JSON Schema (array of anything).
@@ -227,6 +236,7 @@ export function generateJsonApi(resources: Resources, serverHttpURL: string) {
227236
description?: string;
228237
enum?: unknown[];
229238
type?: unknown;
239+
oneOf?: unknown;
230240
format?: string;
231241
const?: unknown;
232242
nullable?: boolean;
@@ -241,9 +251,11 @@ export function generateJsonApi(resources: Resources, serverHttpURL: string) {
241251
if (attr.const !== undefined) {
242252
prop.enum = Array.isArray(prop.enum) ? prop.enum.filter((value) => value === attr.const) : [attr.const];
243253
}
244-
// Only a typed schema can be nullable — a bare `{ nullable: true }` says nothing in 3.0,
245-
// matching the guard the shared emitter applies.
246-
if (nullable && prop.nullable === undefined && prop.type !== undefined) prop.nullable = true;
254+
// Only a schema that says something can be nullable — a bare `{ nullable: true }` says nothing
255+
// in 3.0. A `oneOf` union qualifies alongside a plain `type`.
256+
if (nullable && prop.nullable === undefined && (prop.type !== undefined || prop.oneOf !== undefined)) {
257+
prop.nullable = true;
258+
}
247259
// 3.0's `nullable` does not widen an `enum`; without `null` in the list a validator
248260
// rejects it regardless of the flag.
249261
if (prop.nullable && Array.isArray(prop.enum) && !prop.enum.includes(null)) {

unitTests/components/mcp/tools/convergence.test.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ function makeResources() {
2424
Widget.properties = {
2525
id: { type: 'string', primaryKey: true },
2626
label: { type: 'string', description: 'Human-readable label' },
27+
mixed: { type: ['string', 'number'] },
2728
};
2829
for (const v of ['get', 'put', 'patch', 'delete', 'search', 'post']) Widget.prototype[v] = function () {};
2930
Widget.get = async (t) => ({ id: t.id });
@@ -75,6 +76,22 @@ describe('mcp/openapi — #1920 description convergence across surfaces', () =>
7576
// Convergence: the per-property description is identical on both surfaces.
7677
assert.equal(create.inputSchema.properties.label.description, schema.properties.label.description);
7778
});
79+
80+
it('expresses a declared type union on each surface in that surface’s own dialect', () => {
81+
const resources = makeResources();
82+
83+
_setResourcesForTest(resources);
84+
registerApplicationTools();
85+
const create = getTool('create_Widget');
86+
// MCP speaks JSON Schema, which has type unions — pass the author's declaration through.
87+
assert.deepEqual(create.inputSchema.properties.mixed.type, ['string', 'number']);
88+
89+
// OpenAPI 3.0 has no type arrays; the equivalent is `oneOf`. Same declaration, two encodings —
90+
// what must NOT happen is either surface narrowing it to `string`.
91+
const schema = generateJsonApi(resources, 'https://harper.fast').components.schemas.Widget;
92+
assert.deepEqual(schema.properties.mixed.oneOf, [{ type: 'string' }, { type: 'number' }]);
93+
assert.equal(schema.properties.mixed.type, undefined);
94+
});
7895
});
7996

8097
// #1941 / #1942 — the two surfaces used to diverge below the top level: OpenAPI dropped hints inside

unitTests/resources/graphqlMetadata.test.js

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,5 +199,24 @@ describe('GraphQL parser — metadata capture (#1095)', () => {
199199
assert.strictEqual(note.type, 'string');
200200
assert.strictEqual(note.nullable, true, 'the null member must become nullable, not be dropped');
201201
});
202+
203+
it('preserves a genuine multi-type union instead of keeping only the first member', () => {
204+
const [mixed] = projectPropertiesToAttributes({ mixed: { type: ['string', 'number'] } });
205+
assert.deepStrictEqual(mixed.types, ['string', 'number']);
206+
assert.strictEqual(mixed.type, 'string', 'single-type consumers still see the first member');
207+
});
208+
209+
it('round-trips a union back to the declared fragment (properties -> attributes -> properties)', () => {
210+
const declared = { mixed: { type: ['string', 'number'] }, maybe: { type: ['string', 'null'] } };
211+
const round = projectAttributesToProperties(projectPropertiesToAttributes(declared));
212+
assert.deepStrictEqual(round.mixed.type, ['string', 'number']);
213+
assert.deepStrictEqual(round.maybe.type, ['string', 'null']);
214+
});
215+
216+
it('keeps a `["null"]`-only declaration nullable rather than silently untyped', () => {
217+
const [nothing] = projectPropertiesToAttributes({ nothing: { type: ['null'] } });
218+
assert.strictEqual(nothing.nullable, true);
219+
assert.deepStrictEqual(nothing.types, ['null']);
220+
});
202221
});
203222
});

unitTests/resources/openApi.test.js

Lines changed: 60 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,16 @@ describe('openApi — declared dialect compliance (3.0.3)', () => {
371371
kind: { type: 'string', const: 'widget' },
372372
nothing: { type: 'null' },
373373
maybe: { type: ['string', 'null'] },
374-
nested: { type: 'object', properties: { inner: { type: 'string', const: 'x' }, deepNull: { type: 'null' } } },
374+
mixed: { type: ['string', 'number'] },
375+
mixedMaybe: { type: ['string', 'integer', 'null'] },
376+
nested: {
377+
type: 'object',
378+
properties: {
379+
inner: { type: 'string', const: 'x' },
380+
deepNull: { type: 'null' },
381+
deepMixed: { type: ['string', 'number'] },
382+
},
383+
},
375384
list: { type: 'array', items: { type: 'string', const: 'y' } },
376385
nullableEnum: { type: 'string', enum: ['a', 'b'], nullable: true },
377386
nullableConst: { type: 'string', const: 'fixed', nullable: true },
@@ -488,9 +497,58 @@ describe('openApi — declared dialect compliance (3.0.3)', () => {
488497
expect(body.properties.status.enum, 'nullable enum admits null').to.deep.equal(['a', 'b', null]);
489498
});
490499

500+
it('carries nullability onto the emitted scalar schema', () => {
501+
// The walk assertions above only prove `type: 'null'` and unions are gone; they would pass just as
502+
// happily if nullability were dropped instead of translated.
503+
const props = buildDocument().components.schemas.Widget.properties;
504+
expect(props.maybe).to.deep.equal({ type: 'string', nullable: true });
505+
expect(props.nothing.nullable).to.equal(true);
506+
});
507+
508+
it('widens a nullable `enum` with `null` (3.0 `nullable` does not do it)', () => {
509+
const props = buildDocument().components.schemas.Widget.properties;
510+
expect(props.nullableEnum.nullable).to.equal(true);
511+
expect(props.nullableEnum.enum).to.deep.equal(['a', 'b', null]);
512+
// `const` + `nullable`: the single-value enum still has to admit null.
513+
expect(props.nullableConst.enum).to.deep.equal(['fixed', null]);
514+
});
515+
516+
it('translates a genuine multi-type union to `oneOf`', () => {
517+
// 3.0 has no type arrays. Keeping only the first member would narrow the contract silently —
518+
// a client would be told `mixed` is a string when the resource also accepts a number.
519+
const props = buildDocument().components.schemas.Widget.properties;
520+
expect(props.mixed).to.deep.equal({ oneOf: [{ type: 'string' }, { type: 'number' }] });
521+
expect(props.mixed).to.not.have.property('type');
522+
});
523+
524+
it('carries nullability alongside a union', () => {
525+
const props = buildDocument().components.schemas.Widget.properties;
526+
expect(props.mixedMaybe.oneOf).to.deep.equal([{ type: 'string' }, { type: 'integer' }]);
527+
expect(props.mixedMaybe.nullable).to.equal(true);
528+
});
529+
530+
it('translates a union nested inside an object, not just at the top level', () => {
531+
// The nested path is the one #1941/#1942 exist to keep in step with the top-level one; a union
532+
// declared two levels down has to reach the same `oneOf`.
533+
const nested = buildDocument().components.schemas.Widget.properties.nested;
534+
expect(nested.properties.deepMixed.oneOf).to.deep.equal([{ type: 'string' }, { type: 'number' }]);
535+
expect(nested.properties.deepMixed).to.not.have.property('type');
536+
});
537+
491538
it('emits the properties under test (guards the walk assertions against an empty document)', () => {
492539
const props = buildDocument().components.schemas.Widget.properties;
493-
for (const key of ['kind', 'nothing', 'maybe', 'nested', 'list', 'nullableEnum', 'nullableConst', 'when']) {
540+
for (const key of [
541+
'kind',
542+
'nothing',
543+
'maybe',
544+
'mixed',
545+
'mixedMaybe',
546+
'nested',
547+
'list',
548+
'nullableEnum',
549+
'nullableConst',
550+
'when',
551+
]) {
494552
expect(props, `fixture property ${key} missing — walk assertions would pass vacuously`).to.have.property(key);
495553
}
496554
});

0 commit comments

Comments
 (0)