Merge pull request #3222 from github/mbg/upload-lib/post-process

Perform SARIF post-processing independently of upload

src/analyze-action.ts

@@ -52,7 +52,7 @@ import {
 } from "./trap-caching";
 import * as uploadLib from "./upload-lib";
 import { UploadResult } from "./upload-lib";
-import { uploadSarif } from "./upload-sarif";
+import { postProcessAndUploadSarif } from "./upload-sarif";
 import * as util from "./util";
 
 interface AnalysisStatusReport
@@ -344,20 +344,24 @@ async function run() {
     }
     core.setOutput("db-locations", dbLocations);
     core.setOutput("sarif-output", path.resolve(outputDir));
-    const uploadInput = actionsUtil.getOptionalInput("upload");
-    if (runStats && actionsUtil.getUploadValue(uploadInput) === "always") {
+    const uploadKind = actionsUtil.getUploadValue(
+      actionsUtil.getOptionalInput("upload"),
+    );
+    if (runStats) {
       const checkoutPath = actionsUtil.getRequiredInput("checkout_path");
       const category = actionsUtil.getOptionalInput("category");
 
       if (await features.getValue(Feature.AnalyzeUseNewUpload)) {
-        uploadResults = await uploadSarif(
+        uploadResults = await postProcessAndUploadSarif(
           logger,
           features,
+          uploadKind,
           checkoutPath,
           outputDir,
           category,
+          actionsUtil.getOptionalInput("post-processed-sarif-path"),
         );
-      } else {
+      } else if (uploadKind === "always") {
         uploadResults = {};
 
         if (isCodeScanningEnabled(config)) {
@@ -383,6 +387,9 @@ async function run() {
               analyses.CodeQuality,
             );
         }
+      } else {
+        uploadResults = {};
+        logger.info("Not uploading results");
       }
 
       // Set the SARIF id outputs only if we have results for them, to avoid

src/upload-lib.ts

@@ -688,51 +688,39 @@ export function buildPayload(
   return payloadObj;
 }
 
-/**
- * Uploads a single SARIF file or a directory of SARIF files depending on what `inputSarifPath` refers
- * to.
- */
-export async function uploadFiles(
-  inputSarifPath: string,
-  checkoutPath: string,
-  category: string | undefined,
-  features: FeatureEnablement,
-  logger: Logger,
-  uploadTarget: analyses.AnalysisConfig,
-): Promise<UploadResult> {
-  const sarifPaths = getSarifFilePaths(
-    inputSarifPath,
-    uploadTarget.sarifPredicate,
-  );
-
-  return uploadSpecifiedFiles(
-    sarifPaths,
-    checkoutPath,
-    category,
-    features,
-    logger,
-    uploadTarget,
-  );
+export interface PostProcessingResults {
+  sarif: util.SarifFile;
+  analysisKey: string;
+  environment: string;
 }
 
 /**
- * Uploads the given array of SARIF files.
+ * Performs post-processing of the SARIF files given by `sarifPaths`.
+ *
+ * @param logger The logger to use.
+ * @param features Information about enabled features.
+ * @param checkoutPath The path where the repo was checked out at.
+ * @param sarifPaths The paths of the SARIF files to post-process.
+ * @param category The analysis category.
+ * @param analysis The analysis configuration.
+ *
+ * @returns Returns the results of post-processing the SARIF files,
+ *          including the resulting SARIF file.
  */
-export async function uploadSpecifiedFiles(
-  sarifPaths: string[],
-  checkoutPath: string,
-  category: string | undefined,
-  features: FeatureEnablement,
+export async function postProcessSarifFiles(
   logger: Logger,
-  uploadTarget: analyses.AnalysisConfig,
-): Promise<UploadResult> {
-  logger.startGroup(`Uploading ${uploadTarget.name} results`);
-  logger.info(`Processing sarif files: ${JSON.stringify(sarifPaths)}`);
+  features: FeatureEnablement,
+  checkoutPath: string,
+  sarifPaths: string[],
+  category: string | undefined,
+  analysis: analyses.AnalysisConfig,
+): Promise<PostProcessingResults> {
+  logger.info(`Post-processing sarif files: ${JSON.stringify(sarifPaths)}`);
 
   const gitHubVersion = await getGitHubVersion();
 
   let sarif: SarifFile;
-  category = uploadTarget.fixCategory(logger, category);
+  category = analysis.fixCategory(logger, category);
 
   if (sarifPaths.length > 1) {
     // Validate that the files we were asked to upload are all valid SARIF files
@@ -768,6 +756,113 @@ export async function uploadSpecifiedFiles(
     environment,
   );
 
+  return { sarif, analysisKey, environment };
+}
+
+/**
+ * Writes the post-processed SARIF file to disk, if needed based on `pathInput` or the `SARIF_DUMP_DIR`.
+ *
+ * @param logger The logger to use.
+ * @param pathInput The input provided for `post-processed-sarif-path`.
+ * @param uploadTarget The upload target.
+ * @param processingResults The results of post-processing SARIF files.
+ */
+export async function writePostProcessedFiles(
+  logger: Logger,
+  pathInput: string | undefined,
+  uploadTarget: analyses.AnalysisConfig,
+  postProcessingResults: PostProcessingResults,
+) {
+  // If there's an explicit input, use that. Otherwise, use the value from the environment variable.
+  const outputPath = pathInput || process.env[EnvVar.SARIF_DUMP_DIR];
+
+  // If we have a non-empty output path, write the SARIF file to it.
+  if (outputPath !== undefined && outputPath.trim() !== "") {
+    dumpSarifFile(
+      JSON.stringify(postProcessingResults.sarif),
+      outputPath,
+      logger,
+      uploadTarget,
+    );
+  } else {
+    logger.debug(`Not writing post-processed SARIF files.`);
+  }
+}
+
+/**
+ * Uploads a single SARIF file or a directory of SARIF files depending on what `inputSarifPath` refers
+ * to.
+ */
+export async function uploadFiles(
+  inputSarifPath: string,
+  checkoutPath: string,
+  category: string | undefined,
+  features: FeatureEnablement,
+  logger: Logger,
+  uploadTarget: analyses.AnalysisConfig,
+): Promise<UploadResult> {
+  const sarifPaths = getSarifFilePaths(
+    inputSarifPath,
+    uploadTarget.sarifPredicate,
+  );
+
+  return uploadSpecifiedFiles(
+    sarifPaths,
+    checkoutPath,
+    category,
+    features,
+    logger,
+    uploadTarget,
+  );
+}
+
+/**
+ * Uploads the given array of SARIF files.
+ */
+async function uploadSpecifiedFiles(
+  sarifPaths: string[],
+  checkoutPath: string,
+  category: string | undefined,
+  features: FeatureEnablement,
+  logger: Logger,
+  uploadTarget: analyses.AnalysisConfig,
+): Promise<UploadResult> {
+  const processingResults: PostProcessingResults = await postProcessSarifFiles(
+    logger,
+    features,
+    checkoutPath,
+    sarifPaths,
+    category,
+    uploadTarget,
+  );
+
+  return uploadPostProcessedFiles(
+    logger,
+    checkoutPath,
+    uploadTarget,
+    processingResults,
+  );
+}
+
+/**
+ * Uploads the results of post-processing SARIF files to the specified upload target.
+ *
+ * @param logger The logger to use.
+ * @param checkoutPath The path at which the repository was checked out.
+ * @param uploadTarget The analysis configuration.
+ * @param postProcessingResults The results of post-processing SARIF files.
+ *
+ * @returns The results of uploading the `postProcessingResults` to `uploadTarget`.
+ */
+export async function uploadPostProcessedFiles(
+  logger: Logger,
+  checkoutPath: string,
+  uploadTarget: analyses.AnalysisConfig,
+  postProcessingResults: PostProcessingResults,
+): Promise<UploadResult> {
+  logger.startGroup(`Uploading ${uploadTarget.name} results`);
+
+  const sarif = postProcessingResults.sarif;
   const toolNames = util.getToolNames(sarif);
 
   logger.debug(`Validating that each SARIF run has a unique category`);
@@ -775,11 +870,6 @@ export async function uploadSpecifiedFiles(
   logger.debug(`Serializing SARIF for upload`);
   const sarifPayload = JSON.stringify(sarif);
 
-  const dumpDir = process.env[EnvVar.SARIF_DUMP_DIR];
-  if (dumpDir) {
-    dumpSarifFile(sarifPayload, dumpDir, logger, uploadTarget);
-  }
-
   logger.debug(`Compressing serialized SARIF`);
   const zippedSarif = zlib.gzipSync(sarifPayload).toString("base64");
   const checkoutURI = url.pathToFileURL(checkoutPath).href;
@@ -787,13 +877,13 @@ export async function uploadSpecifiedFiles(
   const payload = buildPayload(
     await gitUtils.getCommitOid(checkoutPath),
     await gitUtils.getRef(),
-    analysisKey,
+    postProcessingResults.analysisKey,
     util.getRequiredEnvParam("GITHUB_WORKFLOW"),
     zippedSarif,
     actionsUtil.getWorkflowRunID(),
     actionsUtil.getWorkflowRunAttempt(),
     checkoutURI,
-    environment,
+    postProcessingResults.environment,
     toolNames,
     await gitUtils.determineBaseBranchHeadCommitOid(),
   );
@@ -839,14 +929,14 @@ function dumpSarifFile(
     fs.mkdirSync(outputDir, { recursive: true });
   } else if (!fs.lstatSync(outputDir).isDirectory()) {
     throw new ConfigurationError(
-      `The path specified by the ${EnvVar.SARIF_DUMP_DIR} environment variable exists and is not a directory: ${outputDir}`,
+      `The path that processed SARIF files should be written to exists, but is not a directory: ${outputDir}`,
     );
   }
   const outputFile = path.resolve(
     outputDir,
     `upload${uploadTarget.sarifExtension}`,
   );
-  logger.info(`Dumping processed SARIF file to ${outputFile}`);
+  logger.info(`Writing processed SARIF file to ${outputFile}`);
   fs.writeFileSync(outputFile, sarifPayload);
 }
 

src/upload-sarif-action.ts

@@ -16,7 +16,7 @@ import {
   isThirdPartyAnalysis,
 } from "./status-report";
 import * as upload_lib from "./upload-lib";
-import { uploadSarif } from "./upload-sarif";
+import { postProcessAndUploadSarif } from "./upload-sarif";
 import {
   ConfigurationError,
   checkActionVersion,
@@ -90,9 +90,10 @@ async function run() {
     const checkoutPath = actionsUtil.getRequiredInput("checkout_path");
     const category = actionsUtil.getOptionalInput("category");
 
-    const uploadResults = await uploadSarif(
+    const uploadResults = await postProcessAndUploadSarif(
       logger,
       features,
+      "always",
       checkoutPath,
       sarifPath,
       category,

src/upload-sarif.test.ts

@@ -9,7 +9,7 @@ import { getRunnerLogger } from "./logging";
 import { createFeatures, setupTests } from "./testing-utils";
 import { UploadResult } from "./upload-lib";
 import * as uploadLib from "./upload-lib";
-import { uploadSarif } from "./upload-sarif";
+import { postProcessAndUploadSarif } from "./upload-sarif";
 import * as util from "./util";
 
 setupTests(test);
@@ -19,7 +19,27 @@ interface UploadSarifExpectedResult {
   expectedFiles?: string[];
 }
 
-const uploadSarifMacro = test.macro({
+function mockPostProcessSarifFiles() {
+  const postProcessSarifFiles = sinon.stub(uploadLib, "postProcessSarifFiles");
+
+  for (const analysisKind of Object.values(AnalysisKind)) {
+    const analysisConfig = getAnalysisConfig(analysisKind);
+    postProcessSarifFiles
+      .withArgs(
+        sinon.match.any,
+        sinon.match.any,
+        sinon.match.any,
+        sinon.match.any,
+        sinon.match.any,
+        analysisConfig,
+      )
+      .resolves({ sarif: { runs: [] }, analysisKey: "", environment: "" });
+  }
+
+  return postProcessSarifFiles;
+}
+
+const postProcessAndUploadSarifMacro = test.macro({
   exec: async (
     t: ExecutionContext<unknown>,
     sarifFiles: string[],
@@ -33,21 +53,16 @@ const uploadSarifMacro = test.macro({
 
       const toFullPath = (filename: string) => path.join(tempDir, filename);
 
-      const uploadSpecifiedFiles = sinon.stub(
+      const postProcessSarifFiles = mockPostProcessSarifFiles();
+      const uploadPostProcessedFiles = sinon.stub(
         uploadLib,
-        "uploadSpecifiedFiles",
+        "uploadPostProcessedFiles",
       );
 
       for (const analysisKind of Object.values(AnalysisKind)) {
-        uploadSpecifiedFiles
-          .withArgs(
-            sinon.match.any,
-            sinon.match.any,
-            sinon.match.any,
-            features,
-            logger,
-            getAnalysisConfig(analysisKind),
-          )
+        const analysisConfig = getAnalysisConfig(analysisKind);
+        uploadPostProcessedFiles
+          .withArgs(logger, sinon.match.any, analysisConfig, sinon.match.any)
           .resolves(expectedResult[analysisKind as AnalysisKind]?.uploadResult);
       }
 
@@ -56,53 +71,57 @@ const uploadSarifMacro = test.macro({
         fs.writeFileSync(sarifFile, "");
       }
 
-      const actual = await uploadSarif(logger, features, "", testPath);
+      const actual = await postProcessAndUploadSarif(
+        logger,
+        features,
+        "always",
+        "",
+        testPath,
+      );
 
       for (const analysisKind of Object.values(AnalysisKind)) {
         const analysisKindResult = expectedResult[analysisKind];
         if (analysisKindResult) {
           // We are expecting a result for this analysis kind, check that we have it.
           t.deepEqual(actual[analysisKind], analysisKindResult.uploadResult);
-          // Additionally, check that the mocked `uploadSpecifiedFiles` was called with only the file paths
+          // Additionally, check that the mocked `postProcessSarifFiles` was called with only the file paths
           // that we expected it to be called with.
           t.assert(
-            uploadSpecifiedFiles.calledWith(
+            postProcessSarifFiles.calledWith(
+              logger,
+              features,
+              sinon.match.any,
               analysisKindResult.expectedFiles?.map(toFullPath) ??
                 fullSarifPaths,
               sinon.match.any,
-              sinon.match.any,
-              features,
-              logger,
               getAnalysisConfig(analysisKind),
             ),
           );
         } else {
           // Otherwise, we are not expecting a result for this analysis kind. However, note that `undefined`
-          // is also returned by our mocked `uploadSpecifiedFiles` when there is no expected result for this
+          // is also returned by our mocked `uploadProcessedFiles` when there is no expected result for this
           // analysis kind.
           t.is(actual[analysisKind], undefined);
-          // Therefore, we also check that the mocked `uploadSpecifiedFiles` was not called for this analysis kind.
+          // Therefore, we also check that the mocked `uploadProcessedFiles` was not called for this analysis kind.
           t.assert(
-            !uploadSpecifiedFiles.calledWith(
-              sinon.match.any,
-              sinon.match.any,
-              sinon.match.any,
-              features,
+            !uploadPostProcessedFiles.calledWith(
               logger,
+              sinon.match.any,
               getAnalysisConfig(analysisKind),
+              sinon.match.any,
             ),
-            `uploadSpecifiedFiles was called for ${analysisKind}, but should not have been.`,
+            `uploadProcessedFiles was called for ${analysisKind}, but should not have been.`,
           );
         }
       }
     });
   },
-  title: (providedTitle = "") => `uploadSarif - ${providedTitle}`,
+  title: (providedTitle = "") => `processAndUploadSarif - ${providedTitle}`,
 });
 
 test(
   "SARIF file",
-  uploadSarifMacro,
+  postProcessAndUploadSarifMacro,
   ["test.sarif"],
   (tempDir) => path.join(tempDir, "test.sarif"),
   {
@@ -117,7 +136,7 @@ test(
 
 test(
   "JSON file",
-  uploadSarifMacro,
+  postProcessAndUploadSarifMacro,
   ["test.json"],
   (tempDir) => path.join(tempDir, "test.json"),
   {
@@ -132,7 +151,7 @@ test(
 
 test(
   "Code Scanning files",
-  uploadSarifMacro,
+  postProcessAndUploadSarifMacro,
   ["test.json", "test.sarif"],
   undefined,
   {
@@ -148,7 +167,7 @@ test(
 
 test(
   "Code Quality file",
-  uploadSarifMacro,
+  postProcessAndUploadSarifMacro,
   ["test.quality.sarif"],
   (tempDir) => path.join(tempDir, "test.quality.sarif"),
   {
@@ -163,7 +182,7 @@ test(
 
 test(
   "Mixed files",
-  uploadSarifMacro,
+  postProcessAndUploadSarifMacro,
   ["test.sarif", "test.quality.sarif"],
   undefined,
   {
@@ -183,3 +202,65 @@ test(
     },
   },
 );
+
+test("postProcessAndUploadSarif doesn't upload if upload is disabled", async (t) => {
+  await util.withTmpDir(async (tempDir) => {
+    const logger = getRunnerLogger(true);
+    const features = createFeatures([]);
+
+    const toFullPath = (filename: string) => path.join(tempDir, filename);
+
+    const postProcessSarifFiles = mockPostProcessSarifFiles();
+    const uploadPostProcessedFiles = sinon.stub(
+      uploadLib,
+      "uploadPostProcessedFiles",
+    );
+
+    fs.writeFileSync(toFullPath("test.sarif"), "");
+    fs.writeFileSync(toFullPath("test.quality.sarif"), "");
+
+    const actual = await postProcessAndUploadSarif(
+      logger,
+      features,
+      "never",
+      "",
+      tempDir,
+    );
+
+    t.truthy(actual);
+    t.assert(postProcessSarifFiles.calledTwice);
+    t.assert(uploadPostProcessedFiles.notCalled);
+  });
+});
+
+test("postProcessAndUploadSarif writes post-processed SARIF files if output directory is provided", async (t) => {
+  await util.withTmpDir(async (tempDir) => {
+    const logger = getRunnerLogger(true);
+    const features = createFeatures([]);
+
+    const toFullPath = (filename: string) => path.join(tempDir, filename);
+
+    const postProcessSarifFiles = mockPostProcessSarifFiles();
+
+    fs.writeFileSync(toFullPath("test.sarif"), "");
+    fs.writeFileSync(toFullPath("test.quality.sarif"), "");
+
+    const postProcessedOutPath = path.join(tempDir, "post-processed");
+    const actual = await postProcessAndUploadSarif(
+      logger,
+      features,
+      "never",
+      "",
+      tempDir,
+      "",
+      postProcessedOutPath,
+    );
+
+    t.truthy(actual);
+    t.assert(postProcessSarifFiles.calledTwice);
+    t.assert(fs.existsSync(path.join(postProcessedOutPath, "upload.sarif")));
+    t.assert(
+      fs.existsSync(path.join(postProcessedOutPath, "upload.quality.sarif")),
+    );
+  });
+});

src/upload-sarif.ts

@@ -1,3 +1,4 @@
+import { UploadKind } from "./actions-util";
 import * as analyses from "./analyses";
 import { FeatureEnablement } from "./feature-flags";
 import { Logger } from "./logging";
@@ -10,22 +11,26 @@ export type UploadSarifResults = Partial<
 >;
 
 /**
- * Finds SARIF files in `sarifPath` and uploads them to the appropriate services.
+ * Finds SARIF files in `sarifPath`, post-processes them, and uploads them to the appropriate services.
  *
  * @param logger The logger to use.
  * @param features Information about enabled features.
+ * @param uploadKind The kind of upload that is requested.
  * @param checkoutPath The path where the repository was checked out at.
  * @param sarifPath The path to the file or directory to upload.
  * @param category The analysis category.
+ * @param postProcessedOutputPath The path to a directory to which the post-processed SARIF files should be written to.
  *
  * @returns A partial mapping from analysis kinds to the upload results.
  */
-export async function uploadSarif(
+export async function postProcessAndUploadSarif(
   logger: Logger,
   features: FeatureEnablement,
+  uploadKind: UploadKind,
   checkoutPath: string,
   sarifPath: string,
   category?: string,
+  postProcessedOutputPath?: string,
 ): Promise<UploadSarifResults> {
   const sarifGroups = await upload_lib.getGroupedSarifFilePaths(
     logger,
@@ -37,14 +42,33 @@ export async function uploadSarif(
     sarifGroups,
   )) {
     const analysisConfig = analyses.getAnalysisConfig(analysisKind);
-    uploadResults[analysisKind] = await upload_lib.uploadSpecifiedFiles(
-      sarifFiles,
-      checkoutPath,
-      category,
-      features,
+    const postProcessingResults = await upload_lib.postProcessSarifFiles(
       logger,
+      features,
+      checkoutPath,
+      sarifFiles,
+      category,
       analysisConfig,
     );
+
+    // Write the post-processed SARIF files to disk. This will only write them if needed based on user inputs
+    // or environment variables.
+    await upload_lib.writePostProcessedFiles(
+      logger,
+      postProcessedOutputPath,
+      analysisConfig,
+      postProcessingResults,
+    );
+
+    // Only perform the actual upload of the post-processed files if `uploadKind` is `always`.
+    if (uploadKind === "always") {
+      uploadResults[analysisKind] = await upload_lib.uploadPostProcessedFiles(
+        logger,
+        checkoutPath,
+        analysisConfig,
+        postProcessingResults,
+      );
+    }
   }
 
   return uploadResults;