From 600bf8e301937f72868845fc6baeaff97ae999a8 Mon Sep 17 00:00:00 2001
From: barrett-schonefeld
 <59850348+barrett-schonefeld@users.noreply.github.com>
Date: Tue, 25 Feb 2020 11:27:28 -0600
Subject: [PATCH] feat: list valid types and formats in error messages (#145)

Changes the way problems with type/format values are handled so that messages are more descriptive about what is actually wrong
---
 .../semantic-validators/parameters-ibm.js     |  57 ----
 .../2and3/semantic-validators/schema-ibm.js   | 162 +++++++++--
 .../validation/2and3/parameters-ibm.js        |  70 -----
 test/plugins/validation/2and3/schema-ibm.js   | 265 +++++++++++++++++-
 4 files changed, 385 insertions(+), 169 deletions(-)

diff --git a/src/plugins/validation/2and3/semantic-validators/parameters-ibm.js b/src/plugins/validation/2and3/semantic-validators/parameters-ibm.js
index 3b3cbbb60..d34efd1ca 100644
--- a/src/plugins/validation/2and3/semantic-validators/parameters-ibm.js
+++ b/src/plugins/validation/2and3/semantic-validators/parameters-ibm.js
@@ -8,8 +8,6 @@
 // Header parameters must not define a content-type or an accept-type.
 // http://watson-developer-cloud.github.io/api-guidelines/swagger-coding-style#do-not-explicitly-define-a-content-type-header-parameter
 
-const pick = require('lodash/pick');
-const includes = require('lodash/includes');
 const { checkCase, isParameterObject, walk } = require('../../../utils');
 const MessageCarrier = require('../../../utils/messageCarrier');
 
@@ -107,18 +105,6 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) {
         }
       }
 
-      const checkStatus = config.invalid_type_format_pair;
-      if (checkStatus !== 'off') {
-        const valid = formatValid(obj, isOAS3);
-        if (!valid) {
-          messages.addMessage(
-            path,
-            'Parameter type+format is not well-defined.',
-            checkStatus
-          );
-        }
-      }
-
       const isParameterRequired = obj.required;
       let isDefaultDefined;
       if (isOAS3) {
@@ -139,46 +125,3 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) {
 
   return messages;
 };
