Fix bug: `match-results-forward` command generates invalid Sarif (duplicated rules). #2505

yongyan-gh · 2022-07-20T22:32:34Z

Description

Fixes for below issue:
match-results-forward command generates invalid SARIF when results contain SubRuleId (#2486)

When merging results and rules in multiple runs, we create a rule-id to index mapping (Dictioanry) to keep tracking all referenced rules. It uses ruleId from result entity as the key. If there are multiple unique sub ruleIds of a parent rule in the results, it caches the same parent rule for each unique sub ruleId, which caused the invalid Sarif output.

yongyan-gh · 2022-07-22T16:42:53Z

src/Sarif/Visitors/RunMergingVisitor.cs

+                    ruleId = possilbeParentRuleId;
+                }
+            }
+
            if (!string.IsNullOrEmpty(ruleId))
            {
                if (RuleIdToIndex.TryGetValue(ruleId, out int ruleIndex))


RuleIdToIndex

It uses this rule-id to index mapping to track all referenced rules.
If there are multiple unique sub ruleIds of a same parent rule in the results, it caches the same parent rule for each unique sub ruleId, which caused the invalid Sarif output (same rule duplicated multiple times) #ByDesign

eddynaka · 2022-07-22T17:33:12Z

src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs

@@ -278,6 +278,7 @@ private List<ExtractedResult> ExtractResultsFromRuns(IEnumerable<Run> sarifRuns)
            run.Graphs = graphs;
            run.Invocations = invocations;
            run.Properties = properties;
+            run.VersionControlProvenance = MergeVersionControlDetails(currentRuns.Concat(previousRuns));


MergeVersionControlDetails

can you verify if we have other relevant properties that we should keep? #Resolved

there are couple properties of run like Addresses/WebRequest/WebResponse etc are not been set.
I am not sure if any special requirements to merge these properties. Can I just combine the values of the properties in the final run object?

moved this change to another PR. resolving this comment.

eddynaka · 2022-07-22T17:36:43Z

src/Sarif/Visitors/RunMergingVisitor.cs

+                {
+                    ruleId = possilbeParentRuleId;
+                }
+            }


can't we replace this for a smaller logic?

ReportingDescriptor rule = result.GetRule(CurrentRun);
use rule everywhere when possible. #Resolved

agree. The additional checks is to make sure the case is a rule with sub rule. I can trust result.GetRule(...) and remove these checks.

updated with simple implementation

eddynaka · 2022-07-22T17:37:14Z

src/Sarif/Visitors/RunMergingVisitor.cs


                    RuleIdToIndex[ruleId] = newIndex;
                    node.RuleIndex = newIndex;
                }

-                node.RuleId = ruleId;
+                node.RuleId = resultRuleId;


resultRuleId

why do we have to change this? #Resolved

can't we remove this entire line or just replace if node.ruleid is null/empty?

Also, why are we using resultRuleId when the remainder of the logic in this if statement is around using ruleId?

The resultRuleId keeps the value from result entity, ruleId is the value from referenced rule entity. They may be different if result uses a sub rule id of the rule.
e.g.
in rules array there is a rule id is "SEC105/003", value of ruleId should be "SEC105/003"
a result has rule id "SEC105/003/maybenull", which is a sub rule of above rule, resultRuleId keeps this id.
ruleId should be used as key of the cache RuleIdToIndex.
resultRuleId used to set on result rule id.

updated the implementation pls see latest iteration

marmegh · 2022-07-22T17:41:32Z

We also need a release history update for this.

In reply to: 1192800154

marmegh · 2022-07-22T17:46:53Z

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;


Do we really need both of these? Why isn't this just:
string ruleId = node.ResolvedRuleId(CurrentRun);? #Resolved

this was to keep the value of the result's rule id and can be used later, i updated the implemenation pls see the latest iteration.

…b.com/microsoft/sarif-sdk into users/yongyan-gh/ResultMatchingBugFix

eddynaka · 2022-07-23T00:07:22Z

