diff --git a/src/__tests__/mergeAllOf.spec.ts b/src/__tests__/mergeAllOf.spec.ts new file mode 100644 index 0000000..1aae0fe --- /dev/null +++ b/src/__tests__/mergeAllOf.spec.ts @@ -0,0 +1,23 @@ +import { ResolvingError } from '../errors'; +import { mergeAllOf } from '../mergers/mergeAllOf'; +import type { SchemaFragment } from '../types'; +import type { WalkingOptions } from '../walker/types'; + +describe('mergeAllOf', () => { + // Regression: mutually recursive allOf (A -> B -> A) caused _mergeAllOf to + // call itself with the same resolved fragment before seen-cache was populated, + // producing a RangeError (call stack overflow) in the caller. + it('throws ResolvingError (not a stack overflow) for mutually recursive allOf schemas', () => { + const schemaA: SchemaFragment = { allOf: [{ $ref: '#/B' }] }; + const schemaB: SchemaFragment = { allOf: [{ $ref: '#/A' }] }; + const refs: Record = { '#/A': schemaA, '#/B': schemaB }; + + const walkingOptions: WalkingOptions = { + mergeAllOf: true, + resolveRef: (_path, $ref) => refs[$ref] ?? {}, + maxRefDepth: null, + }; + + expect(() => mergeAllOf({ allOf: [{ $ref: '#/A' }] }, [], walkingOptions, new WeakMap())).toThrow(ResolvingError); + }); +}); diff --git a/src/mergers/mergeAllOf.ts b/src/mergers/mergeAllOf.ts index 24d89dd..89ac061 100644 --- a/src/mergers/mergeAllOf.ts +++ b/src/mergers/mergeAllOf.ts @@ -6,19 +6,22 @@ import type { WalkerRefResolver, WalkingOptions } from '../walker/types'; const resolveAllOf = require('@stoplight/json-schema-merge-allof'); -const store = new WeakMap>(); - function _mergeAllOf( fragment: SchemaFragment, path: string[], resolveRef: WalkerRefResolver | null, seen: WeakMap, + resolvedInPriorIterations: Set | null, ): SchemaFragment { const cached = seen.get(fragment); if (cached !== void 0) { return cached; } + // Track $refs resolved in THIS iteration so we can add them to + // resolvedInPriorIterations after the call completes. + const refsThisIteration = resolvedInPriorIterations !== null ? new Set() : null; + const merged = resolveAllOf(fragment, { deep: false, resolvers: resolveAllOf.stoplightResolvers, @@ -33,36 +36,26 @@ function _mergeAllOf( throw new ResolvingError('Circular reference detected'); } - const allRefs = store.get(resolveRef)!; - let schemaRefs = allRefs.get(fragment); - - if (schemaRefs === void 0) { - schemaRefs = [$ref]; - allRefs.set(fragment, schemaRefs); - } else if (schemaRefs.includes($ref)) { - const resolved = resolveRef(null, $ref); - return 'allOf' in resolved ? _mergeAllOf(resolved, path, resolveRef, seen) : resolved; - } else { - schemaRefs.push($ref); - } - - const resolved = resolveRef(null, $ref); - - if (Array.isArray(resolved.allOf)) { - for (const member of resolved.allOf) { - const index = schemaRefs.indexOf(member.$ref); - if (typeof member.$ref === 'string' && index !== -1 && index !== schemaRefs.lastIndexOf(member.$ref)) { - throw new ResolvingError('Circular reference detected'); - } - } + // A $ref seen in a prior do-while iteration means the chain is + // circular (e.g. A→B→A). Throw instead of looping forever. + if (resolvedInPriorIterations?.has($ref) === true) { + throw new ResolvingError('Circular reference detected'); } - return resolved; + refsThisIteration?.add($ref); + return resolveRef(null, $ref); }, } : null), }); + // Promote this iteration's refs so the next iteration can detect cycles. + if (resolvedInPriorIterations !== null && refsThisIteration !== null) { + for (const ref of refsThisIteration) { + resolvedInPriorIterations.add(ref); + } + } + seen.set(fragment, merged); return merged; } @@ -73,13 +66,13 @@ export function mergeAllOf( walkingOptions: WalkingOptions, seen: WeakMap, ) { - if (walkingOptions.resolveRef !== null && !store.has(walkingOptions.resolveRef)) { - store.set(walkingOptions.resolveRef, new WeakMap()); - } + // One set shared across all do-while iterations; grows monotonically so any + // $ref seen in iteration N will be detected as circular in iteration N+1. + const resolvedInPriorIterations = walkingOptions.resolveRef !== null ? new Set() : null; let merged = fragment; do { - merged = _mergeAllOf(merged, path, walkingOptions.resolveRef, seen); + merged = _mergeAllOf(merged, path, walkingOptions.resolveRef, seen, resolvedInPriorIterations); } while ('allOf' in merged); return merged;