Merge pull request #2316 from github/koesie10/data-extension-editor-errors

Improve error handling in data extension editor

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/unit-tests/data-extensions-editor/yaml.test.ts

@@ -149,14 +149,14 @@ describe("loadDataExtensionYaml", () => {
   });
 
   it("returns undefined if given a string", () => {
-    const data = loadDataExtensionYaml(`extensions:
+    expect(() =>
+      loadDataExtensionYaml(`extensions:
   - addsTo:
       pack: codeql/java-all
       extensible: sinkModel
     data:
       - ["org.sql2o","Connection",true,"createQuery","(String)","","Argument[0]","sql","manual"]
-`);
-
-    expect(data).toBeUndefined();
+`),
+    ).toThrow("Invalid data extension YAML:  must be object");
   });
 });

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