refactor(downgrader): reuse aliasEnd, skip the second pass for path item fields, fix oracle drift

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<string, unknown>) => boolean): string {
+function skipAliases(ref: string, ctx: Context, follow: (next: string, target: Record<string, unknown>) => 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<string, unknown>, 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<Record<string, Fields>>): 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<Record<string, Fields>>, 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<string, Location>()
   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<string>()
-  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<string>()
@@ -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 {

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<string, unknown>): unknown {
   return { $ref: value.$ref }

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 {

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' },

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()
+  })
+})

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<string, unknown>)[key]