Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation #2433

eddynaka · 2022-01-28T18:46:03Z

Description

The single/multi-threaded command base was generating all the artifacts even if we were not producing results.

Test

Testing multiple files with and without the hashes flag and checking if artifacts were produced.

This PR has a dependency on this: #2434, which will reduce the number of files to be analyzed once we merge it. That PR is just rebaselining our test files, preventing new changes from receiving all the changes we are seeing in this.

eddynaka · 2022-01-28T18:55:46Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

+                _run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri },
+                                   addToFilesTableIfNotPresent: _persistArtifacts,
+                                   dataToInsert: _dataToInsert,
+                                   hashData: context.Hashes);


we should only persist if:

we have results

if we have hashes/textfiles/binaryfiles flag enabled

eddynaka · 2022-01-28T18:56:16Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

@@ -485,10 +491,6 @@ private async Task<bool> HashAsync()
                            _hashToFilesMap[hashData.Sha256] = paths;
                        }

-                        _run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri },
-                                           dataToInsert: _dataToInsert,
-                                           hashData: hashData);


removing this to prevent us from generating artifacts for all the files #ByDesign

eddynaka · 2022-01-28T18:57:06Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

@@ -1165,7 +1165,7 @@ public void AnalyzeCommandBase_AutomationDetailsTests()
            }
        }

-        [Fact(Timeout = 5000)]
+        [Fact(Timeout = 5000, Skip = "Artifacts will be different while we don't fix SarifLogger and AnalyzeCommandBase.")]
        public void AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest()


AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest

This test is comparing the output of a single thread vs multithreaded command base.

With this change, both will have different outputs which will be fixed in the future. #Closed

already fixed in this pr

michaelcfanning · 2022-01-28T19:10:20Z

src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs

@@ -1165,7 +1165,7 @@ public void AnalyzeCommandBase_AutomationDetailsTests()
            }
        }

-        [Fact(Timeout = 5000)]
+        [Fact(Timeout = 5000, Skip = "Artifacts will be different while we don't fix SarifLogger and AnalyzeCommandBase.")]
        public void AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest()


AnalyzeCommandBase_ShouldGenerateSameResultsWhenRunningSingleAndMultiThread_CoyoteTest

SingleAndMultithreaded #Closed

michaelcfanning · 2022-01-28T19:11:08Z

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

@@ -170,7 +170,7 @@ private static void VerifySuppressCommand(SuppressOptions options)

            if (options.ExpiryInDays > 0 && suppression.TryGetProperty("expiryUtc", out DateTime expiryUtc))
            {
-                expiryUtc.Should().BeCloseTo(DateTime.UtcNow.AddDays(options.ExpiryInDays));
+                expiryUtc.Should().BeCloseTo(DateTime.UtcNow.AddDays(options.ExpiryInDays), precision: 500);


500

used a shared constant #Resolved

michaelcfanning · 2022-01-28T19:11:24Z

Where is my release note?

In reply to: 1024526220

eddynaka · 2022-01-28T21:30:44Z

src/Sarif/HashUtilities.cs

@@ -31,15 +31,14 @@ internal static IFileSystem FileSystem

        public static IDictionary<string, HashData> MultithreadedComputeTargetFileHashes(IEnumerable<string> analysisTargets, bool suppressConsoleOutput = false)
        {
-            if (analysisTargets == null) { return null; }
+            var fileToHashDataMap = new ConcurrentDictionary<string, HashData>();
+            if (analysisTargets == null) { return fileToHashDataMap; }


This will prevent a NullReferenceException when we use the reerence variable from the SarifLogger

eddynaka · 2022-01-28T21:31:12Z

src/Sarif/Writers/SarifLogger.cs

@@ -447,12 +447,16 @@ private void CaptureArtifact(ArtifactLocation fileLocation)
                catch (ArgumentException) { } // Unrecognized encoding name
            }

+            HashData hashData = null;
+            AnalysisTargetToHashDataMap?.TryGetValue(fileLocation.Uri.OriginalString, out hashData);


AnalysisTargetToHashDataMap?.TryGetValue

if we have the hash, let's re-use it.
this will prevent us from hashing the same file twice

eddynaka · 2022-01-28T21:31:42Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -169,7 +169,7 @@ private void Analyze(TOptions options, AggregatingLogger logger)
            targets = ValidateTargetsExist(_rootContext, targets);

            // 5. Initialize report file, if configured.
-            InitializeOutputFile(options, _rootContext, targets);


InitializeOutputFile(options, _rootContext, targets);

we won't initialize the outputfile with targets because we don't want to emit all targets in the artifacts.

eddynaka · 2022-01-28T21:32:05Z

src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs

                        paths.Add(localPath);
                        context.Hashes = hashData;
+                        _pathToHashDataMap?.Add(localPath, hashData);
                    }


