fix: handle allOf/ref/circular structure interactions (via #1435)

* fix: support path parameter inclusion when used twice in a path

* fix quote

* add document test runner

* harness!

* break test cases into two files

* rework runner

* generateAbsoluteRefPatches in allOf

* handle nock interception state

* migrate expect@1 spies to jest

via `jscodeshift -t node_modules/jest-codemods/dist/transformers/expect.js test`

* install `jest-stare` test reporter

* guard externally-nullable values in generateAbsoluteRefPatches

* migrate misc. tests to new $$ref format

* absolutify $$ref artifact pointers

* use absolutifyPointer in generateAbsoluteRefPatches

* expect circular reference resolution

* add test cases for multi-member nested remoteRef->localRef allOf

* add nested allOf->local+remoteRef->localRef->remoteRelativeRef cases

* modify test to cover $ref nested within allOf member

* clean up generateAbsoluteRefPatches

* assert no errors in instantiation test cases

* uninstall `mocha-webpack`

* uninstall `jest-stare`

* update lockfile

* linter fixes

* remove comment

* add failing circular reference cases

* update SWOS-109 circular reference tests

* add file-level skips

* create new `useCircularStructures` option

* absolutify $refs that will be left unresolved because they complete a circular link

* ignore presence of allOf path portions in ref plugin's pointerAlreadyInPath

.gitignore

@@ -23,5 +23,7 @@ test/specmap/data/private
 test/webpack-bundle/.tmp
 browser
 
+jest-stare
+
 # Automated releases
 release/.version

package.json

@@ -73,12 +73,13 @@
     "eslint": "^3.18.0",
     "eslint-config-airbnb-base": "^11.1.1",
     "eslint-plugin-import": "^2.11.0",
-    "expect": "^1.20.2",
+    "expect": "^24.8.0",
     "fetch-mock": "^5.12.0",
     "glob": "^7.1.1",
-    "jest": "^23.1.0",
+    "jest": "^23.6.0",
     "json-loader": "^0.5.4",
     "license-checker": "^8.0.3",
+    "nock": "^10.0.6",
     "npm-run-all": "^4.1.3",
     "release-it": "^7.4.8",
     "traverse": "^0.6.6",

src/index.js

@@ -71,6 +71,7 @@ Swagger.prototype = {
       spec: this.spec,
       url: this.url,
       allowMetaPatches: this.allowMetaPatches,
+      useCircularStructures: this.useCircularStructures,
       requestInterceptor: this.requestInterceptor || null,
       responseInterceptor: this.responseInterceptor || null
     }).then((obj) => {

src/resolver.js

@@ -32,7 +32,7 @@ export default function resolve(obj) {
   const {
     fetch, spec, url, mode, allowMetaPatches = true, pathDiscriminator,
     modelPropertyMacro, parameterMacro, requestInterceptor,
-    responseInterceptor, skipNormalization
+    responseInterceptor, skipNormalization, useCircularStructures,
   } = obj
 
   let {http, baseDoc} = obj
@@ -81,7 +81,8 @@ export default function resolve(obj) {
       allowMetaPatches, // allows adding .meta patches, which include adding `$$ref`s to the spec
       pathDiscriminator, // for lazy resolution
       parameterMacro,
-      modelPropertyMacro
+      modelPropertyMacro,
+      useCircularStructures,
     }).then(skipNormalization ? async a => a : normalizeSwagger)
   }
 }

src/specmap/helpers.js

@@ -1,7 +1,5 @@
-/* eslint-disable import/prefer-default-export */
-//
-// if/when another helper is added to this file,
-// please remove the eslint override and this comment!
+import traverse from 'traverse'
+import URL from 'url'
 
 // This will match if the direct parent's key exactly matches an item.
 const freelyNamedKeyParents = [
@@ -53,3 +51,31 @@ export function isFreelyNamed(parentPath) {
     (freelyNamedAncestors.some(el => parentStr.indexOf(el) > -1))
   )
 }
+
+export function generateAbsoluteRefPatches(obj, basePath, {
+  specmap,
+  getBaseUrlForNodePath = path => specmap.getContext([...basePath, ...path]).baseDoc,
+  targetKeys = ['$ref', '$$ref']
+} = {}) {
+  const patches = []
+
+  traverse(obj).forEach(function () {
+    if (targetKeys.indexOf(this.key) > -1) {
+      const nodePath = this.path // this node's path, relative to `obj`
+      const fullPath = basePath.concat(this.path)
+
+      const absolutifiedRefValue = absolutifyPointer(this.node, getBaseUrlForNodePath(nodePath))
+
+      patches.push(specmap.replace(fullPath, absolutifiedRefValue))
+    }
+  })
+
+  return patches
+}
+
+export function absolutifyPointer(pointer, baseUrl) {
+  const [urlPart, fragmentPart] = pointer.split('#')
+  const newRefUrlPart = URL.resolve(urlPart || '', baseUrl || '')
+
+  return fragmentPart ? `${newRefUrlPart}#${fragmentPart}` : newRefUrlPart
+}

src/specmap/index.js

