Improve handling of unknown QL pack roots for multi-query MRVAs

Author: charisk

extensions/ql-vscode/src/common/files.ts

@@ -1,5 +1,5 @@
 import { pathExists, stat, readdir, opendir } from "fs-extra";
-import { isAbsolute, join, relative, resolve } from "path";
+import { isAbsolute, join, relative, resolve, sep } from "path";
 import { tmpdir as osTmpdir } from "os";
 
 /**
@@ -132,3 +132,30 @@ export function isIOError(e: any): e is IOError {
 export function tmpdir(): string {
   return osTmpdir();
 }
+
+/**
+ * Finds the common parent directory of an arbitrary number of paths. If the paths are absolute,
+ * the result is also absolute. If the paths are relative, the result is relative.
+ * @param paths The array of paths.
+ * @returns The common parent directory of the paths.
+ */
+export function findCommonParentDir(...paths: string[]): string {
+  // Split each path into its components
+  const pathParts = paths.map((path) => path.split(sep));
+
+  let commonDir = "";
+  // Iterate over the components of the first path and checks if the same
+  // component exists at the same position in all the other paths. If it does,
+  // add the component to the common directory. If it doesn't, stop the
+  // iteration and returns the common directory found so far.
+  for (let i = 0; i < pathParts[0].length; i++) {
+    const part = pathParts[0][i];
+    if (pathParts.every((parts) => parts[i] === part)) {
+      commonDir = `${commonDir}${part}${sep}`;
+    } else {
+      break;
+    }
+  }
+
+  return commonDir;
+}

extensions/ql-vscode/src/common/ql.ts

