Improve handling of unknown QL pack roots for multi-query MRVAs (#3289)

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 { dirname, isAbsolute, join, relative, resolve } from "path";
 import { tmpdir as osTmpdir } from "os";
 
 /**
@@ -132,3 +132,36 @@ export function isIOError(e: any): e is IOError {
 export function tmpdir(): string {
   return osTmpdir();
 }
+
+/**
+ * Finds the common parent directory of an arbitrary number of absolute paths. The result
+ * will be an absolute path.
+ * @param paths The array of paths.
+ * @returns The common parent directory of the paths.
+ */
+export function findCommonParentDir(...paths: string[]): string {
+  if (paths.length === 0) {
+    throw new Error("At least one path must be provided");
+  }
+  if (paths.some((path) => !isAbsolute(path))) {
+    throw new Error("All paths must be absolute");
+  }
+
+  paths = paths.map((path) => normalizePath(path));
+
+  let commonDir = paths[0];
+  while (!paths.every((path) => containsPath(commonDir, path))) {
+    if (isTopLevelPath(commonDir)) {
+      throw new Error(
+        "Reached filesystem root and didn't find a common parent directory",
+      );
+    }
+    commonDir = dirname(commonDir);
+  }
+
+  return commonDir;
+}
+
+function isTopLevelPath(path: string): boolean {
+  return dirname(path) === path;
+}

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,79 @@
+import { dirname } from "path";
+import { containsPath, 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 commonParentDir = findCommonParentDir(...queryFiles);
+
+  // Check that all queries are in a workspace folder (the same one),
+  // so that we don't return a pack root that's outside the workspace.
+  // This is to avoid accessing files outside the workspace folders.
+  for (const workspaceFolder of workspaceFolders) {
+    if (containsPath(workspaceFolder, commonParentDir)) {
+      return commonParentDir;
+    }
+  }
+
+  throw Error(
+    "All queries must be within the workspace and within the same workspace root",
+  );
+}

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

@@ -1,7 +1,8 @@
-import { join } from "path";
+import { join, parse } from "path";
 
 import {
   containsPath,
+  findCommonParentDir,
   gatherQlFiles,
   getDirectoryNamesInsidePath,
   pathsEqual,
@@ -11,6 +12,7 @@ import {
 import type { DirResult } from "tmp";
 import { dirSync } from "tmp";
 import { ensureDirSync, symlinkSync, writeFileSync } from "fs-extra";
+import "../../matchers/toEqualPath";
 
 describe("files", () => {
   const dataDir = join(__dirname, "../../data");
@@ -62,9 +64,29 @@ describe("files", () => {
       const file4 = join(dataDir, "multiple-result-sets.ql");
       const file5 = join(dataDir, "query.ql");
 
+      const vaDir = join(dataDir, "variant-analysis-query-packs");
+      const file6 = join(vaDir, "workspace1", "dir1", "query1.ql");
+      const file7 = join(vaDir, "workspace1", "pack1", "query1.ql");
+      const file8 = join(vaDir, "workspace1", "pack1", "query2.ql");
+      const file9 = join(vaDir, "workspace1", "pack2", "query1.ql");
+      const file10 = join(vaDir, "workspace1", "query1.ql");
+      const file11 = join(vaDir, "workspace2", "query1.ql");
+
       const result = await gatherQlFiles([dataDir]);
       expect(result.sort()).toEqual([
-        [file1, file2, file3, file4, file5],
+        [
+          file1,
+          file2,
+          file3,
+          file4,
+          file5,
+          file6,
+          file7,
+          file8,
+          file9,
+          file10,
+          file11,
+        ],
         true,
       ]);
     });
@@ -88,10 +110,30 @@ describe("files", () => {
       const file4 = join(dataDir, "multiple-result-sets.ql");
       const file5 = join(dataDir, "query.ql");
 
+      const vaDir = join(dataDir, "variant-analysis-query-packs");
+      const file6 = join(vaDir, "workspace1", "dir1", "query1.ql");
+      const file7 = join(vaDir, "workspace1", "pack1", "query1.ql");
+      const file8 = join(vaDir, "workspace1", "pack1", "query2.ql");
+      const file9 = join(vaDir, "workspace1", "pack2", "query1.ql");
+      const file10 = join(vaDir, "workspace1", "query1.ql");
+      const file11 = join(vaDir, "workspace2", "query1.ql");
+
       const result = await gatherQlFiles([file1, dataDir, file3, file4, file5]);
       result[0].sort();
       expect(result.sort()).toEqual([
-        [file1, file2, file3, file4, file5],
+        [
+          file1,
+          file2,
+          file3,
+          file4,
+          file5,
+          file6,
+          file7,
+          file8,
+          file9,
+          file10,
+          file11,
+        ],
         true,
       ]);
     });
@@ -417,3 +459,125 @@ describe("walkDirectory", () => {
     expect(files.sort()).toEqual([file1, file2, file3, file4, file5, file6]);
   });
 });
+
+describe("findCommonParentDir", () => {
+  const rootDir = parse(process.cwd()).root;
+
+  it("should fail if not all paths are not absolute", async () => {
+    const paths = [
+      join("foo", "bar", "baz"),
+      join("/foo", "bar", "qux"),
+      join("/foo", "bar", "quux"),
+    ];
+
+    expect(() => findCommonParentDir(...paths)).toThrow(
+      "All paths must be absolute",
+    );
+  });
+
+  it("should fail if no path are provided", async () => {
+    expect(() => findCommonParentDir()).toThrow(
+      "At least one path must be provided",
+    );
+  });
+
+  it("should find the common parent dir for multiple paths with common parent", () => {
+    const paths = [
+      join("/foo", "bar", "baz"),
+      join("/foo", "bar", "qux"),
+      join("/foo", "bar", "quux"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(join("/foo", "bar"));
+  });
+
+  it("should return empty path if paths have no common parent", () => {
+    const paths = [
+      join("/foo", "bar", "baz"),
+      join("/qux", "quux", "corge"),
+      join("/grault", "garply"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(rootDir);
+  });
+
+  it("should handle a mix of dirs and files", async () => {
+    const paths = [
+      join("/foo", "bar", "baz"),
+      join("/foo", "bar", "qux.ql"),
+      join("/foo", "bar", "quux"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(join("/foo", "bar"));
+  });
+
+  it("should handle dirs that have the same name", async () => {
+    const paths = [
+      join("/foo", "foo", "bar"),
+      join("/foo", "foo", "baz"),
+      join("/foo", "foo"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(join("/foo", "foo"));
+  });
+
+  it("should handle dirs that have the same subdir structure but different base path", async () => {
+    const paths = [
+      join("/foo", "bar"),
+      join("/bar", "foo", "bar"),
+      join("/foo", "foo", "bar"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(rootDir);
+  });
+
+  it("should handle a single path", async () => {
+    const paths = [join("/foo", "bar", "baz")];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(join("/foo", "bar", "baz"));
+  });
+
+  it("should return the same path if all paths are identical", () => {
+    const paths = [
+      join("/foo", "bar", "baz"),
+      join("/foo", "bar", "baz"),
+      join("/foo", "bar", "baz"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(join("/foo", "bar", "baz"));
+  });
+
+  it("should return the directory path if paths only differ by the file extension", () => {
+    const paths = [
+      join("/foo", "bar", "baz.txt"),
+      join("/foo", "bar", "baz.jpg"),
+      join("/foo", "bar", "baz.pdf"),
+    ];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(join("/foo", "bar"));
+  });
+
+  it("should handle empty paths", () => {
+    const paths = ["/", "/", "/"];
+
+    const commonDir = findCommonParentDir(...paths);
+
+    expect(commonDir).toEqualPath(rootDir);
+  });
+});

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(
+      "All queries must be within the workspace and within the same workspace root",
+    );
+  });
+
+  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(
+      "All queries must be within the workspace and within the same workspace root",
+    );
+  });
+
+  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"));
+  });
+});

extensions/ql-vscode/test/vscode-tests/cli-integration/packaging/packaging.test.ts

@@ -127,6 +127,9 @@ describe("Packaging commands", () => {
         expect.objectContaining({
           label: "semmle/targets-extension",
         }),
+        expect.objectContaining({
+          label: "test-queries",
+        }),
       ],
       expect.anything(),
     );