Work around duplicate locations bug in diagnostics export - #1605
henrymercer merged 6 commits into
Conversation
Sorry, something went wrong.
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? |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
Looked through and makes sense to me. Approving but will wait for Andrew's review too!
Sorry, something went wrong.
|
|
||
| // 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?
Sorry, something went wrong.
aeisenberg
left a comment
There was a problem hiding this comment.
I have a handful of questions and a stylistic suggestion. Though, I think this generally looks good assuming we don't want a full feature flag.
Sorry, something went wrong.
| 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?
Sorry, something went wrong.
| ); | ||
| 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)) { |
Sorry, something went wrong.
| 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)) |
Sorry, something went wrong.
| 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)) { |
Sorry, something went wrong.
| ...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)) { |
Sorry, something went wrong.
|
Thanks folks for the review. I'll address these improvements as followup to avoid blocking this on another review cycle. |
Sorry, something went wrong.
This PR works around a bug in diagnostics export that could lead to an invalid SARIF file being produced by database interpret-results or database 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 each notification object 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-results and database export-diagnostics and remove any duplicate locations from the runs[].invocations[].toolExecutionNotifications property.
We add unit tests and integration tests, as well as an environment variable CODEQL_ACTION_DISABLE_DUPLICATE_LOCATION_FIX to disable this functionality if unexpected problems arise.
Merge / deployment checklist