@@ -32,16 +32,16 @@ export async function getQlPackFilePath(
  * Recursively find the directory containing qlpack.yml or codeql-pack.yml. If
  * no such directory is found, the directory containing the query file is returned.
  * @param queryFile The query file to start from.
- * @returns The path to the pack root.
+ * @returns The path to the pack root or undefined if it doesn't exist.
  */
-export async function findPackRoot(queryFile: string): Promise<string> {
+export async function findPackRoot(
+  queryFile: string,
+): Promise<string | undefined> {
   let dir = dirname(queryFile);
   while (!(await getQlPackFilePath(dir))) {
     dir = dirname(dir);
     if (isFileSystemRoot(dir)) {
-      // there is no qlpack.yml or codeql-pack.yml in this directory or any parent directory.
-      // just use the query file's directory as the pack root.
-      return dirname(queryFile);
+      return undefined;
     }
   }
 

extensions/ql-vscode/src/variant-analysis/ql.ts

@@ -0,0 +1,87 @@
+import { dirname } from "path";
+import { findCommonParentDir } from "../common/files";
+import { findPackRoot } from "../common/ql";
+
+/**
+ * This function finds the root directory of the QL pack that contains the provided query files.
+ * It handles several cases:
+ * - If no query files are provided, it throws an error.
+ * - If all query files are in the same QL pack, it returns the root directory of that pack.
+ * - If the query files are in different QL packs, it throws an error.
+ * - If some query files are in a QL pack and some aren't, it throws an error.
+ * - If none of the query files are in a QL pack, it returns the common parent directory of the query files. However,
+ *   if there are more than one query files and they're not in the same workspace folder, it throws an error.
+ *
+ * @param queryFiles - An array of file paths for the query files.
+ * @param workspaceFolders - An array of workspace folder paths.
+ * @returns The root directory of the QL pack that contains the query files, or the common parent directory of the query files.
+ */
+export async function findVariantAnalysisQlPackRoot(
+  queryFiles: string[],
+  workspaceFolders: string[],
+): Promise<string> {
+  if (queryFiles.length === 0) {
+    throw Error("No query files provided");
+  }
+
+  // Calculate the pack root for each query file
+  const packRoots: Array<string | undefined> = [];
+  for (const queryFile of queryFiles) {
+    const packRoot = await findPackRoot(queryFile);
+    packRoots.push(packRoot);
+  }
+
+  if (queryFiles.length === 1) {
+    return packRoots[0] ?? dirname(queryFiles[0]);
+  }
+
+  const uniquePackRoots = Array.from(new Set(packRoots));
+
+  if (uniquePackRoots.length > 1) {
+    if (uniquePackRoots.includes(undefined)) {
+      throw Error("Some queries are in a pack and some aren't");
+    } else {
+      throw Error("Some queries are in different packs");
+    }
+  }
+
+  if (uniquePackRoots[0] === undefined) {
+    return findQlPackRootForQueriesWithNoPack(queryFiles, workspaceFolders);
+  } else {
+    // All in the same pack, return that pack's root
+    return uniquePackRoots[0];
+  }
+}
+
+/**
+ * For queries that are not in a pack, a potential pack root is the
+ * common parent dir of all the queries. However, we only want to
+ * return this if all the queries are in the same workspace folder.
+ */
+function findQlPackRootForQueriesWithNoPack(
+  queryFiles: string[],
+  workspaceFolders: string[],
+): string {
+  const queryFileWorkspaceFolders = queryFiles.map((queryFile) =>
+    workspaceFolders.find((workspaceFolder) =>
+      queryFile.startsWith(workspaceFolder),
+    ),
+  );
+
+  // Check if any query file is not part of the workspace.
+  if (queryFileWorkspaceFolders.includes(undefined)) {
+    throw Error(
+      "Queries that are not part of a pack need to be part of the workspace",
+    );
+  }
+
+  // Check if query files are not in the same workspace folder.
+  if (new Set(queryFileWorkspaceFolders).size > 1) {
+    throw Error(
+      "Queries that are not part of a pack need to be in the same workspace folder",
+    );
+  }
+
+  // They're in the same workspace folder, so we can find a common parent dir.
+  return findCommonParentDir(...queryFiles);
+}

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

@@ -90,8 +90,10 @@ import { handleRequestError } from "./custom-errors";
 import { createMultiSelectionCommand } from "../common/vscode/selection-commands";
 import { askForLanguage, findLanguage } from "../codeql-cli/query-language";
 import type { QlPackDetails } from "./ql-pack-details";
-import { findPackRoot, getQlPackFilePath } from "../common/ql";
+import { getQlPackFilePath } from "../common/ql";
 import { tryGetQueryMetadata } from "../codeql-cli/query-metadata";
+import { getOnDiskWorkspaceFolders } from "../common/vscode/workspace-folders";
+import { findVariantAnalysisQlPackRoot } from "./ql";
 
 const maxRetryCount = 3;
 
@@ -312,19 +314,12 @@ export class VariantAnalysisManager
       throw new Error("Please select a .ql file to run as a variant analysis");
     }
 
-    const qlPackRootPath = await findPackRoot(queryFiles[0].fsPath);
+    const qlPackRootPath = await findVariantAnalysisQlPackRoot(
+      queryFiles.map((f) => f.fsPath),
+      getOnDiskWorkspaceFolders(),
+    );
     const qlPackFilePath = await getQlPackFilePath(qlPackRootPath);
 
-    // Make sure that all remaining queries have the same pack root
-    for (let i = 1; i < queryFiles.length; i++) {
-      const packRoot = await findPackRoot(queryFiles[i].fsPath);
-      if (packRoot !== qlPackRootPath) {
-        throw new Error(
-          "Please select queries that all belong to the same query pack",
-        );
-      }
-    }
-
     // Open popup to ask for language if not already hardcoded
     const language = qlPackFilePath
       ? await findLanguage(this.cliServer, queryFiles[0])

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/dir1/query1.ql

@@ -0,0 +1 @@
+select 42, 3.14159, "hello world", true

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/pack1/qlpack.yml

@@ -0,0 +1,2 @@
+name: test-queries
+version: 0.0.0

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/pack1/query1.ql

@@ -0,0 +1 @@
+select 42, 3.14159, "hello world", true

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/pack1/query2.ql

@@ -0,0 +1 @@
+select 42, 3.14159, "hello world", true

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/pack2/qlpack.yml

@@ -0,0 +1,2 @@
+name: test-queries
+version: 0.0.0

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/pack2/query1.ql

@@ -0,0 +1 @@
+select 42, 3.14159, "hello world", true

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace1/query1.ql

@@ -0,0 +1 @@
+select 42, 3.14159, "hello world", true

extensions/ql-vscode/test/data/variant-analysis-query-packs/workspace2/query1.ql

@@ -0,0 +1 @@
+select 42, 3.14159, "hello world", true

extensions/ql-vscode/test/unit-tests/common/files.test.ts

@@ -2,6 +2,7 @@ import { join } from "path";
 
 import {
   containsPath,
+  findCommonParentDir,
   gatherQlFiles,
   getDirectoryNamesInsidePath,
   pathsEqual,
@@ -417,3 +418,29 @@ describe("walkDirectory", () => {
     expect(files.sort()).toEqual([file1, file2, file3, file4, file5, file6]);
   });
 });
+
+describe("findCommonParentDir", () => {
+  it("should find the common parent dir for multiple paths with common parent", () => {
+    const paths = ["/foo/bar/baz", "/foo/bar/qux", "/foo/bar/quux"];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqual("/foo/bar/");
+  });
+
+  it("should return the root if paths have no common parent other than root", () => {
+    const paths = ["/foo/bar/baz", "/qux/quux/corge", "/grault/garply"];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqual("/");
+  });
+
+  it("should return empty string for relative paths with no common parent", () => {
+    const paths = ["foo/bar/baz", "qux/quux/corge", "grault/garply"];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqual("");
+  });
+});

extensions/ql-vscode/test/unit-tests/variant-analysis/ql.test.ts

@@ -0,0 +1,134 @@
+import { join } from "path";
+import { findVariantAnalysisQlPackRoot } from "../../../src/variant-analysis/ql";
+import "../../matchers/toEqualPath";
+
+describe("findVariantAnalysisQlPackRoot", () => {
+  const testDataDir = join(
+    __dirname,
+    "../../data/variant-analysis-query-packs",
+  );
+
+  const workspaceFolders = [
+    getFullPath("workspace1"),
+    getFullPath("workspace2"),
+  ];
+
+  function getFullPath(relativePath: string) {
+    return join(testDataDir, relativePath);
+  }
+
+  it("should throw an error if no query files are provided", async () => {
+    await expect(
+      findVariantAnalysisQlPackRoot([], workspaceFolders),
+    ).rejects.toThrow("No query files provided");
+  });
+
+  it("should return the pack root of a single query in a pack", async () => {
+    const queryFiles = [getFullPath("workspace1/pack1/query1.ql")];
+
+    const packRoot = await findVariantAnalysisQlPackRoot(
+      queryFiles,
+      workspaceFolders,
+    );
+
+    expect(packRoot).toEqualPath(getFullPath("workspace1/pack1"));
+  });
+
+  it("should return the pack root of a single query not in a pack", async () => {
+    const queryFiles = [getFullPath("workspace1/query1.ql")];
+
+    const packRoot = await findVariantAnalysisQlPackRoot(
+      queryFiles,
+      workspaceFolders,
+    );
+
+    expect(packRoot).toEqualPath(getFullPath("workspace1"));
+  });
+
+  it("should return the pack root of a single query not in a pack or workspace", async () => {
+    const queryFiles = [getFullPath("dir1/query1.ql")];
+
+    const packRoot = await findVariantAnalysisQlPackRoot(
+      queryFiles,
+      workspaceFolders,
+    );
+
+    expect(packRoot).toEqualPath(getFullPath("dir1"));
+  });
+
+  it("should throw an error if some queries are in a pack and some are not", async () => {
+    const queryFiles = [
+      getFullPath("workspace1/pack1/query1.ql"),
+      getFullPath("workspace1/query1.ql"),
+    ];
+
+    await expect(
+      findVariantAnalysisQlPackRoot(queryFiles, workspaceFolders),
+    ).rejects.toThrow("Some queries are in a pack and some aren't");
+  });
+
+  it("should throw an error if queries are in different packs", async () => {
+    const queryFiles = [
+      getFullPath("workspace1/pack1/query1.ql"),
+      getFullPath("workspace1/pack2/query1.ql"),
+    ];
+
+    await expect(
+      findVariantAnalysisQlPackRoot(queryFiles, workspaceFolders),
+    ).rejects.toThrow("Some queries are in different packs");
+  });
+
+  it("should throw an error if query files are not in a pack and in different workspace folders", async () => {
+    const queryFiles = [
+      getFullPath("workspace1/query1.ql"),
+      getFullPath("workspace2/query1.ql"),
+    ];
+
+    await expect(
+      findVariantAnalysisQlPackRoot(queryFiles, workspaceFolders),
+    ).rejects.toThrow(
+      "Queries that are not part of a pack need to be in the same workspace folder",
+    );
+  });
+
+  it("should throw an error if query files are not part of any workspace folder", async () => {
+    const queryFiles = [
+      getFullPath("workspace3/query1.ql"),
+      getFullPath("workspace3/query2.ql"),
+    ];
+
+    await expect(
+      findVariantAnalysisQlPackRoot(queryFiles, workspaceFolders),
+    ).rejects.toThrow(
+      "Queries that are not part of a pack need to be part of the workspace",
+    );
+  });
+
+  it("should return the common parent directory if no queries are in a pack", async () => {
+    const queryFiles = [
+      getFullPath("workspace1/query1.ql"),
+      getFullPath("workspace1/dir1/query1.ql"),
+    ];
+
+    const result = await findVariantAnalysisQlPackRoot(
+      queryFiles,
+      workspaceFolders,
+    );
+
+    expect(result).toEqualPath(getFullPath("workspace1"));
+  });
+
+  it("should return the pack root if all query files are in the same pack", async () => {
+    const queryFiles = [
+      getFullPath("workspace1/pack1/query1.ql"),
+      getFullPath("workspace1/pack1/query2.ql"),
+    ];
+
+    const result = await findVariantAnalysisQlPackRoot(
+      queryFiles,
+      workspaceFolders,
+    );
+
+    expect(result).toEqualPath(getFullPath("workspace1/pack1"));
+  });
+});