Updating this will enable us to use the code from SarifLogger to create the artifacts #ByDesign

eddynaka · 2022-01-28T21:32:32Z

....Sarif/TestData/Multitool/BaselineOption/ExpectedOutputs/TEST1001.ValidateWithBaseline.sarif

-            "index": 0
-          }
-        }
-      ],


this will go away, since we are not using any flag to emit artifacts.

eddynaka · 2022-01-28T21:32:59Z

....Sarif/TestData/Multitool/BaselineOption/ExpectedOutputs/TEST1001.ValidateWithBaseline.sarif

            },
            {
              "id": "SARIF2012",
-              "name": "ProvideRuleProperties",


name

the order of the properties were enforced by shaopeng in a previous PR.

michaelcfanning · 2022-01-28T21:45:15Z

src/ReleaseHistory.md

@@ -2,6 +2,7 @@

 ## Unreleased

+* BUGFIX: Fix `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` from outputting all artifacts to SARIF even if no results were produced when Hashes is enabled. [#2433](https://github.com/microsoft/sarif-sdk/pull/2433)


BUGFIX

BREAKING: AnalyzeCommandBase previously persisted all scan target artifacts to SARIF logs rather than only persisting artifacts referenced by an analysis result, when an option to persist hashes, text file or binary information was set. MultithreadedAnalyzeCommandBase previously persisted all scan targets artifacts to SARIF logs in cases when hash insertion was eenabled rather than only persisting artifacts referenced by an analysis result. #Resolved

michaelcfanning · 2022-01-28T21:47:31Z

src/Sarif/HashUtilities.cs

@@ -31,15 +31,14 @@ internal static IFileSystem FileSystem

        public static IDictionary<string, HashData> MultithreadedComputeTargetFileHashes(IEnumerable<string> analysisTargets, bool suppressConsoleOutput = false)
        {
-            if (analysisTargets == null) { return null; }


null

Please revert this change. You should be careful about allocating objects to indicate no results. Why not just have the caller handle null? This code succeeded previously, so you probably have only just introduced the requirement for this to be non-null. #Resolved

michaelcfanning · 2022-01-28T21:48:55Z

src/Sarif.Driver/Sdk/AnalyzeCommandBase.cs

@@ -169,7 +169,7 @@ private void Analyze(TOptions options, AggregatingLogger logger)
            targets = ValidateTargetsExist(_rootContext, targets);

            // 5. Initialize report file, if configured.
-            InitializeOutputFile(options, _rootContext, targets);


targets

Revert this change please it's unnecessarily breaking customers. #Resolved

This API isn't exposed to customers. Only tools that use it will see a difference.

…fixing-multithreaded-artifacts

eddynaka · 2022-01-28T22:38:13Z

src/Sarif/OptionallyEmittedData.cs

@@ -80,6 +80,9 @@ public enum OptionallyEmittedData : int
        // Enrich SARIF log with git blame information
        GitBlameInformation = 0x1000,

+        // Enrich with artifacts only.
+        Artifacts = 0x2000,


Artifacts

To keep the same behavior as before, I introduced a new flag that will be able to emit the artifacts only

eddynaka · 2022-01-31T17:55:44Z

...Data/Multitool/ValidateCommand/ExpectedOutputs/SARIF2012.ProvideRuleProperties_Invalid.sarif

@@ -120,7 +120,7 @@
                  "index": 0
                },
                "region": {
-                  "startLine": 22,


22

we faced a similar issue in this PR:
https://github.com/microsoft/sarif-sdk/pull/2420/files#diff-277d16bd3a7355417dd787cfc0961db73b7f32e73ad49650468d4f4bb6e5c477

We should investigate this.

Yes, let's open an issue on it.

…://github.com/microsoft/sarif-sdk into users/ednakamu/fixing-multithreaded-artifacts

michaelcfanning

…ifacts generation (#2433) * Fixing multithreaded artifacts generation * Fixing tests and flags * Loosing precision. * Applying fix for AnalyzeCommandBase * Enabling tests * Updating test case and release history * Creating const to prevent magical numbers everywhere * Rebaselining tests * Creating Artifacts flag to keep previous behavior * Addressing PR feedback. * Rollback changes * Update SARIF2012.ProvideRuleProperties_Invalid.sarif * updating back * Ordering deprecated names

) * Add new visitor to get deterministic SARIF log by sorting results * Fix dotnet format issue * updating format * remove unnecessary using Format & minor fixes * Add Run Comparer to support sorting logs with multiple runs. * Add command argument unit tests fix dotnet format * use ContainsKey to avoid allocating variable * Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation (#2433) * Fixing multithreaded artifacts generation * Fixing tests and flags * Loosing precision. * Applying fix for AnalyzeCommandBase * Enabling tests * Updating test case and release history * Creating const to prevent magical numbers everywhere * Rebaselining tests * Creating Artifacts flag to keep previous behavior * Addressing PR feedback. * Rollback changes * Update SARIF2012.ProvideRuleProperties_Invalid.sarif * updating back * Ordering deprecated names * `SarifLogger` now emits an artifacts table entry if `artifactLocation` is not null for tool configuration and tool execution notifications. (#2437) * Fixing artifacts generation when logging notifications * Updating release history. * Updating ReleaseHistory * Fix `ArgumentException` when recurse is enabled and two file target specifiers generates the same file paths (#2438) * Fixing ArgumentException when passing two filePaths that generates duplicated file analysis * Fixing dotnet-format issues and updating releasehistory * Removing comments * Addressing PR feedback * Addressing PR feedback * Addressing PR review issues Add suppression support (#2435) * Add suppression support * Add incompatibility check and make suppressions non-null Co-authored-by: Eddy Nakamura <eddynaka@gmail.com> Update releasehistory fix couple test cases * Fix issues in PR review * Add xml comments * Fix test issues * fix dotnet format * Addressing review feedbacks * Fix tests * Update extension methods names * Change xml doc comments to normal comments Co-authored-by: Eddy Nakamura <eddynaka@gmail.com> Co-authored-by: Michael C. Fanning <michael.fanning@microsoft.com>

eddynaka added 2 commits January 28, 2022 10:45

Fixing multithreaded artifacts generation

2247cbe

Fixing tests and flags

6972472

eddynaka commented Jan 28, 2022

View reviewed changes

Loosing precision.

5d87498

michaelcfanning reviewed Jan 28, 2022

View reviewed changes

eddynaka added 3 commits January 28, 2022 13:10

Applying fix for AnalyzeCommandBase

af33d03

Enabling tests

3869b86

Updating test case and release history

70dffb5

eddynaka commented Jan 28, 2022

View reviewed changes

Creating const to prevent magical numbers everywhere

171133f

michaelcfanning reviewed Jan 28, 2022

View reviewed changes

eddynaka added 3 commits January 28, 2022 14:05

Rebaselining tests

d322ee4

Merge branch 'users/ednakamu/rebaselining-files' into users/ednakamu/…

e800ead

…fixing-multithreaded-artifacts

Creating Artifacts flag to keep previous behavior

fd9bd16

eddynaka commented Jan 28, 2022

View reviewed changes

Addressing PR feedback.

6996260

eddynaka changed the title ~~Fixing multithreaded artifacts generation~~ Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation Jan 28, 2022

eddynaka and others added 2 commits January 28, 2022 15:44

Rollback changes

8a78dd8

Update SARIF2012.ProvideRuleProperties_Invalid.sarif

44eb8d2

updating back

cabbd5f

eddynaka commented Jan 31, 2022

View reviewed changes

eddynaka marked this pull request as ready for review January 31, 2022 17:56

eddynaka added 2 commits January 31, 2022 15:21

Ordering deprecated names

a3408b3

Merge branch 'users/ednakamu/fixing-multithreaded-artifacts' of https…

dbaae40

…://github.com/microsoft/sarif-sdk into users/ednakamu/fixing-multithreaded-artifacts

eddynaka mentioned this pull request Jan 31, 2022

Artifacts should only be logged in the SARIF if we have results #2431

Closed

michaelcfanning approved these changes Jan 31, 2022

View reviewed changes

michaelcfanning merged commit 6263379 into main Jan 31, 2022

michaelcfanning deleted the users/ednakamu/fixing-multithreaded-artifacts branch January 31, 2022 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation #2433

Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation #2433

eddynaka commented Jan 28, 2022 •

edited

eddynaka Jan 28, 2022

eddynaka Jan 28, 2022 •

edited

eddynaka Jan 28, 2022 •

edited

eddynaka Jan 28, 2022

michaelcfanning Jan 28, 2022 •

edited by eddynaka

eddynaka Jan 28, 2022

michaelcfanning Jan 28, 2022 •

edited by eddynaka

eddynaka Jan 28, 2022

michaelcfanning commented Jan 28, 2022 •

edited by eddynaka

eddynaka Jan 28, 2022

eddynaka Jan 28, 2022

eddynaka Jan 28, 2022

eddynaka Jan 28, 2022 •

edited

eddynaka Jan 28, 2022

eddynaka Jan 28, 2022

michaelcfanning Jan 28, 2022 •

edited by eddynaka

michaelcfanning Jan 28, 2022 •

edited by eddynaka

eddynaka Jan 28, 2022

michaelcfanning Jan 28, 2022 •

edited by eddynaka

eddynaka Jan 28, 2022

eddynaka Jan 28, 2022

eddynaka Jan 31, 2022

michaelcfanning Jan 31, 2022

michaelcfanning left a comment

		@@ -2,6 +2,7 @@

		## Unreleased

		* BUGFIX: Fix `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` from outputting all artifacts to SARIF even if no results were produced when Hashes is enabled. [#2433](https://github.com/microsoft/sarif-sdk/pull/2433)

Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation #2433

Fixing AnalyzeCommandBase and MultithreadedAnalyzeCommandBase artifacts generation #2433

Conversation

eddynaka commented Jan 28, 2022 • edited

Description

Test

Choose a reason for hiding this comment

eddynaka Jan 28, 2022 • edited

Choose a reason for hiding this comment

eddynaka Jan 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 28, 2022 • edited by eddynaka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 28, 2022 • edited by eddynaka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning commented Jan 28, 2022 • edited by eddynaka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka Jan 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 28, 2022 • edited by eddynaka

Choose a reason for hiding this comment

michaelcfanning Jan 28, 2022 • edited by eddynaka

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 28, 2022 • edited by eddynaka

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

michaelcfanning left a comment

Choose a reason for hiding this comment

Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation #2433

Fixing `AnalyzeCommandBase` and `MultithreadedAnalyzeCommandBase` artifacts generation #2433

eddynaka commented Jan 28, 2022 •

edited

eddynaka Jan 28, 2022 •

edited

eddynaka Jan 28, 2022 •

edited

michaelcfanning Jan 28, 2022 •

edited by eddynaka

michaelcfanning Jan 28, 2022 •

edited by eddynaka

michaelcfanning commented Jan 28, 2022 •

edited by eddynaka

eddynaka Jan 28, 2022 •

edited

michaelcfanning Jan 28, 2022 •

edited by eddynaka

michaelcfanning Jan 28, 2022 •

edited by eddynaka

michaelcfanning Jan 28, 2022 •

edited by eddynaka