Fix bug: `merge` command generates multiple runs #2513

yongyan-gh · 2022-07-28T23:24:37Z

Description

Fixes for below issues:

merge command generates SARIF with multiple runs ([SARIF Multitool] merge command generates SARIF with multiple runs #2488)
By default, the results are grouped into multiple runs per rule id + tool version.
When merge-runs argument is set, group the results by tool version, this will place all results of runs produced by same tool same version into a single run.
Another issue it fixes is System.IO.IOException/System.IO.DirectoryNotFoundException exception when execute merge command with --split PerRule option, and the Sarif files to be merged contains rule Id has invalid character for a file name. E.g. a sub rule id "SEC105/001".
With --split PerRule option, merge command creates separate Sarif files for each rule id. E.g. "SEC105/001_merged.sarif". The character "/" in the rule Id is an invalid file name character, it will cause the error to create the merged file.

yongyan-gh · 2022-07-28T23:38:50Z

src/Sarif.Multitool.Library/MergeCommand.cs

@@ -117,7 +121,7 @@ public int Run(MergeOptions mergeOptions)
                    mergedLog.SchemaUri = mergedLog.Version.ConvertToSchemaUri();

                    FileSystem.DirectoryCreateDirectory(outputDirectory);
-                    outputFilePath = Path.Combine(outputDirectory, GetOutputFileName(_options, key));
+                    outputFilePath = Path.Combine(outputDirectory, ReplaceInvalidCharInFileName(GetOutputFileName(_options, key), "."));


ReplaceInvalidCharInFileName

If PerRule split strategy is specified, it creates a SarifLog file per rule, the file name is {ruleId}_merged.sarif.

If the ruleId is a sub rule Id, e.g. SEC105/001, it contains invalid file name char '/' and will cause saving the log file failed.

This is the fix for this issue. #ByDesign

Where is the unit test for this issue? Did I miss it?

dedicated unit tests added. pls review.

yongyan-gh · 2022-07-28T23:45:35Z

src/Sarif.Multitool.Library/MergeCommand.cs

@@ -179,7 +183,14 @@ private async Task<bool> MergeSarifLogsAsync()
                                    };
                                }

-                                key = CreateRuleKey(result.RuleId, run);
+                                if (_options.MergeRuns)


MergeRuns

Acccording to merge-runs argument description

[Option( "merge-runs", HelpText = "Merge runs of the same tool + verion combination (requires " + "eliding run-specific details such as invocations data.")] public bool MergeRuns { get; set; }

If merge-runs is false (default), the merged log runs are aggregated by key ruleId+tool name+tool version.
if it is set to true, the merged log runs will be aggregated by key tool name+tool version. This means all results produced by same tool and same version will be merged into 1 single run. #ByDesign

Yes, your understanding is correct. Do we really have the term verion burned into our help? If so, can you please correct this to version?

fixed typo.

src/Sarif.Multitool.Library/MergeCommand.cs

KalleOlaviNiemitalo · 2022-07-29T09:25:51Z

Does the tool rewrite JSON Pointers in sarif: hyperlinks as noted in SARIF-v2.1.0 §3.11.6 Messages with embedded links? If so, does this pull request require any changes in how the tool does that?

In reply to: 1199064853

michaelcfanning · 2022-07-31T17:36:05Z

src/ReleaseHistory.md

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

+## Unreleased
+
+* BUGFIX: Fix issue `merge` command generates SARIF with multiple runs [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)


Fix

This description does not clearly describe the issue and the fix. @marmegh or @EasyRhinoMSFT can you please help Yong with this?

Updated pls review. cc @VCOLAWTON

So, let's look at what we're fixing here.

The problem: the merge command was generating a sarif with a run per rule instead of a run per tool.

The description here does not capture that. We need to tell the customers what behavior they may have been experiencing that is now changed. Specific first, concise second.

"Fixed merge command producing runs per rule to the expected runs per tool."

Thanks for the suggestions. Updated both, pls review.

michaelcfanning · 2022-07-31T17:36:24Z

src/ReleaseHistory.md

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

+## Unreleased
+
+* BUGFIX: Fix issue `merge` command generates SARIF with multiple runs [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)


BUGFIX

There's no reference in the release notes to the invalid characters issue. #Resolved

Has someone built/run your tests and examined code coverage? Do you have any information for us on the completeness of code & data coverage for your tests?

I ran code coverage analysis in VS. All code paths I made change were covered. In term of MergeCommand coverage, it was 84%, added couple test cases its now 94%

michaelcfanning · 2022-07-31T17:41:18Z

src/Sarif.Multitool.Library/MergeCommand.cs

