diff --git a/packages/downgrader/src/shared.ts b/packages/downgrader/src/shared.ts index 091a5d4..515b099 100644 --- a/packages/downgrader/src/shared.ts +++ b/packages/downgrader/src/shared.ts @@ -731,7 +731,7 @@ function isRemovedAlias(ref: string, ctx: Context, convert: Convert): boolean { return end !== undefined && ctx.dangles(end) && convertsToDrop(end, ctx, convert) } -export function skipAliases(ref: string, ctx: Context, follow: (next: string, target: Record) => boolean): string { +function skipAliases(ref: string, ctx: Context, follow: (next: string, target: Record) => boolean): string { const hops = new Set([ref]) let hop = ref for (;;) { @@ -824,6 +824,32 @@ function isPathItemPointer(tokens: readonly string[]): boolean { && (tokens.length === 4 ? first === 'components' : isOperationPointer(tokens.slice(0, -3))) } +// Whether `tokens` points at or into one of `fields`, which converting a Path +// Item drops. A Path Item that is copied rather than converted keeps every +// field, so the walk rejects those: extensions of `paths`, and anything under +// a Callback Object that is a Reference Object or under a value that is not +// an object. +function isInDroppedPathItemField(root: unknown, tokens: readonly string[] | undefined, fields: readonly string[]): boolean { + if (tokens === undefined) { + return false + } + let node = root + for (const [index, token] of tokens.entries()) { + if ( + !isRecord(node) + || (index === 1 && tokens[0] === 'paths' && !isPath(token)) + || (tokens[index - 2] === 'callbacks' && getRef(node) !== undefined) + ) { + return false + } + if (fields.includes(token) && isPathItemPointer(tokens.slice(0, index))) { + return true + } + node = child(node, token) + } + return false +} + function mergeMissing(out: Record, target: unknown): void { if (isRecord(target)) { for (const [key, item] of Object.entries(target)) { @@ -947,10 +973,20 @@ export function mergeRef(convert: Convert): Finish { } } -export function removedPrefixes(tables: Readonly>): string[] { - return Object.entries(tables).flatMap(([base, fields]) => - [...fields].filter(([, field]) => field === DROP).map(([key]) => `#${base}/${key}/`), - ) +interface Removed { + readonly pathItemFields: readonly string[] + readonly prefixes: readonly string[] +} + +function droppedKeys(fields: Fields): string[] { + return [...fields].filter(([, field]) => field === DROP).map(([key]) => key) +} + +export function removedParts(tables: Readonly>, pathItem: Fields): Removed { + return { + pathItemFields: droppedKeys(pathItem), + prefixes: Object.entries(tables).flatMap(([base, fields]) => droppedKeys(fields).map(key => `#${base}/${key}/`)), + } } function danglesIn(output: unknown, source: unknown, tokens: readonly string[] | undefined): boolean { @@ -972,7 +1008,7 @@ function danglesIn(output: unknown, source: unknown, tokens: readonly string[] | return to === undefined || (isRecord(to) && PLACEHOLDERS.has(to)) } -export function downgrade(root: unknown, convert: Convert, removed: readonly string[] = []): unknown { +export function downgrade(root: unknown, convert: Convert, removed: Removed = { pathItemFields: [], prefixes: [] }): unknown { const locations = new Map() const locateRef = (ref: string): Location => { let location = locations.get(ref) @@ -1015,7 +1051,15 @@ export function downgrade(root: unknown, convert: Convert, removed: readonly str return (isRecord(target) || typeof target === 'boolean') && aliasEnd(ref) !== undefined } const dangling = new Set() - const isRemovedPart = (ref: string): boolean => removed.some(prefix => ref.startsWith(prefix)) + const isRemovedPart = (ref: string): boolean => removed.prefixes.some(prefix => ref.startsWith(prefix)) + // Without this, a reference into a dropped Path Item field would only be + // found dangling once a pass ends, costing a second pass over everything. + // Most references spell out none of those fields, so they are not parsed. + // Links and mappings still go by `isRemovedPart` alone, so one into such a + // field is removed only when its target dangles. + const isKnownGone = (ref: string): boolean => + isRemovedPart(ref) + || (removed.pathItemFields.some(field => ref.includes(field)) && isInDroppedPathItemField(root, parsePointer(ref), removed.pathItemFields)) let previous = root for (;;) { const kept = new Set() @@ -1026,7 +1070,7 @@ export function downgrade(root: unknown, convert: Convert, removed: readonly str copies: new Map(), dangles: (ref) => { if (!dangling.has(ref) && !kept.has(ref)) { - if ((isRemovedPart(ref) || (previous !== root && danglesIn(previous, root, parsePointer(ref)))) && isInlinable(ref)) { + if ((isKnownGone(ref) || (previous !== root && danglesIn(previous, root, parsePointer(ref)))) && isInlinable(ref)) { dangling.add(ref) } else { diff --git a/packages/downgrader/src/v3.1-to-v3.0.ts b/packages/downgrader/src/v3.1-to-v3.0.ts index 5e685f4..8e5aa7a 100644 --- a/packages/downgrader/src/v3.1-to-v3.0.ts +++ b/packages/downgrader/src/v3.1-to-v3.0.ts @@ -30,7 +30,7 @@ import { placeholder, rebasedRef, refOr, - removedPrefixes, + removedParts, resourceOf, setOwn, } from './shared' @@ -192,7 +192,7 @@ const DOCUMENT_FIELDS = defineFields({ webhooks: DROP, }) -const REMOVED = removedPrefixes({ '': DOCUMENT_FIELDS, '/components': COMPONENTS_FIELDS }) +const REMOVED = removedParts({ '': DOCUMENT_FIELDS, '/components': COMPONENTS_FIELDS }, PATH_ITEM_FIELDS) function reference(value: Record): unknown { return { $ref: value.$ref } diff --git a/packages/downgrader/src/v3.2-to-v3.1.ts b/packages/downgrader/src/v3.2-to-v3.1.ts index bf13c56..53a92fa 100644 --- a/packages/downgrader/src/v3.2-to-v3.1.ts +++ b/packages/downgrader/src/v3.2-to-v3.1.ts @@ -27,8 +27,7 @@ import { map, mergeRef, refOr, - removedPrefixes, - skipAliases, + removedParts, } from './shared' const V32_DIALECT_PREFIX = 'https://spec.openapis.org/oas/3.2/dialect/' @@ -181,7 +180,7 @@ const DOCUMENT_FIELDS = defineFields({ webhooks: map(convertPathItem), }) -const REMOVED = removedPrefixes({ '': DOCUMENT_FIELDS, '/components': COMPONENTS_FIELDS }) +const REMOVED = removedParts({ '': DOCUMENT_FIELDS, '/components': COMPONENTS_FIELDS }, PATH_ITEM_FIELDS) function convertServer(value: unknown, ctx: Context): unknown { return convertObject(value, ctx, SERVER_FIELDS) @@ -269,7 +268,11 @@ function convertMediaType(value: unknown, ctx: Context): unknown { function convertContentEntry(value: unknown, ctx: Context): unknown { const ref = getRef(value) - return ref === undefined ? convertMediaType(value, ctx) : inline(skipAliases(ref, ctx, () => true), ctx, convertContentEntry) + if (ref === undefined) { + return convertMediaType(value, ctx) + } + const end = ctx.aliasEnd(ref) + return end === undefined ? DROP : inline(end, ctx, convertMediaType) } function convertRequestBody(value: unknown, ctx: Context): unknown { diff --git a/packages/downgrader/tests/v3.2-to-v3.1/spec/removed-parts.test.ts b/packages/downgrader/tests/v3.2-to-v3.1/spec/removed-parts.test.ts index e70eb1b..3d89d1c 100644 --- a/packages/downgrader/tests/v3.2-to-v3.1/spec/removed-parts.test.ts +++ b/packages/downgrader/tests/v3.2-to-v3.1/spec/removed-parts.test.ts @@ -6,7 +6,7 @@ // - parameter lists that lost `querystring` entries, since the indices of // the entries after a removed one shift -import { cyclicCallbackGraph, dig } from '../../helpers' +import { countReads, cyclicCallbackGraph, dig } from '../../helpers' import { expectValidAs } from '../../validate' import { convertComponent, convertPathItem, convertSpec } from './helpers' @@ -214,6 +214,25 @@ describe('reference Objects', () => { }) }) + // Every Path Item conversion drops `query` and `additionalOperations`, so + // references into them are known to dangle from the input alone. Finding + // that out only when a pass ends would convert the whole document again. + it('handles references into query and additionalOperations in a single pass', () => { + const reads = { count: 0 } + const result = convertSpec({ + components: { + links: { L: { operationRef: '#/paths/~1a/query' } }, + responses: { R: { $ref: '#/paths/~1a/additionalOperations/COPY/responses/200' } }, + }, + paths: { + '/a': { additionalOperations: { COPY: { responses: { 200: { summary: 'Copied' } } } }, query: {} }, + '/b': countReads({ get: { responses: {} } }, 'get', reads), + }, + }) + expect(reads.count).toBe(1) + expect(result.components).toEqual({ links: {}, responses: { R: { description: 'Copied' } } }) + }) + it('follows chains through removed parts and keeps the reference where a chain reaches a surviving part', () => { const result = convertSpec({ components: { @@ -402,6 +421,31 @@ describe('references left as written', () => { }) }) + // Only converted Path Items lose `query`. An extension of `paths`, and a + // Callback Object that is a Reference Object, are copied with all their + // fields, and a component or a file may just be named `query`, so + // references to them still resolve. + it('leaves references to a query that the conversion keeps as written', () => { + const responses = { 200: { description: 'kept' } } + const references = { + Callback: { $ref: '#/components/callbacks/C/{$url}/query/responses/200' }, + Extension: { $ref: '#/paths/x-shared/query/responses/200' }, + File: { $ref: 'query.yaml' }, + Named: { $ref: '#/components/responses/query' }, + query: { description: 'named' }, + } + const result = convertSpec({ + components: { + callbacks: { C: { '$ref': '#/components/callbacks/D', '{$url}': { query: { responses } } }, D: {} }, + responses: references, + }, + paths: { 'x-shared': { query: { responses } } }, + }) + expect(dig(result, 'components', 'responses')).toEqual(references) + expect(dig(result, 'components', 'callbacks', 'C', '{$url}', 'query')).toEqual({ responses }) + expect(dig(result, 'paths', 'x-shared', 'query')).toEqual({ responses }) + }) + it('leaves references whose alias chain loops as written', () => { const parameters = { A: { $ref: '#/components/parameters/B' }, diff --git a/packages/downgrader/tests/validate.test.ts b/packages/downgrader/tests/validate.test.ts new file mode 100644 index 0000000..ffe65a4 --- /dev/null +++ b/packages/downgrader/tests/validate.test.ts @@ -0,0 +1,18 @@ +import { expectNoNewDanglingRefs } from './validate' + +// The oracle checks only references that resolved in the input, so a +// reference that wrongly resolved there would be held to the output too. +describe('expectNoNewDanglingRefs', () => { + it('fails when a reference that resolved in the input no longer does', () => { + expect(() => expectNoNewDanglingRefs({ a: [1] }, { r: { $ref: '#/a/0' } })).toThrow() + }) + + // https://www.rfc-editor.org/rfc/rfc6901#section-4 + it.each([ + ['an array property that is not an index', '#/a/length'], + ['an index with a leading zero', '#/a/00'], + ['a plain-name fragment, which names an anchor', '#pet'], + ])('does not resolve %s', (_name, ref) => { + expect(() => expectNoNewDanglingRefs({ a: [1], et: 1 }, { r: { $ref: ref } })).not.toThrow() + }) +}) diff --git a/packages/downgrader/tests/validate.ts b/packages/downgrader/tests/validate.ts index 15c32cb..4add380 100644 --- a/packages/downgrader/tests/validate.ts +++ b/packages/downgrader/tests/validate.ts @@ -10,7 +10,13 @@ const validator = new Validator() /** * Resolves a local `$ref` such as `#/components/schemas/Pet` against `root`. * The fragment is percent-decoded first (RFC 3986) and then split into - * JSON Pointer tokens with `~1` and `~0` unescaped (RFC 6901). + * JSON Pointer tokens with `~1` and `~0` unescaped (RFC 6901). A fragment + * that does not start with `/`, such as the anchor `#pet`, is not a pointer. + * Only an index without leading zeros selects an array element. + * + * This duplicates `parsePointer` and `child` in `src/shared.ts` on purpose. + * The conversion uses them to decide which references dangle, so an oracle + * built on them would agree with any bug in them. */ function resolvePointer(root: unknown, ref: string): unknown { if (!ref.startsWith('#')) { @@ -26,10 +32,18 @@ function resolvePointer(root: unknown, ref: string): unknown { if (pointer === '') { return root } + if (!pointer.startsWith('/')) { + return undefined + } let current = root for (const token of pointer.slice(1).split('/')) { const key = token.replaceAll('~1', '/').replaceAll('~0', '~') - if (typeof current !== 'object' || current === null || !Object.hasOwn(current, key)) { + if ( + typeof current !== 'object' + || current === null + || (Array.isArray(current) && !/^(?:0|[1-9]\d*)$/.test(key)) + || !Object.hasOwn(current, key) + ) { return undefined } current = (current as Record)[key]