Skip to content

Commit 5901753

Browse files
devin-ai-integration[bot]jsklanbot_apk
authored
fix(cli): resolve allOf composition bugs in V3 OpenAPI importer (#14873)
* chore(seed): add allOf composition test fixtures Add two test-definition fixtures exercising allOf edge cases: - allof: default settings (extends mode) - allof-inline: inline-all-of-schemas enabled, shares the same spec Covers array items narrowing, primitive constraint narrowing via $ref, required propagation, metadata inheritance, and multi-parent overlap. * chore(internal): add allOf debug pipeline script and IR snapshots Add debug-allof-pipeline.ts for running allof/allof-inline fixtures through the full CLI pipeline (openapi-ir → write-definition → ir). Include generated IR snapshots confirming remaining allOf bugs. * test(cli): add assertion-based allOf tests reproducing SPS Commerce bugs Add V3 importer test fixture and assertion tests for the allOf edge cases reported in Pylon #18189 / FER-9158. Tests validate: - Case A: array items narrowing preserves parent required status - Case B: property-level allOf with $ref + inline primitive - Case C: required field preservation through allOf composition 7 of 8 assertions currently fail, confirming the bugs. The task is complete when all 8 pass. * fix(cli): resolve allOf composition bugs in V3 OpenAPI importer - Enhance shouldMergeAllOf path to resolve $ref elements instead of bailing out - Add cycle detection via Set to prevent infinite loops on circular refs - Handle property-level allOf with $ref to non-object types (e.g. enums) by referencing the original type instead of creating synthetic copies - Update test infrastructure to serialize IR discriminants for assertions - Update snapshots for allof-spscommerce fixture Co-Authored-By: bot_apk <apk@cognition.ai> * fix: resolve biome noNonNullAssertion lint errors in SchemaOrReferenceConverter Co-Authored-By: bot_apk <apk@cognition.ai> * fix: add biome-ignore for pre-existing noNonNullAssertion in test file Co-Authored-By: bot_apk <apk@cognition.ai> * fix: update v3-sdks snapshots for allOf merge changes Co-Authored-By: bot_apk <apk@cognition.ai> * fix: update convertIRtoJsonSchema snapshots for allof/allof-inline Co-Authored-By: bot_apk <apk@cognition.ai> * update config * Add case 6 * seed outputs * Automated update of seed files * fix: update convertIRtoJsonSchema snapshots for new allof types Co-Authored-By: bot_apk <apk@cognition.ai> * fix: thread visitedRefs across recursive SchemaConverter calls for cross-schema cycle detection Co-Authored-By: bot_apk <apk@cognition.ai> * fix: format SchemaConverter.ts constructor for biome Co-Authored-By: bot_apk <apk@cognition.ai> * fix: update ir-generator-tests test-definitions for allof-inline Co-Authored-By: bot_apk <apk@cognition.ai> * fix: update ir-generator-tests test-definitions for allof Co-Authored-By: bot_apk <apk@cognition.ai> * fix: deduplicate merged allOf refs and guard composition-keyword shortcuts Deduplicate $ref entries in the merged allOf array after mergeWith array-concatenation to prevent false-positive cycle detection in diamond inheritance patterns (A → B, C; B, C → D). Add allOf/oneOf/anyOf checks to the resolved-schema guard in maybeConvertSingularAllOfReferenceObject so the shortcut only fires for true scalar/enum primitives, not schemas defined via composition keywords whose inline constraints would be silently discarded. * fix: guard single-element allOf cycles and add versions.yml entry Thread visitedRefs through the single-element allOf branch in SchemaConverter to detect self-referential and indirect cycles (e.g. A → B → A via single-element allOf chains), matching the protection already present in the multi-element shouldMergeAllOf path. Add the required versions.yml changelog entry for the allOf composition fixes in this PR. * nits * fix(cli): address review feedback on allOf composition - Preserve outer `inlined` status through allOf merge path instead of hardcoding `inlined: true` - Guard variants-flattening against $ref-resolved schemas to prevent destructive flattening of named union types - Seed dedup seenRefs from resolvedRefs to prevent duplicate property declarations in diamond inheritance - Bail out of allOf shortcut when inline elements contain `type: "null"` to preserve nullable semantics in OpenAPI 3.1 patterns - Add optional guard for required executionContext field in Case B test - Remove debug script (scripts/debug-allof-pipeline.ts) - Remove orphaned allof-spscommerce snapshot files - Move changelog to changes/unreleased/ per release-versioning convention * fix(cli): address remaining review comments on allOf composition - Fix inlined: true -> inlined: this.inlined in single-element allOf path - Propagate inlinedTypes in both allOf shortcuts in SchemaOrReferenceConverter - Update stale test comment and remove duplicate biome-ignore - Update v3-sdks snapshots Co-Authored-By: judah <jsklan.development@gmail.com> * fix(cli): separate resolvedRefs concerns and add format keyword guard - SchemaConverter.ts: Use separate localResolvedRefs set for tracking refs within current allOf array. Check this.visitedRefs for ancestor cycles only. Same-array duplicate $refs now skip (continue) instead of triggering hasCycle=true. - SchemaOrReferenceConverter.ts: Add !s.format guard to inline elements check and !resolved.format guard to resolved schema check in maybeConvertSingularAllOfReferenceObject, preventing the shortcut from discarding inline format constraints like {format: "date-time"}. Co-Authored-By: bot_apk <apk@cognition.ai> * style: fix biome formatting for inline format guard Co-Authored-By: bot_apk <apk@cognition.ai> * fix(cli): preserve outer schema metadata in shouldMergeAllOf path Copy TYPE_INVARIANT_KEYS (description, deprecated, example, default, readOnly, writeOnly, etc.) from the outer this.schema into mergedSchema before constructing mergedConverter, so metadata fields are not silently dropped for schemas routed through the shouldMergeAllOf path. Co-Authored-By: bot_apk <apk@cognition.ai> * refactor(cli): replace lodash.mergeWith with per-key allOf schema merge Replace the generic lodash.mergeWith deep merge in the shouldMergeAllOf path with a purpose-built mergeAllOfSchemas function that uses explicit per-key strategies for OpenAPI schema keywords: - required: set union - properties/items: recursive deep merge - example/default: deep merge objects - deprecated/readOnly/writeOnly: OR (true if any says true) - min/max constraints: most restrictive value - description/type/format/etc: outer schema wins - allOf from children: flattened before merge (prevents leakage) This eliminates the post-merge fixup code (allOf dedup block, TYPE_INVARIANT_KEYS loop) that was the source of cascading review issues. Also adds !s.items and array-form null-type guards to the allOf shortcut in SchemaOrReferenceConverter. * fix(cli): fix type errors in allOf schema merge and eliminate `as any` in tests Use `"items" in s` instead of `s.items` to avoid TS error on NonArraySchemaObject, and replace all `as any` casts in mergeAllOfSchemas tests with proper OpenAPI types. * fix(cli): harden allOf merge pipeline against $ref leaks, nested flattening, and metadata loss Filter unresolved $ref objects from nested allOf flattening to prevent spurious top-level $ref keys corrupting merged schemas. Make flattenNestedAllOf recursive so deeply-nested allOf structures are fully expanded. Add oneOf/anyOf to SKIP_FROM_CHILDREN so composition keywords from resolved child schemas don't leak into the merged result and erroneously trigger union conversion. Propagate outer schema metadata (description, deprecated) through the single-element allOf path for non-object types. * Revert seed output changes to reduce diff --------- Co-authored-by: jsklan <jsklan.development@gmail.com> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: bot_apk <apk@cognition.ai> Co-authored-by: jsklan <jsklan@users.noreply.github.com>
1 parent bff9e50 commit 5901753

63 files changed

Lines changed: 28901 additions & 330 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

packages/cli/api-importers/v3-importer-commons/src/converters/schema/SchemaConverter.ts

Lines changed: 94 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,12 @@
11
import * as FernIr from "@fern-api/ir-sdk";
2-
import { mergeWith } from "lodash-es";
32
import { OpenAPIV3_1 } from "openapi-types";
4-
53
import { AbstractConverter, AbstractConverterContext, Extensions } from "../../index.js";
64
import { createTypeReferenceFromFernType } from "../../utils/CreateTypeReferenceFromFernType.js";
75
import { ExampleConverter } from "../ExampleConverter.js";
86
import { ArraySchemaConverter } from "./ArraySchemaConverter.js";
97
import { EnumSchemaConverter } from "./EnumSchemaConverter.js";
108
import { MapSchemaConverter } from "./MapSchemaConverter.js";
9+
import { mergeAllOfSchemas } from "./mergeAllOfSchemas.js";
1110
import { ObjectSchemaConverter } from "./ObjectSchemaConverter.js";
1211
import { OneOfSchemaConverter } from "./OneOfSchemaConverter.js";
1312
import { PrimitiveSchemaConverter } from "./PrimitiveSchemaConverter.js";
@@ -31,6 +30,7 @@ export declare namespace SchemaConverter {
3130
schema: OpenAPIV3_1.SchemaObject;
3231
inlined?: boolean;
3332
nameOverride?: string;
33+
visitedRefs?: Set<string>;
3434
}
3535

3636
export interface ConvertedSchema {
@@ -51,13 +51,23 @@ export class SchemaConverter extends AbstractConverter<AbstractConverterContext<
5151
private readonly inlined: boolean;
5252
private readonly audiences: string[];
5353
private readonly nameOverride?: string;
54-
55-
constructor({ context, breadcrumbs, schema, id, inlined = false, nameOverride }: SchemaConverter.Args) {
54+
private readonly visitedRefs: Set<string>;
55+
56+
constructor({
57+
context,
58+
breadcrumbs,
59+
schema,
60+
id,
61+
inlined = false,
62+
nameOverride,
63+
visitedRefs
64+
}: SchemaConverter.Args) {
5665
super({ context, breadcrumbs });
5766
this.schema = schema;
5867
this.id = id;
5968
this.inlined = inlined;
6069
this.nameOverride = nameOverride;
70+
this.visitedRefs = visitedRefs ?? new Set<string>();
6171
this.audiences =
6272
this.context.getAudiences({
6373
operation: this.schema,
@@ -168,23 +178,53 @@ export class SchemaConverter extends AbstractConverter<AbstractConverterContext<
168178
this.schema.allOf?.length === 1 &&
169179
this.schema.allOf[0] != null
170180
) {
181+
const allOfElement = this.schema.allOf[0];
182+
183+
// Guard against single-element allOf cycles (e.g. A → B → A via single-element allOf chains)
184+
if (this.context.isReferenceObject(allOfElement)) {
185+
const refPath = allOfElement.$ref;
186+
if (this.visitedRefs.has(refPath)) {
187+
return undefined;
188+
}
189+
}
190+
171191
const allOfSchema = this.context.resolveMaybeReference<OpenAPIV3_1.SchemaObject>({
172-
schemaOrReference: this.schema.allOf[0],
192+
schemaOrReference: allOfElement,
173193
breadcrumbs: this.breadcrumbs
174194
});
175195

176196
if (allOfSchema != null) {
197+
const visitedRefsForChild = this.context.isReferenceObject(allOfElement)
198+
? new Set<string>([...this.visitedRefs, allOfElement.$ref])
199+
: this.visitedRefs;
200+
177201
const allOfConverter = new SchemaConverter({
178202
context: this.context,
179203
breadcrumbs: [...this.breadcrumbs, "allOf", "0"],
180204
schema: allOfSchema,
181205
id: this.id,
182-
inlined: true
206+
inlined: this.inlined,
207+
visitedRefs: visitedRefsForChild
183208
});
184209

185210
const allOfResult = allOfConverter.convert();
186211

187212
if (allOfResult?.convertedSchema.typeDeclaration?.shape.type !== "object") {
213+
// Propagate outer schema metadata (description, deprecated, etc.)
214+
// that would otherwise be lost since allOfConverter got the child schema
215+
if (allOfResult != null) {
216+
const decl = allOfResult.convertedSchema.typeDeclaration;
217+
if (this.schema.description != null && decl.docs == null) {
218+
decl.docs = this.schema.description;
219+
}
220+
const outerAvailability = this.context.getAvailability({
221+
node: this.schema,
222+
breadcrumbs: this.breadcrumbs
223+
});
224+
if (outerAvailability != null && decl.availability == null) {
225+
decl.availability = outerAvailability;
226+
}
227+
}
188228
return allOfResult;
189229
}
190230
}
@@ -196,18 +236,49 @@ export class SchemaConverter extends AbstractConverter<AbstractConverterContext<
196236
this.schema.allOf.length >= 1;
197237

198238
if (shouldMergeAllOf) {
199-
let mergedSchema: Record<string, unknown> = {};
239+
const localResolvedRefs = new Set<string>();
240+
const resolvedElements: OpenAPIV3_1.SchemaObject[] = [];
241+
let hasCycle = false;
242+
200243
for (const allOfSchema of this.schema.allOf ?? []) {
244+
let schemaToMerge: OpenAPIV3_1.SchemaObject;
245+
201246
if (this.context.isReferenceObject(allOfSchema)) {
202-
return undefined;
247+
const refPath = allOfSchema.$ref;
248+
// Check ancestor set for true cross-schema cycles
249+
if (this.visitedRefs.has(refPath)) {
250+
hasCycle = true;
251+
break;
252+
}
253+
// Skip same-array duplicates (e.g. allOf: [$ref:Base, $ref:Base])
254+
// without triggering the cycle breaker
255+
if (localResolvedRefs.has(refPath)) {
256+
continue;
257+
}
258+
localResolvedRefs.add(refPath);
259+
260+
const resolved = this.context.resolveMaybeReference<OpenAPIV3_1.SchemaObject>({
261+
schemaOrReference: allOfSchema,
262+
breadcrumbs: this.breadcrumbs
263+
});
264+
if (resolved == null) {
265+
return undefined;
266+
}
267+
schemaToMerge = resolved;
268+
} else {
269+
schemaToMerge = allOfSchema;
203270
}
204271

205272
// Handle bare oneOf/anyOf elements used for mutual exclusion patterns
206273
// (e.g., oneOf with variants containing `not: {}` properties).
207274
// Flatten variant properties into the merged schema as optional properties.
208-
const variants =
209-
(allOfSchema as OpenAPIV3_1.SchemaObject).oneOf ?? (allOfSchema as OpenAPIV3_1.SchemaObject).anyOf;
210-
if (variants != null && allOfSchema.type == null && allOfSchema.properties == null) {
275+
const variants = schemaToMerge.oneOf ?? schemaToMerge.anyOf;
276+
if (
277+
!this.context.isReferenceObject(allOfSchema) &&
278+
variants != null &&
279+
schemaToMerge.type == null &&
280+
schemaToMerge.properties == null
281+
) {
211282
const flattenedProperties: Record<string, unknown> = {};
212283
for (const variantSchemaOrRef of variants) {
213284
const variantSchema = this.context.isReferenceObject(variantSchemaOrRef)
@@ -235,31 +306,29 @@ export class SchemaConverter extends AbstractConverter<AbstractConverterContext<
235306
}
236307
}
237308
if (Object.keys(flattenedProperties).length > 0) {
238-
// Add flattened properties as optional on the merged schema
239-
const existingProperties = (mergedSchema.properties as Record<string, unknown>) ?? {};
240-
mergedSchema.properties = { ...existingProperties, ...flattenedProperties };
241-
// Do not add to required — variant properties are optional on the parent
309+
resolvedElements.push({ properties: flattenedProperties } as OpenAPIV3_1.SchemaObject);
242310
}
243311
continue;
244312
}
245313

246-
mergedSchema = mergeWith(mergedSchema, allOfSchema, (objValue, srcValue) => {
247-
if (srcValue === allOfSchema) {
248-
return objValue;
249-
}
250-
if (Array.isArray(objValue) && Array.isArray(srcValue)) {
251-
return [...objValue, ...srcValue];
252-
}
253-
return undefined;
254-
});
314+
resolvedElements.push(schemaToMerge);
315+
}
316+
317+
// If a circular reference was detected, fall back to the ObjectSchemaConverter path
318+
if (hasCycle) {
319+
return undefined;
255320
}
256321

322+
const mergedSchema = mergeAllOfSchemas(this.schema, resolvedElements);
323+
324+
const allResolvedRefs = new Set<string>([...this.visitedRefs, ...localResolvedRefs]);
257325
const mergedConverter = new SchemaConverter({
258326
context: this.context,
259327
breadcrumbs: this.breadcrumbs,
260328
schema: mergedSchema,
261329
id: this.id,
262-
inlined: true
330+
inlined: this.inlined,
331+
visitedRefs: allResolvedRefs
263332
});
264333
return mergedConverter.convert();
265334
}

packages/cli/api-importers/v3-importer-commons/src/converters/schema/SchemaOrReferenceConverter.ts

Lines changed: 59 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -82,26 +82,71 @@ export class SchemaOrReferenceConverter extends AbstractConverter<
8282
}
8383

8484
private maybeConvertSingularAllOfReferenceObject(): SchemaOrReferenceConverter.Output | undefined {
85-
if (
86-
this.context.isReferenceObject(this.schemaOrReference) ||
87-
this.schemaOrReference.allOf == null ||
88-
this.schemaOrReference.allOf.length !== 1
89-
) {
85+
if (this.context.isReferenceObject(this.schemaOrReference) || this.schemaOrReference.allOf == null) {
9086
return undefined;
9187
}
92-
const allOfReference = this.schemaOrReference.allOf[0];
93-
if (this.context.isReferenceObject(allOfReference)) {
94-
const response = this.context.convertReferenceToTypeReference({
95-
reference: allOfReference,
88+
89+
// Single $ref allOf — convert directly as a type reference
90+
if (this.schemaOrReference.allOf.length === 1) {
91+
const allOfReference = this.schemaOrReference.allOf[0];
92+
if (this.context.isReferenceObject(allOfReference)) {
93+
const response = this.context.convertReferenceToTypeReference({
94+
reference: allOfReference,
95+
breadcrumbs: this.breadcrumbs
96+
});
97+
if (response.ok) {
98+
return {
99+
type: this.wrapTypeReference(response.reference),
100+
inlinedTypes: response.inlinedTypes ?? {}
101+
};
102+
}
103+
}
104+
return undefined;
105+
}
106+
107+
// When allOf has exactly one $ref to a non-object type (e.g. enum) and the
108+
// remaining elements are inline primitive constraints (no properties, no enum,
109+
// no composition keywords), reference the $ref type directly instead of
110+
// creating a synthetic merged copy.
111+
const refElements = this.schemaOrReference.allOf.filter((s) => this.context.isReferenceObject(s));
112+
const inlineElements = this.schemaOrReference.allOf.filter(
113+
(s) => !this.context.isReferenceObject(s)
114+
) as OpenAPIV3_1.SchemaObject[];
115+
const singleRef = refElements.length === 1 ? refElements[0] : undefined;
116+
117+
if (
118+
singleRef != null &&
119+
inlineElements.every(
120+
(s) => !s.properties && !s.enum && !s.oneOf && !s.anyOf && !s.allOf && !s.format && !("items" in s)
121+
) &&
122+
!inlineElements.some((s) => s.type === "null" || (Array.isArray(s.type) && s.type.includes("null")))
123+
) {
124+
const resolved = this.context.resolveMaybeReference<OpenAPIV3_1.SchemaObject>({
125+
schemaOrReference: singleRef,
96126
breadcrumbs: this.breadcrumbs
97127
});
98-
if (response.ok) {
99-
return {
100-
type: this.wrapTypeReference(response.reference),
101-
inlinedTypes: {}
102-
};
128+
if (
129+
resolved != null &&
130+
resolved.type !== "object" &&
131+
!resolved.properties &&
132+
!resolved.allOf &&
133+
!resolved.oneOf &&
134+
!resolved.anyOf &&
135+
!resolved.format
136+
) {
137+
const response = this.context.convertReferenceToTypeReference({
138+
reference: singleRef as OpenAPIV3_1.ReferenceObject,
139+
breadcrumbs: this.breadcrumbs
140+
});
141+
if (response.ok) {
142+
return {
143+
type: this.wrapTypeReference(response.reference),
144+
inlinedTypes: response.inlinedTypes ?? {}
145+
};
146+
}
103147
}
104148
}
149+
105150
return undefined;
106151
}
107152

0 commit comments

Comments
 (0)