-
-function formatValid(obj, isOAS3) {
-  // References will be checked when the parameters / definitions / components are scanned.
-  if (obj.$ref || (obj.schema && obj.schema.$ref)) {
-    return true;
-  }
-  const schema = obj.schema || pick(obj, ['type', 'format', 'items']);
-  if (!schema.type) {
-    return false;
-  }
-  switch (schema.type) {
-    case 'integer':
-      return (
-        !schema.format ||
-        includes(['int32', 'int64'], schema.format.toLowerCase())
-      );
-    case 'number':
-      return (
-        !schema.format ||
-        includes(['float', 'double'], schema.format.toLowerCase())
-      );
-    case 'string':
-      return (
-        !schema.format ||
-        includes(
-          ['byte', 'date', 'date-time', 'password'],
-          schema.format.toLowerCase()
-        )
-      );
-    case 'boolean':
-      return schema.format === undefined; // No valid formats for boolean -- should be omitted
-    case 'array':
-      if (!schema.items) {
-        return false;
-      }
-      return formatValid(schema.items);
-    case 'object':
-      return true; // TODO: validate nested schemas
-    case 'file':
-      return !isOAS3 && obj.in === 'formData';
-  }
-  return false;
-}
diff --git a/src/plugins/validation/2and3/semantic-validators/schema-ibm.js b/src/plugins/validation/2and3/semantic-validators/schema-ibm.js
index b0eda1e81..9ed80ddad 100644
--- a/src/plugins/validation/2and3/semantic-validators/schema-ibm.js
+++ b/src/plugins/validation/2and3/semantic-validators/schema-ibm.js
@@ -50,8 +50,12 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) {
             This includes responses, request bodies, parameters (with content rather than schema),
             both at the operation level and within the top-level "components" object
     */
-
-    const modelLocations = ['definitions', 'schemas', 'properties'];
+    const modelLocations = [
+      'definitions',
+      'schemas',
+      'properties',
+      'parameters'
+    ];
     if (
       current === 'schema' ||
       current === 'items' ||
@@ -158,60 +162,156 @@ function generateFormatErrors(schema, contextPath, config, isOAS3, messages) {
   }
 
   checkStatus = config.invalid_type_format_pair;
-  if (checkStatus !== 'off' && !formatValid(schema, contextPath, isOAS3)) {
+  if (checkStatus !== 'off') {
+    typeFormatErrors(schema, contextPath, isOAS3, messages, checkStatus);
+  }
+}
+
+function typeFormatErrors(obj, path, isOAS3, messages, checkStatus) {
+  // error if format defined but not type
+  if (!obj.type && obj.format) {
     messages.addMessage(
-      contextPath.concat(['type']),
-      'Property type+format is not well-defined.',
+      path.concat(['format']),
+      'Format defined without a type.',
       checkStatus
     );
   }
-}
 
-function formatValid(property, path, isOAS3) {
-  if (property.$ref) {
-    return true;
+  // we will check ref in defintions section
+  // only proceed if type defined
+  if (obj.$ref || !obj.type) {
+    return;
   }
-  // if type is not present, skip validation of format
-  if (!property.type) {
-    return true;
+
+  const validIntegerFormats = ['int32', 'int64'];
+  const validNumberFormats = ['float', 'double'];
+  const validStringFormats = [
+    'byte',
+    'binary',
+    'date',
+    'date-time',
+    'password'
+  ];
+  const validTypes = [
+    'integer',
+    'number',
+    'string',
+    'boolean',
+    'object',
+    'array'
+  ];
+  if (!isOAS3) {
+    validTypes.push('file');
   }
-  let valid = true;
-  switch (property.type) {
+  switch (obj.type) {
     case 'integer':
-      valid =
-        !property.format ||
-        includes(['int32', 'int64'], property.format.toLowerCase());
+      if (
+        obj.format &&
+        !includes(validIntegerFormats, obj.format.toLowerCase())
+      ) {
+        messages.addMessage(
+          path.concat(['type']),
+          `Schema of type integer should use one of the following formats: ${validIntegerFormats.join(
+            ', '
+          )}.`,
+          checkStatus
+        );
+      }
       break;
     case 'number':
-      valid =
-        !property.format ||
-        includes(['float', 'double'], property.format.toLowerCase());
+      if (
+        obj.format &&
+        !includes(validNumberFormats, obj.format.toLowerCase())
+      ) {
+        messages.addMessage(
+          path.concat(['type']),
+          `Schema of type number should use one of the following formats: ${validNumberFormats.join(
+            ', '
+          )}.`,
+          checkStatus
+        );
+      }
       break;
     case 'string':
-      valid =
-        !property.format ||
-        includes(
-          ['byte', 'binary', 'date', 'date-time', 'password'],
-          property.format.toLowerCase()
+      if (
+        obj.format &&
+        !includes(validStringFormats, obj.format.toLowerCase())
+      ) {
+        messages.addMessage(
+          path.concat(['type']),
+          `Schema of type string should use one of the following formats: ${validStringFormats.join(
+            ', '
+          )}.`,
+          checkStatus
         );
+      }
       break;
     case 'boolean':
-      valid = property.format === undefined; // No valid formats for boolean -- should be omitted
+      // No valid formats for boolean, format should be undefined
+      if (obj.format !== undefined) {
+        messages.addMessage(
+          path.concat(['type']),
+          `Schema of type boolean should not have a format.`,
+          checkStatus
+        );
+      }
       break;
     case 'object':
+      // No valid format pairings for object, format should be undefined
+      if (obj.format !== undefined) {
+        messages.addMessage(
+          path.concat(['type']),
+          'Schema of type object should not have a format.',
+          checkStatus
+        );
+      }
+      break;
     case 'array':
-      valid = true;
+      // No valid format pairings for array, format should be undefined
+      if (obj.format !== undefined) {
+        messages.addMessage(
+          path.concat(['type']),
+          'Schema of type array should not have a format.',
+          checkStatus
+        );
+      }
       break;
     case 'file':
       // schemas of type file are allowed in swagger2 for responses and parameters
-      // of type 'formData' - the violating parameters are caught by parameters-ibm
+      // of type 'formData'
       // note: type file is only allowed for root schemas (not properties, etc.)
-      valid = !isOAS3 && isRootSchema(path);
+      if (isOAS3 || (!obj.in && !isRootSchema(path))) {
+        messages.addMessage(
+          path.concat(['type']),
+          'File type only valid for swagger2 and must be used as root schema.',
+          checkStatus
+        );
+      } else if (obj.in && obj.in !== 'formData') {
+        messages.addMessage(
+          path.concat(['type']),
+          'File type parameter must use in: formData.',
+          checkStatus
+        );
+      }
+      // Format should not be defined for schema of type file.
+      // Error reported in addition to potential error above.
+      // Only check for swagger2 because type file should not be used in OAS3.
+      if (!isOAS3 && obj.format !== undefined) {
+        messages.addMessage(
+          path.concat(['type']),
+          'Schema of type file should not have a format.',
+          checkStatus
+        );
+      }
       break;
     default:
-      valid = false;
+      // invalid type
+      messages.addMessage(
+        path.concat(['type']),
+        `Invalid type. Valid types are: ${validTypes.join(', ')}.`,
+        checkStatus
+      );
   }
-  return valid;
 }
 
 // http://watson-developer-cloud.github.io/api-guidelines/swagger-coding-style#models
diff --git a/test/plugins/validation/2and3/parameters-ibm.js b/test/plugins/validation/2and3/parameters-ibm.js
index 3ea6f0fbb..890483dba 100644
--- a/test/plugins/validation/2and3/parameters-ibm.js
+++ b/test/plugins/validation/2and3/parameters-ibm.js
@@ -119,40 +119,6 @@ describe('validation plugin - semantic - parameters-ibm', () => {
       expect(res.warnings.length).toEqual(0);
     });
 
-    it('should return an error when type does not match format', () => {
-      const spec = {
-        paths: {
-          '/pets': {
-            get: {
-              parameters: [
-                {
-                  name: 'good_name',
-                  in: 'query',
-                  description: 'This is a good description.',
-                  type: 'number',
-                  format: 'int32'
-                }
-              ]
-            }
-          }
-        }
-      };
-
-      const res = validate({ jsSpec: spec }, config);
-      expect(res.errors.length).toEqual(1);
-      expect(res.errors[0].path).toEqual([
-        'paths',
-        '/pets',
-        'get',
-        'parameters',
-        '0'
-      ]);
-      expect(res.errors[0].message).toEqual(
-        'Parameter type+format is not well-defined.'
-      );
-      expect(res.warnings.length).toEqual(0);
-    });
-
     it('should not validate within extensions', () => {
       const spec = {
         paths: {
@@ -486,42 +452,6 @@ describe('validation plugin - semantic - parameters-ibm', () => {
       );
     });
 
-    it('should return an error when parameter type+format is not well-defined', () => {
-      const spec = {
-        paths: {
-          '/pets': {
-            get: {
-              parameters: [
-                {
-                  name: 'good_name',
-                  in: 'query',
-                  description: 'This is a good description.',
-                  schema: {
-                    type: 'number',
-                    format: 'int32'
-                  }
-                }
-              ]
-            }
-          }
-        }
-      };
-
-      const res = validate({ jsSpec: spec, isOAS3: true }, config);
-      expect(res.errors.length).toEqual(1);
-      expect(res.errors[0].path).toEqual([
-        'paths',
-        '/pets',
-        'get',
-        'parameters',
-        '0'
-      ]);
-      expect(res.errors[0].message).toEqual(
-        'Parameter type+format is not well-defined.'
-      );
-      expect(res.warnings.length).toEqual(0);
-    });
-
     it('should flag a required parameter that specifies a default value', () => {
       const spec = {
         paths: {
diff --git a/test/plugins/validation/2and3/schema-ibm.js b/test/plugins/validation/2and3/schema-ibm.js
index cd9364c35..4144309d0 100644
--- a/test/plugins/validation/2and3/schema-ibm.js
+++ b/test/plugins/validation/2and3/schema-ibm.js
@@ -34,7 +34,7 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type number should use one of the following formats: float, double.'
     );
     expect(res.warnings.length).toEqual(0);
   });
@@ -54,7 +54,7 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
     expect(res.errors.length).toEqual(1);
     expect(res.errors[0].path).toEqual(['definitions', 'WordStyle', 'type']);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type number should use one of the following formats: float, double.'
     );
     expect(res.warnings.length).toEqual(0);
   });
@@ -90,7 +90,7 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type number should use one of the following formats: float, double.'
     );
     expect(res.warnings.length).toEqual(0);
   });
@@ -151,7 +151,7 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type number should use one of the following formats: float, double.'
     );
     expect(res.warnings.length).toEqual(0);
   });
@@ -179,6 +179,82 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
     expect(res.warnings.length).toEqual(0);
   });
 
