From 5841151090373859347eeb01678c7f357e1cef90 Mon Sep 17 00:00:00 2001 From: "Michael C. Fanning" Date: Wed, 14 Nov 2018 17:26:35 -0800 Subject: [PATCH] Properly persist run level property bags (#1136) --- .../SarifLogResultMatcherTests.cs | 111 ++++++++++++++---- .../DataStructures/MatchedResults.cs | 2 +- .../DictionaryMergeBehaviors.cs | 4 +- .../ResultMatching/SarifLogMatcher.cs | 25 ++-- 4 files changed, 106 insertions(+), 36 deletions(-) diff --git a/src/Sarif.UnitTests/Baseline/ResultMatching/SarifLogResultMatcherTests.cs b/src/Sarif.UnitTests/Baseline/ResultMatching/SarifLogResultMatcherTests.cs index 810a2157d..9eb6964e4 100644 --- a/src/Sarif.UnitTests/Baseline/ResultMatching/SarifLogResultMatcherTests.cs +++ b/src/Sarif.UnitTests/Baseline/ResultMatching/SarifLogResultMatcherTests.cs @@ -15,10 +15,15 @@ public class SarifLogResultMatcherTests { private readonly ITestOutputHelper output; - private readonly SarifLogResultMatcher baseliner = new SarifLogResultMatcher( - exactResultMatchers: new [] { ExactMatchers.ExactResultMatcherFactory.GetIdenticalResultMatcher(considerPropertyBagsWhenComparing: true) }, + private static readonly SarifLogResultMatcher s_preserveMostRecentPropertyBagMatcher = new SarifLogResultMatcher( + exactResultMatchers: new [] { ExactMatchers.ExactResultMatcherFactory.GetIdenticalResultMatcher(considerPropertyBagsWhenComparing: false) }, heuristicMatchers: null, - propertyBagMergeBehaviors: DictionaryMergeBehavior.InitializeFromPrevious); + propertyBagMergeBehaviors: DictionaryMergeBehavior.InitializeFromMostRecent); + + private static readonly SarifLogResultMatcher s_preserveOldestPropertyBagMatcher = new SarifLogResultMatcher( + exactResultMatchers: new[] { ExactMatchers.ExactResultMatcherFactory.GetIdenticalResultMatcher(considerPropertyBagsWhenComparing: false) }, + heuristicMatchers: null, + propertyBagMergeBehaviors: DictionaryMergeBehavior.InitializeFromOldest); public SarifLogResultMatcherTests(ITestOutputHelper outputHelper) { @@ -51,7 +56,7 @@ public void SarifLogResultMatcher_BaselinesTwoSimpleSarifLogs() result.CorrelationGuid = Guid.NewGuid().ToString(); } - SarifLog calculatedNextBaseline = baseliner.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); + SarifLog calculatedNextBaseline = s_preserveOldestPropertyBagMatcher.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); calculatedNextBaseline.Runs.Should().HaveCount(1); @@ -110,15 +115,83 @@ public void SarifLogResultMatcher_BaselinesSarifLogsWithProperties() baselineLog.Runs[0].SetProperty(uniqueToBaselinePropertyName, uniqueToBaselinePropertyValue); currentLog.Runs[0].SetProperty(uniqueToCurrentPropertyName, uniqueToCurrentPropertyValue); - SarifLog calculatedNextBaseline = baseliner.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); + SarifLog calculatedNextBaseline = s_preserveOldestPropertyBagMatcher.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); + + string value = null; // The default property bag matching behavior is to retain the property bag in its entirety from the baseline calculatedNextBaseline.Runs[0].Properties.Should().NotBeNull(); calculatedNextBaseline.Runs[0].Properties.Count.Should().Be(2); - + calculatedNextBaseline.Runs[0].GetProperty(sharedPropertyName).Should().Be(currentSharedPropertyValue); calculatedNextBaseline.Runs[0].GetProperty(uniqueToBaselinePropertyName).Should().Be(uniqueToBaselinePropertyValue); - calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToCurrentPropertyName, out string value).Should().BeFalse(); + calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToCurrentPropertyName, out value).Should().BeFalse(); + + calculatedNextBaseline = s_preserveMostRecentPropertyBagMatcher.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); + + // The default property bag matching behavior is to retain the property bag in its entirety from the baseline + calculatedNextBaseline.Runs[0].Properties.Should().NotBeNull(); + calculatedNextBaseline.Runs[0].Properties.Count.Should().Be(2); + + calculatedNextBaseline.Runs[0].GetProperty(sharedPropertyName).Should().Be(currentSharedPropertyValue); + calculatedNextBaseline.Runs[0].GetProperty(uniqueToCurrentPropertyName).Should().Be(uniqueToCurrentPropertyValue); + calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToBaselinePropertyName, out value).Should().BeFalse(); + } + + [Fact] + public void SarifLogResultMatcher_MatchMultipleCurrentLogsWithNoBaseline() + { + Random random = RandomSarifLogGenerator.GenerateRandomAndLog(this.output); + + SarifLog mostRecentLog = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 1); + SarifLog oldestLog = mostRecentLog.DeepClone(); + + string sharedPropertyName = nameof(sharedPropertyName); + string currentSharedPropertyValue = Guid.NewGuid().ToString(); + + string uniqueToMostRecentPropertyName = nameof(uniqueToMostRecentPropertyName); + string uniqueToMostRecentPropertyValue = Guid.NewGuid().ToString(); + + string uniqueToOldestPropertyName = nameof(uniqueToOldestPropertyName); + string uniqueToOldestPropertyValue = Guid.NewGuid().ToString(); + + mostRecentLog.Runs[0].SetProperty(sharedPropertyName, currentSharedPropertyValue); + oldestLog.Runs[0].SetProperty(sharedPropertyName, currentSharedPropertyValue); + + mostRecentLog.Runs[0].SetProperty(uniqueToMostRecentPropertyName, uniqueToMostRecentPropertyValue); + oldestLog.Runs[0].SetProperty(uniqueToOldestPropertyName, uniqueToOldestPropertyValue); + + SarifLog calculatedNextBaseline = s_preserveOldestPropertyBagMatcher.Match( + previousLogs: null, + currentLogs: new SarifLog[] { oldestLog, mostRecentLog }).First(); + + calculatedNextBaseline.Runs[0].Properties.Should().NotBeNull(); + calculatedNextBaseline.Runs[0].Properties.Count.Should().Be(2); + + string value = null; + + // The default property bag matching behavior is to retain the oldest property bag available. Since we have no + // baseline in this test, we expect the preserved property bag to be associated with the first (i.e., oldest) + // run in the currentLogs property + + calculatedNextBaseline.Runs[0].GetProperty(sharedPropertyName).Should().Be(currentSharedPropertyValue); + calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToOldestPropertyName, out value).Should().BeTrue(); + calculatedNextBaseline.Runs[0].GetProperty(uniqueToOldestPropertyName).Should().Be(uniqueToOldestPropertyValue); + + calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToMostRecentPropertyName, out value).Should().BeFalse(); + + calculatedNextBaseline = s_preserveMostRecentPropertyBagMatcher.Match( + previousLogs: null, + currentLogs: new SarifLog[] { oldestLog, mostRecentLog }).First(); + + calculatedNextBaseline.Runs[0].Properties.Should().NotBeNull(); + calculatedNextBaseline.Runs[0].Properties.Count.Should().Be(2); + + calculatedNextBaseline.Runs[0].GetProperty(sharedPropertyName).Should().Be(currentSharedPropertyValue); + calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToMostRecentPropertyName, out value).Should().BeTrue(); + calculatedNextBaseline.Runs[0].GetProperty(uniqueToMostRecentPropertyName).Should().Be(uniqueToMostRecentPropertyValue); + + calculatedNextBaseline.Runs[0].TryGetProperty(uniqueToOldestPropertyName, out value).Should().BeFalse(); } [Fact] @@ -138,7 +211,7 @@ public void SarifLogResultMatcher_CurrentLogEmpty_AllAbsent() result.CorrelationGuid = Guid.NewGuid().ToString(); } - SarifLog calculatedNextBaseline = baseliner.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); + SarifLog calculatedNextBaseline = s_preserveOldestPropertyBagMatcher.Match(new SarifLog[] { baselineLog }, new SarifLog[] { currentLog }).First(); calculatedNextBaseline.Runs.Should().HaveCount(1); @@ -161,7 +234,7 @@ public void SarifLogResultMatcher_PreviousLogNull_WorksAsExpected() SarifLog currentLog = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 1); currentLog.Runs[0].Id = new RunAutomationDetails { InstanceGuid = Guid.NewGuid().ToString() }; - SarifLog calculatedNextBaseline = baseliner.Match(null, new SarifLog[] { currentLog }).First(); + SarifLog calculatedNextBaseline = s_preserveOldestPropertyBagMatcher.Match(null, new SarifLog[] { currentLog }).First(); calculatedNextBaseline.Runs.Should().HaveCount(1); @@ -197,7 +270,7 @@ public void SarifLogResultMatcher_PreviousLogEmpty_WorksAsExpected() SarifLog currentLog = RandomSarifLogGenerator.GenerateSarifLogWithRuns(random, 1); currentLog.Runs[0].Id = new RunAutomationDetails { InstanceGuid = Guid.NewGuid().ToString() }; - SarifLog calculatedNextBaseline = baseliner.Match(new SarifLog[0], new SarifLog[] { currentLog }).First(); + SarifLog calculatedNextBaseline = s_preserveOldestPropertyBagMatcher.Match(new SarifLog[0], new SarifLog[] { currentLog }).First(); calculatedNextBaseline.Runs.Should().HaveCount(1); @@ -259,7 +332,7 @@ public void SarifLogResultMatcher_MultipleLogsDuplicateData_WorksAsExpected() } } }; - SarifLog result = baseliner.Match(new SarifLog[0], new SarifLog[] { current1, current2 }).First(); + SarifLog result = s_preserveOldestPropertyBagMatcher.Match(new SarifLog[0], new SarifLog[] { current1, current2 }).First(); result.Runs[0].Files.Should().HaveCount(1); @@ -298,7 +371,7 @@ public void SarifLogResultMatcher_MultipleLogsInvalidData_ThrowsInvalidOperation } } }; - Assert.Throws(() => baseliner.Match(new SarifLog[0], new SarifLog[] { current1, current2 })); + Assert.Throws(() => s_preserveOldestPropertyBagMatcher.Match(new SarifLog[0], new SarifLog[] { current1, current2 })); } @@ -315,23 +388,13 @@ public void SarifLogResultMatcher_PreservesPropertiesProperly () SetPropertyOnAllFileAndResultObjects(baselineLog, "Key", baselinePropertyValue); SetPropertyOnAllFileAndResultObjects(currentLog, "Key", currentPropertyValue); - // Retain property bag values from baseline items - var matcher = new SarifLogResultMatcher( - exactResultMatchers: new IResultMatcher[] { ExactMatchers.ExactResultMatcherFactory.GetIdenticalResultMatcher(considerPropertyBagsWhenComparing: false) }, - heuristicMatchers: null, - propertyBagMergeBehaviors: DictionaryMergeBehavior.InitializeFromPrevious); - SarifLog matchedLog = matcher.Match(baselineLog.DeepClone(), currentLog.DeepClone()); + SarifLog matchedLog = s_preserveOldestPropertyBagMatcher.Match(baselineLog.DeepClone(), currentLog.DeepClone()); matchedLog.Runs[0].Results.Where((r) => { return r.GetProperty("Key") == baselinePropertyValue; }).Count().Should().Be(matchedLog.Runs[0].Results.Count); matchedLog.Runs[0].Files.Values.Where((r) => { return r.GetProperty("Key") == baselinePropertyValue; }).Count().Should().Be(matchedLog.Runs[0].Files.Count); - matcher = new SarifLogResultMatcher( - exactResultMatchers: new IResultMatcher[] { ExactMatchers.ExactResultMatcherFactory.GetIdenticalResultMatcher(considerPropertyBagsWhenComparing: false) }, - heuristicMatchers: null, - propertyBagMergeBehaviors: DictionaryMergeBehavior.InitializeFromCurrent); - // Retain property bag values from most current run - matchedLog = matcher.Match(baselineLog.DeepClone(), currentLog.DeepClone()); + matchedLog = s_preserveMostRecentPropertyBagMatcher.Match(baselineLog.DeepClone(), currentLog.DeepClone()); matchedLog.Runs[0].Results.Where((r) => { return r.GetProperty("Key") == currentPropertyValue; }).Count().Should().Be(matchedLog.Runs[0].Results.Count); matchedLog.Runs[0].Files.Values.Where((r) => { return r.GetProperty("Key") == currentPropertyValue; }).Count().Should().Be(matchedLog.Runs[0].Files.Count); } diff --git a/src/Sarif/Baseline/ResultMatching/DataStructures/MatchedResults.cs b/src/Sarif/Baseline/ResultMatching/DataStructures/MatchedResults.cs index dda00c8f1..22f1f9801 100644 --- a/src/Sarif/Baseline/ResultMatching/DataStructures/MatchedResults.cs +++ b/src/Sarif/Baseline/ResultMatching/DataStructures/MatchedResults.cs @@ -56,7 +56,7 @@ public Result CalculateNewBaselineResult(DictionaryMergeBehavior propertyBagMerg ResultMatchingProperties = MergeDictionaryPreferFirst(ResultMatchingProperties, OriginalResultMatchingProperties); if (PreviousResult != null && - propertyBagMergeBehavior == DictionaryMergeBehavior.InitializeFromPrevious) + propertyBagMergeBehavior == DictionaryMergeBehavior.InitializeFromOldest) { result.Properties = PreviousResult.Result.Properties; } diff --git a/src/Sarif/Baseline/ResultMatching/DictionaryMergeBehaviors.cs b/src/Sarif/Baseline/ResultMatching/DictionaryMergeBehaviors.cs index 3f1bd51d2..6d0ee44a9 100644 --- a/src/Sarif/Baseline/ResultMatching/DictionaryMergeBehaviors.cs +++ b/src/Sarif/Baseline/ResultMatching/DictionaryMergeBehaviors.cs @@ -9,8 +9,8 @@ public enum DictionaryMergeBehavior { None = 0, // By default, we will always exclusively preserve the earliest // properties that have been generated. We will not attempt any merging. - InitializeFromPrevious = None, - InitializeFromCurrent = 0x1, // On setting this bit, we will discard earlier properties + InitializeFromOldest = None, + InitializeFromMostRecent = 0x1,// On setting this bit, we will discard earlier properties // in favor of those that are most recently generated. } } diff --git a/src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs b/src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs index 3b5cc77fb..87c76b7d1 100644 --- a/src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs +++ b/src/Sarif/Baseline/ResultMatching/SarifLogMatcher.cs @@ -222,14 +222,21 @@ private SarifLog ConstructSarifLogFromMatchedResults( // We flow the baseline instance id forward (which becomes the // baseline guid for the merged log) run.BaselineInstanceGuid = previousRuns.First().Id?.InstanceGuid; + } + - // We preserve either the most current or the oldest property bag associated with - // a run. Note that this implies ordering of current and previous runs with - // the most recent runs being at the front of the enumerable set. This is - // a non-obvious subtlety that we should look at resolving in a cleaned up design - properties = PropertyBagMergeBehavior.HasFlag(DictionaryMergeBehavior.InitializeFromCurrent) - ? currentRuns.First().Properties - : previousRuns.Last().Properties; + if (PropertyBagMergeBehavior.HasFlag(DictionaryMergeBehavior.InitializeFromOldest)) + { + // Find the 'oldest' log file and initialize properties from that log property bag + properties = previousRuns.FirstOrDefault() != null + ? previousRuns.First().Properties + : currentRuns.First().Properties; + } + else + { + // Find the most recent log file instance and retain its property bag + // Find the 'oldest' log file and initialize properties from that log property bag + properties = currentRuns.Last().Properties; } properties = properties ?? new Dictionary(); @@ -297,7 +304,7 @@ private SarifLog ConstructSarifLogFromMatchedResults( invocations.AddRange(currentRun.Invocations); } - if (PropertyBagMergeBehavior == DictionaryMergeBehavior.InitializeFromCurrent) + if (PropertyBagMergeBehavior == DictionaryMergeBehavior.InitializeFromMostRecent) { properties = currentRun.Properties; } @@ -381,7 +388,7 @@ internal static void MergeDictionaryInto( if (basePropertyBagHolder != null) { - basePropertyBagHolder.Properties = propertyBagMergeBehavior == DictionaryMergeBehavior.InitializeFromCurrent + basePropertyBagHolder.Properties = propertyBagMergeBehavior == DictionaryMergeBehavior.InitializeFromMostRecent ? propertiesToMerge : baseProperties; }