Use sarif parser for reopened results

extensions/ql-vscode/src/query-results.ts

@@ -19,6 +19,7 @@ import { QueryStatus } from './query-status';
 import { RemoteQueryHistoryItem } from './remote-queries/remote-query-history-item';
 import { QueryEvaluationInfo, QueryWithResults } from './run-queries-shared';
 import { formatLegacyMessage } from './legacy-query-server/run-queries';
+import { sarifParser } from './sarif-parser';
 
 /**
  * query-results.ts
@@ -160,10 +161,12 @@ export async function interpretResultsSarif(
   sourceInfo?: cli.SourceInfo
 ): Promise<SarifInterpretationData> {
   const { resultsPath, interpretedResultsPath } = resultsPaths;
+  let res;
   if (await fs.pathExists(interpretedResultsPath)) {
-    return { ...JSON.parse(await fs.readFile(interpretedResultsPath, 'utf8')), t: 'SarifInterpretationData' };
+    res = await sarifParser(interpretedResultsPath);
+  } else {
+    res = await cli.interpretBqrsSarif(ensureMetadataIsComplete(metadata), resultsPath, interpretedResultsPath, sourceInfo);
   }
-  const res = await cli.interpretBqrsSarif(ensureMetadataIsComplete(metadata), resultsPath, interpretedResultsPath, sourceInfo);
   return { ...res, t: 'SarifInterpretationData' };
 }
 

extensions/ql-vscode/src/sarif-parser.ts

@@ -1,33 +1,34 @@
 import * as Sarif from 'sarif';
 import * as fs from 'fs-extra';
-import { parser } from 'stream-json';
-import { pick } from 'stream-json/filters/Pick';
 import Assembler = require('stream-json/Assembler');
-import { chain } from 'stream-chain';
 import { getErrorMessage } from './pure/helpers-pure';
+import Pick = require('stream-json/filters/Pick');
 
 const DUMMY_TOOL: Sarif.Tool = { driver: { name: '' } };
 
 export async function sarifParser(interpretedResultsPath: string): Promise<Sarif.Log> {
   try {
     // Parse the SARIF file into token streams, filtering out only the results array.
-    const p = parser();
-    const pipeline = chain([
-      fs.createReadStream(interpretedResultsPath),
-      p,
-      pick({ filter: 'runs.0.results' })
-    ]);
+    const pipeline = fs.createReadStream(interpretedResultsPath).pipe(Pick.withParser({ filter: 'runs.0.results' }));
 
     // Creates JavaScript objects from the token stream
     const asm = Assembler.connectTo(pipeline);
 
-    // Returns a constructed Log object with the results or an empty array if no results were found.
+    // Returns a constructed Log object with the results of an empty array if no results were found.
     // If the parser fails for any reason, it will reject the promise.
     return await new Promise((resolve, reject) => {
+      let alreadyDone = false;
       pipeline.on('error', (error) => {
         reject(error);
       });
 
+      // If the parser pipeline completes before the assembler, we've reached end of file and have not found any results.
+      pipeline.on('end', () => {
+        if (!alreadyDone) {
+          reject(new Error('Invalid SARIF file: expecting at least one run with result.'));
+        }
+      });
+
       asm.on('done', (asm) => {
 
         const log: Sarif.Log = {
@@ -41,6 +42,7 @@ export async function sarifParser(interpretedResultsPath: string): Promise<Sarif
         };
 
         resolve(log);
+        alreadyDone = true;
       });
     });
   } catch (e) {

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

@@ -155,7 +155,10 @@ describe('query-results', () => {
     });
   });
 
-  it('should interpretResultsSarif', async () => {
+  it('should interpretResultsSarif', async function() {
+    // up to 2 minutes per test
+    this.timeout(2 * 60 * 1000);
+
     const spy = sandbox.mock();
     spy.returns({ a: '1234' });
     const mockServer = {
@@ -203,10 +206,10 @@ describe('query-results', () => {
       resultsPath, interpretedResultsPath, sourceInfo
     );
 
-    // try a third time, but this time we get from file
+    // try a third time, but this time we get from a valid small SARIF file
     spy.reset();
     fs.writeFileSync(interpretedResultsPath, JSON.stringify({
-      a: 6
+      runs: [{ results: [] }] // A run needs results to succeed.
     }), 'utf8');
     const results3 = await interpretResultsSarif(
       mockServer,
@@ -216,7 +219,81 @@ describe('query-results', () => {
       },
       sourceInfo as SourceInfo
     );
-    expect(results3).to.deep.eq({ a: 6, t: 'SarifInterpretationData' });
+    // We do not re-interpret if we are reading from a SARIF file. 
+    expect(spy).to.not.have.been.called;
+
+    expect(results3).to.have.property('t', 'SarifInterpretationData');
+    expect(results3).to.have.nested.property('runs[0].results');
+
+    // try a fourth time, but this time we use an invalid small SARIF file
+    spy.reset();
+    fs.writeFileSync(interpretedResultsPath, JSON.stringify({
+      a: '6' // Invalid: no runs or results
+    }), 'utf8');
+
+    await expect(
+      interpretResultsSarif(
+        mockServer,
+        metadata,
+        {
+          resultsPath, interpretedResultsPath
+        },
+        sourceInfo as SourceInfo)
+    ).to.be.rejectedWith('Parsing output of interpretation failed: Invalid SARIF file: expecting at least one run with result.');
+
+    // We do not attempt to re-interpret if we are reading from a SARIF file. 
+    expect(spy).to.not.have.been.called;
+
+    // Try a fifth time with a valid large SARIF file
+    spy.reset();
+    const validSarifStream = fs.createWriteStream(interpretedResultsPath, { flags: 'w' });
+
+    validSarifStream.write(JSON.stringify({
+      runs: [{ results: [] }] // A run needs results to succeed.
+    }), 'utf8');
+
+    for (let i = 0; i < 10000000; i++) {
+      validSarifStream.write(JSON.stringify({
+        a: '6'
+      }), 'utf8');
+    }
+    validSarifStream.end();
+
+    const results5 = await interpretResultsSarif(
+      mockServer,
+      metadata,
+      {
+        resultsPath, interpretedResultsPath
+      },
+      sourceInfo as SourceInfo
+    );
+    // We do not re-interpret if we are reading from a SARIF file. 
+    expect(spy).to.not.have.been.called;
+
+    expect(results5).to.have.property('t', 'SarifInterpretationData');
+    expect(results5).to.have.nested.property('runs[0].results');
+
+    // Try a sixth time with an invalid large SARIF file
+    spy.reset();
+    const invalidSarifStream = fs.createWriteStream(interpretedResultsPath, { flags: 'w' });
+    for (let i = 0; i < 10000000; i++) {
+      invalidSarifStream.write(JSON.stringify({
+        a: '6'
+      }), 'utf8');
+    }
+
+    await expect(
+      interpretResultsSarif(
+        mockServer,
+        metadata,
+        {
+          resultsPath, interpretedResultsPath
+        },
+        sourceInfo as SourceInfo)
+    ).to.be.rejectedWith('Parsing output of interpretation failed: Invalid SARIF file: expecting at least one run with result.');
+
+    // We do not attempt to re-interpret if we are reading from a SARIF file. 
+    expect(spy).to.not.have.been.called;
   });
 
   describe('splat and slurp', () => {