Merge pull request #1208 from github/aeisenberg/better-error-message

More readable error message for invalid `queries` block and invalid `query-filters` blocl

CHANGELOG.md

@@ -2,7 +2,7 @@
 
 ## [UNRELEASED]
 
-No user facing changes.
+- Improve error messages when the code scanning configuration file includes an invalid `queries` block or an invalid `query-filters` block. [#1208](https://github.com/github/codeql-action/pull/1208)
 
 ## 2.1.20 - 22 Aug 2022
 

lib/analyze.js

@@ -354,6 +354,9 @@ function validateQueryFilters(queryFilters) {
     if (!queryFilters) {
         return [];
     }
+    if (!Array.isArray(queryFilters)) {
+        throw new Error(`Query filters must be an array of "include" or "exclude" entries. Found ${typeof queryFilters}`);
+    }
     const errors = [];
     for (const qf of queryFilters) {
         const keys = Object.keys(qf);

lib/analyze.test.js

@@ -259,6 +259,11 @@ const util = __importStar(require("./util"));
     t.throws(() => {
         return (0, analyze_1.validateQueryFilters)([{ xxx: "foo" }]);
     }, { message: /Only "include" or "exclude" filters are allowed/ });
+    t.throws(() => {
+        return (0, analyze_1.validateQueryFilters)({ exclude: "foo" });
+    }, {
+        message: /Query filters must be an array of "include" or "exclude" entries/,
+    });
 });
 const convertPackToQuerySuiteEntryMacro = ava_1.default.macro({
     exec: (t, packSpec, suiteEntry) => t.deepEqual((0, analyze_1.convertPackToQuerySuiteEntry)(packSpec), suiteEntry),

lib/config-utils.js

@@ -19,7 +19,7 @@ var __importStar = (this && this.__importStar) || function (mod) {
     return result;
 };
 Object.defineProperty(exports, "__esModule", { value: true });
-exports.getConfig = exports.getPathToParsedConfigFile = exports.initConfig = exports.parsePacks = exports.validatePackSpecification = exports.prettyPrintPack = exports.parsePacksSpecification = exports.parsePacksFromConfig = exports.calculateAugmentation = exports.getDefaultConfig = exports.getUnknownLanguagesError = exports.getNoLanguagesError = exports.getConfigFileDirectoryGivenMessage = exports.getConfigFileFormatInvalidMessage = exports.getConfigFileRepoFormatInvalidMessage = exports.getConfigFileDoesNotExistErrorMessage = exports.getConfigFileOutsideWorkspaceErrorMessage = exports.getLocalPathDoesNotExist = exports.getLocalPathOutsideOfRepository = exports.getPacksStrInvalid = exports.getPacksInvalid = exports.getPacksInvalidSplit = exports.getPathsInvalid = exports.getPathsIgnoreInvalid = exports.getQueryUsesInvalid = exports.getQueriesInvalid = exports.getDisableDefaultQueriesInvalid = exports.getNameInvalid = exports.validateAndSanitisePath = exports.defaultAugmentationProperties = void 0;
+exports.getConfig = exports.getPathToParsedConfigFile = exports.initConfig = exports.parsePacks = exports.validatePackSpecification = exports.prettyPrintPack = exports.parsePacksSpecification = exports.parsePacksFromConfig = exports.calculateAugmentation = exports.getDefaultConfig = exports.getUnknownLanguagesError = exports.getNoLanguagesError = exports.getConfigFileDirectoryGivenMessage = exports.getConfigFileFormatInvalidMessage = exports.getConfigFileRepoFormatInvalidMessage = exports.getConfigFileDoesNotExistErrorMessage = exports.getConfigFileOutsideWorkspaceErrorMessage = exports.getLocalPathDoesNotExist = exports.getLocalPathOutsideOfRepository = exports.getPacksStrInvalid = exports.getPacksInvalid = exports.getPacksInvalidSplit = exports.getPathsInvalid = exports.getPathsIgnoreInvalid = exports.getQueryUsesInvalid = exports.getQueriesMissingUses = exports.getQueriesInvalid = exports.getDisableDefaultQueriesInvalid = exports.getNameInvalid = exports.validateAndSanitisePath = exports.defaultAugmentationProperties = void 0;
 const fs = __importStar(require("fs"));
 const path = __importStar(require("path"));
 // We need to import `performance` on Node 12
@@ -305,6 +305,10 @@ function getQueriesInvalid(configFile) {
     return getConfigFilePropertyError(configFile, QUERIES_PROPERTY, "must be an array");
 }
 exports.getQueriesInvalid = getQueriesInvalid;
+function getQueriesMissingUses(configFile) {
+    return getConfigFilePropertyError(configFile, QUERIES_PROPERTY, "must be an array, with each entry having a 'uses' property");
+}
+exports.getQueriesMissingUses = getQueriesMissingUses;
 function getQueryUsesInvalid(configFile, queryUses) {
     return getConfigFilePropertyError(configFile, `${QUERIES_PROPERTY}.${QUERIES_USES_PROPERTY}`, `must be a built-in suite (${builtinSuites.join(" or ")}), a relative path, or be of the form "owner/repo[/path]@ref"${queryUses !== undefined ? `\n Found: ${queryUses}` : ""}`);
 }
@@ -586,9 +590,8 @@ async function loadConfig(languagesInput, rawQueriesInput, rawPacksInput, config
             throw new Error(getQueriesInvalid(configFile));
         }
         for (const query of queriesArr) {
-            if (!(QUERIES_USES_PROPERTY in query) ||
+            if (typeof query[QUERIES_USES_PROPERTY] !== "string") {
-                typeof query[QUERIES_USES_PROPERTY] !== "string") {
+                throw new Error(getQueriesMissingUses(configFile));
-                throw new Error(getQueryUsesInvalid(configFile));
             }
             await parseQueryUses(languages, codeQL, queries, packs, query[QUERIES_USES_PROPERTY], tempDir, workspacePath, apiDetails, featureFlags, logger, configFile);
         }

lib/config-utils.test.js

@@ -698,7 +698,7 @@ doInvalidInputTest("paths invalid type", `paths: 17`, configUtils.getPathsInvali
 doInvalidInputTest("queries uses invalid type", `
   queries:
   - uses:
-      - hello: world`, configUtils.getQueryUsesInvalid);
+      - hello: world`, configUtils.getQueriesMissingUses);
 function doInvalidQueryUsesTest(input, expectedErrorMessageGenerator) {
     // Invalid contents of a "queries.uses" field.
     // Should fail with the expected error message

src/analyze.test.ts

@@ -313,6 +313,16 @@ test("validateQueryFilters", (t) => {
     },
     { message: /Only "include" or "exclude" filters are allowed/ }
   );
+
+  t.throws(
+    () => {
+      return validateQueryFilters({ exclude: "foo" } as any);
+    },
+    {
+      message:
+        /Query filters must be an array of "include" or "exclude" entries/,
+    }
+  );
 });
 
 const convertPackToQuerySuiteEntryMacro = test.macro({

src/analyze.ts

@@ -593,6 +593,12 @@ export function validateQueryFilters(queryFilters?: configUtils.QueryFilter[]) {
     return [];
   }
 
+  if (!Array.isArray(queryFilters)) {
+    throw new Error(
+      `Query filters must be an array of "include" or "exclude" entries. Found ${typeof queryFilters}`
+    );
+  }
+
   const errors: string[] = [];
   for (const qf of queryFilters) {
     const keys = Object.keys(qf);

src/config-utils.test.ts

@@ -1328,7 +1328,7 @@ doInvalidInputTest(
   queries:
   - uses:
       - hello: world`,
-  configUtils.getQueryUsesInvalid
+  configUtils.getQueriesMissingUses
 );
 
 function doInvalidQueryUsesTest(

src/config-utils.ts

@@ -683,6 +683,14 @@ export function getQueriesInvalid(configFile: string): string {
   );
 }
 
+export function getQueriesMissingUses(configFile: string): string {
+  return getConfigFilePropertyError(
+    configFile,
+    QUERIES_PROPERTY,
+    "must be an array, with each entry having a 'uses' property"
+  );
+}
+
 export function getQueryUsesInvalid(
   configFile: string | undefined,
   queryUses?: string
@@ -1181,11 +1189,8 @@ async function loadConfig(
       throw new Error(getQueriesInvalid(configFile));
     }
     for (const query of queriesArr) {
-      if (
+      if (typeof query[QUERIES_USES_PROPERTY] !== "string") {
-        !(QUERIES_USES_PROPERTY in query) ||
+        throw new Error(getQueriesMissingUses(configFile));
-        typeof query[QUERIES_USES_PROPERTY] !== "string"
-      ) {
-        throw new Error(getQueryUsesInvalid(configFile));
       }
       await parseQueryUses(
         languages,