Merge pull request #2167 from snyk/refactor/replace-cc-parser-with-sp…

…lit-functions refactor: replace cc parser with split functions [CC-1021]
snyk · Aug 17, 2021 · f50bca7 · f50bca7
2 parents 707801d + 1ed7d11
commit f50bca7
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 33 deletions.
diff --git a/package.json b/package.json
@@ -75,7 +75,7 @@
   "dependencies": {
     "@open-policy-agent/opa-wasm": "^1.2.0",
     "@snyk/cli-interface": "2.11.0",
-    "@snyk/cloud-config-parser": "^1.9.2",
+    "@snyk/cloud-config-parser": "^1.10.2",
     "@snyk/code-client": "4.0.0",
     "@snyk/dep-graph": "^1.27.1",
     "@snyk/fix": "1.650.0",
@@ -147,7 +147,7 @@
   "devDependencies": {
     "@types/cross-spawn": "^6.0.2",
     "@types/fs-extra": "^9.0.11",
-    "@types/jest": "^26.0.20",
+    "@types/jest": "^27.0.1",
     "@types/lodash": "^4.14.161",
     "@types/needle": "^2.0.4",
     "@types/node": "^14.14.31",

diff --git a/src/cli/commands/test/iac-local-execution/extract-line-number.ts b/src/cli/commands/test/iac-local-execution/extract-line-number.ts
@@ -2,15 +2,16 @@ import { IaCErrorCodes } from './types';
 import { CustomError } from '../../../../lib/errors';
 import {
   CloudConfigFileTypes,
-  issuesToLineNumbers,
+  MapsDocIdToTree,
+  getLineNumber,
 } from '@snyk/cloud-config-parser';
 import { UnsupportedFileTypeError } from './file-parser';
 import * as analytics from '../../../../lib/analytics';
 import * as Debug from 'debug';
 import { getErrorStringCode } from './error-utils';
 const debug = Debug('iac-extract-line-number');
 
-function getFileTypeForLineNumber(fileType: string): CloudConfigFileTypes {
+export function getFileTypeForParser(fileType: string): CloudConfigFileTypes {
   switch (fileType) {
     case 'yaml':
     case 'yml':
@@ -25,16 +26,12 @@ function getFileTypeForLineNumber(fileType: string): CloudConfigFileTypes {
 }
 
 export function extractLineNumber(
-  fileContent: string,
-  fileType: string,
   cloudConfigPath: string[],
+  fileType: CloudConfigFileTypes,
+  treeByDocId: MapsDocIdToTree,
 ): number {
   try {
-    return issuesToLineNumbers(
-      fileContent,
-      getFileTypeForLineNumber(fileType),
-      cloudConfigPath,
-    );
+    return getLineNumber(cloudConfigPath, fileType, treeByDocId);
   } catch {
     const err = new FailedToExtractLineNumberError();
     analytics.add('error-code', err.code);

diff --git a/src/cli/commands/test/iac-local-execution/results-formatter.ts b/src/cli/commands/test/iac-local-execution/results-formatter.ts
@@ -12,9 +12,10 @@ import * as path from 'path';
 import { SEVERITY } from '../../../../lib/snyk-test/common';
 import { IacProjectType } from '../../../../lib/iac/constants';
 import { CustomError } from '../../../../lib/errors';
-import { extractLineNumber } from './extract-line-number';
+import { extractLineNumber, getFileTypeForParser } from './extract-line-number';
 import { getErrorStringCode } from './error-utils';
 import { isLocalFolder } from '../../../../lib/detect';
+import { MapsDocIdToTree, getTrees } from '@snyk/cloud-config-parser';
 
 const SEVERITIES = [SEVERITY.LOW, SEVERITY.MEDIUM, SEVERITY.HIGH];
 
@@ -53,27 +54,24 @@ function formatScanResult(
   meta: TestMeta,
   options: IaCTestFlags,
 ): FormattedResult {
+  const fileType = getFileTypeForParser(scanResult.fileType);
+  let treeByDocId: MapsDocIdToTree;
+  try {
+    treeByDocId = getTrees(fileType, scanResult.fileContent);
+  } catch (err) {
+    // we do nothing intentionally.
+    // Even if the building of the tree fails in the external parser,
+    // we still pass an undefined tree and not calculated line number for those
+  }
+
   const formattedIssues = scanResult.violatedPolicies.map((policy) => {
     const cloudConfigPath =
       scanResult.docId !== undefined
         ? [`[DocId: ${scanResult.docId}]`].concat(parsePath(policy.msg))
         : policy.msg.split('.');
 
-    const flagsRequiringLineNumber = [
-      'json',
-      'sarif',
-      'json-file-output',
-      'sarif-file-output',
-    ];
-    const shouldExtractLineNumber = flagsRequiringLineNumber.some(
-      (flag) => options[flag],
-    );
-    const lineNumber: number = shouldExtractLineNumber
-      ? extractLineNumber(
-          scanResult.fileContent,
-          scanResult.fileType,
-          cloudConfigPath,
-        )
+    const lineNumber: number = treeByDocId
+      ? extractLineNumber(cloudConfigPath, fileType, treeByDocId)
       : -1;
 
     return {

diff --git a/test/jest/unit/iac-unit-tests/results-formatter.spec.ts b/test/jest/unit/iac-unit-tests/results-formatter.spec.ts
@@ -10,16 +10,18 @@ import {
   policyStub,
   generateScanResults,
 } from './results-formatter.fixtures';
-import { issuesToLineNumbers } from '@snyk/cloud-config-parser';
+import * as cloudConfigParserModule from '@snyk/cloud-config-parser';
 import { PolicyMetadata } from '../../../../src/cli/commands/test/iac-local-execution/types';
 
-jest.mock('@snyk/cloud-config-parser');
-
+jest.mock('@snyk/cloud-config-parser', () => ({
+  ...jest.requireActual('@snyk/cloud-config-parser'),
+}));
+const validTree = { '0': { nodes: [] } };
 describe('formatScanResults', () => {
   it.each([
     [
       { severityThreshold: SEVERITY.HIGH },
-      expectedFormattedResultsWithoutLineNumber,
+      expectedFormattedResultsWithLineNumber,
     ],
     [
       { severityThreshold: SEVERITY.HIGH, sarif: true },
@@ -40,7 +42,10 @@ describe('formatScanResults', () => {
   ])(
     'given %p options object, returns the expected results',
     (optionsObject, expectedResult) => {
-      (issuesToLineNumbers as jest.Mock).mockReturnValue(3);
+      jest
+        .spyOn(cloudConfigParserModule, 'getTrees')
+        .mockReturnValue(validTree);
+      jest.spyOn(cloudConfigParserModule, 'getLineNumber').mockReturnValue(3);
       const formattedResults = formatScanResults(
         generateScanResults(),
         optionsObject,
@@ -51,9 +56,50 @@ describe('formatScanResults', () => {
       expect(formattedResults[0]).toEqual(expectedResult);
     },
   );
-  // TODO: add tests for the multi-doc yaml grouping
 });
 
+describe('parser failures should return -1 for lineNumber', () => {
+  beforeEach(async () => {
+    jest.restoreAllMocks();
+  });
+
+  it('creates a valid tree, but the getLineNumber() fails', () => {
+    jest.spyOn(cloudConfigParserModule, 'getTrees').mockReturnValue(validTree);
+    jest
+      .spyOn(cloudConfigParserModule, 'getLineNumber')
+      .mockImplementation(() => {
+        throw new Error();
+      });
+    const formattedResults = formatScanResults(
+      generateScanResults(),
+      { severityThreshold: SEVERITY.HIGH },
+      meta,
+    );
+
+    expect(formattedResults.length).toEqual(1);
+    expect(formattedResults[0]).toEqual(
+      expectedFormattedResultsWithoutLineNumber,
+    );
+  });
+
+  it('sends an invalid tree and getLineNumber() fails', () => {
+    jest
+      .spyOn(cloudConfigParserModule, 'getTrees')
+      .mockReturnValue(null as any);
+    const formattedResults = formatScanResults(
+      generateScanResults(),
+      { severityThreshold: SEVERITY.HIGH },
+      meta,
+    );
+
+    expect(formattedResults.length).toEqual(1);
+    expect(formattedResults[0]).toEqual(
+      expectedFormattedResultsWithoutLineNumber,
+    );
+  });
+});
+
+// TODO: add tests for the multi-doc yaml grouping
 describe('filterPoliciesBySeverity', () => {
   it('returns the formatted results filtered by severity - no default threshold', () => {
     const scanResults = generateScanResults();