Merge pull request #1631 from github/shati-patel/storage-dir

Get `variantAnalysisStoragePath` from the variant analysis manager + create `timestamp` file

Author: shati-patel

extensions/ql-vscode/src/remote-queries/run-remote-query.ts

@@ -278,7 +278,7 @@ export async function runRemoteQuery(
 
       const processedVariantAnalysis = processVariantAnalysis(variantAnalysisSubmission, variantAnalysisResponse);
 
-      variantAnalysisManager.onVariantAnalysisSubmitted(processedVariantAnalysis);
+      await variantAnalysisManager.onVariantAnalysisSubmitted(processedVariantAnalysis);
 
       void logger.log(`Variant analysis:\n${JSON.stringify(processedVariantAnalysis, null, 2)}`);
 

extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts

@@ -1,3 +1,5 @@
+import * as path from 'path';
+
 import * as ghApiClient from './gh-api/gh-api-client';
 import { CancellationToken, commands, EventEmitter, ExtensionContext, window } from 'vscode';
 import { DisposableObject } from '../pure/disposable-object';
@@ -23,6 +25,7 @@ import { CodeQLCliServer } from '../cli';
 import { getControllerRepo } from './run-remote-query';
 import { processUpdatedVariantAnalysis } from './variant-analysis-processor';
 import PQueue from 'p-queue';
+import { createTimestampFile } from '../helpers';
 
 export class VariantAnalysisManager extends DisposableObject implements VariantAnalysisViewManager<VariantAnalysisView> {
   private readonly _onVariantAnalysisAdded = this.push(new EventEmitter<VariantAnalysis>());
@@ -38,14 +41,14 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
   constructor(
     private readonly ctx: ExtensionContext,
     cliServer: CodeQLCliServer,
-    storagePath: string,
+    private readonly storagePath: string,
     logger: Logger,
   ) {
     super();
     this.variantAnalysisMonitor = this.push(new VariantAnalysisMonitor(ctx));
     this.variantAnalysisMonitor.onVariantAnalysisChange(this.onVariantAnalysisUpdated.bind(this));
 
-    this.variantAnalysisResultsManager = this.push(new VariantAnalysisResultsManager(cliServer, storagePath, logger));
+    this.variantAnalysisResultsManager = this.push(new VariantAnalysisResultsManager(cliServer, logger));
     this.variantAnalysisResultsManager.onResultLoaded(this.onRepoResultLoaded.bind(this));
   }
 
@@ -86,7 +89,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
       throw new Error(`No variant analysis with id: ${variantAnalysisId}`);
     }
 
-    await this.variantAnalysisResultsManager.loadResults(variantAnalysisId, repositoryFullName);
+    await this.variantAnalysisResultsManager.loadResults(variantAnalysisId, this.getVariantAnalysisStorageLocation(variantAnalysisId), repositoryFullName);
   }
 
   private async onVariantAnalysisUpdated(variantAnalysis: VariantAnalysis | undefined): Promise<void> {
@@ -99,7 +102,9 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
     await this.getView(variantAnalysis.id)?.updateView(variantAnalysis);
   }
 
-  public onVariantAnalysisSubmitted(variantAnalysis: VariantAnalysis): void {
+  public async onVariantAnalysisSubmitted(variantAnalysis: VariantAnalysis): Promise<void> {
+    await this.prepareStorageDirectory(variantAnalysis.id);
+
     this._onVariantAnalysisAdded.fire(variantAnalysis);
   }
 
@@ -157,7 +162,7 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
       repoState.downloadStatus = VariantAnalysisScannedRepositoryDownloadStatus.InProgress;
       await this.onRepoStateUpdated(variantAnalysisSummary.id, repoState);
 
-      await this.variantAnalysisResultsManager.download(credentials, variantAnalysisSummary.id, repoTask);
+      await this.variantAnalysisResultsManager.download(credentials, variantAnalysisSummary.id, repoTask, this.getVariantAnalysisStorageLocation(variantAnalysisSummary.id));
     }
 
     repoState.downloadStatus = VariantAnalysisScannedRepositoryDownloadStatus.Succeeded;
@@ -176,6 +181,23 @@ export class VariantAnalysisManager extends DisposableObject implements VariantA
     return this.queue.pending;
   }
 