@@ -307,6 +318,11 @@

            return prefix ?? string.Empty;
        }
+
+        private static string ReplaceInvalidCharInFileName(string fileName, string replacement)


ReplaceInvalidCharInFileName

You provided no unit tests for this helper. Generally, a helper with a single line of code with a single upstream caller doesn't warrant dedicating its own method. #Resolved

dedicated unit tests added pls review

michaelcfanning · 2022-07-31T17:42:34Z

src/Sarif.Multitool.Library/MergeCommand.cs

@@ -31,6 +32,9 @@
        private readonly Dictionary<string, HashSet<Result>> _ruleIdToResultsMap;
        private readonly Dictionary<string, RunMergingVisitor> _ruleIdToMergeVisitorsMap;

+        private static readonly string s_invalidFileNameCharRegexString = $"[{Regex.Escape(new string(Path.GetInvalidFileNameChars()))}]";


s_invalidFileNameCharRegexString

This functionality seems very general and not something at all specific to merging SARIF logs. Should it be somewhere else? A file utilities class? Should it simply be an extension method somewhere? #Resolved

yes, moved the function to PathExtensions changed it to an extension method of string.

michaelcfanning · 2022-07-31T17:43:29Z

src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandUnitTests.cs

+            }
+        }
+
+        private static Run CreateTestRun(int numberOfResult, string toolName = null, bool createSubRule = false)


CreateTestRun

Don't we have other code somewhere that autogenerates test files? If so, can we please us it instead of creating this duplicative helper? #Resolved

Moved the RandomSarifLogGenerator from Test.UnitTests.Sarif to Test.Utilities.Sarif so can re-use its functionalities.

michaelcfanning · 2022-07-31T17:44:40Z

src/Test.UnitTests.Sarif.Multitool.Library/MergeCommandUnitTests.cs