+  it('should record an error when a file parameter does not use in: formData', () => {
+    const spec = {
+      paths: {
+        '/some/path/{id}': {
+          get: {
+            parameters: [
+              {
+                name: 'file_param',
+                in: 'query',
+                description: 'a file param',
+                type: 'file'
+              }
+            ]
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: false }, config);
+    expect(res.errors.length).toEqual(1);
+    expect(res.errors[0].message).toEqual(
+      'File type parameter must use in: formData.'
+    );
+  });
+
+  it('should record an error when a file parameter does not use in: formData AND if format defined', () => {
+    const spec = {
+      paths: {
+        '/some/path/{id}': {
+          get: {
+            parameters: [
+              {
+                name: 'file_param',
+                in: 'query',
+                description: 'a file param',
+                type: 'file',
+                format: 'file_should_not_have_a_format'
+              }
+            ]
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: false }, config);
+    expect(res.errors.length).toEqual(2);
+    expect(res.errors[0].message).toEqual(
+      'File type parameter must use in: formData.'
+    );
+    expect(res.errors[1].message).toEqual(
+      'Schema of type file should not have a format.'
+    );
+  });
+
+  it('should not record an error when a file parameter uses in: formData', () => {
+    const spec = {
+      paths: {
+        '/some/path/{id}': {
+          get: {
+            parameters: [
+              {
+                name: 'file_param',
+                in: 'formData',
+                description: 'a file param',
+                type: 'file'
+              }
+            ]
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: false }, config);
+    expect(res.errors.length).toEqual(0);
+  });
+
   it('should non return an error for a definition with root type of file', () => {
     const spec = {
       definitions: {
@@ -194,6 +270,22 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
     expect(res.warnings.length).toEqual(0);
   });
 
+  it('should report file as a valid type for swagger2 when an invalid type is provided', () => {
+    const spec = {
+      definitions: {
+        SomeSchema: {
+          type: 'invalid_type'
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec }, config);
+    expect(res.errors.length).toEqual(1);
+    expect(res.errors[0].message).toEqual(
+      'Invalid type. Valid types are: integer, number, string, boolean, object, array, file.'
+    );
+  });
+
   it('should return an error for a response schema with non-root type file', () => {
     const spec = {
       paths: {
@@ -231,7 +323,7 @@ describe('validation plugin - semantic - schema-ibm - Swagger 2', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'File type only valid for swagger2 and must be used as root schema.'
     );
 
     expect(res.warnings.length).toEqual(0);
@@ -761,11 +853,163 @@ describe('validation plugin - semantic - schema-ibm - OpenAPI 3', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type integer should use one of the following formats: int32, int64.'
     );
     expect(res.warnings.length).toEqual(0);
   });
 
+  it('should record an error when items use an invalid type+format pair', () => {
+    const spec = {
+      components: {
+        schemas: {
+          Thing: {
+            type: 'object',
+            description: 'thing',
+            properties: {
+              thing: {
+                type: 'array',
+                description: 'thing array',
+                items: {
+                  description: 'invalid items',
+                  type: 'object',
+                  format: 'objects_should_not_have_formats'
+                }
+              }
+            }
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: true }, config);
+    expect(res.errors.length).toEqual(1);
+    expect(res.errors[0].path).toEqual([
+      'components',
+      'schemas',
+      'Thing',
+      'properties',
+      'thing',
+      'items',
+      'type'
+    ]);
+    expect(res.errors[0].message).toEqual(
+      'Schema of type object should not have a format.'
+    );
+  });
+
+  it('should not record an error when items and object properties use valid type+format pairs', () => {
+    const spec = {
+      components: {
+        schemas: {
+          Thing1: {
+            type: 'object',
+            description: 'thing',
+            properties: {
+              prop1: {
+                description: 'an object property',
+                type: 'string',
+                format: 'byte'
+              }
+            }
+          },
+          Thing2: {
+            type: 'array',
+            description: 'an array',
+            items: {
+              type: 'number',
+              format: 'float'
+            }
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: true }, config);
+    expect(res.errors.length).toEqual(0);
+  });
+
+  it('should record an error when objects use an invalid type+format pair', () => {
+    const spec = {
+      components: {
+        schemas: {
+          Thing: {
+            type: 'object',
+            description: 'thing',
+            properties: {
+              thing: {
+                type: 'object',
+                description: 'thing object',
+                properties: {
+                  prop1: {
+                    description: 'a property',
+                    type: 'array',
+                    format: 'arrays_should_not_have_formats',
+                    items: {
+                      type: 'invalid_type'
+                    }
+                  }
+                }
+              }
+            }
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: true }, config);
+    expect(res.errors.length).toEqual(2);
+    expect(res.errors[0].path).toEqual([
+      'components',
+      'schemas',
+      'Thing',
+      'properties',
+      'thing',
+      'properties',
+      'prop1',
+      'type'
+    ]);
+    expect(res.errors[0].message).toEqual(
+      'Schema of type array should not have a format.'
+    );
+    expect(res.errors[1].path).toEqual([
+      'components',
+      'schemas',
+      'Thing',
+      'properties',
+      'thing',
+      'properties',
+      'prop1',
+      'items',
+      'type'
+    ]);
+    expect(res.errors[1].message).toEqual(
+      'Invalid type. Valid types are: integer, number, string, boolean, object, array.'
+    );
+  });
+
+  it('should record an error when schema has a format but no type', () => {
+    const spec = {
+      components: {
+        schemas: {
+          Thing: {
+            type: 'object',
+            description: 'thing',
+            properties: {
+              thing: {
+                description: 'should not have format without type',
+                format: 'byte'
+              }
+            }
+          }
+        }
+      }
+    };
+
+    const res = validate({ jsSpec: spec, isOAS3: true }, config);
+    expect(res.errors.length).toEqual(1);
+    expect(res.errors[0].message).toEqual('Format defined without a type.');
+  });
+
   it('should return an error for a response schema of type file', () => {
     const spec = {
       paths: {
@@ -801,7 +1045,7 @@ describe('validation plugin - semantic - schema-ibm - OpenAPI 3', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'File type only valid for swagger2 and must be used as root schema.'
     );
     expect(res.warnings.length).toEqual(0);
   });
@@ -861,7 +1105,7 @@ describe('validation plugin - semantic - schema-ibm - OpenAPI 3', () => {
       'type'
     ]);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type boolean should not have a format.'
     );
     expect(res.warnings.length).toEqual(0);
   });
@@ -874,12 +1118,11 @@ describe('validation plugin - semantic - schema-ibm - OpenAPI 3', () => {
     const res = validate({ jsSpec: spec, isOAS3: true }, config);
     expect(res.errors.length).toEqual(2);
     expect(res.errors[0].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Invalid type. Valid types are: integer, number, string, boolean, object, array.'
     );
     expect(res.errors[1].message).toEqual(
-      'Property type+format is not well-defined.'
+      'Schema of type string should use one of the following formats: byte, binary, date, date-time, password.'
     );
-    expect(res.warnings.length).toEqual(0);
   });
 
   it('should report an error when allOf, anyOf, or oneOf is not an array', () => {