Merge pull request #1774 from github/aeisenberg/fix-mrva-packs

Allow synthetic variant analysis packs to handle `${workspace}`

.gitignore

@@ -16,7 +16,6 @@ artifacts/
 # Visual Studio workspace state
 .vs/
 
-# Rush files
-/common/temp/**
-package-deps.json
-**/.rush/temp
+# CodeQL metadata
+.cache/
+.codeql/

extensions/ql-vscode/src/cli.ts

@@ -1605,6 +1605,11 @@ export class CliVersionConstraint {
    */
   public static CLI_VERSION_WITH_NEW_QUERY_SERVER = new SemVer("2.11.1");
 
+  /**
+   * CLI version that supports `${workspace}` references in qlpack.yml files.
+   */
+  public static CLI_VERSION_WITH_WORKSPACE_RFERENCES = new SemVer("2.11.3");
+
   constructor(private readonly cli: CodeQLCliServer) {
     /**/
   }
@@ -1734,4 +1739,10 @@ export class CliVersionConstraint {
       CliVersionConstraint.CLI_VERSION_WITH_NEW_QUERY_SERVER,
     );
   }
+
+  async supportsWorkspaceReferences() {
+    return this.isVersionAtLeast(
+      CliVersionConstraint.CLI_VERSION_WITH_WORKSPACE_RFERENCES,
+    );
+  }
 }

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

