Skip to content

Commit 32bb1ea

Browse files
committed
fix(core): preserve parent constraints and clone props per variant (#3432)
Address review feedback on the #3432 inline-parent-props rewrite: - Shallow-copy the parent schema and only strip the keys that would re-create the cycle (oneOf, discriminator, allOf, anyOf), so object-level constraints like additionalProperties, minProperties, description, etc. carry through to the inlined entry. The previous implementation only kept type/properties/required, silently dropping every other constraint. - Per-variant shallow-clone of properties and required so downstream in-place mutations on one variant don't leak across siblings under the same parent. - Drop the inline entry entirely when nothing meaningful beyond type:'object' survives — the second allOf member (variant's own object) already asserts object-ness, and dropping keeps the existing empty-parent snapshot stable. Adds two focused unit tests: one asserts that additionalProperties:false, minProperties, and description propagate from parent to inlined variant (and that oneOf/discriminator/allOf do not), the other asserts that sibling variants get independent properties objects.
1 parent 22bd9ae commit 32bb1ea

2 files changed

Lines changed: 135 additions & 7 deletions

File tree

packages/core/src/getters/discriminators.test.ts

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,109 @@ describe('resolveDiscriminators getter', () => {
436436
expect(inlined?.required).toEqual(['commonField']);
437437
});
438438

439+
it('preserves parent object-level constraints when inlining (#3432)', () => {
440+
// Replacing the parent $ref with `{type:'object', properties, required}`
441+
// alone would silently drop other parent constraints like
442+
// `additionalProperties`, `minProperties`, `description`, etc. The inline
443+
// schema must carry those forward so variant validation semantics match
444+
// the dereferenced behavior we replaced.
445+
const schemas: OpenApiSchemasObject = {
446+
Parent: {
447+
type: 'object',
448+
additionalProperties: false,
449+
minProperties: 1,
450+
description: 'parent shape',
451+
required: ['kind', 'commonField'],
452+
properties: {
453+
kind: { type: 'string', enum: ['a'] },
454+
commonField: { type: 'string' },
455+
},
456+
discriminator: {
457+
propertyName: 'kind',
458+
mapping: {
459+
a: '#/components/schemas/VariantA',
460+
},
461+
},
462+
oneOf: [{ $ref: '#/components/schemas/VariantA' }],
463+
},
464+
VariantA: {
465+
allOf: [
466+
{ $ref: '#/components/schemas/Parent' },
467+
{ type: 'object', properties: { extraA: { type: 'number' } } },
468+
],
469+
},
470+
};
471+
472+
const result = resolveDiscriminators(structuredClone(schemas), context);
473+
const variantA = result.VariantA as NonNullable<
474+
OpenApiSchemasObject[string]
475+
>;
476+
const allOf = variantA.allOf as
477+
| (OpenApiSchemaObject | OpenApiReferenceObject)[]
478+
| undefined;
479+
const inlined = allOf?.[0] as Record<string, unknown> | undefined;
480+
481+
expect(inlined?.additionalProperties).toBe(false);
482+
expect(inlined?.minProperties).toBe(1);
483+
expect(inlined?.description).toBe('parent shape');
484+
// Composition keys that would re-create the cycle must NOT be copied.
485+
expect(inlined).not.toHaveProperty('oneOf');
486+
expect(inlined).not.toHaveProperty('discriminator');
487+
expect(inlined).not.toHaveProperty('allOf');
488+
});
489+
490+
it('gives each variant its own properties object (#3432)', () => {
491+
// The inlined parent properties must be cloned per variant — sharing the
492+
// same Record across siblings would couple downstream in-place mutations
493+
// (e.g. one variant's property tweak leaking into the other).
494+
const schemas: OpenApiSchemasObject = {
495+
Parent: {
496+
type: 'object',
497+
required: ['kind', 'shared'],
498+
properties: {
499+
kind: { type: 'string', enum: ['a', 'b'] },
500+
shared: { type: 'string' },
501+
},
502+
discriminator: {
503+
propertyName: 'kind',
504+
mapping: {
505+
a: '#/components/schemas/VariantA',
506+
b: '#/components/schemas/VariantB',
507+
},
508+
},
509+
oneOf: [
510+
{ $ref: '#/components/schemas/VariantA' },
511+
{ $ref: '#/components/schemas/VariantB' },
512+
],
513+
},
514+
VariantA: {
515+
allOf: [
516+
{ $ref: '#/components/schemas/Parent' },
517+
{ type: 'object', properties: { extraA: { type: 'number' } } },
518+
],
519+
},
520+
VariantB: {
521+
allOf: [
522+
{ $ref: '#/components/schemas/Parent' },
523+
{ type: 'object', properties: { extraB: { type: 'boolean' } } },
524+
],
525+
},
526+
};
527+
528+
const result = resolveDiscriminators(structuredClone(schemas), context);
529+
const aAllOf = (
530+
result.VariantA as NonNullable<OpenApiSchemasObject[string]>
531+
).allOf as (OpenApiSchemaObject | OpenApiReferenceObject)[] | undefined;
532+
const bAllOf = (
533+
result.VariantB as NonNullable<OpenApiSchemasObject[string]>
534+
).allOf as (OpenApiSchemaObject | OpenApiReferenceObject)[] | undefined;
535+
const aInlined = aAllOf?.[0] as OpenApiSchemaObject | undefined;
536+
const bInlined = bAllOf?.[0] as OpenApiSchemaObject | undefined;
537+
538+
expect(aInlined?.properties).not.toBe(bInlined?.properties);
539+
expect(aInlined).not.toBe(bInlined);
540+
});
541+
439542
it('leaves allOf untouched when parent has no top-level oneOf (#3432 guard)', () => {
440543
// Sanity check: the existing recursive-discriminator-allof shape (parent
441544
// is a plain interface, variants inherit via allOf) must keep emitting an

packages/core/src/getters/discriminators.ts

Lines changed: 32 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -182,14 +182,39 @@ export function resolveDiscriminators(
182182
rewritten.push(item);
183183
continue;
184184
}
185+
// Preserve the parent's other object-level constraints
186+
// (additionalProperties, minProperties, description, etc.) by shallow-
187+
// cloning the parent and only stripping the parts that would re-create
188+
// the cycle or are now meaningless on the variant.
189+
const inlinedParent = {
190+
...(parentSchema as Record<string, unknown>),
191+
} as OpenApiSchemaObject;
192+
delete (inlinedParent as Record<string, unknown>).oneOf;
193+
delete (inlinedParent as Record<string, unknown>).discriminator;
194+
delete (inlinedParent as Record<string, unknown>).allOf;
195+
delete (inlinedParent as Record<string, unknown>).anyOf;
196+
185197
if (hasInheritableProps) {
186-
rewritten.push({
187-
type: 'object',
188-
properties: inheritableProps,
189-
...(inheritableRequired && inheritableRequired.length > 0
190-
? { required: inheritableRequired }
191-
: {}),
192-
} as OpenApiSchemaObject);
198+
// Fresh per-variant clone so downstream in-place mutations on one
199+
// variant don't leak across siblings.
200+
inlinedParent.properties = { ...inheritableProps };
201+
} else {
202+
delete (inlinedParent as Record<string, unknown>).properties;
203+
}
204+
if (inheritableRequired && inheritableRequired.length > 0) {
205+
inlinedParent.required = [...inheritableRequired];
206+
} else {
207+
delete (inlinedParent as Record<string, unknown>).required;
208+
}
209+
210+
// Drop the entry entirely when the parent contributed nothing beyond
211+
// a bare `type: 'object'` — the second allOf member (the variant's
212+
// own inline object) already asserts object-ness.
213+
const meaningfulKeys = Object.keys(
214+
inlinedParent as Record<string, unknown>,
215+
).filter((key) => key !== 'type');
216+
if (meaningfulKeys.length > 0) {
217+
rewritten.push(inlinedParent);
193218
}
194219
}
195220

0 commit comments

Comments
 (0)