+        public void MergeCommand_WhenSplitPerRule_LogShouldAggregateByRuleToolVersion()
+        {
+            // 2 logs 2 runs, same tool version, 6 rules
+            SarifLog sarifLog1 = new SarifLog { Runs = new[] { CreateTestRun(6, null, true) } };


SarifLog

this code doesn't meet our style standard, you should be using 'var' instead of an explicit type, because the type itself is clearly specified after the new keyword.

Why did this code pass build? Are we not enforcing this standard in our diagnostics configuration? #Resolved

Fixed. Its no enforced. In VS there are messages (not warning or erro) in error list showing this kind of issue.

michaelcfanning · 2022-07-31T17:48:01Z

Um, this is an excellent question. We support multiple varieties of intra-logfile link, one of which is a simply reference to a location id (which I think should generally be stable as I believe it's associated with an id). The support for the SARIF schema, I also think, is mostly non-existent. And so I'd guess we have no code whatsoever to assist with this. Yong, we should create an issue for this?

In reply to: 1199064853

…/github.com/microsoft/sarif-sdk into users/yongyan-gh/resultmatching-splitlogbug

yongyan-gh · 2022-08-02T23:45:33Z

Right, the merge command does not update the pointers. Instead it calls FixupVisitor to resolve result's artifact location if it has index pointer points to artifacts array, and remove the artifact array. This needs further efforts, created issue #2520 for tracking.

In reply to: 1200469594

VCOLAWTON · 2022-08-02T23:49:13Z

src/ReleaseHistory.md

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


Can you send me a few sentences that explain what this document is used for? I can refine the language for you.

VCOLAWTON

Need an overview, please see comment. Thanks, Yong! @yongyan-gh

marmegh · 2022-08-05T18:40:16Z

src/ReleaseHistory.md

+## Unreleased
+
+* BUGFIX: Fix the issue `merge` command generates SARIF with multiple runs even specified `--merge-runs` option [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)
+* BUGFIX: Fix `System.IO.IOException` and `System.IO.DirectoryNotFoundException` when merge Sarif files contains invalid file name characters in the rule id with `--split PerRule` option.


I don't see any other references to these exceptions in this change? Was this broken out into one of your other PRs? Or was this included and not documented in a closed PR? #Resolved

This error was not reported by any issue, but found during my testing for merge command. My other 2 PRs are for match-result-forward command. Its a straightforward fix, so I think it can be fixed along with the multiple runs issue.

Can you point me to the specific test implemented to prevent regression?

sure, in this test `MergeCommandUnitTests.MergeCommand_WhenSplitPerRule_LogShouldAggregateByRuleToolVersion', it creates sarif with id contains invalid char '/', e.g. 'TESTRULE/001', and the fix replaced the invalid char and generate valid file name e.g. 'TESTRULE.001_merged.sarif'

shaopeng-gh · 2022-08-17T20:12:10Z

src/Sarif.Driver/PathExtensions.cs

@@ -19,6 +20,8 @@ namespace Microsoft.CodeAnalysis.Sarif.Driver
    /// <summary>Path extension functions.</summary>
    public static class PathExtensions
    {
+        private static readonly Regex s_invalidFileNameCharRegex = new Regex($"[{Regex.Escape(new string(Path.GetInvalidFileNameChars()))}]", RegexOptions.Compiled | RegexOptions.CultureInvariant);


Path.GetInvalidFileNameChars()

not an actual issue,
just to note that with current way we will possibly generate different name in Linux and Windows,
and a name without any invalid char (win+Linux) would be
working in both os, easier to share.

the other way of cause is to hard code the combination of them.
each has its pros and cons, just to call it out.

shaopeng-gh

shaopeng-gh · 2022-08-18T01:15:11Z

Need to resolve the conflicts.

In reply to: 1218848244

yongyan-gh · 2022-08-18T03:24:21Z

resolved

In reply to: 1218848244

yongyan-gh · 2022-08-18T07:45:51Z

src/Test.Utilities.Sarif/RandomSarifLogGenerator.cs

@@ -13,7 +13,7 @@
    /// <summary>
    /// Generate random sarif logs for testing.
    /// </summary>
-    internal static class RandomSarifLogGenerator
+    public static class RandomSarifLogGenerator


public

Move this helper to the common Teset.Utilities.Sarif project so it can be used by other test projects. #ByDesign

michaelcfanning · 2022-08-18T14:37:14Z

src/ReleaseHistory.md

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

+## Unreleased
+
+* BUGFIX: Fixed `merge` command producing runs per rule to the expected runs per tool. [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)


er rule to the expected r

this doesn't describe the adverse condition precisely or the state once resolved #Resolved

updated as suggested

michaelcfanning · 2022-08-18T14:37:31Z

src/ReleaseHistory.md

+## Unreleased
+
+* BUGFIX: Fixed `merge` command producing runs per rule to the expected runs per tool. [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)
+* BUGFIX: Fixed `System.IO.IOException` or `System.IO.DirectoryNotFoundException` when `merge` command producing per rule files.


command producing per rule files

I don't understand this #Resolved

updated the note as suggested

michaelcfanning · 2022-08-18T14:39:08Z

src/Sarif.Multitool.Library/MergeCommand.cs

-                    outputFilePath = Path.Combine(outputDirectory, GetOutputFileName(_options, key));
+                    outputFilePath = Path.Combine(
+                        outputDirectory,
+                        GetOutputFileName(_options, key).ReplaceInvalidCharInFileName("."));


GetOutputFileName

Please comment this code to describe why we're doing this.

// We must replace invalid file characters in the constructed output file name that may appear in any rule ids incorporated into the file name candidate.

#Resolved

michaelcfanning · 2022-08-18T14:45:32Z

src/ReleaseHistory.md

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

+## Unreleased
+
+* BUGFIX: Fixed `merge` command producing runs per rule to the expected runs per tool. [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)


Fixed

Use this:

Update merge command to properly produce runs by tool and version when passed the --merge-runs argument. #Resolved

michaelcfanning · 2022-08-18T14:47:09Z

src/ReleaseHistory.md

+## Unreleased
+
+* BUGFIX: Fixed `merge` command producing runs per rule to the expected runs per tool. [#2488](https://github.com/microsoft/sarif-sdk/pull/2488)
+* BUGFIX: Fixed `System.IO.IOException` or `System.IO.DirectoryNotFoundException` when `merge` command producing per rule files.


Fixed `System.IO.IOException

Please use this:

'Eliminate IOException and DirectoryNotFoundException exceptions thrown by merge command when splitting by rule (due to invalid file characters in rule ids). #Resolved

michaelcfanning · 2022-08-18T14:51:08Z

src/Test.UnitTests.Sarif.Driver/PathExtensionsTests.cs

+            outputHelper.WriteLine($"TestName: {nameof(PathExtensionsTests)} has random seed {randomSeed}");
+        }
+
+        [Theory]


Theory

Please stop introducing [Theory] usage into the code base, this functionality uses reflection and historically has caused performance issues.

All this is really buying is is the need not to inject a foreach in the method that calls into your helper. #Resolved

changed to Fact pattern

michaelcfanning · 2022-08-18T14:51:33Z

src/Test.UnitTests.Sarif.Driver/PathExtensionsTests.cs

+            VerifyReplaceInvalidCharInFileName(fileName, replacement, expectFileName);
+        }
+
+        [Theory]


Theory

No more [Theory] usage. #Resolved

Changed to [Fact] test pattern

