fix: do not crash when operation has illegal $ref, print error instead (

#103)

* add hasRefProperty utility module, refactor some logic to use it

src/plugins/utils/caseConventionCheck.js

@@ -3,8 +3,8 @@
   directly follow letter characters, without an underscore in between. The
   snakecase module in lodash (which was previously used) did not allow this
   behavior. This is especially important in API paths e.g. '/api/v1/path'
-
 */
+
 const lowerSnakeCase = /^[a-z][a-z0-9]*(_[a-z0-9]+)*$/;
 const upperSnakeCase = /^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$/;
 const upperCamelCase = /^[A-Z][a-z0-9]+([A-Z][a-z0-9]+)*$/;

src/plugins/utils/hasRefProperty.js

@@ -0,0 +1,29 @@
+const at = require('lodash/at');
+
+/*
+  Checks the unresolved spec to see if the object at path `path`
+  has a `$ref` property. Useful when validating a resolved spec
+  and want to know if a certain object was referenced or defined
+  inline.
+*/
+
+module.exports = (jsSpec, path) => {
+  if (Array.isArray(path)) {
+    path = convertArrayToBracketNotation(path);
+  } else {
+    // if not array, assuming it is a dot separated string
+    //
+    // note: it is not a good idea to use this pattern,
+    // as parameter names sometimes have periods in them.
+    // only arrays should be passed in
+    path = convertArrayToBracketNotation(path.split('.'));
+  }
+
+  const objectAtGivenPath = at(jsSpec, path)[0];
+  return Boolean(objectAtGivenPath && objectAtGivenPath.$ref);
+};
+
+// returns a string with format parentKey['nextKey']['nextKey']['etc']
+function convertArrayToBracketNotation(path) {
+  return path.reduce((result, element) => result + `['${element}']`);
+}

src/plugins/utils/index.js

@@ -3,3 +3,4 @@
 module.exports.checkCase = require('./caseConventionCheck');
 module.exports.walk = require('./walk');
 module.exports.isParameterObject = require('./isParameter');
+module.exports.hasRefProperty = require('./hasRefProperty');

src/plugins/validation/2and3/semantic-validators/operations-shared.js

@@ -18,9 +18,9 @@ const pick = require('lodash/pick');
 const map = require('lodash/map');
 const each = require('lodash/each');
 const findIndex = require('lodash/findIndex');
-const { checkCase } = require('../../../utils');
+const { checkCase, hasRefProperty } = require('../../../utils');
 
-module.exports.validate = function({ resolvedSpec, isOAS3 }, config) {
+module.exports.validate = function({ jsSpec, resolvedSpec, isOAS3 }, config) {
   const result = {};
   result.error = [];
   result.warning = [];
@@ -41,11 +41,21 @@ module.exports.validate = function({ resolvedSpec, isOAS3 }, config) {
       'options',
       'trace'
     ]);
+
     each(pathOps, (op, opKey) => {
       if (!op || op['x-sdk-exclude'] === true) {
         return;
       }
 
+      // check for operations that have a $ref property
+      // these are illegal in the spec
+      if (hasRefProperty(jsSpec, ['paths', pathKey, opKey])) {
+        result.error.push({
+          path: `paths.${pathKey}.${opKey}.$ref`,
+          message: '$ref found in illegal location'
+        });
+      }
+
       // check for unique name/in properties in params
       each(op.parameters, (param, paramIndex) => {
         const nameAndInComboIndex = findIndex(op.parameters, {

src/plugins/validation/oas3/semantic-validators/operations.js

@@ -6,7 +6,7 @@
 
 const pick = require('lodash/pick');
 const each = require('lodash/each');
-const at = require('lodash/at');
+const { hasRefProperty } = require('../../../utils');
 
 module.exports.validate = function({ resolvedSpec, jsSpec }, config) {
   const result = {};
@@ -58,16 +58,18 @@ module.exports.validate = function({ resolvedSpec, jsSpec }, config) {
           const explodingBody = oneContentType && isJson && !hasArraySchema;
 
           // referenced request bodies have names
-          const referencedRequestBody = Boolean(
-            at(jsSpec, `paths['${pathName}']['${opName}']['requestBody']`)[0]
-              .$ref
-          );
+          const hasReferencedRequestBody = hasRefProperty(jsSpec, [
+            'paths',
+            pathName,
+            opName,
+            'requestBody'
+          ]);
 
           // form params do not need names
           if (
             !isFormParameter(firstMimeType) &&
             !explodingBody &&
-            !referencedRequestBody &&
+            !hasReferencedRequestBody &&
             !hasRequestBodyName
           ) {
             const checkStatus = config.no_request_body_name;

test/plugins/has-ref-property.js

@@ -0,0 +1,90 @@
+const expect = require('expect');
+const { hasRefProperty } = require('../../src/plugins/utils');
+
+const spec = {
+  paths: {
+    '/resource': {
+      post: {
+        description: 'a simple operation',
+        requestBody: {
+          $ref: 'external.yaml#/resource-post-request-body'
+        },
+        responses: {
+          '200': {
+            description: 'a simple response'
+          },
+          '400': {
+            $ref: '#/components/responses/ErrorResponse'
+          }
+        }
+      }
+    }
+  },
+  components: {
+    responses: {
+      ErrorResponse: {
+        description: 'error response with content',
+        content: {
+          'application/json': {
+            schema: {
+              $ref: 'external.yaml#/error-response-schema'
+            }
+          }
+        }
+      }
+    }
+  }
+};
+
+describe('has ref property - util', () => {
+  it('should return true when array leads to $ref property', () => {
+    const path = ['paths', '/resource', 'post', 'requestBody'];
+    const hasRef = hasRefProperty(spec, path);
+
+    expect(hasRef).toBe(true);
+  });
+
+  it('should return true when dot-separated-string leads to $ref property', () => {
+    const path = 'paths./resource.post.requestBody';
+    const hasRef = hasRefProperty(spec, path);
+
+    expect(hasRef).toBe(true);
+  });
+
+  it('should return false when array does not lead to $ref property', () => {
+    const path = ['paths', '/resource', 'post', 'responses', '200'];
+    const hasRef = hasRefProperty(spec, path);
+
+    expect(hasRef).toBe(false);
+  });
+
+  it('should return false when dot-separated-string does not lead to $ref property', () => {
+    const path = 'paths./resource.post.responses.200';
+    const hasRef = hasRefProperty(spec, path);
+
+    expect(hasRef).toBe(false);
+  });
+
+  it('should return false when path leads to somewhere non-existent in the spec', () => {
+    const path = ['paths', '/resource', 'get', 'responses', '200'];
+    const hasRef = hasRefProperty(spec, path);
+
+    expect(hasRef).toBe(false);
+  });
+
+  it('should return false when path leads through $ref - currently unsupported', () => {
+    const path = [
+      'paths',
+      '/resource',
+      'post',
+      'responses',
+      '400',
+      'content',
+      'application/json',
+      'schema'
+    ];
+    const hasRef = hasRefProperty(spec, path);
+
+    expect(hasRef).toBe(false);
+  });
+});

test/plugins/validation/2and3/operations-shared.js

@@ -852,5 +852,36 @@ describe('validation plugin - semantic - operations-shared', function() {
         'tag is not defined at the global level: not a tag'
       );
     });
+
+    it('should complain about a $ref in an operation', function() {
+      const jsSpec = {
+        paths: {
+          '/resource': {
+            post: {
+              $ref: 'external.yaml#/some-post'
+            }
+          }
+        }
+      };
+
+      const resolvedSpec = {
+        paths: {
+          '/resource': {
+            post: {
+              description: 'illegally referenced operation',
+              operationId: 'create_resource',
+              summary: 'simple operation'
+            }
+          }
+        }
+      };
+
+      const res = validate({ jsSpec, resolvedSpec, isOAS3: true }, config);
+      expect(res.errors.length).toEqual(1);
+      expect(res.warnings.length).toEqual(0);
+
+      expect(res.errors[0].path).toEqual('paths./resource.post.$ref');
+      expect(res.errors[0].message).toEqual('$ref found in illegal location');
+    });
   });
 });

test/plugins/validation/oas3/operations.js

@@ -223,4 +223,44 @@ describe('validation plugin - semantic - operations - oas3', function() {
     );
     expect(res.errors.length).toEqual(0);
   });
+
+  it('should not crash when request body is behind a ref', function() {
+    const jsSpec = {
+      paths: {
+        '/resource': {
+          $ref: 'external.yaml#/some-path'
+        }
+      }
+    };
+
+    const resolvedSpec = {
+      paths: {
+        '/resource': {
+          post: {
+            operationId: 'create_resource',
+            summary: 'simple operation',
+            requestBody: {
+              description: 'body',
+              content: {
+                'application/json': {
+                  schema: {
+                    type: 'string'
+                  }
+                }
+              }
+            },
+            responses: {
+              '200': {
+                description: 'success'
+              }
+            }
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec, resolvedSpec, isOAS3: true }, config);
+    expect(res.errors.length).toEqual(0);
+    expect(res.warnings.length).toEqual(0);
+  });
 });