Merge pull request #3167 from github/mbg/upload-sarif/find-then-filter

Find, then filter, SARIF files for `upload-sarif` Action

src/analyses.ts

@@ -1,3 +1,5 @@
+import { fixCodeQualityCategory } from "./actions-util";
+import { Logger } from "./logging";
 import { ConfigurationError } from "./util";
 
 export enum AnalysisKind {
@@ -61,6 +63,8 @@ export interface AnalysisConfig {
   /** A predicate on filenames to decide whether a SARIF file
    * belongs to this kind of analysis. */
   sarifPredicate: (name: string) => boolean;
+  /** Analysis-specific adjustment of the category. */
+  fixCategory: (logger: Logger, category?: string) => string | undefined;
   /** A prefix for environment variables used to track the uniqueness of SARIF uploads. */
   sentinelPrefix: string;
 }
@@ -74,6 +78,7 @@ export const CodeScanning: AnalysisConfig = {
   sarifPredicate: (name) =>
     name.endsWith(CodeScanning.sarifExtension) &&
     !CodeQuality.sarifPredicate(name),
+  fixCategory: (_, category) => category,
   sentinelPrefix: "CODEQL_UPLOAD_SARIF_",
 };
 
@@ -84,5 +89,29 @@ export const CodeQuality: AnalysisConfig = {
   target: SARIF_UPLOAD_ENDPOINT.CODE_QUALITY,
   sarifExtension: ".quality.sarif",
   sarifPredicate: (name) => name.endsWith(CodeQuality.sarifExtension),
+  fixCategory: fixCodeQualityCategory,
   sentinelPrefix: "CODEQL_UPLOAD_QUALITY_SARIF_",
 };
+
+/**
+ * Gets the `AnalysisConfig` corresponding to `kind`.
+ * @param kind The analysis kind to get the `AnalysisConfig` for.
+ * @returns The `AnalysisConfig` corresponding to `kind`.
+ */
+export function getAnalysisConfig(kind: AnalysisKind): AnalysisConfig {
+  // Using a switch statement here accomplishes two things:
+  // 1. The type checker believes us that we have a case for every `AnalysisKind`.
+  // 2. If we ever add another member to `AnalysisKind`, the type checker will alert us that we have to add a case.
+  switch (kind) {
+    case AnalysisKind.CodeScanning:
+      return CodeScanning;
+    case AnalysisKind.CodeQuality:
+      return CodeQuality;
+  }
+}
+
+// Since we have overlapping extensions (i.e. ".sarif" includes ".quality.sarif"),
+// we want to scan a folder containing SARIF files in an order that finds the more
+// specific extensions first. This constant defines an array in the order of analyis
+// configurations with more specific extensions to less specific extensions.
+export const SarifScanOrder = [CodeQuality, CodeScanning];

src/analyze-action.ts

@@ -356,16 +356,14 @@ async function run() {
       }
 
       if (isCodeQualityEnabled(config)) {
+        const analysis = analyses.CodeQuality;
         const qualityUploadResult = await uploadLib.uploadFiles(
           outputDir,
           actionsUtil.getRequiredInput("checkout_path"),
-          actionsUtil.fixCodeQualityCategory(
-            logger,
-            actionsUtil.getOptionalInput("category"),
-          ),
+          actionsUtil.getOptionalInput("category"),
           features,
           logger,
-          analyses.CodeQuality,
+          analysis,
         );
         core.setOutput("quality-sarif-id", qualityUploadResult.sarifID);
       }

src/analyze.test.ts

@@ -334,7 +334,7 @@ test("resolveQuerySuiteAlias", (t) => {
   for (const suite of defaultSuites) {
     const resolved = resolveQuerySuiteAlias(KnownLanguage.go, suite);
     t.assert(
-      resolved.endsWith(".qls"),
+      path.extname(resolved) === ".qls",
       "Resolved default suite doesn't end in .qls",
     );
     t.assert(

src/analyze.ts

@@ -7,7 +7,6 @@ import * as del from "del";
 import * as yaml from "js-yaml";
 
 import {
-  fixCodeQualityCategory,
   getRequiredInput,
   getTemporaryDirectory,
   PullRequestBranches,
@@ -781,7 +780,7 @@ export async function runQueries(
     // accepted by the Code Quality backend.
     let category = automationDetailsId;
     if (analysis.kind === analyses.AnalysisKind.CodeQuality) {
-      category = fixCodeQualityCategory(logger, automationDetailsId);
+      category = analysis.fixCategory(logger, automationDetailsId);
     }
 
     const sarifFile = path.join(

src/config/db-config.test.ts

@@ -153,7 +153,6 @@ const packSpecPrettyPrintingMacro = test.macro({
   title: (
     _providedTitle: string | undefined,
     packStr: string,
-    // eslint-disable-next-line @typescript-eslint/no-unused-vars
     _packObj: dbConfig.Pack,
   ) => `Prettyprint pack spec: '${packStr}'`,
 });

src/debug-artifacts.ts

@@ -59,7 +59,7 @@ export async function uploadCombinedSarifArtifacts(
         for (const outputDir of outputDirs) {
           const sarifFiles = fs
             .readdirSync(path.resolve(baseTempDir, outputDir))
-            .filter((f) => f.endsWith(".sarif"));
+            .filter((f) => path.extname(f) === ".sarif");
 
           for (const sarifFile of sarifFiles) {
             toUpload.push(path.resolve(baseTempDir, outputDir, sarifFile));

src/upload-lib.test.ts

@@ -3,7 +3,7 @@ import * as path from "path";
 
 import test from "ava";
 
-import { CodeQuality, CodeScanning } from "./analyses";
+import { AnalysisKind, CodeQuality, CodeScanning } from "./analyses";
 import { getRunnerLogger, Logger } from "./logging";
 import { setupTests } from "./testing-utils";
 import * as uploadLib from "./upload-lib";
@@ -127,27 +127,97 @@ test("finding SARIF files", async (t) => {
     fs.writeFileSync(path.join(tmpDir, "a.quality.sarif"), "");
     fs.writeFileSync(path.join(tmpDir, "dir1", "b.quality.sarif"), "");
 
+    const expectedSarifFiles = [
+      path.join(tmpDir, "a.sarif"),
+      path.join(tmpDir, "b.sarif"),
+      path.join(tmpDir, "dir1", "d.sarif"),
+      path.join(tmpDir, "dir1", "dir2", "e.sarif"),
+    ];
     const sarifFiles = uploadLib.findSarifFilesInDir(
       tmpDir,
       CodeScanning.sarifPredicate,
     );
 
-    t.deepEqual(sarifFiles, [
-      path.join(tmpDir, "a.sarif"),
-      path.join(tmpDir, "b.sarif"),
-      path.join(tmpDir, "dir1", "d.sarif"),
-      path.join(tmpDir, "dir1", "dir2", "e.sarif"),
-    ]);
+    t.deepEqual(sarifFiles, expectedSarifFiles);
 
+    const expectedQualitySarifFiles = [
+      path.join(tmpDir, "a.quality.sarif"),
+      path.join(tmpDir, "dir1", "b.quality.sarif"),
+    ];
     const qualitySarifFiles = uploadLib.findSarifFilesInDir(
       tmpDir,
       CodeQuality.sarifPredicate,
     );
 
-    t.deepEqual(qualitySarifFiles, [
-      path.join(tmpDir, "a.quality.sarif"),
-      path.join(tmpDir, "dir1", "b.quality.sarif"),
-    ]);
+    t.deepEqual(qualitySarifFiles, expectedQualitySarifFiles);
+
+    const groupedSarifFiles = await uploadLib.getGroupedSarifFilePaths(
+      getRunnerLogger(true),
+      tmpDir,
+    );
+
+    t.not(groupedSarifFiles, undefined);
+    t.not(groupedSarifFiles[AnalysisKind.CodeScanning], undefined);
+    t.not(groupedSarifFiles[AnalysisKind.CodeQuality], undefined);
+    t.deepEqual(
+      groupedSarifFiles[AnalysisKind.CodeScanning],
+      expectedSarifFiles,
+    );
+    t.deepEqual(
+      groupedSarifFiles[AnalysisKind.CodeQuality],
+      expectedQualitySarifFiles,
+    );
+  });
+});
+
+test("getGroupedSarifFilePaths - Code Quality file", async (t) => {
+  await withTmpDir(async (tmpDir) => {
+    const sarifPath = path.join(tmpDir, "a.quality.sarif");
+    fs.writeFileSync(sarifPath, "");
+
+    const groupedSarifFiles = await uploadLib.getGroupedSarifFilePaths(
+      getRunnerLogger(true),
+      sarifPath,
+    );
+
+    t.not(groupedSarifFiles, undefined);
+    t.is(groupedSarifFiles[AnalysisKind.CodeScanning], undefined);
+    t.not(groupedSarifFiles[AnalysisKind.CodeQuality], undefined);
+    t.deepEqual(groupedSarifFiles[AnalysisKind.CodeQuality], [sarifPath]);
+  });
+});
+
+test("getGroupedSarifFilePaths - Code Scanning file", async (t) => {
+  await withTmpDir(async (tmpDir) => {
+    const sarifPath = path.join(tmpDir, "a.sarif");
+    fs.writeFileSync(sarifPath, "");
+
+    const groupedSarifFiles = await uploadLib.getGroupedSarifFilePaths(
+      getRunnerLogger(true),
+      sarifPath,
+    );
+
+    t.not(groupedSarifFiles, undefined);
+    t.not(groupedSarifFiles[AnalysisKind.CodeScanning], undefined);
+    t.is(groupedSarifFiles[AnalysisKind.CodeQuality], undefined);
+    t.deepEqual(groupedSarifFiles[AnalysisKind.CodeScanning], [sarifPath]);
+  });
+});
+
+test("getGroupedSarifFilePaths - Other file", async (t) => {
+  await withTmpDir(async (tmpDir) => {
+    const sarifPath = path.join(tmpDir, "a.json");
+    fs.writeFileSync(sarifPath, "");
+
+    const groupedSarifFiles = await uploadLib.getGroupedSarifFilePaths(
+      getRunnerLogger(true),
+      sarifPath,
+    );
+
+    t.not(groupedSarifFiles, undefined);
+    t.not(groupedSarifFiles[AnalysisKind.CodeScanning], undefined);
+    t.is(groupedSarifFiles[AnalysisKind.CodeQuality], undefined);
+    t.deepEqual(groupedSarifFiles[AnalysisKind.CodeScanning], [sarifPath]);
   });
 });
 

src/upload-lib.ts

@@ -459,6 +459,79 @@ export function getSarifFilePaths(
   return sarifFiles;
 }
 
+type GroupedSarifFiles = Partial<Record<analyses.AnalysisKind, string[]>>;
+
+/**
+ * Finds SARIF files in `sarifPath`, and groups them by analysis kind, following `SarifScanOrder`.
+ *
+ * @param logger The logger to use.
+ * @param sarifPath The path of a file or directory to recursively scan for SARIF files.
+ * @returns The `.sarif` files found in `sarifPath`, grouped by analysis kind.
+ */
+export async function getGroupedSarifFilePaths(
+  logger: Logger,
+  sarifPath: string,
+): Promise<GroupedSarifFiles> {
+  const stats = fs.statSync(sarifPath, { throwIfNoEntry: false });
+
+  if (stats === undefined) {
+    // This is always a configuration error, even for first-party runs.
+    throw new ConfigurationError(`Path does not exist: ${sarifPath}`);
+  }
+
+  const results: GroupedSarifFiles = {};
+
+  if (stats.isDirectory()) {
+    let unassignedSarifFiles = findSarifFilesInDir(
+      sarifPath,
+      (name) => path.extname(name) === ".sarif",
+    );
+    logger.debug(
+      `Found the following .sarif files in ${sarifPath}: ${unassignedSarifFiles.join(", ")}`,
+    );
+
+    for (const analysisConfig of analyses.SarifScanOrder) {
+      const filesForCurrentAnalysis = unassignedSarifFiles.filter(
+        analysisConfig.sarifPredicate,
+      );
+      if (filesForCurrentAnalysis.length > 0) {
+        logger.debug(
+          `The following SARIF files are for ${analysisConfig.name}: ${filesForCurrentAnalysis.join(", ")}`,
+        );
+        // Looping through the array a second time is not efficient, but more readable.
+        // Change this to one loop for both calls to `filter` if this becomes a bottleneck.
+        unassignedSarifFiles = unassignedSarifFiles.filter(
+          (name) => !analysisConfig.sarifPredicate(name),
+        );
+        results[analysisConfig.kind] = filesForCurrentAnalysis;
+      } else {
+        logger.debug(`Found no SARIF files for ${analysisConfig.name}`);
+      }
+    }
+
+    if (unassignedSarifFiles.length !== 0) {
+      logger.warning(
+        `Found files in ${sarifPath} which do not belong to any analysis: ${unassignedSarifFiles.join(", ")}`,
+      );
+    }
+  } else {
+    for (const analysisConfig of analyses.SarifScanOrder) {
+      if (
+        analysisConfig.kind === analyses.AnalysisKind.CodeScanning ||
+        analysisConfig.sarifPredicate(sarifPath)
+      ) {
+        logger.debug(
+          `Using '${sarifPath}' as a SARIF file for ${analysisConfig.name}.`,
+        );
+        results[analysisConfig.kind] = [sarifPath];
+        break;
+      }
+    }
+  }
+
+  return results;
+}
+
 // Counts the number of results in the given SARIF file
 function countResultsInSarif(sarif: string): number {
   let numResults = 0;
@@ -655,6 +728,7 @@ export async function uploadSpecifiedFiles(
   const gitHubVersion = await getGitHubVersion();
 
   let sarif: SarifFile;
+  category = uploadTarget.fixCategory(logger, category);
 
   if (sarifPaths.length > 1) {
     // Validate that the files we were asked to upload are all valid SARIF files

src/upload-sarif.test.ts

@@ -4,87 +4,16 @@ import * as path from "path";
 import test, { ExecutionContext } from "ava";
 import * as sinon from "sinon";
 
-import {
-  AnalysisConfig,
-  AnalysisKind,
-  CodeQuality,
-  CodeScanning,
-} from "./analyses";
+import { AnalysisKind, getAnalysisConfig } from "./analyses";
 import { getRunnerLogger } from "./logging";
 import { createFeatures, setupTests } from "./testing-utils";
 import { UploadResult } from "./upload-lib";
 import * as uploadLib from "./upload-lib";
-import { findAndUpload, uploadSarif } from "./upload-sarif";
+import { uploadSarif } from "./upload-sarif";
 import * as util from "./util";
 
 setupTests(test);
 
-const findAndUploadMacro = test.macro({
-  exec: async (
-    t: ExecutionContext<unknown>,
-    sarifFiles: string[],
-    analysis: AnalysisConfig,
-    sarifPath: (tempDir: string) => string = (tempDir) => tempDir,
-    expectedResult: UploadResult | undefined,
-  ) => {
-    await util.withTmpDir(async (tempDir) => {
-      sinon.stub(uploadLib, "uploadSpecifiedFiles").resolves(expectedResult);
-      const logger = getRunnerLogger(true);
-      const features = createFeatures([]);
-
-      for (const sarifFile of sarifFiles) {
-        fs.writeFileSync(path.join(tempDir, sarifFile), "");
-      }
-
-      const stats = fs.statSync(sarifPath(tempDir));
-      const actual = await findAndUpload(
-        logger,
-        features,
-        sarifPath(tempDir),
-        stats,
-        "",
-        analysis,
-      );
-
-      t.deepEqual(actual, expectedResult);
-    });
-  },
-  title: (providedTitle = "") => `findAndUpload - ${providedTitle}`,
-});
-
-test(
-  "no matching files",
-  findAndUploadMacro,
-  ["test.json"],
-  CodeScanning,
-  undefined,
-  undefined,
-);
-
-test(
-  "matching files for Code Scanning with directory path",
-  findAndUploadMacro,
-  ["test.sarif"],
-  CodeScanning,
-  undefined,
-  {
-    statusReport: {},
-    sarifID: "some-id",
-  },
-);
-
-test(
-  "matching files for Code Scanning with file path",
-  findAndUploadMacro,
-  ["test.sarif"],
-  CodeScanning,
-  (tempDir) => path.join(tempDir, "test.sarif"),
-  {
-    statusReport: {},
-    sarifID: "some-id",
-  },
-);
-
 interface UploadSarifExpectedResult {
   uploadResult?: UploadResult;
   expectedFiles?: string[];
@@ -117,9 +46,7 @@ const uploadSarifMacro = test.macro({
             sinon.match.any,
             features,
             logger,
-            analysisKind === AnalysisKind.CodeScanning
-              ? CodeScanning
-              : CodeQuality,
+            getAnalysisConfig(analysisKind),
           )
           .resolves(expectedResult[analysisKind as AnalysisKind]?.uploadResult);
       }
@@ -146,9 +73,7 @@ const uploadSarifMacro = test.macro({
               sinon.match.any,
               features,
               logger,
-              analysisKind === AnalysisKind.CodeScanning
-                ? CodeScanning
-                : CodeQuality,
+              getAnalysisConfig(analysisKind),
             ),
           );
         } else {
@@ -164,9 +89,7 @@ const uploadSarifMacro = test.macro({
               sinon.match.any,
               features,
               logger,
-              analysisKind === AnalysisKind.CodeScanning
-                ? CodeScanning
-                : CodeQuality,
+              getAnalysisConfig(analysisKind),
             ),
             `uploadSpecifiedFiles was called for ${analysisKind}, but should not have been.`,
           );

src/upload-sarif.ts

@@ -1,65 +1,8 @@
-import * as fs from "fs";
-
-import * as actionsUtil from "./actions-util";
 import * as analyses from "./analyses";
 import { FeatureEnablement } from "./feature-flags";
 import { Logger } from "./logging";
 import * as upload_lib from "./upload-lib";
-import { ConfigurationError } from "./util";
-
-/**
- * Searches for SARIF files for the given `analysis` in the given `sarifPath`.
- * If any are found, then they are uploaded to the appropriate endpoint for the given `analysis`.
- *
- * @param logger The logger to use.
- * @param features Information about FFs.
- * @param sarifPath The path to a SARIF file or directory containing SARIF files.
- * @param pathStats Information about `sarifPath`.
- * @param checkoutPath The checkout path.
- * @param analysis The configuration of the analysis we should upload SARIF files for.
- * @param category The SARIF category to use for the upload.
- * @returns The result of uploading the SARIF file(s) or `undefined` if there are none.
- */
-export async function findAndUpload(
-  logger: Logger,
-  features: FeatureEnablement,
-  sarifPath: string,
-  pathStats: fs.Stats,
-  checkoutPath: string,
-  analysis: analyses.AnalysisConfig,
-  category?: string,
-): Promise<upload_lib.UploadResult | undefined> {
-  let sarifFiles: string[] | undefined;
-
-  if (pathStats.isDirectory()) {
-    sarifFiles = upload_lib.findSarifFilesInDir(
-      sarifPath,
-      analysis.sarifPredicate,
-    );
-  } else if (
-    pathStats.isFile() &&
-    (analysis.sarifPredicate(sarifPath) ||
-      (analysis.kind === analyses.AnalysisKind.CodeScanning &&
-        !analyses.CodeQuality.sarifPredicate(sarifPath)))
-  ) {
-    sarifFiles = [sarifPath];
-  } else {
-    return undefined;
-  }
-
-  if (sarifFiles.length !== 0) {
-    return await upload_lib.uploadSpecifiedFiles(
-      sarifFiles,
-      checkoutPath,
-      category,
-      features,
-      logger,
-      analysis,
-    );
-  }
-
-  return undefined;
-}
+import { unsafeEntriesInvariant } from "./util";
 
 // Maps analysis kinds to SARIF IDs.
 export type UploadSarifResults = Partial<
@@ -84,38 +27,24 @@ export async function uploadSarif(
   sarifPath: string,
   category?: string,
 ): Promise<UploadSarifResults> {
-  const pathStats = fs.lstatSync(sarifPath, { throwIfNoEntry: false });
-
-  if (pathStats === undefined) {
-    throw new ConfigurationError(`Path does not exist: ${sarifPath}.`);
-  }
+  const sarifGroups = await upload_lib.getGroupedSarifFilePaths(
+    logger,
+    sarifPath,
+  );
 
   const uploadResults: UploadSarifResults = {};
-  const uploadResult = await findAndUpload(
-    logger,
-    features,
-    sarifPath,
-    pathStats,
-    checkoutPath,
-    analyses.CodeScanning,
-    category,
-  );
-  if (uploadResult !== undefined) {
-    uploadResults[analyses.AnalysisKind.CodeScanning] = uploadResult;
-  }
-
-  // If there are `.quality.sarif` files in `sarifPath`, then upload those to the code quality service.
-  const qualityUploadResult = await findAndUpload(
-    logger,
-    features,
-    sarifPath,
-    pathStats,
-    checkoutPath,
-    analyses.CodeQuality,
-    actionsUtil.fixCodeQualityCategory(logger, category),
-  );
-  if (qualityUploadResult !== undefined) {
-    uploadResults[analyses.AnalysisKind.CodeQuality] = qualityUploadResult;
+  for (const [analysisKind, sarifFiles] of unsafeEntriesInvariant(
+    sarifGroups,
+  )) {
+    const analysisConfig = analyses.getAnalysisConfig(analysisKind);
+    uploadResults[analysisKind] = await upload_lib.uploadSpecifiedFiles(
+      sarifFiles,
+      checkoutPath,
+      category,
+      features,
+      logger,
+      analysisConfig,
+    );
   }
 
   return uploadResults;

src/util.ts

@@ -1287,3 +1287,27 @@ export async function asyncSome<T>(
 export function isDefined<T>(value: T | null | undefined): value is T {
   return value !== undefined && value !== null;
 }
+
+/** Like `Object.keys`, but typed so that the elements of the resulting array have the
+ * same type as the keys of the input object. Note that this may not be sound if the input
+ * object has been cast to `T` from a subtype of `T` and contains additional keys that
+ * are not represented by `keyof T`.
+ */
+export function unsafeKeysInvariant<T extends Record<string, any>>(
+  object: T,
+): Array<keyof T> {
+  return Object.keys(object) as Array<keyof T>;
+}
+
+/** Like `Object.entries`, but typed so that the key elements of the result have the
+ * same type as the keys of the input object. Note that this may not be sound if the input
+ * object has been cast to `T` from a subtype of `T` and contains additional keys that
+ * are not represented by `keyof T`.
+ */
+export function unsafeEntriesInvariant<T extends Record<string, any>>(
+  object: T,
+): Array<[keyof T, Exclude<T[keyof T], undefined>]> {
+  return Object.entries(object).filter(
+    ([_, val]) => val !== undefined,
+  ) as Array<[keyof T, Exclude<T[keyof T], undefined>]>;
+}