diff --git a/src/Sarif.Multitool.Library/MergeCommand.cs b/src/Sarif.Multitool.Library/MergeCommand.cs index 65a7abd27..f3c5110c9 100644 --- a/src/Sarif.Multitool.Library/MergeCommand.cs +++ b/src/Sarif.Multitool.Library/MergeCommand.cs @@ -33,14 +33,12 @@ public class MergeCommand : CommandBase private readonly List _toolKeyOrder; private readonly Dictionary _toolKeyToMergedRun; private readonly Dictionary _toolKeyToVisitor; - private readonly Dictionary> _toolKeyToResults; public MergeCommand(IFileSystem fileSystem = null) : base(fileSystem) { _toolKeyOrder = new List(); _toolKeyToMergedRun = new Dictionary(); _toolKeyToVisitor = new Dictionary(); - _toolKeyToResults = new Dictionary>(); } public int Run(MergeOptions mergeOptions) @@ -163,7 +161,7 @@ private async Task MergeSarifLogsAsync() if (!_toolKeyToVisitor.TryGetValue(toolKey, out RunMergingVisitor visitor)) { visitor = _toolKeyToVisitor[toolKey] = new RunMergingVisitor(); - _toolKeyToResults[toolKey] = new HashSet(Result.ValueComparer); + visitor.DeduplicateResults = true; _toolKeyOrder.Add(toolKey); // The first run of a given tool + version supplies the merged run's @@ -178,17 +176,8 @@ private async Task MergeSarifLogsAsync() continue; } - HashSet seenResults = _toolKeyToResults[toolKey]; foreach (Result result in run.Results) { - // Drop results that are value-identical to one already merged for this - // tool. A sharded scan can re-report the same finding in more than one - // input log; the merged run should carry each finding exactly once. - if (!seenResults.Add(result)) - { - continue; - } - visitor.CurrentRun = run; visitor.VisitResult(result.DeepClone()); } diff --git a/src/Sarif/Visitors/RunMergingVisitor.cs b/src/Sarif/Visitors/RunMergingVisitor.cs index 6edc98e85..0d92de980 100644 --- a/src/Sarif/Visitors/RunMergingVisitor.cs +++ b/src/Sarif/Visitors/RunMergingVisitor.cs @@ -31,6 +31,7 @@ public class RunMergingVisitor : SarifRewritingVisitor private List LogicalLocations { get; } private List Rules { get; } private List Invocations { get; } + private HashSet SeenResults { get; } private Dictionary RuleIdToIndex { get; } private Dictionary, int> LogicalLocationToIndex { get; } @@ -38,6 +39,7 @@ public class RunMergingVisitor : SarifRewritingVisitor private Dictionary InvocationBaseIndexByRun { get; } public Run CurrentRun { get; set; } + public bool DeduplicateResults { get; set; } public RunMergingVisitor() { @@ -46,6 +48,7 @@ public RunMergingVisitor() LogicalLocations = new List(); Rules = new List(); Invocations = new List(); + SeenResults = new HashSet(Result.ValueComparer); RuleIdToIndex = new Dictionary(); LogicalLocationToIndex = new Dictionary, int>(); @@ -117,7 +120,10 @@ public override Result VisitResult(Result node) RemapInvocationIndex(node); Result result = base.VisitResult(node); - Results.Add(result); + if (!DeduplicateResults || SeenResults.Add(result)) + { + Results.Add(result); + } return result; } diff --git a/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs b/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs index d1238e0cf..b4beae59c 100644 --- a/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs +++ b/src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandTests.cs @@ -3,10 +3,12 @@ using System; using System.IO; +using System.Linq; using FluentAssertions; using Moq; +using Newtonsoft.Json.Linq; using Xunit; using Xunit.Abstractions; @@ -46,6 +48,55 @@ public void MergeCommand_WhenThereAreDuplicatedResults_ProducesNonDuplicatedResu RunTest("DuplicatedResults.sarif"); } + [Fact] + public void MergeCommand_WhenDuplicateResultsUseDifferentLocalIndices_ProducesOneResult() + { + string inputFolderPath = Directory.GetCurrentDirectory(); + string outputFileName = Guid.NewGuid().ToString() + SarifConstants.SarifFileExtension; + string outputFilePath = Path.Combine(TestOutputDirectory, outputFileName); + var mockFileSystem = new Mock(); + + mockFileSystem.Setup(x => x.FileExists(outputFilePath)).Returns(false); + mockFileSystem.Setup(x => x.DirectoryExists(inputFolderPath)).Returns(true); + mockFileSystem + .Setup(x => x.DirectoryEnumerateFiles(inputFolderPath, "*.sarif", SearchOption.TopDirectoryOnly)) + .Returns(new[] { "first_run.sarif", "second_run.sarif" }); + mockFileSystem + .Setup(x => x.FileReadAllText("first_run.sarif")) + .Returns(CreateSarifWithLocalIndices( + new[] { "rule-a", "rule-b" }, + new[] { "source/a.cpp", "source/b.cpp" }, + ruleIndex: 0, + artifactIndex: 0)); + mockFileSystem + .Setup(x => x.FileReadAllText("second_run.sarif")) + .Returns(CreateSarifWithLocalIndices( + new[] { "rule-b", "rule-a" }, + new[] { "source/b.cpp", "source/a.cpp" }, + ruleIndex: 1, + artifactIndex: 1)); + mockFileSystem + .Setup(x => x.DirectoryCreateDirectory(TestOutputDirectory)) + .Returns((string path) => Directory.CreateDirectory(path)); + mockFileSystem + .Setup(x => x.FileCreate(outputFilePath)) + .Returns((string path) => File.Create(path)); + + var options = new MergeOptions + { + OutputDirectoryPath = TestOutputDirectory, + TargetFileSpecifiers = new[] { "*.sarif" }, + OutputFileName = outputFileName, + OutputFileOptions = new[] { FilePersistenceOptions.ForceOverwrite, FilePersistenceOptions.PrettyPrint }, + }; + + int returnCode = new MergeCommand(mockFileSystem.Object).Run(options); + returnCode.Should().Be(0); + + JToken results = JObject.Parse(File.ReadAllText(outputFilePath))["runs"][0]["results"]; + results.Count().Should().Be(1); + } + [Fact] public void MergeCommand_WhenPassNoFolderOnlyFile_ProducesCorrectResults() { @@ -115,5 +166,54 @@ private void PrepareFileSystemMock(string inputResourceName, string inputFolderP return; } } + + private static string CreateSarifWithLocalIndices( + string[] ruleIds, + string[] artifactUris, + int ruleIndex, + int artifactIndex) + { + return JObject.FromObject(new + { + version = "2.1.0", + runs = new[] + { + new + { + tool = new + { + driver = new + { + name = "CodeQL", + version = "1.0", + rules = ruleIds.Select(id => new { id }).ToArray(), + }, + }, + artifacts = artifactUris + .Select(uri => new { location = new { uri } }) + .ToArray(), + results = new[] + { + new + { + ruleIndex, + message = new { text = "same finding" }, + locations = new[] + { + new + { + physicalLocation = new + { + artifactLocation = new { index = artifactIndex }, + region = new { startLine = 10, startColumn = 1 }, + }, + }, + }, + }, + }, + }, + }, + }).ToString(); + } } }