@@ -26,7 +26,9 @@ class SpecMap {
       showDebug: false,
       allPatches: [], // only populated if showDebug is true
       pluginProp: 'specMap',
-      libMethods: Object.assign(Object.create(this), lib),
+      libMethods: Object.assign(Object.create(this), lib, {
+        getInstance: () => this
+      }),
       allowMetaPatches: false,
     }, opts)
 

src/specmap/lib/all-of.js

@@ -1,4 +1,4 @@
-import {isFreelyNamed} from '../helpers'
+import {isFreelyNamed, generateAbsoluteRefPatches} from '../helpers'
 
 export default {
   key: 'allOf',
@@ -34,7 +34,12 @@ export default {
     originalDefinitionObj = Object.assign({}, originalDefinitionObj)
     delete originalDefinitionObj.allOf
 
-    const allOfPatches = [specmap.replace(parent, {})].concat(val.map((toMerge, index) => {
+    const patches = []
+
+    // remove existing content
+    patches.push(specmap.replace(parent, {}))
+
+    val.forEach((toMerge, i) => {
       if (!specmap.isObject(toMerge)) {
         if (alreadyAddError) {
           return null
@@ -43,21 +48,36 @@ export default {
 
         const err = new TypeError('Elements in allOf must be objects')
         err.fullPath = fullPath // This is an array
-        return err
+        return patches.push(err)
       }
 
-      return specmap.mergeDeep(parent, toMerge)
-    }))
+      // Deeply merge the member's contents onto the parent location
+      patches.push(specmap.mergeDeep(parent, toMerge))
+
+      // Generate patches that migrate $ref values based on ContextTree information
+
+      // remove ["allOf"], which will not be present when these patches are applied
+      const collapsedFullPath = fullPath.slice(0, -1)
+
+      const absoluteRefPatches = generateAbsoluteRefPatches(toMerge, collapsedFullPath, {
+        getBaseUrlForNodePath: (nodePath) => {
+          return specmap.getContext([...fullPath, i, ...nodePath]).baseDoc
+        },
+        specmap
+      })
+
+      patches.push(...absoluteRefPatches)
+    })
 
     // Merge back the values from the original definition
-    allOfPatches.push(specmap.mergeDeep(parent, originalDefinitionObj))
+    patches.push(specmap.mergeDeep(parent, originalDefinitionObj))
 
     // If there was not an original $$ref value, make sure to remove
     // any $$ref value that may exist from the result of `allOf` merges
     if (!originalDefinitionObj.$$ref) {
-      allOfPatches.push(specmap.remove([].concat(parent, '$$ref')))
+      patches.push(specmap.remove([].concat(parent, '$$ref')))
     }
 
-    return allOfPatches
+    return patches
   }
 }

src/specmap/lib/refs.js

@@ -4,7 +4,7 @@ import qs from 'querystring-browser'
 import url from 'url'
 import lib from '../lib'
 import createError from '../lib/create-error'
-import {isFreelyNamed} from '../helpers'
+import {isFreelyNamed, absolutifyPointer} from '../helpers'
 
 const ABSOLUTE_URL_REGEXP = new RegExp('^([a-z]+://|//)', 'i')
 
@@ -43,6 +43,7 @@ const specmapRefs = new WeakMap()
 const plugin = {
   key: '$ref',
   plugin: (ref, key, fullPath, specmap) => {
+    const specmapInstance = specmap.getInstance()
     const parent = fullPath.slice(0, -1)
     if (isFreelyNamed(parent)) {
       return
@@ -78,7 +79,20 @@ const plugin = {
     let tokens
 
     if (pointerAlreadyInPath(pointer, basePath, parent, specmap)) {
-      return // TODO: add some meta data, to indicate its cyclic!
+      // Cyclic reference!
+      // if `useCircularStructures` is not set, just leave the reference
+      // unresolved, but absolutify it so that we don't leave an invalid $ref
+      // path in the content
+      if (!specmapInstance.useCircularStructures) {
+        const absolutifiedRef = absolutifyPointer(ref, basePath)
+
+        if (ref === absolutifiedRef) {
+          // avoids endless looping
+          // without this, the ref plugin never stops seeing this $ref
+          return null
+        }
+        return lib.replace(fullPath, absolutifiedRef)
+      }
     }
 
     if (basePath == null) {
@@ -115,13 +129,17 @@ const plugin = {
       return [lib.remove(fullPath), promOrVal]
     }
 
-    const patch = lib.replace(parent, promOrVal, {$$ref: ref})
+    const absolutifiedRef = absolutifyPointer(ref, basePath)
+
+    const patch = lib.replace(parent, promOrVal, {$$ref: absolutifiedRef})
     if (basePath && basePath !== baseDoc) {
       return [patch, lib.context(parent, {baseDoc: basePath})]
     }
 
     try {
-      if (!patchValueAlreadyInPath(specmap.state, patch)) {
+      // prevents circular values from being constructed, unless we specifically
+      // want that to happen
+      if (!patchValueAlreadyInPath(specmap.state, patch) || specmapInstance.useCircularStructures) {
         return patch
       }
     }
@@ -383,11 +401,23 @@ function pointerAlreadyInPath(pointer, basePath, parent, specmap) {
   const parentPointer = arrayToJsonPointer(parent)
   const fullyQualifiedPointer = `${basePath || '<specmap-base>'}#${pointer}`
 
+  // dirty hack to strip `allof/[index]` from the path, in order to avoid cases
+  // where we get false negatives because:
+  // - we resolve a path, then
+  // - allOf plugin collapsed `allOf/[index]` out of the path, then
+  // - we try to work on a child $ref within that collapsed path.
+  //
+  // because of the path collapse, we lose track of it in our specmapRefs hash
+  // solution: always throw the allOf constructs out of paths we store
+  // TODO: solve this with a global register, or by writing more metadata in
+  // either allOf or refs plugin
+  const safeParentPointer = parentPointer.replace(/allOf\/\d+\/?/g, '')
+
   // Case 1: direct cycle, e.g. a.b.c.$ref: '/a.b'
   // Detect by checking that the parent path doesn't start with pointer.
   // This only applies if the pointer is internal, i.e. basePath === rootPath (could be null)
   const rootDoc = specmap.contextTree.get([]).baseDoc
-  if (basePath == rootDoc && pointerIsAParent(parentPointer, pointer)) { // eslint-disable-line
+  if (basePath == rootDoc && pointerIsAParent(safeParentPointer, pointer)) { // eslint-disable-line
     return true
   }
 
@@ -413,7 +443,8 @@ function pointerAlreadyInPath(pointer, basePath, parent, specmap) {
 
   // No cycle, this ref will be resolved, so stores it now for future detection.
   // No need to store if has cycle, as parent path is a dead-end and won't be checked again.
-  refs[parentPointer] = (refs[parentPointer] || []).concat(fullyQualifiedPointer)
+
+  refs[safeParentPointer] = (refs[safeParentPointer] || []).concat(fullyQualifiedPointer)
 }
 
 /**

src/subtree-resolver/index.js

@@ -33,7 +33,8 @@ export default async function resolveSubtree(obj, path, opts = {}) {
     requestInterceptor,
     responseInterceptor,
     parameterMacro,
-    modelPropertyMacro
+    modelPropertyMacro,
+    useCircularStructures,
   } = opts
 
   const resolveOptions = {
@@ -42,7 +43,8 @@ export default async function resolveSubtree(obj, path, opts = {}) {
     requestInterceptor,
     responseInterceptor,
     parameterMacro,
-    modelPropertyMacro
+    modelPropertyMacro,
+    useCircularStructures,
   }
 
   const {spec: normalized} = normalizeSwagger({

test/client.js

@@ -2,7 +2,6 @@ import http from 'http'
 import url from 'url'
 import path from 'path'
 import fs from 'fs'
-import {createSpy} from 'expect'
 
 import Swagger from '../src/index'
 
@@ -288,25 +287,25 @@ describe('http', () => {
   })
 
   test('should use requestInterceptor for resolving nested $refs', () => {
-    const requestInterceptor = createSpy().andCallThrough(req => req)
+    const requestInterceptor = jest.fn()
     return Swagger({
       url: 'http://localhost:8000/nested/one.yaml',
       requestInterceptor
     }).then((client) => {
-      expect(requestInterceptor.calls.length).toEqual(3)
-      expect(requestInterceptor.calls[0].arguments[0].url).toEqual('http://localhost:8000/nested/one.yaml')
-      expect(requestInterceptor.calls[1].arguments[0].url).toEqual('http://localhost:8000/nested/two.yaml')
-      expect(requestInterceptor.calls[2].arguments[0].url).toEqual('http://localhost:8000/nested/three.yaml')
+      expect(requestInterceptor.mock.calls.length).toEqual(3)
+      expect(requestInterceptor.mock.calls[0][0].url).toEqual('http://localhost:8000/nested/one.yaml')
+      expect(requestInterceptor.mock.calls[1][0].url).toEqual('http://localhost:8000/nested/two.yaml')
+      expect(requestInterceptor.mock.calls[2][0].url).toEqual('http://localhost:8000/nested/three.yaml')
 
       expect(client.spec).toEqual({
         data: {
           value: 'one!',
           nested: {
-            $$ref: './two.yaml',
+            $$ref: 'http://localhost:8000/nested/two.yaml',
             data: {
               value: 'two!',
               nested: {
-                $$ref: './three.yaml',
+                $$ref: 'http://localhost:8000/nested/three.yaml',
                 value: 'three!'
               }
             }

test/resolver.js

@@ -144,19 +144,19 @@ describe('resolver', () => {
           one: {
             a: {
               b: {
-                $ref: '#/two'
+                $ref: 'http://example.com/swagger.json#/two'
               }
             }
           },
           three: {
             b: {
-              $ref: '#/two'
+              $ref: 'http://example.com/swagger.json#/two'
             }
           },
           two: {
             a: {
               b: {
-                $ref: '#/two'
+                $ref: 'http://example.com/swagger.json#/two'
               }
             }
           }