@@ -21,7 +21,7 @@ import {
 import { ProgressCallback, UserCancellationException } from "../commandRunner";
 import { RequestError } from "@octokit/types/dist-types";
 import { QueryMetadata } from "../pure/interface-types";
-import { REPO_REGEX } from "../pure/helpers-pure";
+import { getErrorMessage, REPO_REGEX } from "../pure/helpers-pure";
 import * as ghApiClient from "./gh-api/gh-api-client";
 import {
   getRepositorySelection,
@@ -99,6 +99,8 @@ async function generateQueryPack(
 
     void logger.log(`Copied ${copiedCount} files to ${queryPackDir}`);
 
+    await fixPackFile(queryPackDir, packRelativePath);
+
     language = await findLanguage(cliServer, Uri.file(targetQueryFileName));
   } else {
     // open popup to ask for language if not already hardcoded
@@ -115,6 +117,7 @@ async function generateQueryPack(
       dependencies: {
         [`codeql/${language}-all`]: "*",
       },
+      defaultSuite: generateDefaultSuite(packRelativePath),
     };
     await fs.writeFile(
       path.join(queryPackDir, "qlpack.yml"),
@@ -125,8 +128,6 @@ async function generateQueryPack(
     throw new UserCancellationException("Could not determine language.");
   }
 
-  await ensureNameAndSuite(queryPackDir, packRelativePath);
-
   // Clear the cliServer cache so that the previous qlpack text is purged from the CLI.
   await cliServer.clearCache();
 
@@ -298,25 +299,41 @@ export async function prepareRemoteQueryRun(
 }
 
 /**
- * Updates the default suite of the query pack. This is used to ensure
- * only the specified query is run.
+ * Fixes the qlpack.yml file to be correct in the context of the MRVA request.
  *
- * Also, ensure the query pack name is set to the name expected by the server.
+ * Performs the following fixes:
+ *
+ * - Updates the default suite of the query pack. This is used to ensure
+ *   only the specified query is run.
+ * - Ensures the query pack name is set to the name expected by the server.
+ * - Removes any `${workspace}` version references from the qlpack.yml file. Converts them
+ *   to `*` versions.
  *
  * @param queryPackDir The directory containing the query pack
  * @param packRelativePath The relative path to the query pack from the root of the query pack
  */
-async function ensureNameAndSuite(
+async function fixPackFile(
   queryPackDir: string,
   packRelativePath: string,
 ): Promise<void> {
   const packPath = path.join(queryPackDir, "qlpack.yml");
   const qlpack = yaml.load(await fs.readFile(packPath, "utf8")) as QlPack;
-  delete qlpack.defaultSuiteFile;
 
+  // update pack name
   qlpack.name = QUERY_PACK_NAME;
 
-  qlpack.defaultSuite = [
+  // update default suite
+  delete qlpack.defaultSuiteFile;
+  qlpack.defaultSuite = generateDefaultSuite(packRelativePath);
+
+  // remove any ${workspace} version references
+  removeWorkspaceRefs(qlpack);
+
+  await fs.writeFile(packPath, yaml.dump(qlpack));
+}
+
+function generateDefaultSuite(packRelativePath: string) {
+  return [
     {
       description: "Query suite for variant analysis",
     },
@@ -324,7 +341,6 @@ async function ensureNameAndSuite(
       query: packRelativePath.replace(/\\/g, "/"),
     },
   ];
-  await fs.writeFile(packPath, yaml.dump(qlpack));
 }
 
 export function getQueryName(
@@ -385,13 +401,22 @@ export async function getControllerRepo(
       fullName: controllerRepo.full_name,
       private: controllerRepo.private,
     };
-  } catch (e: any) {
+  } catch (e) {
     if ((e as RequestError).status === 404) {
       throw new Error(`Controller repository "${owner}/${repo}" not found`);
     } else {
       throw new Error(
-        `Error getting controller repository "${owner}/${repo}": ${e.message}`,
+        `Error getting controller repository "${owner}/${repo}": ${getErrorMessage(
+          e,
+        )}`,
       );
     }
   }
 }
+export function removeWorkspaceRefs(qlpack: QlPack) {
+  for (const [key, value] of Object.entries(qlpack.dependencies || {})) {
+    if (value === "${workspace}") {
+      qlpack.dependencies[key] = "*";
+    }
+  }
+}

extensions/ql-vscode/src/vscode-tests/cli-integration/data-remote-qlpack-nested/qlpack.yml

@@ -1,4 +1,5 @@
 name: github/remote-query-pack
 version: 0.0.0
 dependencies:
+  # The workspace reference will be removed before creating the MRVA pack.
   codeql/javascript-all: '*'

extensions/ql-vscode/src/vscode-tests/cli-integration/data-remote-qlpack/qlpack.yml

@@ -1,4 +1,4 @@
 name: github/remote-query-pack
 version: 0.0.0
 dependencies:
-  codeql/javascript-all: '*'
+  codeql/javascript-all: '${workspace}'

extensions/ql-vscode/src/vscode-tests/cli-integration/global.helper.ts

@@ -1,5 +1,6 @@
 import * as path from "path";
 import * as tmp from "tmp";
+import * as yaml from "js-yaml";
 import * as fs from "fs-extra";
 import fetch from "node-fetch";
 
@@ -9,6 +10,8 @@ import { CodeQLExtensionInterface } from "../../extension";
 import { DatabaseManager } from "../../databases";
 import { getTestSetting } from "../test-config";
 import { CUSTOM_CODEQL_PATH_SETTING } from "../../config";
+import { CodeQLCliServer } from "../../cli";
+import { removeWorkspaceRefs } from "../../remote-queries/run-remote-query";
 
 // This file contains helpers shared between actual tests.
 
@@ -122,3 +125,52 @@ export async function cleanDatabases(databaseManager: DatabaseManager) {
     await commands.executeCommand("codeQLDatabases.removeDatabase", item);
   }
 }
+
+/**
+ * Conditionally removes `${workspace}` references from a qlpack.yml file.
+ * CLI versions earlier than 2.11.3 do not support `${workspace}` references in the dependencies block.
+ * If workspace references are removed, the qlpack.yml file is re-written to disk
+ * without the `${workspace}` references and the original dependencies are returned.
+ *
+ * @param qlpackFileWithWorkspaceRefs The qlpack.yml file with workspace refs
+ * @param cli The cli to use to check version constraints
+ * @returns The original dependencies with workspace refs, or undefined if the CLI version supports workspace refs and no changes were made
+ */
+export async function fixWorkspaceReferences(
+  qlpackFileWithWorkspaceRefs: string,
+  cli: CodeQLCliServer,
+): Promise<Record<string, string> | undefined> {
+  if (!(await cli.cliConstraints.supportsWorkspaceReferences())) {
+    // remove the workspace references from the qlpack
+    const qlpack = yaml.load(
+      fs.readFileSync(qlpackFileWithWorkspaceRefs, "utf8"),
+    );
+    const originalDeps = { ...qlpack.dependencies };
+    removeWorkspaceRefs(qlpack);
+    fs.writeFileSync(qlpackFileWithWorkspaceRefs, yaml.dump(qlpack));
+    return originalDeps;
+  }
+  return undefined;
+}
+
+/**
+ * Restores the original dependencies with `${workspace}` refs to a qlpack.yml file.
+ * See `fixWorkspaceReferences` for more details.
+ *
+ * @param qlpackFileWithWorkspaceRefs The qlpack.yml file to restore workspace refs
+ * @param originalDeps the original dependencies with workspace refs or undefined if
+ *  no changes were made.
+ */
+export async function restoreWorkspaceReferences(
+  qlpackFileWithWorkspaceRefs: string,
+  originalDeps?: Record<string, string>,
+) {
+  if (!originalDeps) {
+    return;
+  }
+  const qlpack = yaml.load(
+    fs.readFileSync(qlpackFileWithWorkspaceRefs, "utf8"),
+  );
+  qlpack.dependencies = originalDeps;
+  fs.writeFileSync(qlpackFileWithWorkspaceRefs, yaml.dump(qlpack));
+}

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

@@ -30,6 +30,10 @@ import { RemoteQueriesSubmission } from "../../../remote-queries/shared/remote-q
 import { readBundledPack } from "../../utils/bundled-pack-helpers";
 import { RemoteQueriesManager } from "../../../remote-queries/remote-queries-manager";
 import { Credentials } from "../../../authentication";
+import {
+  fixWorkspaceReferences,
+  restoreWorkspaceReferences,
+} from "../global.helper";
 
 describe("Remote queries", function () {
   const baseDir = path.join(
@@ -37,6 +41,10 @@ describe("Remote queries", function () {
     "../../../../src/vscode-tests/cli-integration",
   );
 
+  const qlpackFileWithWorkspaceRefs = getFile(
+    "data-remote-qlpack/qlpack.yml",
+  ).fsPath;
+
   let sandbox: sinon.SinonSandbox;
 
   // up to 3 minutes per test
@@ -51,6 +59,8 @@ describe("Remote queries", function () {
   let logger: any;
   let remoteQueriesManager: RemoteQueriesManager;
 
+  let originalDeps: Record<string, string> | undefined;
+
   // use `function` so we have access to `this`
   beforeEach(async function () {
     sandbox = sinon.createSandbox();
@@ -69,13 +79,6 @@ describe("Remote queries", function () {
     }
 
     ctx = createMockExtensionContext();
-    logger = new OutputChannelLogger("test-logger");
-    remoteQueriesManager = new RemoteQueriesManager(
-      ctx,
-      cli,
-      "fake-storage-dir",
-      logger,
-    );
 
     if (!(await cli.cliConstraints.supportsRemoteQueries())) {
       console.log(
@@ -84,6 +87,14 @@ describe("Remote queries", function () {
       this.skip();
     }
 
+    logger = new OutputChannelLogger("test-logger");
+    remoteQueriesManager = new RemoteQueriesManager(
+      ctx,
+      cli,
+      "fake-storage-dir",
+      logger,
+    );
+
     cancellationTokenSource = new CancellationTokenSource();
 
     progress = sandbox.spy();
@@ -120,10 +131,17 @@ describe("Remote queries", function () {
         }),
     } as unknown as Credentials;
     sandbox.stub(Credentials, "initialize").resolves(mockCredentials);
+
+    // Only new version support `${workspace}` in qlpack.yml
+    originalDeps = await fixWorkspaceReferences(
+      qlpackFileWithWorkspaceRefs,
+      cli,
+    );
   });
 
   afterEach(async () => {
     sandbox.restore();
+    await restoreWorkspaceReferences(qlpackFileWithWorkspaceRefs, originalDeps);
   });
 
   describe("runRemoteQuery", () => {

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

@@ -24,7 +24,11 @@ import * as path from "path";
 
 import { VariantAnalysisManager } from "../../../remote-queries/variant-analysis-manager";
 import { CliVersionConstraint, CodeQLCliServer } from "../../../cli";
-import { storagePath } from "../global.helper";
+import {
+  fixWorkspaceReferences,
+  restoreWorkspaceReferences,
+  storagePath,
+} from "../global.helper";
 import { VariantAnalysisResultsManager } from "../../../remote-queries/variant-analysis-results-manager";
 import { createMockVariantAnalysis } from "../../factories/remote-queries/shared/variant-analysis";
 import * as VariantAnalysisModule from "../../../remote-queries/shared/variant-analysis";
@@ -66,6 +70,7 @@ describe("Variant Analysis Manager", async function () {
   let getVariantAnalysisRepoStub: sinon.SinonStub;
   let getVariantAnalysisRepoResultStub: sinon.SinonStub;
   let variantAnalysisResultsManager: VariantAnalysisResultsManager;
+  let originalDeps: Record<string, string> | undefined;
 
   beforeEach(async () => {
     sandbox = sinon.createSandbox();
@@ -126,6 +131,10 @@ describe("Variant Analysis Manager", async function () {
       __dirname,
       "../../../../src/vscode-tests/cli-integration",
     );
+    const qlpackFileWithWorkspaceRefs = getFile(
+      "data-remote-qlpack/qlpack.yml",
+    ).fsPath;
+
     function getFile(file: string): Uri {
       return Uri.file(path.join(baseDir, file));
     }
@@ -177,6 +186,19 @@ describe("Variant Analysis Manager", async function () {
       await setRemoteRepositoryLists({
         "vscode-codeql": ["github/vscode-codeql"],
       });
+
+      // Only new version support `${workspace}` in qlpack.yml
+      originalDeps = await fixWorkspaceReferences(
+        qlpackFileWithWorkspaceRefs,
+        cli,
+      );
+    });
+
+    afterEach(async () => {
+      await restoreWorkspaceReferences(
+        qlpackFileWithWorkspaceRefs,
+        originalDeps,
+      );
     });
 
     it("should run a variant analysis that is part of a qlpack", async () => {

extensions/ql-vscode/src/vscode-tests/test-config.ts

@@ -106,9 +106,11 @@ export const getTestSetting = (
 };
 
 export const testConfigHelper = async (mocha: Mocha) => {
+  // Allow extra time to read settings. Sometimes this can time out.
+  mocha.timeout(20000);
+
   // Read in all current settings
   await Promise.all(TEST_SETTINGS.map((setting) => setting.initialSetup()));
-
   mocha.rootHooks({
     async beforeEach() {
       // Reset the settings to their initial values before each test