fix: validate specObj for all commands and clone before dereferencing

digio · Sep 14, 2020 · 01482cf · 01482cf
1 parent 623e145
commit 01482cf
Show file tree

Hide file tree

Showing 9 changed files with 100 additions and 35 deletions.
diff --git a/fixtures/invalidSpecV2.json b/fixtures/invalidSpecV2.json
@@ -0,0 +1,13 @@
+{
+  "swagger": "2.0",
+  "info": {
+    "description": "VFE API",
+    "version": "1.0.0",
+    "title": "VFE API",
+    "license": {
+      "name": "Apache 2.0",
+      "url": "http://www.apache.org/licenses/LICENSE-2.0.html"
+    }
+  },
+  "paths": {}
+}
diff --git a/package.json b/package.json
@@ -22,10 +22,10 @@
     "lint": "eslint --max-warnings=0 --fix src/",
     "verify": "eslint --max-warnings=0 src/",
     "test": "npm-run-all test:report",
-    "test:unit": "jest",
-    "test:report": "jest --no-cache --coverage --json --outputFile=test-reports/unit/unit.json",
-    "test:reportlist": "jest --no-cache --coverage",
-    "test:watch": "jest --watchAll",
+    "test:unit": "jest --no-colors",
+    "test:report": "jest --no-colors --no-cache --coverage --json --outputFile=test-reports/unit/unit.json",
+    "test:reportlist": "jest --no-colors --no-cache --coverage",
+    "test:watch": "jest --no-colors --watchAll",
     "upload-coverage": "coveralls < test-reports/coverage/lcov.info",
     "semantic-release": "semantic-release"
   },

diff --git a/src/addGatewayInfo.js b/src/addGatewayInfo.js
@@ -7,6 +7,7 @@ const {
   getExampleObject,
   readJsonFile,
   traverse,
+  validateSpecObj,
   writeOutputFile,
 } = require('./utils');
 const cloneDeep = require('lodash/cloneDeep');
@@ -18,8 +19,9 @@ const WEB_PAGE_LOGO_TOKEN = '$$_logoUrl_$$';
 const WEB_PAGE_TITLE = '$$_title_$$';
 const WEB_PAGE_FAVICON_HREF = '$$_faviconHref_$$';
 
