Artifacts should only be logged in the SARIF if we have results #2431
Conversation
| dataToInsert: dataToInsert, | ||
| encoding: encoding, | ||
| hashData: hashData); | ||
| } |
There was a problem hiding this comment.
This code path is used by the single threaded analysis. When hashes is enabled, it was going to generate all hashes and store everything in the SARIF.
The issue is that if we analyze 1k files and we do not produce any results, we would still generate the artifacts.
As an optimization, we will only store the artifact if we have a result.
There was a problem hiding this comment.
Reviewed all versions that we released and I saw that all versions after 1.4.2 are emitting the artifacts IF the list of analysisTargets is not empty.
There was a problem hiding this comment.
Looking at the other variables, looks like we tried to improve this behavior by adding the _persistArtifacts, but that is occurring too late (after we already added the artifact to the run)
| @@ -447,12 +411,16 @@ private void CaptureArtifact(ArtifactLocation fileLocation) | |||
| catch (ArgumentException) { } // Unrecognized encoding name | |||
| } | |||
|
|
|||
| HashData hashData = null; | |||
| AnalysisTargetToHashDataMap?.TryGetValue(fileLocation.Uri.OriginalString, out hashData); | |||
There was a problem hiding this comment.
AnalysisTargetToHashDataMap?.TryGetValue(fileLocation.Uri.OriginalString, out hashData);
For the single threaded analysis, the AnalysisTargetToHashDataMap will have the hashes and here, we will just use the information that we already have and store in the artifacts.
The CaptureArtifact method is only used when we are logging the artifacts of a result.
There was a problem hiding this comment.
If hashes isn't enabled, analysisTargetToHashDataMap will be null, so we are using the null check to guarantee and prevent a null reference exception.
| @@ -485,10 +492,6 @@ private async Task<bool> HashAsync() | |||
| _hashToFilesMap[hashData.Sha256] = paths; | |||
| } | |||
|
|
|||
| _run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri }, | |||
| dataToInsert: _dataToInsert, | |||
| hashData: hashData); | |||
There was a problem hiding this comment.
instead of adding artifacts for all files, we are moving this to the place when we have an actual result.
The hash itself will already be in the context, so, no need to calculate it again.
| @@ -1258,21 +1258,93 @@ private void AnalyzeScenarios(int[] scenarios) | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void AnalyzeCommandBase_MultithreadedShouldUseCacheIfFilesAreTheSame() | |||
| public void AnalyzeCommandBase_Multithreaded_ShouldOnlyLogArtifactsWhenHashesIsEnabled() | |||
| } | ||
|
|
||
| [Fact] | ||
| public void AnalyzeCommmandBase_SingleThreaded_ShouldOnlyLogArtifactsWhenHashesIsEnabled() |
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
we require a result with a location to see the artifacts.
| levels: new List<FailureLevel> { FailureLevel.Warning, FailureLevel.Error }, | ||
| kinds: new List<ResultKind> { ResultKind.Fail })) | ||
| { | ||
| } |
There was a problem hiding this comment.
no results are being emitted, with that, no artifacts should be logged. This test was wrong.
|
This PR got replaced by this: #2433 |
Description
During some analysis, we saw that we are emitting an artifact even if we don't have a result. This is not good because it will make the SARIF gigantic in case you analyze hundreds/thousands of files.
Proposal
We will only emit an artifact if we have a result.
Tests
Created one 'file' of each level/kind and checked the SARIF output. It should only generate a list of artifacts equal to the locations you have in the results.