michaelcfanning

yongyan-gh added 2 commits July 28, 2022 16:18

Fix merge issue generate multiple runs

977e02f

Remove extra whitespaces

cb96d07

yongyan-gh commented Jul 28, 2022

View reviewed changes

yongyan-gh marked this pull request as ready for review July 28, 2022 23:46

yongyan-gh requested review from eddynaka and michaelcfanning as code owners July 28, 2022 23:46

yongyan-gh requested review from EasyRhinoMSFT, marmegh and shaopeng-gh July 28, 2022 23:46

github-advanced-security bot found potential problems Jul 28, 2022

View reviewed changes

src/Sarif.Multitool.Library/MergeCommand.cs Fixed Show resolved Hide resolved

michaelcfanning reviewed Jul 31, 2022

View reviewed changes

yongyan-gh added 9 commits August 2, 2022 03:27

address review feedbacks

9df7c63

Fix test failure in linux/mac

88f5ff2

fix dotnet format

7a74252

Add more test coverage

49c4f5d

Merge branch 'main' into users/yongyan-gh/resultmatching-splitlogbug

5579d3a

Remove unused using.

bca52ca

Merge branch 'users/yongyan-gh/resultmatching-splitlogbug' of https:/…

4e04a60

…/github.com/microsoft/sarif-sdk into users/yongyan-gh/resultmatching-splitlogbug

Update release history

38c2fcc

Fix a typo in merge-runs description

be5099b

yongyan-gh mentioned this pull request Aug 2, 2022

Handling Sarif pointers in merge command #2520

Open

VCOLAWTON reviewed Aug 2, 2022

View reviewed changes

VCOLAWTON suggested changes Aug 2, 2022

View reviewed changes

Update release note

1211181

marmegh reviewed Aug 5, 2022

View reviewed changes

Update release notes

c53ddf1

yongyan-gh requested a review from cfaucon as a code owner August 5, 2022 21:58

Merge branch 'main' into users/yongyan-gh/resultmatching-splitlogbug

a5d4563

shaopeng-gh reviewed Aug 17, 2022

View reviewed changes

shaopeng-gh approved these changes Aug 18, 2022

View reviewed changes

Merge branch 'main' into users/yongyan-gh/resultmatching-splitlogbug

3574c6a

yongyan-gh commented Aug 18, 2022

View reviewed changes

Cleanup code

c00275f

michaelcfanning reviewed Aug 18, 2022

View reviewed changes

michaelcfanning approved these changes Aug 18, 2022

View reviewed changes

Address review feedbacks

8280722

michaelcfanning merged commit e1bb6fe into main Aug 19, 2022

michaelcfanning deleted the users/yongyan-gh/resultmatching-splitlogbug branch August 19, 2022 14:48

This was referenced Feb 21, 2023

Releasing v3.2.0 #2622

Closed

Releasing 4.0.0 #2623

Merged

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

Fix bug: merge command generates multiple runs #2513

Fix bug: merge command generates multiple runs #2513

Conversation

yongyan-gh commented Jul 28, 2022 • edited Loading

Description

yongyan-gh Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongyan-gh Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KalleOlaviNiemitalo commented Jul 29, 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

michaelcfanning Jul 31, 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

michaelcfanning Jul 31, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 31, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 31, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jul 31, 2022 • edited by yongyan-gh Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Jul 31, 2022

yongyan-gh commented Aug 2, 2022

Choose a reason for hiding this comment

VCOLAWTON left a comment

Choose a reason for hiding this comment

marmegh Aug 5, 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

shaopeng-gh Aug 17, 2022 • edited Loading

Choose a reason for hiding this comment

shaopeng-gh left a comment

Choose a reason for hiding this comment

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

yongyan-gh commented Aug 18, 2022

yongyan-gh Aug 18, 2022 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Aug 18, 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: `merge` command generates multiple runs #2513

Fix bug: `merge` command generates multiple runs #2513

yongyan-gh commented Jul 28, 2022 •

edited

Loading

yongyan-gh Jul 28, 2022 •

edited

Loading

yongyan-gh Jul 28, 2022 •

edited

Loading

KalleOlaviNiemitalo commented Jul 29, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jul 31, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jul 31, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jul 31, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jul 31, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jul 31, 2022 •

edited by yongyan-gh

Loading

marmegh Aug 5, 2022 •

edited by yongyan-gh

Loading

shaopeng-gh Aug 17, 2022 •

edited

Loading

shaopeng-gh commented Aug 18, 2022 •

edited by yongyan-gh

Loading

yongyan-gh Aug 18, 2022 •

edited

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Aug 18, 2022 •

edited by yongyan-gh

Loading