-function addGatewayInfo(specFile, server, config) {
+async function addGatewayInfo(specFile, server, config) {
   const specObj = readJsonFile(specFile);
+  await validateSpecObj({ specObj }); // We have to validate before we try to read the file cntents
   const absSpecFilePath = getAbsSpecFilePath(specFile);
   const serverUrl = server || specObj.servers[0].url;
 
@@ -43,7 +45,7 @@ function addGatewayInfo(specFile, server, config) {
     },
   );
 
-  pipeline({ specObj, serverUrl, specFile, absSpecFilePath, config });
+  return pipeline({ specObj, serverUrl, specFile, absSpecFilePath, config });
 }
 
 function setServerUrl(params) {

diff --git a/src/cli.js b/src/cli.js
@@ -14,12 +14,12 @@ program
   .option('-q, --quiet', 'no logging')
   .option('-v, --verbose', 'verbose logging')
   // eslint-disable-next-line @getify/proper-arrows/where
-  .action((jsonFile, cmd) => {
+  .action(async (jsonFile, cmd) => {
     const { log, config } = getLogAndConfig('lint', cmd);
 
     const { lintCommand } = require('./lint');
     try {
-      lintCommand(jsonFile, config);
+      await lintCommand(jsonFile, config);
     } catch (err) {
       log.error(err);
     }
@@ -36,14 +36,15 @@ program
   .option('-v, --verbose', 'Verbose logging')
   .option('-d, --dry-run', 'Dry run (no changes made)')
   // eslint-disable-next-line @getify/proper-arrows/where
-  .action((jsonFile, serverUrl, cmd) => {
+  .action(async (jsonFile, serverUrl, cmd) => {
     const { log, config } = getLogAndConfig('record', cmd);
     log.debug(`command args: ${jsonFile}`);
 
     const { recordCommand } = require('./record');
     try {
-      recordCommand(jsonFile, serverUrl, config);
+      await recordCommand(jsonFile, serverUrl, config);
     } catch (err) {
+      // console.error(err);
       log.error(err);
     }
   });
@@ -60,7 +61,7 @@ program
   .option('-v, --verbose', 'Verbose logging')
   .option('-d, --dry-run', 'Dry run (no changes made)')
   // eslint-disable-next-line @getify/proper-arrows/where
-  .action((jsonFile, outputJsonFile, serverUrl, cmd) => {
+  .action(async (jsonFile, outputJsonFile, serverUrl, cmd) => {
     // Merge the outputJsonFile into the config.outputFile property,
     // so we can reuse the pathing logic there
     const { log, config } = getLogAndConfig('build', { ...cmd, output: outputJsonFile });
@@ -72,7 +73,7 @@ program
 
     const { addGatewayInfo } = require('./addGatewayInfo');
     try {
-      addGatewayInfo(jsonFile, serverUrl, config);
+      await addGatewayInfo(jsonFile, serverUrl, config);
     } catch (err) {
       log.error(err);
     }

diff --git a/src/compare.js b/src/compare.js
@@ -18,17 +18,17 @@ const {
   validateSpecObj,
 } = require('./utils');
 
-function compareCommand(specFile, server, config) {
+async function compareCommand(specFile, server, config) {
   // Read the spec file outside of the pipeline, so that we can inject it (and re-write it)
   // at the end. Not very functional, but easier than passing the data all the way down the pipe.
   const specObj = readJsonFile(specFile);
+  const { openapi } = await validateSpecObj({ specObj }); // We have to validate before we try to read the file cntents
   const absSpecFilePath = getAbsSpecFilePath(specFile);
   const destPath = getAbsSpecFilePath(config.outputFile ? config.outputFile : specFile);
   const serverUrl = server || specObj.servers[0].url;
 
   // define the data processing pipeline
   const pipeline = pipe(
-    validateSpecObj, // Adds openapi property
     getFetchConfigForAPIEndpoints,
     addParamsToFetchConfig,
     fetchResponses,
@@ -38,7 +38,7 @@ function compareCommand(specFile, server, config) {
     returnExitCode,
   );
 
-  return pipeline({ specObj, serverUrl, destPath, specFile, absSpecFilePath, config });
+  return pipeline({ openapi, specObj, serverUrl, destPath, specFile, absSpecFilePath, config });
 }
 
 /**

diff --git a/src/e2e.spec.js b/src/e2e.spec.js
@@ -92,6 +92,17 @@ describe('CLI', () => {
         ]
       `);
     });
+
+    it('should display an error when the spec is not valid', async () => {
+      const result = await runCommand(`lint ./fixtures/invalidSpecV2.json`);
+
+      expect(result).toMatchInlineSnapshot(`
+        "[31merror[39m: One or more errors exist in the OpenApi definition
+          Property not allowed: swagger
+          Missing required property: openapi
+        "
+      `);
+    });
   });
 
   describe('record', () => {
@@ -138,6 +149,17 @@ describe('CLI', () => {
         "
       `);
     });
+
+    it('should display an error when the spec is not valid', async () => {
+      const result = await runCommand(`record ./fixtures/invalidSpecV2.json`);
+
+      expect(result).toMatchInlineSnapshot(`
+        "[31merror[39m: One or more errors exist in the OpenApi definition
+          Property not allowed: swagger
+          Missing required property: openapi
+        "
+      `);
+    });
   });
 
   describe('build should create a spec file that is OpenAPI 3.x compliant', () => {
@@ -196,6 +218,17 @@ describe('CLI', () => {
         ]
       `);
     });
+
+    it('should display an error when the spec is not valid', async () => {
+      const result = await runCommand(`build ./fixtures/invalidSpecV2.json foo.txt`);
+
+      expect(result).toMatchInlineSnapshot(`
+        "[31merror[39m: One or more errors exist in the OpenApi definition
+          Property not allowed: swagger
+          Missing required property: openapi
+        "
+      `);
+    });
   });
 
   describe('compare', () => {
@@ -214,6 +247,17 @@ describe('CLI', () => {
       `);
     });
 
+    it('should display an error when the spec is not valid', async () => {
+      const result = await runCommand(`compare ./fixtures/invalidSpecV2.json foo.txt`);
+
+      expect(result).toMatchInlineSnapshot(`
+        "[31merror[39m: One or more errors exist in the OpenApi definition
+          Property not allowed: swagger
+          Missing required property: openapi
+        "
+      `);
+    });
+
     describe('by value', () => {
       it('should indicate there are no differences when there are no differences', async () => {
         const result = await runCommand(`compare ./fixtures/threeExamplesWithCorrectStatus.json`);
@@ -237,19 +281,19 @@ describe('CLI', () => {
           [34minfo[39m: Fetching 4 of 4: https://jsonplaceholder.typicode.com/posts/wrong-param get
           [34minfo[39m: Comparing by VALUE
           [31merror[39m: /posts/2
-          [32m- Expected[39m
-          [31m+ Received[39m
-
-          [2m  Object {[22m
-          [2m    \\"body\\": \\"est rerum tempore vitae[22m
-          [2m  sequi sint nihil reprehenderit dolor beatae ea dolores neque[22m
-          [2m  fugiat blanditiis voluptate porro vel nihil molestiae ut reiciendis[22m
-          [2m  qui aperiam non debitis possimus qui neque nisi nulla\\",[22m
-          [2m    \\"id\\": 2,[22m
-          [32m-   \\"title\\": \\"Non matching title\\",[39m
-          [31m+   \\"title\\": \\"qui est esse\\",[39m
-          [2m    \\"userId\\": 1,[22m
-          [2m  }[22m
+          - Expected
+          + Received
+
+            Object {
+              \\"body\\": \\"est rerum tempore vitae
+            sequi sint nihil reprehenderit dolor beatae ea dolores neque
+            fugiat blanditiis voluptate porro vel nihil molestiae ut reiciendis
+            qui aperiam non debitis possimus qui neque nisi nulla\\",
+              \\"id\\": 2,
+          -   \\"title\\": \\"Non matching title\\",
+          +   \\"title\\": \\"qui est esse\\",
+              \\"userId\\": 1,
+            }
 
           [31merror[39m: ❌ Differences were detected
           "

diff --git a/src/lint.js b/src/lint.js
@@ -6,19 +6,20 @@ const {
   getFetchConfigForAPIEndpoints,
   getExampleObject,
   addParamsToFetchConfig,
+  validateSpecObj,
 } = require('./utils');
 const logger = require('winston');
 
-function lintCommand(specFile, config) {
+async function lintCommand(specFile, config) {
   const specObj = readJsonFile(specFile);
   const absSpecFilePath = getAbsSpecFilePath(specFile);
 
   // Read the file, lint it, write it
-  const pipeline = pipe(lintSpec, writeOutputFile, () => {
+  const pipeline = pipe(validateSpecObj, lintSpec, writeOutputFile, () => {
     logger.info('Linting complete.');
   });
 
-  pipeline({ specObj, specFile, absSpecFilePath, config });
+  return pipeline({ specObj, specFile, absSpecFilePath, config });
 }
 
 function lintSpec(params) {

diff --git a/src/record.js b/src/record.js
@@ -11,22 +11,25 @@ const {
   getFetchConfigForAPIEndpoints,
   addParamsToFetchConfig,
   readJsonFile,
+  validateSpecObj,
   writeJsonFile,
   writeOutputFile,
 } = require('./utils');
 const { lintSpec } = require('./lint');
 const { compareResponseData } = require('./compare');
 
-function recordCommand(specFile, server, config) {
+async function recordCommand(specFile, server, config) {
   // Read the spec file outside of the pipeline, so that we can inject it (and re-write it)
   // at the end. Not very functional, but easier than passing the data all the way down the pipe.
   const specObj = readJsonFile(specFile);
+  await validateSpecObj({ specObj }); // We have to validate before we try to read the file cntents
   const absSpecFilePath = getAbsSpecFilePath(specFile);
   const destPath = getAbsSpecFilePath(config.outputFile ? config.outputFile : specFile);
   const serverUrl = server || specObj.servers[0].url;
 
   // define the data processing pipeline
   const pipeline = pipe(
+    validateSpecObj,
     getFetchConfigForAPIEndpoints,
     addParamsToFetchConfig,
     fetchResponses,
@@ -37,7 +40,7 @@ function recordCommand(specFile, server, config) {
     writeOutputFile,
   );
 
-  pipeline({ specObj, serverUrl, destPath, specFile, absSpecFilePath, config });
+  return pipeline({ specObj, serverUrl, destPath, specFile, absSpecFilePath, config });
 }
 
 /**

diff --git a/src/utils.js b/src/utils.js
@@ -389,10 +389,11 @@ function cloneDeep(obj) {
 
 async function validateSpecObj(params) {
   const { specObj } = params;
-  const [openapi, error] = Enforcer.v3_0.OpenApi(await Enforcer.dereference(specObj));
+  const [openapi, error] = Enforcer.v3_0.OpenApi(await Enforcer.dereference(cloneDeep(specObj)));
 
   if (error) {
-    throw error;
+    // The default error from Enforcer is a weird object
+    throw new Error(error.toString());
   }
 
   return { ...params, openapi };