Rename methods and address comments

Author: aeisenberg

extensions/ql-vscode/src/remote-queries/analyses-results-manager.ts

@@ -14,7 +14,7 @@ import { extractAnalysisAlerts } from './sarif-processing';
 import { CodeQLCliServer } from '../cli';
 import { extractRawResults } from './bqrs-processing';
 import { asyncFilter, getErrorMessage } from '../pure/helpers-pure';
-import { toDownloadPath } from './download-link';
+import { createDownloadPath } from './download-link';
 
 export class AnalysesResultsManager {
   // Store for the results of various analyses for each remote query.
@@ -51,17 +51,17 @@ export class AnalysesResultsManager {
    * it will be downloaded. If it is already downloaded, it will be loaded into memory.
    * If it is already in memory, this will be a no-op.
    *
-   * @param allAnalysesToDownload List of analyses to ensure are downloaded and in memory
+   * @param allAnalysesToLoad List of analyses to ensure are downloaded and in memory
    * @param token Optional cancellation token
    * @param publishResults Optional function to publish the results after loading
    */
   public async loadAnalysesResults(
-    allAnalysesToDownload: AnalysisSummary[],
+    allAnalysesToLoad: AnalysisSummary[],
     token?: CancellationToken,
     publishResults: (analysesResults: AnalysisResults[]) => Promise<void> = () => Promise.resolve()
   ): Promise<void> {
     // Filter out analyses that we have already in memory.
-    const analysesToDownload = allAnalysesToDownload.filter(x => !this.isAnalysisInMemory(x));
+    const analysesToDownload = allAnalysesToLoad.filter(x => !this.isAnalysisInMemory(x));
 
     const credentials = await Credentials.initialize(this.ctx);
 
@@ -166,11 +166,11 @@ export class AnalysesResultsManager {
   public async loadDownloadedArtifacts(
     allAnalysesToCheck: AnalysisSummary[]
   ) {
-    const allDownloadedAnalyses = await asyncFilter(allAnalysesToCheck, x => this.isDownloadedNotInMemory(x));
+    const allDownloadedAnalyses = await asyncFilter(allAnalysesToCheck, x => this.isAnalysisDownloadedNotInMemory(x));
     await this.loadAnalysesResults(allDownloadedAnalyses);
   }
 
-  private async isDownloadedNotInMemory(analysis: AnalysisSummary): Promise<boolean> {
+  private async isAnalysisDownloadedNotInMemory(analysis: AnalysisSummary): Promise<boolean> {
     const queryId = analysis.downloadLink.queryId;
     const resultsForQuery = this.internalGetAnalysesResults(queryId);
     const analysisResults = resultsForQuery.find(r => r.nwo === analysis.nwo);
@@ -180,7 +180,7 @@ export class AnalysesResultsManager {
     }
 
     // Check if the analysis results are already downloaded, but not in memory
-    return await fs.pathExists(toDownloadPath(this.storagePath, analysis.downloadLink));
+    return await fs.pathExists(createDownloadPath(this.storagePath, analysis.downloadLink));
   }
 
   private async readBqrsResults(filePath: string, fileLinkPrefix: string): Promise<AnalysisRawResults> {

extensions/ql-vscode/src/remote-queries/download-link.ts

@@ -35,6 +35,6 @@ export interface DownloadLink {
  *
  * @returns A full path to the download location of the artifact
  */
-export function toDownloadPath(storagePath: string, downloadLink: DownloadLink, extension = '') {
+export function createDownloadPath(storagePath: string, downloadLink: DownloadLink, extension = '') {
   return path.join(storagePath, downloadLink.queryId, downloadLink.id + (extension ? `.${extension}` : ''));
 }

extensions/ql-vscode/src/remote-queries/gh-actions-api-client.ts

@@ -5,7 +5,7 @@ import { showAndLogWarningMessage, tmpDir } from '../helpers';
 import { Credentials } from '../authentication';
 import { logger } from '../logging';
 import { RemoteQueryWorkflowResult } from './remote-query-workflow-result';
-import { DownloadLink, toDownloadPath } from './download-link';
+import { DownloadLink, createDownloadPath } from './download-link';
 import { RemoteQuery } from './remote-query';
 import { RemoteQueryFailureIndexItem, RemoteQueryResultIndex, RemoteQuerySuccessIndexItem } from './remote-query-result-index';
 
@@ -82,14 +82,14 @@ export async function downloadArtifactFromLink(
 
   const octokit = await credentials.getOctokit();
 
-  const extractedPath = toDownloadPath(storagePath, downloadLink);
+  const extractedPath = createDownloadPath(storagePath, downloadLink);
 
   // first check if we already have the artifact
   if (!(await fs.pathExists(extractedPath))) {
     // Download the zipped artifact.
     const response = await octokit.request(`GET ${downloadLink.urlPath}/zip`, {});
 
-    const zipFilePath = toDownloadPath(storagePath, downloadLink, 'zip');
+    const zipFilePath = createDownloadPath(storagePath, downloadLink, 'zip');
     await saveFile(`${zipFilePath}`, response.data as ArrayBuffer);
 
     // Extract the zipped artifact.

extensions/ql-vscode/src/remote-queries/remote-queries-interface.ts

@@ -46,11 +46,15 @@ export class RemoteQueriesInterfaceManager {
     this.getPanel().reveal(undefined, true);
 
     await this.waitForPanelLoaded();
+    const model = await this.buildViewModel(query, queryResult);
     await this.postMessage({
       t: 'setRemoteQueryResult',
-      queryResult: await this.buildViewModel(query, queryResult)
+      queryResult: model
     });
 
+    // Ensure all pre-downloaded artifacts are loaded into memory
+    await this.analysesResultsManager.loadDownloadedArtifacts(model.analysisSummaries);
+
     await this.setAnalysisResults(this.analysesResultsManager.getAnalysesResults(queryResult.queryId));
   }
 
@@ -69,7 +73,7 @@ export class RemoteQueriesInterfaceManager {
     const analysisSummaries = this.buildAnalysisSummaries(queryResult.analysisSummaries);
     const affectedRepositories = queryResult.analysisSummaries.filter(r => r.resultCount > 0);
 
-    const model = {
+    return {
       queryTitle: query.queryName,
       queryFileName: queryFileName,
       queryFilePath: query.queryFilePath,
@@ -84,10 +88,6 @@ export class RemoteQueriesInterfaceManager {
       analysisSummaries: analysisSummaries,
       analysisFailures: queryResult.analysisFailures,
     };
-
-    // Ensure all pre-downloaded artifacts are loaded into memory
-    await this.analysesResultsManager.loadDownloadedArtifacts(model.analysisSummaries);
-    return model;
   }
 
   getPanel(): WebviewPanel {

extensions/ql-vscode/src/remote-queries/sarif-processing.ts

@@ -54,9 +54,9 @@ function extractResultAlerts(
   for (const location of result.locations ?? []) {
     const physicalLocation = location.physicalLocation!;
     const filePath = physicalLocation.artifactLocation!.uri!;
-    const codeSnippet = getCodeSnippet(physicalLocation.contextRegion!);
+    const codeSnippet = getCodeSnippet(physicalLocation.contextRegion, physicalLocation.region);
     const highlightedRegion = physicalLocation.region
-      ? getHighlightedRegion(physicalLocation.region!)
+      ? getHighlightedRegion(physicalLocation.region)
       : undefined;
 
     const analysisAlert: AnalysisAlert = {
@@ -156,24 +156,21 @@ export function tryGetRule(
   return undefined;
 }
 
-function getCodeSnippet(region: sarif.Region): CodeSnippet {
+function getCodeSnippet(region?: sarif.Region, alternateRegion?: sarif.Region): CodeSnippet | undefined {
+  region = region ?? alternateRegion;
 
   if (!region) {
-    // Handle SARIF generated from queries that do not have a region.
-    return {
-      startLine: 1,
-      endLine: 1,
-      text: ''
-    };
+    return undefined;
   }
-  const text = region.snippet!.text!;
+
+  const text = region.snippet?.text || '';
   const { startLine, endLine } = parseSarifRegion(region);
 
   return {
     startLine,
     endLine,
     text
-  } as CodeSnippet;
+  };
 }
 
 function getHighlightedRegion(region: sarif.Region): HighlightedRegion {
@@ -204,7 +201,7 @@ function getCodeFlows(
         for (const threadFlowLocation of threadFlow.locations) {
           const physicalLocation = threadFlowLocation!.location!.physicalLocation!;
           const filePath = physicalLocation!.artifactLocation!.uri!;
-          const codeSnippet = getCodeSnippet(physicalLocation.contextRegion!);
+          const codeSnippet = getCodeSnippet(physicalLocation.contextRegion, physicalLocation.region);
           const highlightedRegion = physicalLocation.region
             ? getHighlightedRegion(physicalLocation.region)
             : undefined;

extensions/ql-vscode/src/remote-queries/shared/analysis-result.ts

@@ -21,7 +21,7 @@ export interface AnalysisAlert {
   shortDescription: string;
   severity: ResultSeverity;
   fileLink: FileLink;
-  codeSnippet: CodeSnippet;
+  codeSnippet?: CodeSnippet;
   highlightedRegion?: HighlightedRegion;
   codeFlows: CodeFlow[];
 }

extensions/ql-vscode/src/remote-queries/view/FileCodeSnippet.tsx

@@ -181,17 +181,17 @@ const FileCodeSnippet = ({
   messageChildren,
 }: {
   fileLink: FileLink,
-  codeSnippet: CodeSnippet,
+  codeSnippet?: CodeSnippet,
   highlightedRegion?: HighlightedRegion,
   severity?: ResultSeverity,
   message?: AnalysisMessage,
   messageChildren?: React.ReactNode,
 }) => {
 
-  const code = codeSnippet.text.split('\n');
+  const code = codeSnippet?.text.split('\n') || [];
 
-  const startingLine = codeSnippet.startLine;
-  const endingLine = codeSnippet.endLine;
+  const startingLine = codeSnippet?.startLine || 0;
+  const endingLine = codeSnippet?.endLine || 0;
 
   const titleFileUri = createRemoteFileRef(
     fileLink,

extensions/ql-vscode/src/vscode-tests/no-workspace/download-link.test.ts

@@ -2,7 +2,7 @@ import { expect } from 'chai';
 import 'mocha';
 import * as path from 'path';
 
-import { DownloadLink, toDownloadPath } from '../../remote-queries/download-link';
+import { DownloadLink, createDownloadPath } from '../../remote-queries/download-link';
 
 describe('toDownloadPath', () => {
   it('should return the correct path', () => {
@@ -12,9 +12,11 @@ describe('toDownloadPath', () => {
       innerFilePath: '',
       queryId: 'def'
     };
-
     const expectedPath = path.join('storage', 'def', 'abc');
-    expect(toDownloadPath('storage', downloadLink)).to.equal(expectedPath);
+
+    const actualPath = createDownloadPath('storage', downloadLink);
+
+    expect(actualPath).to.equal(expectedPath);
   });
 
   it('should return the correct path with extension', () => {
@@ -26,6 +28,9 @@ describe('toDownloadPath', () => {
     };
 
     const expectedPath = path.join('storage', 'def', 'abc.zip');
-    expect(toDownloadPath('storage', downloadLink, 'zip')).to.equal(expectedPath);
+
+    const actualPath = createDownloadPath('storage', downloadLink, 'zip');
+
+    expect(actualPath).to.equal(expectedPath);
   });
 });

extensions/ql-vscode/test/pure-tests/sarif-processing.test.ts

@@ -419,15 +419,42 @@ describe('SARIF processing', () => {
       expectResultParsingError(result.errors[0]);
     });
 
-    it('should return errors for result locations with no context region', () => {
+    it('should not return errors for result locations with no snippet', () => {
       const sarif = buildValidSarifLog();
       sarif.runs![0]!.results![0]!.locations![0]!.physicalLocation!.contextRegion!.snippet = undefined;
 
       const result = extractAnalysisAlerts(sarif, fakefileLinkPrefix);
 
+      const expectedCodeSnippet = {
+        startLine: result.alerts[0].codeSnippet!.startLine,
+        endLine: result.alerts[0].codeSnippet!.endLine,
+        text: ''
+      };
+
+      const actualCodeSnippet = result.alerts[0].codeSnippet;
+
       expect(result).to.be.ok;
-      expect(result.errors.length).to.equal(1);
-      expectResultParsingError(result.errors[0]);
+      expectNoParsingError(result);
+      expect(actualCodeSnippet).to.deep.equal(expectedCodeSnippet);
+    });
+
+    it('should not return errors for result locations with no contextRegion', () => {
+      const sarif = buildValidSarifLog();
+      sarif.runs![0]!.results![0]!.locations![0]!.physicalLocation!.contextRegion = undefined;
+
+      const result = extractAnalysisAlerts(sarif, fakefileLinkPrefix);
+
+      const expectedCodeSnippet = {
+        startLine: result.alerts[0].highlightedRegion!.startLine,
+        endLine: result.alerts[0].highlightedRegion!.endLine,
+        text: ''
+      };
+
+      const actualCodeSnippet = result.alerts[0].codeSnippet;
+
+      expect(result).to.be.ok;
+      expectNoParsingError(result);
+      expect(actualCodeSnippet).to.deep.equal(expectedCodeSnippet);
     });
 
     it('should not return errors for result locations with no region', () => {
@@ -438,6 +465,7 @@ describe('SARIF processing', () => {
 
       expect(result).to.be.ok;
       expect(result.alerts.length).to.equal(1);
+      expectNoParsingError(result);
     });
 
     it('should return errors for result locations with no physical location', () => {
@@ -537,9 +565,9 @@ describe('SARIF processing', () => {
       expect(result).to.be.ok;
       expect(result.errors.length).to.equal(0);
       expect(result.alerts.length).to.equal(3);
-      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet.text === 'foo')).to.be.ok;
-      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet.text === 'bar')).to.be.ok;
-      expect(result.alerts.find(a => getMessageText(a.message) === 'msg2' && a.codeSnippet.text === 'baz')).to.be.ok;
+      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet!.text === 'foo')).to.be.ok;
+      expect(result.alerts.find(a => getMessageText(a.message) === 'msg1' && a.codeSnippet!.text === 'bar')).to.be.ok;
+      expect(result.alerts.find(a => getMessageText(a.message) === 'msg2' && a.codeSnippet!.text === 'baz')).to.be.ok;
       expect(result.alerts.every(a => a.severity === 'Warning')).to.be.true;
     });
 
@@ -595,6 +623,11 @@ describe('SARIF processing', () => {
     expect(msg.startsWith('Error when processing SARIF result')).to.be.true;
   }
 
+  function expectNoParsingError(result: { errors: string[] | undefined }) {
+    const array = result.errors || [];
+    expect(array.length, array.join()).to.equal(0);
+  }
+
   function buildValidSarifLog(): sarif.Log {
     return {
       version: '2.1.0',