src/Sarif/Visitors/RunMergingVisitor.cs

                    int newIndex = Rules.Count;
-                    Rules.Add(rule);
+                    Rules.Add(rule ?? node.GetRule(CurrentRun));


?? node.GetRule(CurrentRun)

you can remove this, because is exact the same as line 82 #Resolved

eddynaka · 2022-07-23T00:08:55Z

Also, the title of the PR should tell more about the bug. What bug are you fixing? Is it one? Many? Should be in the same PR or should be separate?

In reply to: 1192800154

yongyan-gh · 2022-07-25T21:58:29Z

updated release history. since both issue are related to result-match-foward command, i put the fix in same pr.

In reply to: 1193015158

shaopeng-gh · 2022-07-26T01:00:41Z

src/Sarif/Baseline/ResultMatching/SarifLogResultMatcher.cs

+
+        private static IList<VersionControlDetails> MergeVersionControlDetails(IEnumerable<Run> runs)
+        {
+            // use HashSet with EqualityComparer to avoid duplicated entries


u

Cap and add . at the end #Closed

I would remove the entire comment. I don't see the difference between comment and the actual code.

removed the comments.

eddynaka · 2022-07-26T01:44:17Z

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

I would either separate in two PRs so each PR has its own fix. Or, make an improvement in the text to be one entry that explains the current behavior for both issues and point to the correct PullRequest Id. #Resolved

Separated into 2 PRs.

yongyan-gh · 2022-07-27T17:56:37Z

remove fix for #2487 to another PR. keep only fix for #2486

In reply to: 1194682619

marmegh · 2022-08-10T16:49:49Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # 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 invalid Sarif when results contain sub ruleId. #2486

I'm not sure if this is just unclear or not descriptive enough. What was invalid about the SARIF results before this fix? #Resolved

The issue of invalid Sarif produced before the fix is it contains duplicated rule definitions in rules array if the Sarif results contain results have sub ruleId.

E.g. if there are 2 results' ruleId is "TESTRULE/001", the rules array creates 2 duplicated rules of "TESTRULE".

Will update the description with the duplicated rules issue, and update the issue with the detail errors. What do you think?

So, this is fixing an issue where the match-results-forward command generates Sarif with duplicated results if there is a sub ruleId?
Yes, please update both.

And just to be clear, also update the release note here. If the above captures everything, you can copy/paste most of it. Don't forget the backticks around ruleId.

marmegh · 2022-08-10T18:07:03Z

src/Sarif/Visitors/RunMergingVisitor.cs

-            string ruleId = node.ResolvedRuleId(CurrentRun);
+            string resultRuleId = node.ResolvedRuleId(CurrentRun);
+            ReportingDescriptor rule = node.GetRule(CurrentRun);
+            string ruleId = rule?.Id ?? resultRuleId;


string ruleId = rule?.Id ?? resultRuleId;

Is there a case where rule.Id might not be null and resultRuleId may be another value?

Yes, if the ruleId from result (resultRuleId) is a sub ruleId. The rule's Id (rule.Id) is the parent ruleId.

e.g. a result has sub rule id ("SEC105/001/allocator"), and a rule index points to its parent rule definition in rules array.

"results": [ { "ruleId": "SEC105/001/allocator", "ruleIndex": 0, ... }

"rules": [ { "id": "SEC105/001", // index 0 ... }

The fix here is to use the parent ruleId instead of sub ruleId as the RuleIdToIndex dictionary's key, to avoid the (parent) rule definition to be added multiple times.

marmegh · 2022-08-10T20:53:03Z

src/Test.UnitTests.Sarif/Visitors/RunMergingVisitorTests.cs

+                {
+                    subRuleId.StartsWith(parentRuleId).Should().BeTrue();
+                }
+            }


We can consolidate all of this:

int rule_001Count = 0; int rule_002Count = 0; int rule_003Count = 0; int rule004Count = 0; int rule_005Count = 0; int rule_006Count = 0; int rule_DoesNotExistCount = 0; mergedRun.Results.Count.Should().Be(11); //Even better if you have a way of calculating this expected value from the inputs above. foreach (Result result in mergedRune.Results) { ...// keep everything the same and add something like this: if(result.Id == "Rule/001") { rule_001Count+=1; } } rule_001Count.Should().Be(expectedRule001Count); //etc

We're trying to avoid iterating over the same lists multiple times. Let me know if this makes sense.
#Resolved

agree, updated & removed duplicated iterations

marmegh · 2022-08-10T22:08:59Z

            throw new InvalidOperationException("RemapIndicesVisitor requires CurrentRun to be set before Visiting Results.");

Code coverage (via Live Unit Testing) shows this is the only segment of code in VisitResults that is not covered by a test. Stepped through the added tests and confirmed they cover remaining if/else blocks. All changed code appears fully covered.

If at all possible to sneak in a unit test to cover this section, that would help improve our code coverage overall.

In reply to: 1211334197

Refers to: src/Sarif/Visitors/RunMergingVisitor.cs:77 in 9548a62. [](commit_id = 9548a62, deletion_comment = False)

yongyan-gh · 2022-08-11T08:51:34Z

            throw new InvalidOperationException("RemapIndicesVisitor requires CurrentRun to be set before Visiting Results.");

test case added for the case CurrentRun is null.

In reply to: 1211334197

Refers to: src/Sarif/Visitors/RunMergingVisitor.cs:77 in 9548a62. [](commit_id = 9548a62, deletion_comment = False)

shaopeng-gh · 2022-08-18T01:14:50Z

Need to resolve the conflicts.

In reply to: 1218847548

shaopeng-gh

yongyan-gh · 2022-08-19T08:33:20Z

resolved

In reply to: 1218847548

michaelcfanning · 2022-08-19T16:37:40Z

src/ReleaseHistory.md

@@ -1,5 +1,9 @@
 # SARIF Package Release History (SDK, Driver, Converters, and Multitool)

+## 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)


Fix an issue where the match-results-forward command generates Sarif with duplicated rules if there are results with sub `ruleId

Please change this to:

Remove duplicated rule definitions when executing match-results-forward commands for results with sub-rule ids. #Resolved

michaelcfanning

yongyan-gh added 3 commits July 20, 2022 15:23

ResultMatching bug fixes

b19a692

Merge branch 'main' into users/yongyan-gh/ResultMatchingBugFix

a33defd

Merge branch 'main' into users/yongyan-gh/ResultMatchingBugFix

e246613

yongyan-gh commented Jul 22, 2022

View reviewed changes

yongyan-gh marked this pull request as ready for review July 22, 2022 16:44

yongyan-gh requested review from eddynaka and michaelcfanning as code owners July 22, 2022 16:44

yongyan-gh requested review from EasyRhinoMSFT, marmegh and shaopeng-gh July 22, 2022 16:45

eddynaka reviewed Jul 22, 2022

View reviewed changes

marmegh reviewed Jul 22, 2022

View reviewed changes

yongyan-gh added 2 commits July 22, 2022 16:55

Update sub rule check logic

1b52af7

Merge branch 'users/yongyan-gh/ResultMatchingBugFix' of https://githu…

0583464

…b.com/microsoft/sarif-sdk into users/yongyan-gh/ResultMatchingBugFix

eddynaka reviewed Jul 23, 2022

View reviewed changes

Addressing PR comments

2c6e293

yongyan-gh changed the title ~~ResultMatching bug fixes~~ Fixes 2 bugs of match-results-forward command: invalid Sarif (duplicated rules) and missing VersionControlProvenance data. Jul 25, 2022

Update release history

3fca0c9

shaopeng-gh reviewed Jul 26, 2022

View reviewed changes

eddynaka reviewed Jul 26, 2022

View reviewed changes

separate bug fix for #2487 from this PR

9548a62

yongyan-gh changed the title ~~Fixes 2 bugs of match-results-forward command: invalid Sarif (duplicated rules) and missing VersionControlProvenance data.~~ Fix bug: match-results-forward command generate invalid Sarif (duplicated rules). Jul 27, 2022

yongyan-gh changed the title ~~Fix bug: match-results-forward command generate invalid Sarif (duplicated rules).~~ Fix bug: match-results-forward command generates invalid Sarif (duplicated rules). Aug 5, 2022

marmegh reviewed Aug 10, 2022

View reviewed changes

Update test cases and release note

621b32e

yongyan-gh requested a review from cfaucon as a code owner August 11, 2022 08:49

yongyan-gh mentioned this pull request Aug 11, 2022

[SARIF Multitool] match-results-forward command generates invalid SARIF when results contain SubRuleId #2486

Open

yongyan-gh added 2 commits August 11, 2022 02:26

Merge branch 'main' into users/yongyan-gh/ResultMatchingBugFix

a4a80a9

Merge branch 'main' into users/yongyan-gh/ResultMatchingBugFix

705e50c

shaopeng-gh approved these changes Aug 18, 2022

View reviewed changes

michaelcfanning reviewed Aug 19, 2022

View reviewed changes

michaelcfanning approved these changes Aug 19, 2022

View reviewed changes

yongyan-gh added 2 commits August 19, 2022 11:18

Merge branch 'main' into users/yongyan-gh/ResultMatchingBugFix

b4175c3

Update release note

3d42b04

michaelcfanning merged commit 7dbdf41 into main Aug 19, 2022

michaelcfanning deleted the users/yongyan-gh/ResultMatchingBugFix branch August 19, 2022 21:15

Fix bug: match-results-forward command generates invalid Sarif (duplicated rules). #2505

Fix bug: match-results-forward command generates invalid Sarif (duplicated rules). #2505

Conversation

yongyan-gh commented Jul 20, 2022 • edited Loading

Description

yongyan-gh Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

eddynaka Jul 22, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh Jul 27, 2022 • edited Loading

Choose a reason for hiding this comment

eddynaka Jul 22, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 22, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh commented Jul 22, 2022 • edited by yongyan-gh Loading

marmegh Jul 22, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 23, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

eddynaka commented Jul 23, 2022

yongyan-gh commented Jul 25, 2022

shaopeng-gh Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jul 26, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh commented Jul 27, 2022

marmegh Aug 10, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh Aug 10, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmegh commented Aug 10, 2022 • edited by yongyan-gh Loading

yongyan-gh commented Aug 11, 2022

shaopeng-gh commented Aug 18, 2022 • edited by yongyan-gh Loading

shaopeng-gh left a comment

Choose a reason for hiding this comment

yongyan-gh commented Aug 19, 2022

michaelcfanning Aug 19, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning left a comment

Choose a reason for hiding this comment

Fix bug: `match-results-forward` command generates invalid Sarif (duplicated rules). #2505

Fix bug: `match-results-forward` command generates invalid Sarif (duplicated rules). #2505

yongyan-gh commented Jul 20, 2022 •

edited

Loading

yongyan-gh Jul 22, 2022 •

edited

Loading

eddynaka Jul 22, 2022 •

edited by yongyan-gh

Loading

yongyan-gh Jul 27, 2022 •

edited

Loading

eddynaka Jul 22, 2022 •

edited by yongyan-gh

Loading

eddynaka Jul 22, 2022 •

edited by yongyan-gh

Loading

marmegh commented Jul 22, 2022 •

edited by yongyan-gh

Loading

marmegh Jul 22, 2022 •

edited by yongyan-gh

Loading

eddynaka Jul 23, 2022 •

edited by yongyan-gh

Loading

shaopeng-gh Jul 26, 2022 •

edited

Loading

eddynaka Jul 26, 2022 •

edited by yongyan-gh

Loading

marmegh Aug 10, 2022 •

edited by yongyan-gh

Loading

marmegh Aug 10, 2022 •

edited by yongyan-gh

Loading

marmegh commented Aug 10, 2022 •

edited by yongyan-gh

Loading

shaopeng-gh commented Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 19, 2022 •

edited by yongyan-gh

Loading