Work around duplicate locations bug in diagnostics export #1605
Conversation
Only tests the property we are looking for and avoids problems with different cross-platform behavior.
|
I haven't had a chance to review yet, but a general question. Are we fairly sure the fix will be available in the next CLI? If so, should we be gating this functionality on the CLI version as well? I.e., use the original behaviour of version is greater than 2.12.x? |
The fix has already been merged and patched in the CLI (I backlinked it as it's an internal PR), so I don't believe we need to add an additional gate. EDIT — I think I see what you mean: we only need to do this post-processing if we're at a version < than the 2.12.6. |
|
|
||
| // Fix invalid notifications in the SARIF file output by CodeQL. | ||
| let sarif = JSON.parse( | ||
| fs.readFileSync(intermediateSarifFile, "utf8") | ||
| ) as util.SarifFile; | ||
| sarif = util.fixInvalidNotifications(sarif, logger); | ||
| fs.writeFileSync(sarifFile, JSON.stringify(sarif)); |
There was a problem hiding this comment.
Would it be worthwhile to pull this out into a separate function to make sure that the logic remains the same between here and in diagnostics export?
| export const CODEQL_ACTION_DISABLE_DUPLICATE_LOCATION_FIX = | ||
| "CODEQL_ACTION_DISABLE_DUPLICATE_LOCATION_FIX"; |
There was a problem hiding this comment.
Is it worth turning this into a full feature flag so we can centrally manage enablement of the behaviour?
| ); | ||
| return sarif; | ||
| } | ||
| if (!(sarif.runs instanceof Array)) { |
There was a problem hiding this comment.
Minor: using Array.isArray is generally better. However, in this particular case instanceof is fine. It's only really necessary when using iframes in DOM.
| if (!(sarif.runs instanceof Array)) { | |
| if (!Array.isArray(sarif.runs)) { |
| runs: sarif.runs.map((run) => { | ||
| if ( | ||
| run.tool?.driver?.name !== "CodeQL" || | ||
| !(run.invocations instanceof Array) |
There was a problem hiding this comment.
minor: as above
| !(run.invocations instanceof Array) | |
| !(Array.isArray(run.invocations)) |
| return { | ||
| ...run, | ||
| invocations: run.invocations.map((invocation) => { | ||
| if (!(invocation.toolExecutionNotifications instanceof Array)) { |
There was a problem hiding this comment.
| if (!(invocation.toolExecutionNotifications instanceof Array)) { | |
| if (!Array.isArray(invocation.toolExecutionNotifications)) { |
| ...invocation, | ||
| toolExecutionNotifications: | ||
| invocation.toolExecutionNotifications.map((notification) => { | ||
| if (!(notification.locations instanceof Array)) { |
There was a problem hiding this comment.
| if (!(notification.locations instanceof Array)) { | |
| if (!Array.isArray(notification.locations)) { |
|
Thanks folks for the review. I'll address these improvements as followup to avoid blocking this on another review cycle. |
This PR works around a bug in diagnostics export that could lead to an invalid SARIF file being produced by
database interpret-resultsordatabase export-diagnostics.The bug lies in the CLI functionality that groups diagnostics together. When grouping together multiple diagnostics that have the same location
a, the CLI produced a single diagnostic with multiple identical locations[a, ..., a]. This is not valid SARIF, as the spec mandates that the locations for eachnotificationobject are distinct.To allow us to roll out diagnostics export without waiting for a new CLI release, we workaround this bug in the CodeQL Action. To do this, when diagnostics export is enabled, we go through the SARIF file produced by
database interpret-resultsanddatabase export-diagnosticsand remove any duplicate locations from theruns[].invocations[].toolExecutionNotificationsproperty.We add unit tests and integration tests, as well as an environment variable
CODEQL_ACTION_DISABLE_DUPLICATE_LOCATION_FIXto disable this functionality if unexpected problems arise.Merge / deployment checklist