From 9b38b8273c9dd67125fad4fbc6739bf8ce065c51 Mon Sep 17 00:00:00 2001 From: Barrett Schonefeld Date: Wed, 7 Apr 2021 11:29:37 -0500 Subject: [PATCH] feat: show path to component that caused error in verbose mode Purpose: - Errors may be scattered throughout a resolved schema, so we want to make it easy for users to identify the source component from which the error originates. Changes: - Walk the unresolved spec to see if the error occurs in a ref. Identify the source location and include the path to the component if the validator is in verbose mode. - Update the `printRuleNames` variable to be `verbose` to be more general. Tests: - Add tests to ensure the path to the components added correctly. --- src/cli-validator/runValidator.js | 11 +- .../utils/addPathsToComponents.js | 100 +++++ src/cli-validator/utils/getPathAsArray.js | 3 + src/cli-validator/utils/jsonResults.js | 22 +- src/cli-validator/utils/printResults.js | 16 +- .../tests/expected-output.test.js | 41 +- .../tests/utils/addPathsToComponents.test.js | 369 ++++++++++++++++++ 7 files changed, 550 insertions(+), 12 deletions(-) create mode 100644 src/cli-validator/utils/addPathsToComponents.js create mode 100644 src/cli-validator/utils/getPathAsArray.js create mode 100644 test/cli-validator/tests/utils/addPathsToComponents.test.js diff --git a/src/cli-validator/runValidator.js b/src/cli-validator/runValidator.js index e10dd6ea7..3c61ded15 100644 --- a/src/cli-validator/runValidator.js +++ b/src/cli-validator/runValidator.js @@ -18,6 +18,7 @@ const printError = require('./utils/printError'); const preprocessFile = require('./utils/preprocessFile'); const spectralValidator = require('../spectral/utils/spectral-validator'); const dedupFunction = require('../cli-validator/utils/noDeduplication'); +const addPathsToComponents = require('./utils/addPathsToComponents'); const { Spectral } = require('@stoplight/spectral'); // import the init module for creating a .validaterc file const init = require('./utils/init.js'); @@ -48,7 +49,7 @@ const processInput = async function(program) { const limitsFileOverride = program.limits; - const printRuleNames = program.verbose > 0; + const verbose = program.verbose > 0; // turn off coloring if explicitly requested if (turnOffColoring) { @@ -314,15 +315,19 @@ const processInput = async function(program) { } } + if (verbose) { + addPathsToComponents(results, swagger.jsSpec); + } + if (jsonOutput) { - printJson(results, originalFile, errorsOnly); + printJson(results, originalFile, verbose, errorsOnly); } else { if (results.error || results.warning || results.info || results.hint) { print( results, chalk, printValidators, - printRuleNames, + verbose, reportingStats, originalFile, errorsOnly diff --git a/src/cli-validator/utils/addPathsToComponents.js b/src/cli-validator/utils/addPathsToComponents.js new file mode 100644 index 000000000..84e125c7e --- /dev/null +++ b/src/cli-validator/utils/addPathsToComponents.js @@ -0,0 +1,100 @@ +const { each } = require('lodash'); +const getPathAsArray = require('./getPathAsArray'); + +// takes validator results dictionary in the format: +// { +// errors: { +// validator-name: [ +// { +// path: [path, to, error], +// message: 'error message', +// rule: rule_name +// }, +// ], +// }, +// warnings: ... +// } +// +// adds path to the originating component if it exists +// modifies the given results object +module.exports = function(originalResults, unresolvedSpec) { + each(originalResults, function(validatorsDict) { + each(validatorsDict, function(errors) { + pointToComponents(errors, unresolvedSpec); + }); + }); +}; + +// takes an array of validator errors: +// [ +// { +// path: [path, to, error], +// message: 'error message', +// rule: rule_name +// }, +// ] +// +// adds componentPath field to existing errors +// modifies existing errors in place +function pointToComponents(errors, unresolvedSpec) { + each(errors, function(err) { + const pathArray = getPathAsArray(err.path); + let componentPath = null; + let refObj = findRef(pathArray, unresolvedSpec); + while (refObj.refString !== null) { + componentPath = [ + ...parseRefString(refObj.refString), + ...refObj.remainingPath + ]; + refObj = findRef(componentPath, unresolvedSpec); + } + // only add the componentPath if the componentPath exists + // and it is the result of a valid path. + // protects against invalid $refs from the user and against + // bugs in the path to the error. + if (componentPath && refObj.validPath) { + err.componentPath = componentPath; + } + }); +} + +function findRef(pathArray, unresolvedSpec) { + let ref = null; + let indexOfRef = 0; + let currentObj = unresolvedSpec; + let validPath = true; + for (let i = 0; i < pathArray.length && currentObj; ++i) { + let nextKey = pathArray[i]; + if (Array.isArray(currentObj)) { + // convert the key to an index + nextKey = Number(nextKey); + if (!(nextKey >= 0 && nextKey < currentObj.length)) { + validPath = false; + break; + } + } else if (!(nextKey in currentObj)) { + if ('$ref' in currentObj) { + ref = currentObj['$ref']; + indexOfRef = i; + } else if (i != pathArray.length - 1) { + // nextKey is not in the object + // no $ref in the object + // nextKey is not the last element of the path + // hence, the given path is invalid + validPath = false; + } + break; + } + currentObj = currentObj[nextKey]; + } + return { + refString: ref, + remainingPath: pathArray.slice(indexOfRef, pathArray.length), + validPath: validPath + }; +} + +function parseRefString(refString) { + const refArray = refString.split('/'); + return refArray.slice(refArray.indexOf('#') + 1, refArray.length); +} diff --git a/src/cli-validator/utils/getPathAsArray.js b/src/cli-validator/utils/getPathAsArray.js new file mode 100644 index 000000000..628c2509c --- /dev/null +++ b/src/cli-validator/utils/getPathAsArray.js @@ -0,0 +1,3 @@ +module.exports = function(path) { + return Array.isArray(path) ? path : path.split('.'); +}; diff --git a/src/cli-validator/utils/jsonResults.js b/src/cli-validator/utils/jsonResults.js index e68aad111..3aa6a51d5 100644 --- a/src/cli-validator/utils/jsonResults.js +++ b/src/cli-validator/utils/jsonResults.js @@ -1,4 +1,5 @@ const each = require('lodash/each'); +const getPathAsArray = require('./getPathAsArray'); // get line-number-producing, 'magic' code from Swagger Editor const getLineNumberForPath = require(__dirname + '/../../plugins/ast/ast') @@ -6,15 +7,21 @@ const getLineNumberForPath = require(__dirname + '/../../plugins/ast/ast') const validatorVersion = require('../../../package.json').version; // function to print the results as json to the console. -function printJson(results, originalFile = null, errorsOnly = false) { +function printJson( + results, + originalFile = null, + verbose = false, + errorsOnly = false +) { // render the results to json in the console with 2 char spacing - results = formatResultsAsObject(results, originalFile, errorsOnly); + results = formatResultsAsObject(results, originalFile, verbose, errorsOnly); console.log(JSON.stringify(results, null, 2)); } function formatResultsAsObject( results, originalFile = null, + verbose = false, errorsOnly = false ) { // initialize the results with the validator version @@ -28,9 +35,7 @@ function formatResultsAsObject( let path = problem.path; // path needs to be an array to get the line number - if (!Array.isArray(path)) { - path = path.split('.'); - } + path = getPathAsArray(path); if (originalFile) { // get line number from the path of strings to the problem @@ -42,6 +47,13 @@ function formatResultsAsObject( // add the line number to the result JSON problem.line = line; + + if (verbose && problem.componentPath) { + problem.componentLine = getLineNumberForPath( + originalFile, + getPathAsArray(problem.componentPath) + ); + } } // initialize object for this type of error (e.g. error, warning, info, hint) if (!formattedResults[type]) { diff --git a/src/cli-validator/utils/printResults.js b/src/cli-validator/utils/printResults.js index dae4f896b..89d5baee9 100644 --- a/src/cli-validator/utils/printResults.js +++ b/src/cli-validator/utils/printResults.js @@ -1,5 +1,6 @@ const each = require('lodash/each'); const pad = require('pad'); +const getPathAsArray = require('./getPathAsArray'); // get line-number-producing, 'magic' code from Swagger Editor const getLineNumberForPath = require(__dirname + '/../../plugins/ast/ast') @@ -10,7 +11,7 @@ module.exports = function print( results, chalk, printValidators, - printRuleNames, + verbose, reportingStats, originalFile, errorsOnly @@ -88,11 +89,22 @@ module.exports = function print( // print the path array as a dot-separated string console.log(chalk[color](` Message : ${problem.message}`)); - if (printRuleNames && problem.rule) { + if (verbose && problem.rule) { console.log(chalk[color](` Rule : ${problem.rule}`)); } console.log(chalk[color](` Path : ${path.join('.')}`)); console.log(chalk[color](` Line : ${lineNumber}`)); + if (verbose && problem.componentPath) { + const componentPath = getPathAsArray(problem.componentPath); + const componentLine = getLineNumberForPath( + originalFile, + componentPath + ); + console.log( + chalk[color](` Component Path : ${componentPath.join('.')}`) + ); + console.log(chalk[color](` Component Line : ${componentLine}`)); + } console.log(); }); }); diff --git a/test/cli-validator/tests/expected-output.test.js b/test/cli-validator/tests/expected-output.test.js index 417caba45..a06b01bf5 100644 --- a/test/cli-validator/tests/expected-output.test.js +++ b/test/cli-validator/tests/expected-output.test.js @@ -332,8 +332,8 @@ describe('test expected output - OpenAPI 3', function() { program.default_mode = true; program.json = true; - await commandLineValidator(program); - // exit code is not set for JSON output + const exitCode = await commandLineValidator(program); + expect(exitCode).toEqual(1); const capturedText = getCapturedText(consoleSpy.mock.calls); const jsonOutput = JSON.parse(capturedText); @@ -372,4 +372,41 @@ describe('test expected output - OpenAPI 3', function() { expect(msg).toHaveProperty('rule') ); }); + + it('should include the path to the component (if it exists) when in verbose mode', async function() { + const program = {}; + program.args = ['./test/cli-validator/mockFiles/oas3/testoneof.yaml']; + program.default_mode = true; + program.verbose = 1; + + const exitCode = await commandLineValidator(program); + expect(exitCode).toEqual(1); + + const capturedText = getCapturedText(consoleSpy.mock.calls); + const allText = capturedText.join(); + expect(allText).toContain('Component Path'); + expect(allText).toContain('Component Line'); + }); + + it('should include the path to the component (if it exists) when in verbose mode and json mode', async function() { + const program = {}; + program.args = ['./test/cli-validator/mockFiles/oas3/testoneof.yaml']; + program.default_mode = true; + program.verbose = 1; + program.json = true; + + const exitCode = await commandLineValidator(program); + expect(exitCode).toEqual(1); + + const capturedText = getCapturedText(consoleSpy.mock.calls); + const jsonOutput = JSON.parse(capturedText); + expect(jsonOutput.warnings[3].componentPath).toEqual([ + 'components', + 'responses', + 'Ok', + 'content', + 'application/json' + ]); + expect(jsonOutput.warnings[3].componentLine).toEqual(6); + }); }); diff --git a/test/cli-validator/tests/utils/addPathsToComponents.test.js b/test/cli-validator/tests/utils/addPathsToComponents.test.js new file mode 100644 index 000000000..47977c30a --- /dev/null +++ b/test/cli-validator/tests/utils/addPathsToComponents.test.js @@ -0,0 +1,369 @@ +const addPathsToComponents = require('../../../../src/cli-validator/utils/addPathsToComponents'); + +describe('test postprocessing util - test component path finding', function() { + it('should correctly add component paths', function() { + const unresolvedSpec = { + paths: { + '/path1': { + get: { + responses: { + '200': { + $ref: '#/components/responses/GenericResponse' + }, + '201': { + $ref: '#/components/responses/GenericResponse' + } + } + } + } + }, + components: { + responses: { + GenericResponse: { + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/GenericSchema' + } + } + } + } + }, + schemas: { + GenericSchema: { + type: 'object', + properties: { + stringProp: { + type: 'string' + } + } + } + } + } + }; + // mimicing errors from a resolved spec + const originalResults = { + errors: { + 'validator-name': [ + { + message: 'simple error message', + path: + 'paths./path1.get.responses.200.content.application/json.schema.properties.stringProp', + rule: 'made_up_rule' + }, + { + message: 'simple error message', + path: + 'paths./path1.get.responses.201.content.application/json.schema.properties.stringProp', + rule: 'made_up_rule' + } + ] + } + }; + addPathsToComponents(originalResults, unresolvedSpec); + const errors = originalResults.errors['validator-name']; + expect(errors.length).toBe(2); + expect(errors[0].componentPath).toEqual([ + 'components', + 'schemas', + 'GenericSchema', + 'properties', + 'stringProp' + ]); + expect(errors[1].componentPath).toEqual([ + 'components', + 'schemas', + 'GenericSchema', + 'properties', + 'stringProp' + ]); + }); + + it('should correctly add component paths when an array index is in the path', function() { + const unresolvedSpec = { + paths: { + '/path1': { + post: { + requestBody: { + $ref: '#/components/requestBodies/OneOfRequest' + } + } + } + }, + components: { + requestBodies: { + OneOfRequest: { + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/OneOfSchema' + } + } + } + } + }, + schemas: { + OneOfSchema: { + oneOf: [ + { + $ref: '#/components/schemas/Cat' + }, + { + $ref: '#/components/schemas/Dog' + } + ] + }, + Cat: { + type: 'object', + properties: { + meows: { + type: 'boolean' + } + } + }, + Dog: { + type: 'object', + properties: { + barks: { + type: 'boolean' + } + } + } + } + } + }; + // mimicing errors from a resolved spec + const originalResults = { + errors: { + 'validator-name': [ + { + message: 'simple error message', + path: + 'paths./path1.post.requestBody.content.application/json.schema.oneOf.0.properties.meows', + rule: 'made_up_rule' + }, + { + message: 'simple error message with array path', + path: [ + 'paths', + '/path1', + 'post', + 'requestBody', + 'content', + 'application/json', + 'schema', + 'oneOf', + '0', + 'properties', + 'meows' + ], + rule: 'made_up_rule' + }, + { + message: 'simple error message with array path with a number index', + path: [ + 'paths', + '/path1', + 'post', + 'requestBody', + 'content', + 'application/json', + 'schema', + 'oneOf', + 0, + 'properties', + 'meows' + ], + rule: 'made_up_rule' + } + ] + } + }; + addPathsToComponents(originalResults, unresolvedSpec); + const errors = originalResults.errors['validator-name']; + expect(errors.length).toBe(3); + expect(errors[0].componentPath).toEqual([ + 'components', + 'schemas', + 'Cat', + 'properties', + 'meows' + ]); + expect(errors[1].componentPath).toEqual([ + 'components', + 'schemas', + 'Cat', + 'properties', + 'meows' + ]); + expect(errors[2].componentPath).toEqual([ + 'components', + 'schemas', + 'Cat', + 'properties', + 'meows' + ]); + }); + + it('should not add a component path when the error path is invalid', function() { + const unresolvedSpec = { + paths: { + '/path1': { + get: { + responses: { + '200': { + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/GenericSchema' + } + } + } + } + } + } + } + }, + components: { + schemas: { + GenericSchema: { + type: 'string' + } + } + } + }; + // invalid error path + const originalResults = { + errors: { + 'validator-name': [ + { + message: 'simple error message', + path: + 'paths./path1.get.requestBody.content.application/json.schema', + rule: 'made_up_rule' + } + ] + } + }; + addPathsToComponents(originalResults, unresolvedSpec); + const errors = originalResults.errors['validator-name']; + expect(errors.length).toBe(1); + expect(errors[0].componentPath).toBeUndefined(); + }); + + it('should not add a component path when the ref is invalid', function() { + const unresolvedSpec = { + paths: { + '/path1': { + get: { + responses: { + '200': { + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/SchemaDoesntExist' + } + } + } + } + } + } + } + }, + components: { + schemas: { + GenericSchema: { + type: 'object', + properties: { + stringProp: { + type: 'string' + } + } + } + } + } + }; + + // invalid error path + const originalResults = { + errors: { + 'validator-name': [ + { + message: 'simple error message', + path: + 'paths./path1.get.responses.200.content.application/json.schema.stringProp', + rule: 'made_up_rule' + } + ] + } + }; + + addPathsToComponents(originalResults, unresolvedSpec); + const errors = originalResults.errors['validator-name']; + expect(errors.length).toBe(1); + expect(errors[0].componentPath).toBeUndefined(); + }); + + it('should not add a component path when the ref is invalid', function() { + const unresolvedSpec = { + swagger: '2.0', + paths: { + '/pet': { + post: { + description: 'post a pet to store', + operationId: 'opId', + produces: ['application/json'], + parameters: [ + { + in: 'body', + name: 'body', + description: 'Pet object that needs to be added to the store', + required: true, + schema: { + $ref: '#/definitions/Pet' + } + } + ] + } + } + }, + definitions: { + Pet: { + type: 'object', + properties: { + id: { + type: 'integer', + format: 'int64' + }, + name: { + type: 'string', + description: 'string' + } + } + } + } + }; + + const originalResults = { + errors: { + 'validator-name': [ + { + message: 'simple error message', + path: 'paths./pet.post.parameters.0.schema.properties.id', + rule: 'made_up_rule' + } + ] + } + }; + + addPathsToComponents(originalResults, unresolvedSpec); + const errors = originalResults.errors['validator-name']; + expect(errors.length).toBe(1); + expect(errors[0].componentPath).toEqual([ + 'definitions', + 'Pet', + 'properties', + 'id' + ]); + }); +});