Repository navigation
fix: Prevent JSONException when SARIF invocations array is empty - #353
Conversation
darkspirit510
left a comment
There was a problem hiding this comment.
Good catch! I wonder how you actually found this one? Just one minor code style question. Else it is fine.
| && firstInvocation(resultFile).has("startTimeUtc") | ||
| && firstInvocation(resultFile).has("endTimeUtc"); | ||
| JSONObject firstRun = firstRun(resultFile); | ||
| if (firstRun.has("invocations") && !firstRun.getJSONArray("invocations").isEmpty()) { |
There was a problem hiding this comment.
What about either using guard statements
private static boolean hasInvocationTimes(ResultFile resultFile) {
JSONObject firstRun = firstRun(resultFile);
if (!firstRun.has("invocations")) {
return false;
}
JSONArray invocations = firstRun.getJSONArray("invocations");
if (invocations.isEmpty()) {
return false;
}
JSONObject firstInvocation = invocations.getJSONObject(0);
return firstInvocation.has("startTimeUtc") && firstInvocation.has("endTimeUtc");
}
or implicit return value
private static boolean hasInvocationTimes(ResultFile resultFile) {
JSONObject firstRun = firstRun(resultFile);
return firstRun.has("invocations")
&& !firstRun.getJSONArray("invocations").isEmpty()
&& firstRun.getJSONArray("invocations").getJSONObject(0).has("startTimeUtc")
&& firstRun.getJSONArray("invocations").getJSONObject(0).has("endTimeUtc");
}
instead?
There was a problem hiding this comment.
Done, went with the first option. Tests are green.
e0a49c8 to
ee44e89
Compare
|
Thanks! I'm currently working on adding support for your benchmark to PVS-Studio. We convert our reports from JSON to SARIF with a separate utility, and that utility has no idea how long the analysis took, so it leaves the Really cool benchmark, by the way :) If this PR gets merged, I'll open another one with a reader for the PVS-Studio analyzer itself. |
darkspirit510
left a comment
There was a problem hiding this comment.
@davewichers can be merged.
Problem
SarifReader.hasInvocationTimes()checked thatinvocationsexists but not that it has any elements. Some tools (e.g. PVS-Studio) emit"invocations": [], sofirstInvocation()threw aJSONExceptionand parsing of the whole report failed.Fix
hasInvocationTimes()now returnsfalsewhen theinvocationsarray is empty, so the reader uses the same fallback as wheninvocationsis missing: the scan time is taken from the result file name if it ends with-<seconds>, otherwise it is reported as "Time not specified". Findings are parsed normally in both cases.This affects all SARIF-based readers.
Tests
SarifReaderTest#fallsBackToFilenameTimeWhenInvocationsArrayIsEmpty: it parses a SARIF report with an emptyinvocationsarray and checks that the time comes from the file name.