From 3a761d080bc99bae400115ef0e97f14cbe96a7cc Mon Sep 17 00:00:00 2001 From: Koen Vlaswinkel Date: Thu, 13 Apr 2023 14:57:19 +0200 Subject: [PATCH] Improve error handling in data extension editor This improves the error handling in the data extension editor by showing more errors to the user and adding validation to data extension model files. --- .../data-extensions-editor-view.ts | 15 ++++--- .../data-extensions-editor/data-schema.json | 45 +++++++++++++++++++ .../external-api-usage-query.ts | 12 ++--- .../generate-flow-model.ts | 19 ++++++-- .../src/data-extensions-editor/yaml.ts | 28 ++++++------ .../external-api-usage-query.test.ts | 22 ++++++--- 6 files changed, 110 insertions(+), 31 deletions(-) create mode 100644 extensions/ql-vscode/src/data-extensions-editor/data-schema.json diff --git a/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts b/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts index 8370e45ca..0afe1f81f 100644 --- a/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts +++ b/extensions/ql-vscode/src/data-extensions-editor/data-extensions-editor-view.ts @@ -14,8 +14,8 @@ import { import { ProgressUpdate } from "../progress"; import { QueryRunner } from "../queryRunner"; import { + showAndLogErrorMessage, showAndLogExceptionWithTelemetry, - showAndLogWarningMessage, } from "../helpers"; import { extLogger } from "../common"; import { readFile, writeFile } from "fs-extra"; @@ -166,7 +166,9 @@ export class DataExtensionsEditorView extends AbstractWebview< const existingModeledMethods = loadDataExtensionYaml(data); if (!existingModeledMethods) { - void showAndLogWarningMessage("Failed to parse data extension YAML."); + void showAndLogErrorMessage( + `Failed to parse data extension YAML ${this.modelFilename}.`, + ); return; } @@ -175,7 +177,11 @@ export class DataExtensionsEditorView extends AbstractWebview< modeledMethods: existingModeledMethods, }); } catch (e: unknown) { - void extLogger.log(`Unable to read data extension YAML: ${e}`); + void showAndLogErrorMessage( + `Unable to read data extension YAML ${ + this.modelFilename + }: ${getErrorMessage(e)}`, + ); } } @@ -208,7 +214,6 @@ export class DataExtensionsEditorView extends AbstractWebview< const bqrsChunk = await readQueryResults({ cliServer: this.cliServer, bqrsPath: queryResult.outputDir.bqrsPath, - logger: extLogger, }); if (!bqrsChunk) { await this.clearProgress(); @@ -233,7 +238,7 @@ export class DataExtensionsEditorView extends AbstractWebview< void showAndLogExceptionWithTelemetry( redactableError( asError(err), - )`Failed to load external APi usages: ${getErrorMessage(err)}`, + )`Failed to load external API usages: ${getErrorMessage(err)}`, ); } } diff --git a/extensions/ql-vscode/src/data-extensions-editor/data-schema.json b/extensions/ql-vscode/src/data-extensions-editor/data-schema.json new file mode 100644 index 000000000..fab11d91a --- /dev/null +++ b/extensions/ql-vscode/src/data-extensions-editor/data-schema.json @@ -0,0 +1,45 @@ +{ + "type": "object", + "properties": { + "extensions": { + "type": "array", + "items": { + "type": "object", + "required": ["addsTo", "data"], + "properties": { + "addsTo": { + "type": "object", + "required": ["pack", "extensible"], + "properties": { + "pack": { + "type": "string" + }, + "extensible": { + "type": "string" + } + } + }, + "data": { + "type": "array", + "items": { + "type": "array", + "items": { + "oneOf": [ + { + "type": "string" + }, + { + "type": "boolean" + }, + { + "type": "number" + } + ] + } + } + } + } + } + } + } +} diff --git a/extensions/ql-vscode/src/data-extensions-editor/external-api-usage-query.ts b/extensions/ql-vscode/src/data-extensions-editor/external-api-usage-query.ts index a5cd3c052..a2adecaa8 100644 --- a/extensions/ql-vscode/src/data-extensions-editor/external-api-usage-query.ts +++ b/extensions/ql-vscode/src/data-extensions-editor/external-api-usage-query.ts @@ -3,12 +3,16 @@ import { qlpackOfDatabase } from "../contextual/queryResolver"; import { file } from "tmp-promise"; import { writeFile } from "fs-extra"; import { dump as dumpYaml } from "js-yaml"; -import { getOnDiskWorkspaceFolders } from "../helpers"; +import { + getOnDiskWorkspaceFolders, + showAndLogExceptionWithTelemetry, +} from "../helpers"; import { Logger, TeeLogger } from "../common"; import { CancellationToken } from "vscode"; import { CodeQLCliServer } from "../cli"; import { DatabaseItem } from "../local-databases"; import { ProgressCallback } from "../progress"; +import { redactableError } from "../pure/errors"; export type RunQueryOptions = { cliServer: Pick; @@ -92,18 +96,16 @@ export async function runQuery({ export type GetResultsOptions = { cliServer: Pick; bqrsPath: string; - logger: Logger; }; export async function readQueryResults({ cliServer, bqrsPath, - logger, }: GetResultsOptions) { const bqrsInfo = await cliServer.bqrsInfo(bqrsPath); if (bqrsInfo["result-sets"].length !== 1) { - void logger.log( - `Expected exactly one result set, got ${bqrsInfo["result-sets"].length}`, + void showAndLogExceptionWithTelemetry( + redactableError`Expected exactly one result set, got ${bqrsInfo["result-sets"].length}`, ); return undefined; } diff --git a/extensions/ql-vscode/src/data-extensions-editor/generate-flow-model.ts b/extensions/ql-vscode/src/data-extensions-editor/generate-flow-model.ts index f6125d0f3..aa7c54acf 100644 --- a/extensions/ql-vscode/src/data-extensions-editor/generate-flow-model.ts +++ b/extensions/ql-vscode/src/data-extensions-editor/generate-flow-model.ts @@ -6,11 +6,16 @@ import { CodeQLCliServer } from "../cli"; import { TeeLogger } from "../common"; import { extensiblePredicateDefinitions } from "./yaml"; import { ProgressCallback } from "../progress"; -import { getOnDiskWorkspaceFolders } from "../helpers"; +import { + getOnDiskWorkspaceFolders, + showAndLogExceptionWithTelemetry, +} from "../helpers"; import { ModeledMethodType, ModeledMethodWithSignature, } from "./modeled-method"; +import { redactableError } from "../pure/errors"; +import { QueryResultType } from "../pure/new-messages"; type FlowModelOptions = { cliServer: CodeQLCliServer; @@ -67,13 +72,21 @@ async function getModeledMethodsFromFlow( token, new TeeLogger(queryRunner.logger, queryRun.outputDir.logPath), ); + if (queryResult.resultType !== QueryResultType.SUCCESS) { + void showAndLogExceptionWithTelemetry( + redactableError`Failed to run ${queryName} query: ${ + queryResult.message ?? "No message" + }`, + ); + return []; + } const bqrsPath = queryResult.outputDir.bqrsPath; const bqrsInfo = await cliServer.bqrsInfo(bqrsPath); if (bqrsInfo["result-sets"].length !== 1) { - throw new Error( - `Expected exactly one result set, got ${bqrsInfo["result-sets"].length}`, + void showAndLogExceptionWithTelemetry( + redactableError`Expected exactly one result set, got ${bqrsInfo["result-sets"].length} for ${queryName}`, ); } diff --git a/extensions/ql-vscode/src/data-extensions-editor/yaml.ts b/extensions/ql-vscode/src/data-extensions-editor/yaml.ts index 95383173d..b66f21a60 100644 --- a/extensions/ql-vscode/src/data-extensions-editor/yaml.ts +++ b/extensions/ql-vscode/src/data-extensions-editor/yaml.ts @@ -1,3 +1,5 @@ +import Ajv from "ajv"; + import { ExternalApiUsage } from "./external-api-usage"; import { ModeledMethod, @@ -5,6 +7,11 @@ import { ModeledMethodWithSignature, } from "./modeled-method"; +import * as dataSchemaJson from "./data-schema.json"; + +const ajv = new Ajv({ allErrors: true }); +const dataSchemaValidate = ajv.compile(dataSchemaJson); + type ExternalApiUsageByType = { externalApiUsage: ExternalApiUsage; modeledMethod: ModeledMethod; @@ -191,8 +198,14 @@ ${extensions.join("\n")}`; export function loadDataExtensionYaml( data: any, ): Record | undefined { - if (typeof data !== "object") { - return undefined; + dataSchemaValidate(data); + + if (dataSchemaValidate.errors) { + throw new Error( + `Invalid data extension YAML: ${dataSchemaValidate.errors + .map((error) => `${error.instancePath} ${error.message}`) + .join(", ")}`, + ); } const extensions = data.extensions; @@ -204,19 +217,8 @@ export function loadDataExtensionYaml( for (const extension of extensions) { const addsTo = extension.addsTo; - if (typeof addsTo !== "object") { - continue; - } - const extensible = addsTo.extensible; - if (typeof extensible !== "string") { - continue; - } - const data = extension.data; - if (!Array.isArray(data)) { - continue; - } const definition = Object.values(extensiblePredicateDefinitions).find( (definition) => definition.extensiblePredicate === extensible, diff --git a/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/external-api-usage-query.test.ts b/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/external-api-usage-query.test.ts index 43a68cb15..4d5d47b66 100644 --- a/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/external-api-usage-query.test.ts +++ b/extensions/ql-vscode/test/vscode-tests/no-workspace/data-extensions-editor/external-api-usage-query.test.ts @@ -10,6 +10,8 @@ import { file } from "tmp-promise"; import { QueryResultType } from "../../../../src/pure/new-messages"; import { readFile } from "fs-extra"; import { load } from "js-yaml"; +import * as helpers from "../../../../src/helpers"; +import { RedactableError } from "../../../../src/pure/errors"; function createMockUri(path = "/a/b/c/foo"): Uri { return { @@ -127,17 +129,27 @@ describe("readQueryResults", () => { bqrsDecode: jest.fn(), }, bqrsPath: "/tmp/results.bqrs", - logger: createMockLogger(), }; + let showAndLogExceptionWithTelemetrySpy: jest.SpiedFunction< + typeof helpers.showAndLogExceptionWithTelemetry + >; + + beforeEach(() => { + showAndLogExceptionWithTelemetrySpy = jest.spyOn( + helpers, + "showAndLogExceptionWithTelemetry", + ); + }); + it("returns undefined when there are no results", async () => { options.cliServer.bqrsInfo.mockResolvedValue({ "result-sets": [], }); expect(await readQueryResults(options)).toBeUndefined(); - expect(options.logger.log).toHaveBeenCalledWith( - expect.stringMatching(/Expected exactly one result set/), + expect(showAndLogExceptionWithTelemetrySpy).toHaveBeenCalledWith( + expect.any(RedactableError), ); }); @@ -166,8 +178,8 @@ describe("readQueryResults", () => { }); expect(await readQueryResults(options)).toBeUndefined(); - expect(options.logger.log).toHaveBeenCalledWith( - expect.stringMatching(/Expected exactly one result set/), + expect(showAndLogExceptionWithTelemetrySpy).toHaveBeenCalledWith( + expect.any(RedactableError), ); });