From b19a692d50b5d397991c24587f0bf78b8cf43ac7 Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Wed, 20 Jul 2022 15:23:38 -0700 Subject: [PATCH 1/7] ResultMatching bug fixes --- .../ResultMatching/SarifLogResultMatcher.cs | 20 +++++ src/Sarif/Visitors/RunMergingVisitor.cs | 23 +++++- .../SarifLogResultMatcherTests.cs | 48 +++++++++++ ...itorTests.cs => RunMergingVisitorTests.cs} | 80 +++++++++++++++++++ 4 files changed, 167 insertions(+), 4 deletions(-) rename src/Test.UnitTests.Sarif/Visitors/{RemapIndicesVisitorTests.cs => RunMergingVisitorTests.cs} (80%) diff --git a/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs b/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs index bd5778480..0407d679b 100644 --- a/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs +++ b/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs @@ -278,6 +278,7 @@ private SarifLog ConstructSarifLogFromMatchedResults( run.Graphs = graphs; run.Invocations = invocations; run.Properties = properties; + run.VersionControlProvenance = MergeVersionControlDetails(currentRuns.Concat(previousRuns)); return new SarifLog() { @@ -343,5 +344,24 @@ internal static void MergeDictionaryInto( } } } + + private static IList MergeVersionControlDetails(IEnumerable runs) + { + // use HashSet with EqualityComparer to avoid duplicated entries + var versionControlSet = new HashSet(VersionControlDetails.ValueComparer); + foreach (Run run in runs) + { + if (run.VersionControlProvenance == null) + { + continue; + } + + foreach (VersionControlDetails versionControlProvenance in run.VersionControlProvenance) + { + versionControlSet.Add(versionControlProvenance); + } + } + return versionControlSet.Any() ? versionControlSet.ToList() : null; + } } } diff --git a/src/Sarif/Visitors/RunMergingVisitor.cs b/src/Sarif/Visitors/RunMergingVisitor.cs index 527c118c3..e1f6dd01b 100644 --- a/src/Sarif/Visitors/RunMergingVisitor.cs +++ b/src/Sarif/Visitors/RunMergingVisitor.cs @@ -78,7 +78,23 @@ public override Result VisitResult(Result node) } // Cache RuleId and set Result.RuleIndex to the (new) index - string ruleId = node.ResolvedRuleId(CurrentRun); + string resultRuleId = node.ResolvedRuleId(CurrentRun); + string ruleId = resultRuleId; + ReportingDescriptor rule = null; + + // Result's rule Id is a sub rule Id + if (!string.IsNullOrEmpty(ruleId) && + ruleId.IndexOf(SarifConstants.HierarchicalComponentSeparator) > 0) + { + rule = node.GetRule(CurrentRun); + string possilbeParentRuleId = rule?.Id; + if (possilbeParentRuleId != null && + ruleId.StartsWith(possilbeParentRuleId + SarifConstants.HierarchicalComponentSeparator)) + { + ruleId = possilbeParentRuleId; + } + } + if (!string.IsNullOrEmpty(ruleId)) { if (RuleIdToIndex.TryGetValue(ruleId, out int ruleIndex)) @@ -87,15 +103,14 @@ public override Result VisitResult(Result node) } else { - ReportingDescriptor rule = node.GetRule(CurrentRun); int newIndex = Rules.Count; - Rules.Add(rule); + Rules.Add(rule ?? node.GetRule(CurrentRun)); RuleIdToIndex[ruleId] = newIndex; node.RuleIndex = newIndex; } - node.RuleId = ruleId; + node.RuleId = resultRuleId; } Result result = base.VisitResult(node); diff --git a/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs b/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs index 2d241ea35..953f2624f 100644 --- a/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs +++ b/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs @@ -387,6 +387,54 @@ public void SarifLogResultMatcher_PreservesPropertiesProperly() matchedLog.Runs[0].Results?.Where((r) => { return r.GetProperty("Key") == currentPropertyValue; }).Count().Should().Be(matchedLog.Runs[0].Results.Count); } + [Fact] + public void SarifLogResultMatcher_PreservesVersionControlProvenanceProperly() + { + Random random = RandomSarifLogGenerator.GenerateRandomAndLog(this.output); + SarifLog logHasNoVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 2); + SarifLog anotherLogHasNoVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 3); + + SarifLog matchedLog = s_preserveOldestPropertyBagMatcher.Match(logHasNoVersionControl, anotherLogHasNoVersionControl); + matchedLog.Runs + .Where(run => run.VersionControlProvenance != null) + .SelectMany(run => run.VersionControlProvenance).Count().Should().Be(0); + + VersionControlDetails versionControl1 = new VersionControlDetails + { + RepositoryUri = new Uri("https://github.com/user/repo"), + Branch = "main", + }; + VersionControlDetails versionControl2 = new VersionControlDetails + { + RepositoryUri = new Uri("https://github.com/user/repo"), + Branch = "myBranch", + }; + VersionControlDetails versionControl3 = new VersionControlDetails + { + RepositoryUri = new Uri("https://github.com/anotheruser/repo"), + }; + + // 3 unqiue versionControlProvenances + SarifLog logHasVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 4); + logHasVersionControl.Runs[0].VersionControlProvenance = new[] { versionControl1 }; + logHasVersionControl.Runs[1].VersionControlProvenance = new[] { versionControl1 }; + logHasVersionControl.Runs[2].VersionControlProvenance = new[] { versionControl2 }; + logHasVersionControl.Runs[3].VersionControlProvenance = new[] { versionControl2 }; + + SarifLog anotherLogHasVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 2); + anotherLogHasVersionControl.Runs[0].VersionControlProvenance = new[] { versionControl3 }; + anotherLogHasVersionControl.Runs[0].VersionControlProvenance = new[] { versionControl3 }; + + matchedLog = s_preserveMostRecentPropertyBagMatcher.Match(logHasVersionControl, anotherLogHasVersionControl); + var versionControlList = matchedLog.Runs + .Where(run => run.VersionControlProvenance != null) + .SelectMany(run => run.VersionControlProvenance).ToList(); + versionControlList.Count.Should().Be(3); + versionControlList.Contains(versionControl1, VersionControlDetails.ValueComparer).Should().BeTrue(); + versionControlList.Contains(versionControl2, VersionControlDetails.ValueComparer).Should().BeTrue(); + versionControlList.Contains(versionControl3, VersionControlDetails.ValueComparer).Should().BeTrue(); + } + private void SetPropertyOnAllResultObjects(SarifLog sarifLog, string propertyKey, string propertyValue) { foreach (Run run in sarifLog.Runs) diff --git a/src/Test.UnitTests.Sarif/Visitors/RemapIndicesVisitorTests.cs b/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs similarity index 80% rename from src/Test.UnitTests.Sarif/Visitors/RemapIndicesVisitorTests.cs rename to src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs index 4c6908ac0..0bd7f48fe 100644 --- a/src/Test.UnitTests.Sarif/Visitors/RemapIndicesVisitorTests.cs +++ b/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT license. See LICENSE file in the project root for full license information. using System.Collections.Generic; +using System.Linq; using FluentAssertions; @@ -359,5 +360,84 @@ private void VerifyRuleMatches(Run mergedRun, Run sourceRun, int firstResultInde firstResultIndex++; } } + + [Fact] + public void RunMergingVisitor_MergeSubRulesProperly() + { + var previousRun = new Run + { + Tool = new Tool + { + Driver = new ToolComponent + { + Name = "Test Tool", + Rules = new List() + { + new ReportingDescriptor() { Id = "Rule/001" }, + new ReportingDescriptor() { Id = "Rule/002" }, + new ReportingDescriptor() { Id = "Rule/003" }, + new ReportingDescriptor() { Id = "Rule/004" }, + new ReportingDescriptor() { Id = "Rule/005" }, + } + }, + }, + Results = new List + { + new Result { RuleId = "Rule/001/SubRule1", RuleIndex = 0 }, + new Result { RuleId = "Rule/001/SubRule2", RuleIndex = 0 }, + new Result { RuleId = "Rule/002/SubRule1", RuleIndex = 1 }, + new Result { RuleId = "Rule/002/SubRule2", RuleIndex = 1 }, + new Result { RuleId = "Rule/004", RuleIndex = 3 }, + } + }; + + var currentRun = new Run + { + Tool = new Tool + { + Driver = new ToolComponent + { + Name = "Test Tool", + Rules = new List() + { + new ReportingDescriptor() { Id = "Rule/004" }, + new ReportingDescriptor() { Id = "Rule/002" }, + new ReportingDescriptor() { Id = "Rule/003" }, + } + }, + }, + Results = new List + { + new Result { RuleId = "Rule/002/SubRule3", RuleIndex = 1 }, + new Result { RuleId = "Rule/002/SubRule4", RuleIndex = 1 }, + new Result { RuleId = "Rule/003/SubRule1", RuleIndex = 2 }, + new Result { RuleId = "Rule/003/SubRule2", RuleIndex = 2 }, + } + }; + + // Use the RunMergingVisitor to merge two runs, via Run.MergeResultsFrom + Run mergedRun = currentRun.DeepClone(); + Run baselineRunCopy = previousRun.DeepClone(); + + mergedRun.MergeResultsFrom(baselineRunCopy); + + // We should get Rule/001, Rule/002, Rule/003 and Rule/004. + // Rule/005 wasn't referenced. + mergedRun.Tool.Driver.Rules.Count.Should().Be(4); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/001").Should().BeTrue(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/002").Should().BeTrue(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/003").Should().BeTrue(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/004").Should().BeTrue(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/005").Should().BeFalse(); + + // Verify each sub rule should match to right parent rule. + mergedRun.Results.Count.Should().Be(9); + foreach (Result result in mergedRun.Results) + { + string subRule = result.RuleId; + string parentRule = result.GetRule(mergedRun).Id; + subRule.StartsWith(parentRule).Should().BeTrue(); + } + } } } From 1b52af7419753391d724611c3b3db3ed4cc88574 Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Fri, 22 Jul 2022 16:55:25 -0700 Subject: [PATCH 2/7] Update sub rule check logic --- src/Sarif/Visitors/RunMergingVisitor.cs | 19 ++-------- .../Visitors/RunMergingVisitorTests.cs | 36 +++++++++++-------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/src/Sarif/Visitors/RunMergingVisitor.cs b/src/Sarif/Visitors/RunMergingVisitor.cs index e1f6dd01b..5db63586e 100644 --- a/src/Sarif/Visitors/RunMergingVisitor.cs +++ b/src/Sarif/Visitors/RunMergingVisitor.cs @@ -79,21 +79,8 @@ public override Result VisitResult(Result node) // Cache RuleId and set Result.RuleIndex to the (new) index string resultRuleId = node.ResolvedRuleId(CurrentRun); - string ruleId = resultRuleId; - ReportingDescriptor rule = null; - - // Result's rule Id is a sub rule Id - if (!string.IsNullOrEmpty(ruleId) && - ruleId.IndexOf(SarifConstants.HierarchicalComponentSeparator) > 0) - { - rule = node.GetRule(CurrentRun); - string possilbeParentRuleId = rule?.Id; - if (possilbeParentRuleId != null && - ruleId.StartsWith(possilbeParentRuleId + SarifConstants.HierarchicalComponentSeparator)) - { - ruleId = possilbeParentRuleId; - } - } + ReportingDescriptor rule = node.GetRule(CurrentRun); + string ruleId = rule?.Id ?? resultRuleId; if (!string.IsNullOrEmpty(ruleId)) { @@ -110,7 +97,7 @@ public override Result VisitResult(Result node) node.RuleIndex = newIndex; } - node.RuleId = resultRuleId; + node.RuleId ??= resultRuleId; } Result result = base.VisitResult(node); diff --git a/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs b/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs index 0bd7f48fe..614deb412 100644 --- a/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs +++ b/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs @@ -375,9 +375,9 @@ public void RunMergingVisitor_MergeSubRulesProperly() { new ReportingDescriptor() { Id = "Rule/001" }, new ReportingDescriptor() { Id = "Rule/002" }, - new ReportingDescriptor() { Id = "Rule/003" }, - new ReportingDescriptor() { Id = "Rule/004" }, - new ReportingDescriptor() { Id = "Rule/005" }, + new ReportingDescriptor() { Id = "Rule/003" }, // not referenced by this run's results + new ReportingDescriptor() { Id = "Rule004" }, + new ReportingDescriptor() { Id = "Rule/006" }, // no reference } }, }, @@ -387,7 +387,7 @@ public void RunMergingVisitor_MergeSubRulesProperly() new Result { RuleId = "Rule/001/SubRule2", RuleIndex = 0 }, new Result { RuleId = "Rule/002/SubRule1", RuleIndex = 1 }, new Result { RuleId = "Rule/002/SubRule2", RuleIndex = 1 }, - new Result { RuleId = "Rule/004", RuleIndex = 3 }, + new Result { RuleId = "Rule004", RuleIndex = 3 }, // not a sub rule } }; @@ -403,6 +403,7 @@ public void RunMergingVisitor_MergeSubRulesProperly() new ReportingDescriptor() { Id = "Rule/004" }, new ReportingDescriptor() { Id = "Rule/002" }, new ReportingDescriptor() { Id = "Rule/003" }, + new ReportingDescriptor() { Id = "Rule/005" }, } }, }, @@ -412,31 +413,36 @@ public void RunMergingVisitor_MergeSubRulesProperly() new Result { RuleId = "Rule/002/SubRule4", RuleIndex = 1 }, new Result { RuleId = "Rule/003/SubRule1", RuleIndex = 2 }, new Result { RuleId = "Rule/003/SubRule2", RuleIndex = 2 }, + new Result { RuleIndex = 3 }, // RuleId is not set ("Rule/005") + new Result { RuleId = "Rule/DoesNotExist" }, // rule definition does not exist, should create a empty rule for it. } }; - // Use the RunMergingVisitor to merge two runs, via Run.MergeResultsFrom Run mergedRun = currentRun.DeepClone(); Run baselineRunCopy = previousRun.DeepClone(); mergedRun.MergeResultsFrom(baselineRunCopy); - // We should get Rule/001, Rule/002, Rule/003 and Rule/004. - // Rule/005 wasn't referenced. - mergedRun.Tool.Driver.Rules.Count.Should().Be(4); + // We should get Rule/001, Rule/002, Rule/003, Rule/004 and Rule/005. + // Rule/006 wasn't referenced. With 1 extra rule "Rule/DoesNotExist". + mergedRun.Tool.Driver.Rules.Count.Should().Be(6); mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/001").Should().BeTrue(); mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/002").Should().BeTrue(); mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/003").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/004").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/005").Should().BeFalse(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule004").Should().BeTrue(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/005").Should().BeTrue(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/006").Should().BeFalse(); + mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/DoesNotExist").Should().BeTrue(); - // Verify each sub rule should match to right parent rule. - mergedRun.Results.Count.Should().Be(9); + mergedRun.Results.Count.Should().Be(11); foreach (Result result in mergedRun.Results) { - string subRule = result.RuleId; - string parentRule = result.GetRule(mergedRun).Id; - subRule.StartsWith(parentRule).Should().BeTrue(); + string subRuleId = result.RuleId; + string parentRuleId = result.GetRule(mergedRun).Id; + if (subRuleId != parentRuleId) + { + subRuleId.StartsWith(parentRuleId).Should().BeTrue(); + } } } } From 2c6e293e793be373d2ba890e446383b2be9653f8 Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Sat, 23 Jul 2022 00:34:42 -0700 Subject: [PATCH 3/7] Addressing PR comments --- src/Sarif/Visitors/RunMergingVisitor.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Sarif/Visitors/RunMergingVisitor.cs b/src/Sarif/Visitors/RunMergingVisitor.cs index 5db63586e..b7c81c4a6 100644 --- a/src/Sarif/Visitors/RunMergingVisitor.cs +++ b/src/Sarif/Visitors/RunMergingVisitor.cs @@ -91,7 +91,7 @@ public override Result VisitResult(Result node) else { int newIndex = Rules.Count; - Rules.Add(rule ?? node.GetRule(CurrentRun)); + Rules.Add(rule); RuleIdToIndex[ruleId] = newIndex; node.RuleIndex = newIndex; From 3fca0c9b44bf7235388b5e099cbb226efc53343b Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Mon, 25 Jul 2022 13:40:07 -0700 Subject: [PATCH 4/7] Update release history --- src/ReleaseHistory.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md index 152d00dbd..9643783f8 100644 --- a/src/ReleaseHistory.md +++ b/src/ReleaseHistory.md @@ -1,5 +1,10 @@ # SARIF Package Release History (SDK, Driver, Converters, and Multitool) +## Unreleased + +* BUGFIX: Fix issue `match-results-forward` command generates invalid Sarif when results contain sub ruleId. [#2486](https://github.com/microsoft/sarif-sdk/pull/2486) +* BUGFIX: Fix issue `match-results-forward` command generates Sarif missing VersionControlDetails data. [#2487](https://github.com/microsoft/sarif-sdk/pull/2487) + ## **v2.4.16** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.16) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.16) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.16) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.16) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.16) * FEATURE: Add `max-file-size-in-kb` argument that allows filtering scan targets by file size. [#2494](https://github.com/microsoft/sarif-sdk/pull/2494) From 9548a62486d516a1ae73a71e28a1eb31a423f605 Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Wed, 27 Jul 2022 10:55:09 -0700 Subject: [PATCH 5/7] separate bug fix for #2487 from this PR --- src/ReleaseHistory.md | 1 - .../ResultMatching/SarifLogResultMatcher.cs | 20 -------- .../SarifLogResultMatcherTests.cs | 48 ------------------- 3 files changed, 69 deletions(-) diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md index 9643783f8..c123545c9 100644 --- a/src/ReleaseHistory.md +++ b/src/ReleaseHistory.md @@ -3,7 +3,6 @@ ## Unreleased * BUGFIX: Fix issue `match-results-forward` command generates invalid Sarif when results contain sub ruleId. [#2486](https://github.com/microsoft/sarif-sdk/pull/2486) -* BUGFIX: Fix issue `match-results-forward` command generates Sarif missing VersionControlDetails data. [#2487](https://github.com/microsoft/sarif-sdk/pull/2487) ## **v2.4.16** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.16) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.16) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.16) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.16) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.16) diff --git a/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs b/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs index 0407d679b..bd5778480 100644 --- a/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs +++ b/src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs @@ -278,7 +278,6 @@ private SarifLog ConstructSarifLogFromMatchedResults( run.Graphs = graphs; run.Invocations = invocations; run.Properties = properties; - run.VersionControlProvenance = MergeVersionControlDetails(currentRuns.Concat(previousRuns)); return new SarifLog() { @@ -344,24 +343,5 @@ internal static void MergeDictionaryInto( } } } - - private static IList MergeVersionControlDetails(IEnumerable runs) - { - // use HashSet with EqualityComparer to avoid duplicated entries - var versionControlSet = new HashSet(VersionControlDetails.ValueComparer); - foreach (Run run in runs) - { - if (run.VersionControlProvenance == null) - { - continue; - } - - foreach (VersionControlDetails versionControlProvenance in run.VersionControlProvenance) - { - versionControlSet.Add(versionControlProvenance); - } - } - return versionControlSet.Any() ? versionControlSet.ToList() : null; - } } } diff --git a/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs b/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs index 953f2624f..2d241ea35 100644 --- a/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs +++ b/src/Test.UnitTests.Sarif/Baseline/ResultMatching/SarifLogResultMatcherTests.cs @@ -387,54 +387,6 @@ public void SarifLogResultMatcher_PreservesPropertiesProperly() matchedLog.Runs[0].Results?.Where((r) => { return r.GetProperty("Key") == currentPropertyValue; }).Count().Should().Be(matchedLog.Runs[0].Results.Count); } - [Fact] - public void SarifLogResultMatcher_PreservesVersionControlProvenanceProperly() - { - Random random = RandomSarifLogGenerator.GenerateRandomAndLog(this.output); - SarifLog logHasNoVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 2); - SarifLog anotherLogHasNoVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 3); - - SarifLog matchedLog = s_preserveOldestPropertyBagMatcher.Match(logHasNoVersionControl, anotherLogHasNoVersionControl); - matchedLog.Runs - .Where(run => run.VersionControlProvenance != null) - .SelectMany(run => run.VersionControlProvenance).Count().Should().Be(0); - - VersionControlDetails versionControl1 = new VersionControlDetails - { - RepositoryUri = new Uri("https://github.com/user/repo"), - Branch = "main", - }; - VersionControlDetails versionControl2 = new VersionControlDetails - { - RepositoryUri = new Uri("https://github.com/user/repo"), - Branch = "myBranch", - }; - VersionControlDetails versionControl3 = new VersionControlDetails - { - RepositoryUri = new Uri("https://github.com/anotheruser/repo"), - }; - - // 3 unqiue versionControlProvenances - SarifLog logHasVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 4); - logHasVersionControl.Runs[0].VersionControlProvenance = new[] { versionControl1 }; - logHasVersionControl.Runs[1].VersionControlProvenance = new[] { versionControl1 }; - logHasVersionControl.Runs[2].VersionControlProvenance = new[] { versionControl2 }; - logHasVersionControl.Runs[3].VersionControlProvenance = new[] { versionControl2 }; - - SarifLog anotherLogHasVersionControl = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 2); - anotherLogHasVersionControl.Runs[0].VersionControlProvenance = new[] { versionControl3 }; - anotherLogHasVersionControl.Runs[0].VersionControlProvenance = new[] { versionControl3 }; - - matchedLog = s_preserveMostRecentPropertyBagMatcher.Match(logHasVersionControl, anotherLogHasVersionControl); - var versionControlList = matchedLog.Runs - .Where(run => run.VersionControlProvenance != null) - .SelectMany(run => run.VersionControlProvenance).ToList(); - versionControlList.Count.Should().Be(3); - versionControlList.Contains(versionControl1, VersionControlDetails.ValueComparer).Should().BeTrue(); - versionControlList.Contains(versionControl2, VersionControlDetails.ValueComparer).Should().BeTrue(); - versionControlList.Contains(versionControl3, VersionControlDetails.ValueComparer).Should().BeTrue(); - } - private void SetPropertyOnAllResultObjects(SarifLog sarifLog, string propertyKey, string propertyValue) { foreach (Run run in sarifLog.Runs) From 621b32e1063bf313c5526cdcccdcc43351c37008 Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Thu, 11 Aug 2022 01:48:53 -0700 Subject: [PATCH 6/7] Update test cases and release note --- src/ReleaseHistory.md | 2 +- .../Visitors/RunMergingVisitorTests.cs | 66 ++++++++++++++++--- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md index c123545c9..aeea14a2c 100644 --- a/src/ReleaseHistory.md +++ b/src/ReleaseHistory.md @@ -2,7 +2,7 @@ ## Unreleased -* BUGFIX: Fix issue `match-results-forward` command generates invalid Sarif when results contain sub ruleId. [#2486](https://github.com/microsoft/sarif-sdk/pull/2486) +* BUGFIX: Fix an issue where the `match-results-forward` command generates Sarif with duplicated rules if there are results with sub `ruleId`. [#2486](https://github.com/microsoft/sarif-sdk/pull/2486) ## **v2.4.16** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.16) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.16) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.16) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.16) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.16) diff --git a/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs b/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs index 614deb412..c696e0e27 100644 --- a/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs +++ b/src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. // Licensed under the MIT license. See LICENSE file in the project root for full license information. +using System; using System.Collections.Generic; using System.Linq; @@ -400,7 +401,7 @@ public void RunMergingVisitor_MergeSubRulesProperly() Name = "Test Tool", Rules = new List() { - new ReportingDescriptor() { Id = "Rule/004" }, + new ReportingDescriptor() { Id = "Rule/004" }, // no reference new ReportingDescriptor() { Id = "Rule/002" }, new ReportingDescriptor() { Id = "Rule/003" }, new ReportingDescriptor() { Id = "Rule/005" }, @@ -423,16 +424,27 @@ public void RunMergingVisitor_MergeSubRulesProperly() mergedRun.MergeResultsFrom(baselineRunCopy); - // We should get Rule/001, Rule/002, Rule/003, Rule/004 and Rule/005. - // Rule/006 wasn't referenced. With 1 extra rule "Rule/DoesNotExist". + // We should get Rule/001, Rule/002, Rule/003, Rule004, Rule/005 and 1 extra rule "Rule/DoesNotExist". + // Rule/004 and Rule/006 weren't referenced. + var expectedRules = new Dictionary(); mergedRun.Tool.Driver.Rules.Count.Should().Be(6); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/001").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/002").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/003").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule004").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/005").Should().BeTrue(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/006").Should().BeFalse(); - mergedRun.Tool.Driver.Rules.Any(r => r.Id == "Rule/DoesNotExist").Should().BeTrue(); + foreach (ReportingDescriptor rule in mergedRun.Tool.Driver.Rules) + { + if (!expectedRules.ContainsKey(rule.Id)) + { + expectedRules[rule.Id] = 0; + } + expectedRules[rule.Id]++; + } + + expectedRules["Rule/001"].Should().Be(1); + expectedRules["Rule/002"].Should().Be(1); + expectedRules["Rule/003"].Should().Be(1); + expectedRules["Rule004"].Should().Be(1); + expectedRules["Rule/005"].Should().Be(1); + expectedRules["Rule/DoesNotExist"].Should().Be(1); + expectedRules.ContainsKey("Rule/004").Should().BeFalse(); + expectedRules.ContainsKey("Rule/006").Should().BeFalse(); mergedRun.Results.Count.Should().Be(11); foreach (Result result in mergedRun.Results) @@ -445,5 +457,39 @@ public void RunMergingVisitor_MergeSubRulesProperly() } } } + + [Fact] + public void RunMergingVisitor_WhenCurrentRunIsNull_ShouldThrowException() + { + Run currentRun = null; + var baselineRun = new Run + { + Tool = new Tool + { + Driver = new ToolComponent + { + Name = "Test Tool", + Rules = new List() + { + new ReportingDescriptor() { Id = "Rule001" }, + new ReportingDescriptor() { Id = "Rule002" }, + } + }, + }, + Results = new List + { + new Result { RuleIndex = 1 }, + new Result { RuleIndex = 0 }, + new Result { RuleIndex = 1 } + } + }; + + RunMergingVisitor currentVisitor = new RunMergingVisitor(); + currentVisitor.Visit(baselineRun); + + currentVisitor.CurrentRun = currentRun; + Action action = () => currentVisitor.VisitResult(baselineRun.Results[0]); + action.Should().Throw("Expect VisitResult method throw InvalidOperationException when RunMergingVisitor.CurrentRun is null"); + } } } From 3d42b0424a2675fab6eb25aa27b2928be9cfc41c Mon Sep 17 00:00:00 2001 From: Yong Yan Date: Fri, 19 Aug 2022 11:25:02 -0700 Subject: [PATCH 7/7] Update release note --- src/ReleaseHistory.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ReleaseHistory.md b/src/ReleaseHistory.md index 41955e701..f077bc8c8 100644 --- a/src/ReleaseHistory.md +++ b/src/ReleaseHistory.md @@ -2,7 +2,7 @@ ## Unreleased -* BUGFIX: Fix an issue where the `match-results-forward` command generates Sarif with duplicated rules if there are results with sub `ruleId`. [#2486](https://github.com/microsoft/sarif-sdk/pull/2486) +* BUGFIX: Remove duplicated rule definitions when executing `match-results-forward` commands for results with sub-rule ids. [#2486](https://github.com/microsoft/sarif-sdk/pull/2486) * BUGFIX: Update `merge` command to properly produce runs by tool and version when passed the `--merge-runs` argument. [#2488](https://github.com/microsoft/sarif-sdk/pull/2488) * BUGFIX: Eliminate `IOException` and `DirectoryNotFoundException` exceptions thrown by `merge` command when splitting by rule (due to invalid file characters in rule ids).