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.

Author: koesie10

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)}`,
       );
     }
   }

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"
+                  }
+                ]
+              }
+            }
+          }
+        }
+      }
+    }
+  }
+}

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<CodeQLCliServer, "resolveQlpacks" | "resolveQueriesInSuite">;
@@ -92,18 +96,16 @@ export async function runQuery({
 export type GetResultsOptions = {
   cliServer: Pick<CodeQLCliServer, "bqrsInfo" | "bqrsDecode">;
   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;
   }

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}`,
     );
   }
 

extensions/ql-vscode/src/data-extensions-editor/yaml.ts

@@ -1,10 +1,17 @@
+import Ajv from "ajv";
+
 import { ExternalApiUsage } from "./external-api-usage";
 import {
   ModeledMethod,
   ModeledMethodType,
   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<string, ModeledMethod> | 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,

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),
     );
   });