Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Jan 31, 2024
Merged
35 changes: 34 additions & 1 deletion extensions/ql-vscode/src/common/files.ts
Original file line number Diff line number Diff line change
@@ -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";

/**
Expand Down Expand Up @@ -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 {
charisk marked this conversation as resolved.
Show resolved Hide resolved
charisk marked this conversation as resolved.
Show resolved Hide resolved
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;
}
10 changes: 5 additions & 5 deletions extensions/ql-vscode/src/common/ql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
79 changes: 79 additions & 0 deletions extensions/ql-vscode/src/variant-analysis/ql.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import { dirname } from "path";
charisk marked this conversation as resolved.
Show resolved Hide resolved
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this special case for one query? I think the logic below still works when there's a single file. Since there's not much below this I wonder if it's simpler to reduce the number of special cases, rather returning early and reducing the amount of code executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a slight difference when there is a single query. If the query is not in the workspace, we currently allow MRVA to run. So I wanted to keep that behaviour. For multiple queries it gets more complicated, because if they're not in a pack we want to find a common dir for them so they need to be in the workspace to avoid reaching places in the filesystem where we shouldn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the query is not in the workspace, we currently allow MRVA to run. So I wanted to keep that behaviour.

Why do we want this behaviour? To me it seems like an oversight and unlikely that anyone would be relying on this to run MRVA using query files outside of their workspace.

Would it not be better to have consistent behaviour when there's one query vs multiple queries?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could have opened a workspace and dragged a query file from somewhere else into VS Code. So I can imagine that happening, but definitely not often.

I agree it would be good to have the same behaviour but perhaps if we want to change it we should have a specific PR for that, with a CHANGELOG note. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A separate PR with a changelog note sounds good to me. Also getting eyes from more members of the team on whether this makes sense. I don't really have any strong opinions.

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.
charisk marked this conversation as resolved.
Show resolved Hide resolved
*/
function findQlPackRootForQueriesWithNoPack(
charisk marked this conversation as resolved.
Show resolved Hide resolved
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",
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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])
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
select 42, 3.14159, "hello world", true
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
name: test-queries
version: 0.0.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
select 42, 3.14159, "hello world", true
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
select 42, 3.14159, "hello world", true
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
name: test-queries
version: 0.0.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
select 42, 3.14159, "hello world", true
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
select 42, 3.14159, "hello world", true
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
select 42, 3.14159, "hello world", true
Loading
Loading