+  public getVariantAnalysisStorageLocation(variantAnalysisId: number): string {
+    return path.join(
+      this.storagePath,
+      `${variantAnalysisId}`
+    );
+  }
+
+  /**
+   * Prepares a directory for storing results for a variant analysis.
+   * This directory contains a timestamp file, which will be
+   * used by the query history manager to determine when the directory
+   * should be deleted.
+   */
+  private async prepareStorageDirectory(variantAnalysisId: number): Promise<void> {
+    await createTimestampFile(this.getVariantAnalysisStorageLocation(variantAnalysisId));
+  }
+
   public async promptOpenVariantAnalysis() {
     const credentials = await Credentials.initialize(this.ctx);
     if (!credentials) { throw Error('Error authenticating with GitHub'); }

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

@@ -39,7 +39,6 @@ export class VariantAnalysisResultsManager extends DisposableObject {
 
   constructor(
     private readonly cliServer: CodeQLCliServer,
-    private readonly storagePath: string,
     private readonly logger: Logger,
   ) {
     super();
@@ -50,12 +49,13 @@ export class VariantAnalysisResultsManager extends DisposableObject {
     credentials: Credentials,
     variantAnalysisId: number,
     repoTask: VariantAnalysisRepoTask,
+    variantAnalysisStoragePath: string,
   ): Promise<void> {
     if (!repoTask.artifact_url) {
       throw new Error('Missing artifact URL');
     }
 
-    const resultDirectory = this.getRepoStorageDirectory(variantAnalysisId, repoTask.repository.full_name);
+    const resultDirectory = this.getRepoStorageDirectory(variantAnalysisStoragePath, repoTask.repository.full_name);
 
     const result = await ghApiClient.getVariantAnalysisRepoResult(
       credentials,
@@ -82,32 +82,35 @@ export class VariantAnalysisResultsManager extends DisposableObject {
 
   public async loadResults(
     variantAnalysisId: number,
+    variantAnalysisStoragePath: string,
     repositoryFullName: string
   ): Promise<VariantAnalysisScannedRepositoryResult> {
     const result = this.cachedResults.get(createCacheKey(variantAnalysisId, repositoryFullName));
 
-    return result ?? await this.loadResultsIntoMemory(variantAnalysisId, repositoryFullName);
+    return result ?? await this.loadResultsIntoMemory(variantAnalysisId, variantAnalysisStoragePath, repositoryFullName);
   }
 
   private async loadResultsIntoMemory(
     variantAnalysisId: number,
+    variantAnalysisStoragePath: string,
     repositoryFullName: string,
   ): Promise<VariantAnalysisScannedRepositoryResult> {
-    const result = await this.loadResultsFromStorage(variantAnalysisId, repositoryFullName);
+    const result = await this.loadResultsFromStorage(variantAnalysisId, variantAnalysisStoragePath, repositoryFullName);
     this.cachedResults.set(createCacheKey(variantAnalysisId, repositoryFullName), result);
     this._onResultLoaded.fire(result);
     return result;
   }
 
   private async loadResultsFromStorage(
     variantAnalysisId: number,
+    variantAnalysisStoragePath: string,
     repositoryFullName: string,
   ): Promise<VariantAnalysisScannedRepositoryResult> {
-    if (!(await this.isVariantAnalysisRepoDownloaded(variantAnalysisId, repositoryFullName))) {
+    if (!(await this.isVariantAnalysisRepoDownloaded(variantAnalysisStoragePath, repositoryFullName))) {
       throw new Error('Variant analysis results not downloaded');
     }
 
-    const storageDirectory = this.getRepoStorageDirectory(variantAnalysisId, repositoryFullName);
+    const storageDirectory = this.getRepoStorageDirectory(variantAnalysisStoragePath, repositoryFullName);
 
     const repoTask: VariantAnalysisRepoTask = await fs.readJson(path.join(storageDirectory, VariantAnalysisResultsManager.REPO_TASK_FILENAME));
 
@@ -144,10 +147,10 @@ export class VariantAnalysisResultsManager extends DisposableObject {
   }
 
   private async isVariantAnalysisRepoDownloaded(
-    variantAnalysisId: number,
+    variantAnalysisStoragePath: string,
     repositoryFullName: string,
   ): Promise<boolean> {
-    return await fs.pathExists(this.getRepoStorageDirectory(variantAnalysisId, repositoryFullName));
+    return await fs.pathExists(this.getRepoStorageDirectory(variantAnalysisStoragePath, repositoryFullName));
   }
 
   private async readBqrsResults(filePath: string, fileLinkPrefix: string, sourceLocationPrefix: string): Promise<AnalysisRawResults> {
@@ -165,16 +168,9 @@ export class VariantAnalysisResultsManager extends DisposableObject {
     return processedSarif.alerts;
   }
 
-  private getStorageDirectory(variantAnalysisId: number): string {
-    return path.join(
-      this.storagePath,
-      `${variantAnalysisId}`
-    );
-  }
-
-  public getRepoStorageDirectory(variantAnalysisId: number, fullName: string): string {
+  public getRepoStorageDirectory(variantAnalysisStoragePath: string, fullName: string): string {
     return path.join(
-      this.getStorageDirectory(variantAnalysisId),
+      variantAnalysisStoragePath,
       fullName
     );
   }

extensions/ql-vscode/src/vscode-tests/cli-integration/remote-queries/variant-analysis-results-manager.test.ts

@@ -15,7 +15,9 @@ import { faker } from '@faker-js/faker';
 import * as ghApiClient from '../../../remote-queries/gh-api/gh-api-client';
 import { VariantAnalysisRepoTask } from '../../../remote-queries/gh-api/variant-analysis';
 
-describe(VariantAnalysisResultsManager.name, () => {
+describe(VariantAnalysisResultsManager.name, function() {
+  this.timeout(10000);
+
   let sandbox: sinon.SinonSandbox;
   let cli: CodeQLCliServer;
   let variantAnalysisId: number;
@@ -33,7 +35,7 @@ describe(VariantAnalysisResultsManager.name, () => {
     try {
       const extension = await extensions.getExtension<CodeQLExtensionInterface | Record<string, never>>('GitHub.vscode-codeql')!.activate();
       cli = extension.cliServer;
-      variantAnalysisResultsManager = new VariantAnalysisResultsManager(cli, storagePath, logger);
+      variantAnalysisResultsManager = new VariantAnalysisResultsManager(cli, logger);
     } catch (e) {
       fail(e as Error);
     }
@@ -45,12 +47,17 @@ describe(VariantAnalysisResultsManager.name, () => {
 
   describe('download', () => {
     let getOctokitStub: sinon.SinonStub;
+    let variantAnalysisStoragePath: string;
     const mockCredentials = {
       getOctokit: () => Promise.resolve({
         request: getOctokitStub
       })
     } as unknown as Credentials;
 
+    beforeEach(async () => {
+      variantAnalysisStoragePath = path.join(storagePath, variantAnalysisId.toString());
+    });
+
     describe('when the artifact_url is missing', async () => {
       it('should not try to download the result', async () => {
         const dummyRepoTask = createMockVariantAnalysisRepoTask();
@@ -60,7 +67,8 @@ describe(VariantAnalysisResultsManager.name, () => {
           await variantAnalysisResultsManager.download(
             mockCredentials,
             variantAnalysisId,
-            dummyRepoTask
+            dummyRepoTask,
+            variantAnalysisStoragePath
           );
 
           expect.fail('Expected an error to be thrown');
@@ -78,7 +86,7 @@ describe(VariantAnalysisResultsManager.name, () => {
       beforeEach(async () => {
         dummyRepoTask = createMockVariantAnalysisRepoTask();
 
-        storageDirectory = variantAnalysisResultsManager.getRepoStorageDirectory(variantAnalysisId, dummyRepoTask.repository.full_name);
+        storageDirectory = variantAnalysisResultsManager.getRepoStorageDirectory(variantAnalysisStoragePath, dummyRepoTask.repository.full_name);
         const sourceFilePath = path.join(__dirname, '../../../../src/vscode-tests/cli-integration/data/variant-analysis-results.zip');
         arrayBuffer = fs.readFileSync(sourceFilePath).buffer;
 
@@ -97,7 +105,8 @@ describe(VariantAnalysisResultsManager.name, () => {
         await variantAnalysisResultsManager.download(
           mockCredentials,
           variantAnalysisId,
-          dummyRepoTask
+          dummyRepoTask,
+          variantAnalysisStoragePath
         );
 
         expect(getVariantAnalysisRepoResultStub.calledOnce).to.be.true;
@@ -107,7 +116,8 @@ describe(VariantAnalysisResultsManager.name, () => {
         await variantAnalysisResultsManager.download(
           mockCredentials,
           variantAnalysisId,
-          dummyRepoTask
+          dummyRepoTask,
+          variantAnalysisStoragePath
         );
 
         expect(fs.existsSync(`${storageDirectory}/results.zip`)).to.be.true;
@@ -117,7 +127,8 @@ describe(VariantAnalysisResultsManager.name, () => {
         await variantAnalysisResultsManager.download(
           mockCredentials,
           variantAnalysisId,
-          dummyRepoTask
+          dummyRepoTask,
+          variantAnalysisStoragePath
         );
 
         expect(fs.existsSync(`${storageDirectory}/results/results.sarif`)).to.be.true;