Skip to content

CodeQL model editor: user should be able to stop modeling before the main editor view is open #3189

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 5, 2024
Merged

CodeQL model editor: user should be able to stop modeling before the main editor view is open #3189

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6a65094
Make preparation progress bar cancellable
norascheuch Jan 3, 2024
0b27912
Make model editor open progress bar cancellable
norascheuch Jan 3, 2024
ad8dc1e
Update Changelog
norascheuch Jan 4, 2024
eb59ead
Catch request cancelled error
norascheuch Jan 4, 2024
745544c
Use error type and error code
norascheuch Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions extensions/ql-vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
- Avoid showing a popup when hovering over source elements in database source files. [#3125](https://github.com/github/vscode-codeql/pull/3125)
- Add comparison of alerts when comparing query results. This allows viewing path explanations for differences in alerts. [#3113](https://github.com/github/vscode-codeql/pull/3113)
- Fix a bug where the CodeQL CLI and variant analysis results were corrupted after extraction in VS Code Insiders. [#3151](https://github.com/github/vscode-codeql/pull/3151) & [#3152](https://github.com/github/vscode-codeql/pull/3152)
- Add option to cancel opening the model editor. [#3189](https://github.com/github/vscode-codeql/pull/3189)

## 1.11.0 - 13 December 2023

Expand Down
15 changes: 13 additions & 2 deletions extensions/ql-vscode/src/model-editor/extension-pack-picker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
import { join } from "path";
import { outputFile, pathExists, readFile } from "fs-extra";
import { dump as dumpYaml, load as loadYaml } from "js-yaml";
import { Uri } from "vscode";
import { CancellationToken, Uri } from "vscode";
import Ajv from "ajv";
import { CodeQLCliServer } from "../codeql-cli/cli";
import { getOnDiskWorkspaceFolders } from "../common/vscode/workspace-folders";
import { ProgressCallback } from "../common/vscode/progress";
import {
ProgressCallback,
UserCancellationException,
} from "../common/vscode/progress";
import { DatabaseItem } from "../databases/local-databases";
import { getQlPackPath, QLPACK_FILENAMES } from "../common/ql";
import { getErrorMessage } from "../common/helpers-pure";
Expand All @@ -31,6 +34,7 @@ export async function pickExtensionPack(
modelConfig: ModelConfig,
logger: NotificationLogger,
progress: ProgressCallback,
token: CancellationToken,
maxStep: number,
): Promise<ExtensionPack | undefined> {
progress({
Expand All @@ -50,6 +54,13 @@ export async function pickExtensionPack(
true,
);

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Open Model editor action cancelled.",
true,
);
}

progress({
message: "Creating extension pack...",
step: 2,
Expand Down
23 changes: 21 additions & 2 deletions extensions/ql-vscode/src/model-editor/model-editor-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@ import { DatabaseItem, DatabaseManager } from "../databases/local-databases";
import { ensureDir } from "fs-extra";
import { join } from "path";
import { App } from "../common/app";
import { withProgress } from "../common/vscode/progress";
import {
UserCancellationException,
withProgress,
} from "../common/vscode/progress";
import { pickExtensionPack } from "./extension-pack-picker";
import { showAndLogErrorMessage } from "../common/logging";
import { dir } from "tmp-promise";
Expand Down Expand Up @@ -159,7 +162,7 @@ export class ModelEditorModule extends DisposableObject {
}

return withProgress(
async (progress) => {
async (progress, token) => {
const maxStep = 4;

if (!(await this.cliServer.cliConstraints.supportsQlpacksKind())) {
Expand All @@ -176,6 +179,7 @@ export class ModelEditorModule extends DisposableObject {
this.modelConfig,
this.app.logger,
progress,
token,
maxStep,
);
if (!modelFile) {
Expand All @@ -188,6 +192,13 @@ export class ModelEditorModule extends DisposableObject {
maxStep,
});

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Open Model editor action cancelled.",
true,
);
}

// Create new temporary directory for query files and pack dependencies
const { path: queryDir, cleanup: cleanupQueryDir } = await dir({
unsafeCleanup: true,
Expand All @@ -211,6 +222,13 @@ export class ModelEditorModule extends DisposableObject {
maxStep,
});

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Open Model editor action cancelled.",
true,
);
}

// Check again just before opening the editor to ensure no model editor has been opened between
// our first check and now.
if (this.modelingStore.isDbOpen(db.databaseUri.toString())) {
Expand Down Expand Up @@ -253,6 +271,7 @@ export class ModelEditorModule extends DisposableObject {
},
{
title: "Opening CodeQL Model Editor",
cancellable: true,
},
);
}
Expand Down
33 changes: 32 additions & 1 deletion extensions/ql-vscode/src/model-editor/model-editor-queries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ import {
import { CancellationToken } from "vscode";
import { CodeQLCliServer } from "../codeql-cli/cli";
import { DatabaseItem } from "../databases/local-databases";
import { ProgressCallback } from "../common/vscode/progress";
import {
ProgressCallback,
UserCancellationException,
} from "../common/vscode/progress";
import { redactableError } from "../common/errors";
import { telemetryListener } from "../common/vscode/telemetry";
import { join } from "path";
Expand Down Expand Up @@ -89,6 +92,13 @@ export async function runModelEditorQueries(
// For a reference of what this should do in the future, see the previous implementation in
// https://github.com/github/vscode-codeql/blob/089d3566ef0bc67d9b7cc66e8fd6740b31c1c0b0/extensions/ql-vscode/src/data-extensions-editor/external-api-usage-query.ts#L33-L72

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Run model editor queries cancelled.",
true,
);
}

progress({
message: "Resolving QL packs",
step: 1,
Expand All @@ -99,6 +109,13 @@ export async function runModelEditorQueries(
await cliServer.resolveQlpacks(additionalPacks, true),
);

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Run model editor queries cancelled.",
true,
);
}

progress({
message: "Resolving query",
step: 2,
Expand Down Expand Up @@ -143,6 +160,13 @@ export async function runModelEditorQueries(
return;
}

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Run model editor queries cancelled.",
true,
);
}

// Read the results and covert to internal representation
progress({
message: "Decoding results",
Expand All @@ -159,6 +183,13 @@ export async function runModelEditorQueries(
return;
}

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Run model editor queries cancelled.",
true,
);
}

progress({
message: "Finalizing results",
step: 1950,
Expand Down
30 changes: 24 additions & 6 deletions extensions/ql-vscode/src/model-editor/model-editor-view.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
CancellationToken,
CancellationTokenSource,
Tab,
TabInputWebview,
Expand All @@ -14,7 +15,11 @@ import {
FromModelEditorMessage,
ToModelEditorMessage,
} from "../common/interface-types";
import { ProgressCallback, withProgress } from "../common/vscode/progress";
import {
ProgressCallback,
UserCancellationException,
withProgress,
} from "../common/vscode/progress";
import { QueryRunner } from "../query-server";
import {
showAndLogErrorMessage,
Expand Down Expand Up @@ -338,8 +343,8 @@ export class ModelEditorView extends AbstractWebview<

await Promise.all([
this.setViewState(),
withProgress((progress) => this.loadMethods(progress), {
cancellable: false,
withProgress((progress, token) => this.loadMethods(progress, token), {
cancellable: true,
}),
this.loadExistingModeledMethods(),
]);
Expand Down Expand Up @@ -423,11 +428,16 @@ export class ModelEditorView extends AbstractWebview<
}
}

protected async loadMethods(progress: ProgressCallback): Promise<void> {
protected async loadMethods(
progress: ProgressCallback,
token?: CancellationToken,
): Promise<void> {
const mode = this.modelingStore.getMode(this.databaseItem);

try {
const cancellationTokenSource = new CancellationTokenSource();
if (!token) {
token = new CancellationTokenSource().token;
}
const queryResult = await runModelEditorQueries(mode, {
cliServer: this.cliServer,
queryRunner: this.queryRunner,
Expand All @@ -441,12 +451,19 @@ export class ModelEditorView extends AbstractWebview<
...update,
message: `Loading models: ${update.message}`,
}),
token: cancellationTokenSource.token,
token,
});
if (!queryResult) {
return;
}

if (token.isCancellationRequested) {
throw new UserCancellationException(
"Model editor: Load methods cancelled.",
true,
);
}

this.modelingStore.setMethods(this.databaseItem, queryResult);
} catch (err) {
void showAndLogExceptionWithTelemetry(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the error + telemetry event is coming from here (since we catch the UserCancellationException and treat it like a normal error). Maybe we can check for the type of error here?
Something like:

Suggested change
} catch (err) {
void showAndLogExceptionWithTelemetry(
} catch (err) {
if (err instanceof UserCancellationException) {
this.panel?.dispose();
return;
}
void showAndLogExceptionWithTelemetry(

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this is where the log message is coming from, so we have the power to silence it. Annoyingly, the error isn't actually a UserCancellationException :shakes_fist:. As far as I can tell the error is coming from a rpc library and not our code. We may need to add a custom check for this type of error, like if the message matches The request \(.*\) has been cancelled.

Adding in this.panel?.dispose() is also good because otherwise the panel lingers open in the "loading" state and you need to close it manually.

So maybe something like

if (
  getErrorMessage(err).match(/The request \(.*\) has been cancelled/i)
) {
  this.panel?.dispose();
  return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks really good! Yes, the catch is catching this error, I was just not sure if there are other instances where we might get a The request \(.*\) has been cancelled that was not due to a user cancellation...

Copy link
Contributor

Choose a reason for hiding this comment

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

Annoyingly, the error isn't actually a UserCancellationException

Is it not? 😅 That if snippet worked when I tested locally (though maybe I didn't test all cases 🤔)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The if works, but it could be imaginable that something went wrong internally when running the query and an error message with the same text is produced, but we now swallow it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That if snippet worked when I tested locally

It didn't work for me 🤔 . I also tried adding a breakpoint there so I could poke at the error manually. I'm very happy to try again if you disagree.

it could be imaginable that something went wrong internally when running the query and an error message with the same text is produced, but we now swallow it.

Maybe but I'd expect the message wouldn't say "cancelled" then. How about we still log it but not show the popup? Keeping telemetry could be nice so we know how many people are cancelling this. We could do something like

vscode-codeql/extensions/ql-vscode/src/databases/github-databases/github-databases-module.ts

Lines 66 to 71 in d0e8112

const error = redactableError(
asError(e),
)`Failed to prompt for GitHub repository download`;
void this.app.logger.log(error.fullMessageWithStack);
this.app.telemetry?.sendError(error);

Copy link
Contributor

Choose a reason for hiding this comment

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

The if works, but it could be imaginable that something went wrong internally when running the query and an error message with the same text is produced, but we now swallow it.

Sorry, I meant that my original suggestion of if (err instanceof UserCancellationException) worked fine for me! I was wondering what Robert meant by saying the error wasn't a UserCancellationException 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah! It depends on when you press the cancel button! There are two progress windows and for the first everything works, for the second it doesn't. So after the dependencies are installed and the query is actually run, we need the text match.

Copy link
Member

Choose a reason for hiding this comment

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

Something which might be less hacky is the following, which checks the type of the other error and that it is a cancellation error:

import { ResponseError } from "vscode-jsonrpc";
import { LSPErrorCodes } from "vscode-languageclient";

// ...

      if (
        err instanceof ResponseError &&
        err.code === LSPErrorCodes.RequestCancelled
      ) {
        this.panel?.dispose();
        return;
      }

Expand Down Expand Up @@ -578,6 +595,7 @@ export class ModelEditorView extends AbstractWebview<
this.modelConfig,
this.app.logger,
progress,
token,
3,
);
if (!modelFile) {
Expand Down
18 changes: 17 additions & 1 deletion ...sions/ql-vscode/test/vscode-tests/no-workspace/model-editor/extension-pack-picker.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { Uri, workspace, WorkspaceFolder } from "vscode";
import {
CancellationTokenSource,
Uri,
workspace,
WorkspaceFolder,
} from "vscode";
import { dump as dumpYaml, load as loadYaml } from "js-yaml";
import { outputFile, readFile } from "fs-extra";
import { join } from "path";
Expand All @@ -24,6 +29,7 @@ describe("pickExtensionPack", () => {
};

const progress = jest.fn();
const token = new CancellationTokenSource().token;
let workspaceFoldersSpy: jest.SpyInstance;
let additionalPacks: string[];
let workspaceFolder: WorkspaceFolder;
Expand Down Expand Up @@ -79,6 +85,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(autoExtensionPack);
Expand Down Expand Up @@ -136,6 +143,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual({
Expand Down Expand Up @@ -190,6 +198,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual({
Expand Down Expand Up @@ -234,6 +243,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(undefined);
Expand All @@ -260,6 +270,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(undefined);
Expand Down Expand Up @@ -288,6 +299,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(undefined);
Expand Down Expand Up @@ -326,6 +338,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(undefined);
Expand Down Expand Up @@ -364,6 +377,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(undefined);
Expand Down Expand Up @@ -405,6 +419,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(undefined);
Expand Down Expand Up @@ -463,6 +478,7 @@ describe("pickExtensionPack", () => {
modelConfig,
logger,
progress,
token,
maxStep,
),
).toEqual(extensionPack);
Expand Down