attempt to hoist better errors

.eslintrc

@@ -1,7 +1,7 @@
 {
   "extends": "eslint:recommended",
   "parserOptions": {
-    "ecmaVersion": 8
+    "ecmaVersion": 2018
   },
   "env": {
     "node": true,

lib/utils/makeValidator.js

@@ -3,21 +3,124 @@
 const jsonschema = require('jsonschema');
 const links = require('./links');
 const functionalConstraints = require('../functional-constraints');
+const { flattenDeep, get } = require('lodash');
+
+const ambiguousTypes = ['anyOf', 'oneOf', 'allOf'];
 
 const makeLinks = (error, makerFunc) => {
-  if (typeof error.schema == 'string') {
+  if (typeof error.schema === 'string') {
     return [makerFunc(error.schema)];
   }
   if (
-    ['anyOf', 'oneOf', 'allOf'].indexOf(error.name) !== -1 &&
+    ambiguousTypes.includes(error.name) &&
     error.argument &&
     error.argument.length
   ) {
-    return error.argument.map(makerFunc);
+    // no way to know what the subschema was, so don't create links for it
+    return error.argument
+      .map(s => (s.includes('subschema') ? '' : makerFunc(s)))
+      .filter(Boolean);
   }
   return [];
 };
 
+const removeFirstAndLastChar = s => s.slice(1, -1);
+// always return a string
+const makePath = (path, newSegment) =>
+  (path ? [path, newSegment].join('.') : newSegment) || '';
+
+const processBaseError = (err, path) => {
+  const completePath = makePath(path, err.property)
+    .replace(/\.instance\.?/g, '.')
+    .replace(/\.instance$/, '');
+
+  const subSchemas = err.message.match(/\[subschema \d+\]/g);
+  if (subSchemas) {
+    subSchemas.forEach((subschema, idx) => {
+      // err.schema is either an anonymous schema object or the name of a named schema
+      if (typeof err.schema === 'string') {
+        // this is basically only for FieldChoicesSchema and I'm not sure why
+        err.message += ' Consult the docs below for valid subschemas.';
+      } else {
+        // the subschemas have a type property
+        err.message = err.message.replace(
+          subschema,
+          err.schema[err.name][idx].type || 'unknown'
+        );
+      }
+    });
+  }
+
+  err.property = completePath;
+  return err;
+};
+
+/**
+ * We have a lot of `anyOf` schemas that return ambiguous errors. This recurses down the schema until it finds the errors that cause the failures replaces the ambiguity.
+ * @param {ValidationError} validationError an individual error
+ * @param {string} path current path in the error chain
+ * @param {Validator} validator validator object to pass around that has all the schemas
+ * @param {object} definition the original schema we're defining
+ */
+const cleanError = (validationError, path, validator, definition) => {
+  if (ambiguousTypes.includes(validationError.name)) {
+    // flatObjectSchema requires each property to be a type. instead of recursing down, it's more valuable to say "hey, it's not of these types"
+    if (validationError.argument.every(s => s.includes('subschema'))) {
+      return processBaseError(validationError, path);
+    }
+
+    // Try against each of A, B, and C to take a guess as to which it's closed to
+    // errorGroups will be an array of arrays of errors
+    const errorGroups = validationError.argument.map((schemaName, idx) => {
+      // this is what we'll validate against next
+      let nextSchema;
+      // schemaName is either "[subschema n]" or "/NamedSchema"
+      if (schemaName.startsWith('[subschema')) {
+        const maybeNamedSchema = validator.schemas[validationError.schema];
+
+        if (maybeNamedSchema) {
+          nextSchema = maybeNamedSchema[validationError.name][idx];
+        } else {
+          // hoist the anonymous subschema up
+          nextSchema = validationError.schema[validationError.name][idx];
+        }
+      } else {
+        nextSchema = validator.schemas[removeFirstAndLastChar(schemaName)];
+      }
+
+      if (validationError.instance === undefined) {
+        // Work around a jsonschema bug: When the value being validated is
+        // falsy, validationError.instance isn't available
+        // See https://github.com/tdegrunt/jsonschema/issues/263
+        const fullPath =
+          path.replace(/^instance\./, '') +
+          validationError.property.replace(/^instance\./, '');
+        validationError.instance = get(definition, fullPath);
+      }
+
+      const res = validator.validate(validationError.instance, nextSchema);
+
+      return res.errors.map(e =>
+        cleanError(
+          e,
+          makePath(path, validationError.property),
+          validator,
+          definition
+        )
+      );
+    });
+
+    // find the group with the fewest errors, that's probably the most accurate
+    // if we're goign to tweak what gets returned, this is where we'll do it
+    // a possible improvement could be treating a longer path favorably, like the python implementation does
+    errorGroups.sort((a, b) => a.length - b.length);
+    return errorGroups[0];
+  } else {
+    // base case
+    return processBaseError(validationError, path);
+  }
+};
+
 const makeValidator = (mainSchema, subSchemas) => {
   const schemas = [mainSchema].concat(subSchemas || []);
   const v = new jsonschema.Validator();
@@ -26,11 +129,15 @@ const makeValidator = (mainSchema, subSchemas) => {
   });
   return {
     validate: definition => {
-      const results = v.validate(definition, mainSchema);
-      results.errors = results.errors.concat(
+      const { errors, ...results } = v.validate(definition, mainSchema);
+      const allErrors = errors.concat(
         functionalConstraints.run(definition, mainSchema)
       );
-      results.errors = results.errors.map(error => {
+      const cleanedErrors = flattenDeep(
+        allErrors.map(e => cleanError(e, '', v, definition))
+      );
+
+      results.errors = cleanedErrors.map(error => {
         error.codeLinks = makeLinks(error, links.makeCodeLink);
         error.docLinks = makeLinks(error, links.makeDocLink);
         return error;

package.json

@@ -17,6 +17,7 @@
     "version": "npm run build && npm run add",
     "postversion": "git push && git push --tags",
     "test": "mocha -t 5000 --recursive test",
+    "test:debug": "mocha -t 5000 --recursive --inspect-brk test",
     "posttest": "eslint lib",
     "smoke-test": "mocha -t 5000 --recursive smoke-test",
     "coverage": "istanbul cover _mocha -- --recursive",

test/readability.js

@@ -1,9 +1,10 @@
 'use strict';
 
-require('should');
+const should = require('should');
 
 const AuthenticationSchema = require('../lib/schemas/AuthenticationSchema');
 const CreateSchema = require('../lib/schemas/CreateSchema');
+const TriggerSchema = require('../lib/schemas/TriggerSchema');
 
 describe('readability', () => {
   it('should have decent messages for anyOf mismatches', () => {
@@ -12,13 +13,12 @@ describe('readability', () => {
       test: 'whateverfake!'
     });
     results.errors.should.have.length(1);
-    results.errors[0].stack.should.eql(
-      'instance.test is not exactly one from </RequestSchema>,</FunctionSchema>'
-    );
+    results.errors[0].stack.should.eql('instance is not of a type(s) object');
+    should(results.errors[0].property.endsWith('instance')).be.false();
   });
 
   it('should have decent messages for minimum length not met', () => {
-    const results = CreateSchema.validate({
+    const results = TriggerSchema.validate({
       key: 'recipe',
       noun: 'Recipe',
       display: {
@@ -31,13 +31,14 @@ describe('readability', () => {
       }
     });
     results.errors.should.have.length(1);
+    should(results.errors[0].property.endsWith('instance')).be.false();
     results.errors[0].stack.should.eql(
       'instance.display.label does not meet minimum length of 2'
     );
   });
 
   it('should have decent messages for value type mismatch', () => {
-    let results = CreateSchema.validate({
+    const results = CreateSchema.validate({
       key: 'recipe',
       noun: 'Recipe',
       display: {
@@ -47,15 +48,35 @@ describe('readability', () => {
       operation: {
         perform: '$func$2$f$',
         sample: { id: 1 },
-        inputFields: [false]
+        inputFields: [123]
       }
     });
     results.errors.should.have.length(1);
-    results.errors[0].stack.should.eql(
-      'instance.operation.inputFields[0] is not exactly one from </FieldSchema>,</FunctionSchema>'
-    );
+    should(results.errors[0].property.endsWith('instance')).be.false();
+    results.errors[0].stack.should.eql('instance is not of a type(s) object');
+  });
 
-    results = CreateSchema.validate({
+  it('should handle falsy values for objects', () => {
+    const results = CreateSchema.validate({
+      key: 'recipe',
+      noun: 'Recipe',
+      display: {
+        label: 'Create Recipe',
+        description: 'Creates a new recipe.'
+      },
+      operation: {
+        perform: '$func$2$f$',
+        sample: { id: 1 },
+        inputFields: [0]
+      }
+    });
+    results.errors.should.have.length(1);
+    should(results.errors[0].property.endsWith('instance')).be.false();
+    results.errors[0].stack.should.eql('instance is not of a type(s) object');
+  });
+
+  it('should surface deep issues', () => {
+    const results = CreateSchema.validate({
       key: 'recipe',
       noun: 'Recipe',
       display: {
@@ -69,10 +90,65 @@ describe('readability', () => {
       }
     });
     results.errors.should.have.length(1);
-    // Ideally it would be the commented version, but it would require significant changes in jsonschema
-    // results.errors[0].stack.should.eql('instance.operation.inputFields[0].default does not meet minimum length of 1');
-    results.errors[0].stack.should.eql(
-      'instance.operation.inputFields[0] is not exactly one from </FieldSchema>,</FunctionSchema>'
+    should(results.errors[0].property.endsWith('instance')).be.false();
+    results.errors[0].property.should.eql(
+      'instance.operation.inputFields[0].default'
+    );
+    results.errors[0].message.should.eql('does not meet minimum length of 1');
+  });
+
+  it('should correctly surface subschema types', () => {
+    const results = CreateSchema.validate({
+      key: 'recipe',
+      noun: 'Recipe',
+      display: {
+        label: 'Create Recipe',
+        description: 'Creates a new recipe.'
+      },
+      operation: {
+        perform: {
+          url: 'https://example.com',
+          body: 123
+        },
+        sample: { id: 1 }
+      }
+    });
+    results.errors.should.have.length(1);
+    results.errors[0].property.should.eql('instance.operation.perform.body');
+    should(
+      results.errors[0].message.includes('null,string,object,array')
+    ).be.true();
+    should(results.errors[0].property.endsWith('instance')).be.false();
+    results.errors[0].docLinks.length.should.eql(0);
+  });
+
+  it('should be helpful for fieldChoices', () => {
+    const results = CreateSchema.validate({
+      key: 'recipe',
+      noun: 'Recipe',
+      display: {
+        label: 'Create Recipe',
+        description: 'Creates a new recipe.'
+      },
+      operation: {
+        perform: '$func$2$f$',
+        sample: { id: 1 },
+        inputFields: [
+          {
+            key: 'adsf',
+            // schema says these should be strings
+            choices: [1, 2, 3]
+          }
+        ]
+      }
+    });
+    results.errors.should.have.length(1);
+    results.errors[0].property.should.eql(
+      'instance.operation.inputFields[0].choices'
     );
+    should(
+      results.errors[0].docLinks[0].endsWith('schema.md#fieldchoicesschema')
+    ).be.true();
+    should(results.errors[0].property.endsWith('instance')).be.false();
   });
 });