diff --git a/src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs b/src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs index 744645ae1..30a010aa8 100644 --- a/src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs +++ b/src/Sarif.Driver/Sdk/MultithreadedAnalyzeCommandBase.cs @@ -307,6 +307,10 @@ private void LogCachingLogger(TContext rootContext, IAnalysisLogger logger, TCon if (results?.Count > 0) { + _run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri }, + dataToInsert: _dataToInsert, + hashData: context.Hashes); + foreach (KeyValuePair> kv in results) { foreach (Result result in kv.Value) @@ -485,10 +489,6 @@ private async Task HashAsync() _hashToFilesMap[hashData.Sha256] = paths; } - _run?.GetFileIndex(new ArtifactLocation { Uri = context.TargetUri }, - dataToInsert: _dataToInsert, - hashData: hashData); - paths.Add(localPath); context.Hashes = hashData; } diff --git a/src/Sarif/Core/Run.cs b/src/Sarif/Core/Run.cs index b9e0ab762..490026fd5 100644 --- a/src/Sarif/Core/Run.cs +++ b/src/Sarif/Core/Run.cs @@ -32,12 +32,11 @@ public Uri ExpandUrisWithUriBaseId(string key, string currentValue = null) throw new InvalidOperationException("Author this code along with tests for originalUriBaseIds that are nested"); } - public int GetFileIndex( - ArtifactLocation fileLocation, - bool addToFilesTableIfNotPresent = true, - OptionallyEmittedData dataToInsert = OptionallyEmittedData.None, - Encoding encoding = null, - HashData hashData = null) + public int GetFileIndex(ArtifactLocation fileLocation, + bool addToFilesTableIfNotPresent = true, + OptionallyEmittedData dataToInsert = OptionallyEmittedData.None, + Encoding encoding = null, + HashData hashData = null) { if (fileLocation == null) { throw new ArgumentNullException(nameof(fileLocation)); } diff --git a/src/Sarif/Writers/SarifLogger.cs b/src/Sarif/Writers/SarifLogger.cs index 04604881c..5481ad337 100644 --- a/src/Sarif/Writers/SarifLogger.cs +++ b/src/Sarif/Writers/SarifLogger.cs @@ -19,7 +19,6 @@ public class SarifLogger : BaseLogger, IDisposable, IAnalysisLogger { private readonly Run _run; private readonly TextWriter _textWriter; - private readonly bool _persistArtifacts; private readonly bool _closeWriterOnDispose; private readonly LogFilePersistenceOptions _logFilePersistenceOptions; private readonly JsonTextWriter _jsonTextWriter; @@ -98,8 +97,7 @@ public SarifLogger( dataToRemove, invocationTokensToRedact, invocationPropertiesToLog, - defaultFileEncoding, - AnalysisTargetToHashDataMap); + defaultFileEncoding); tool = tool ?? Tool.CreateFromAssemblyData(); @@ -116,11 +114,6 @@ public SarifLogger( RuleToIndexMap[_run.Tool.Driver.Rules[i]] = i; } } - - _persistArtifacts = - (_dataToInsert & OptionallyEmittedData.Hashes) != 0 || - (_dataToInsert & OptionallyEmittedData.TextFiles) != 0 || - (_dataToInsert & OptionallyEmittedData.BinaryFiles) != 0; } private SarifLogger( @@ -155,8 +148,7 @@ private void EnhanceRun( OptionallyEmittedData dataToRemove, IEnumerable invocationTokensToRedact, IEnumerable invocationPropertiesToLog, - string defaultFileEncoding = null, - IDictionary filePathToHashDataMap = null) + string defaultFileEncoding = null) { _run.Invocations ??= new List(); if (defaultFileEncoding != null) @@ -164,43 +156,9 @@ private void EnhanceRun( _run.DefaultEncoding = defaultFileEncoding; } - Encoding encoding = SarifUtilities.GetEncodingFromName(_run.DefaultEncoding); - if (analysisTargets != null) { _run.Artifacts ??= new List(); - - foreach (string target in analysisTargets) - { - Uri uri = new Uri(UriHelper.MakeValidUri(target), UriKind.RelativeOrAbsolute); - - HashData hashData = null; - if (dataToInsert.HasFlag(OptionallyEmittedData.Hashes)) - { - filePathToHashDataMap?.TryGetValue(target, out hashData); - } - - var artifact = Artifact.Create( - new Uri(target, UriKind.RelativeOrAbsolute), - dataToInsert, - encoding, - hashData: hashData); - - var fileLocation = new ArtifactLocation - { - Uri = uri - }; - - artifact.Location = fileLocation; - - // This call will insert the file object into run.Files if not already present - artifact.Location.Index = _run.GetFileIndex( - artifact.Location, - addToFilesTableIfNotPresent: true, - dataToInsert: dataToInsert, - encoding: encoding, - hashData: hashData); - } } var invocation = Invocation.Create( @@ -447,12 +405,16 @@ private void CaptureArtifact(ArtifactLocation fileLocation) catch (ArgumentException) { } // Unrecognized encoding name } + HashData hashData = null; + AnalysisTargetToHashDataMap?.TryGetValue(fileLocation.Uri.OriginalString, out hashData); + // Ensure Artifact is in Run.Artifacts and ArtifactLocation.Index is set to point to it int index = _run.GetFileIndex( fileLocation, - addToFilesTableIfNotPresent: _persistArtifacts, - _dataToInsert, - encoding); + addToFilesTableIfNotPresent: true, + dataToInsert: _dataToInsert, + encoding, + hashData); // Remove redundant Uri and UriBaseId once index has been set if (index > -1 && this.Optimize) diff --git a/src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs b/src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs index 834dfa9d2..087d36f3d 100644 --- a/src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs +++ b/src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs @@ -1258,21 +1258,81 @@ private void AnalyzeScenarios(int[] scenarios) } [Fact] - public void AnalyzeCommandBase_MultithreadedShouldUseCacheIfFilesAreTheSame() + public void AnalyzeCommandBase_Multithreaded_ShouldOnlyLogArtifactsWhenResultsAreFound() { - // Generating 20 files with different names but same content. - RunMultithreadedAnalyzeCommand(ComprehensiveKindAndLevelsByFilePath, - generateSameOutput: true, - expectedResultCode: 0, - expectedResultCount: 20, - expectedCacheSize: 1); - - // Generating 20 files with different names and content. - RunMultithreadedAnalyzeCommand(ComprehensiveKindAndLevelsByFilePath, - generateSameOutput: false, - expectedResultCode: 0, - expectedResultCount: 7, - expectedCacheSize: 0); + const int expectedNumberOfArtifacts = 2; + const int expectedNumberOfResultsWithErrors = 1; + const int expectedNumberOfResultsWithWarnings = 1; + var files = new List + { + $@"{Environment.CurrentDirectory}\Error.dll", + $@"{Environment.CurrentDirectory}\Warning.dll", + $@"{Environment.CurrentDirectory}\Note.dll", + $@"{Environment.CurrentDirectory}\Pass.dll", + $@"{Environment.CurrentDirectory}\NotApplicable.exe", + $@"{Environment.CurrentDirectory}\Informational.sys", + $@"{Environment.CurrentDirectory}\Open.cab", + $@"{Environment.CurrentDirectory}\Review.dll", + $@"{Environment.CurrentDirectory}\NoIssues.dll", + }; + + var testCase = new ResultsCachingTestCase + { + Files = files, + PersistLogFileToDisk = true, + FileSystem = CreateDefaultFileSystemForResultsCaching(files, generateSameInput: false) + }; + + var options = new TestAnalyzeOptions + { + TestRuleBehaviors = testCase.TestRuleBehaviors, + OutputFilePath = testCase.PersistLogFileToDisk ? Guid.NewGuid().ToString() : null, + TargetFileSpecifiers = new string[] { Guid.NewGuid().ToString() }, + Kind = new List { ResultKind.Fail }, + Level = new List { FailureLevel.Warning, FailureLevel.Error }, + DataToInsert = new OptionallyEmittedData[] { OptionallyEmittedData.Hashes }, + }; + + Run run = RunAnalyzeCommand(options, testCase, multithreaded: true); + + // Hashes is enabled and we should expect to see two artifacts because we have: + // one result with Error level and one result with Warning level. + run.Artifacts.Should().HaveCount(expectedNumberOfArtifacts); + run.Results.Count(r => r.Level == FailureLevel.Error).Should().Be(expectedNumberOfResultsWithErrors); + run.Results.Count(r => r.Level == FailureLevel.Warning).Should().Be(expectedNumberOfResultsWithWarnings); + } + + [Fact] + public void AnalyzeCommandBase_SingleThreaded_ShouldOnlyLogArtifactsWhenResultsAreFound() + { + const int expectedNumberOfResultsWithErrors = 5; + const int expectedNumberOfResultsWithWarnings = 2; + int totalNumber = expectedNumberOfResultsWithErrors + expectedNumberOfResultsWithWarnings; + + var testCase = new ResultsCachingTestCase + { + Files = ComprehensiveKindAndLevelsByFileName, + PersistLogFileToDisk = true, + FileSystem = null + }; + + var options = new TestAnalyzeOptions + { + TestRuleBehaviors = testCase.TestRuleBehaviors, + OutputFilePath = testCase.PersistLogFileToDisk ? Guid.NewGuid().ToString() : null, + TargetFileSpecifiers = new string[] { Guid.NewGuid().ToString() }, + Kind = new List { ResultKind.Fail }, + Level = new List { FailureLevel.Warning, FailureLevel.Error }, + DataToInsert = new OptionallyEmittedData[] { OptionallyEmittedData.Hashes }, + }; + + Run runSingleThread = RunAnalyzeCommand(options, testCase, multithreaded: false); + + // Hashes is enabled and we should expect to see seven artifacts because we have seven distinct results + // of which five are error and two are warnings. + runSingleThread.Artifacts.Should().NotBeEmpty(); + runSingleThread.Results.Should().HaveCount(totalNumber); + runSingleThread.Artifacts.Should().HaveCount(totalNumber); } private static readonly IList ComprehensiveKindAndLevelsByFileName = new List @@ -1513,50 +1573,6 @@ private static SarifLog RunAnalyzeCommand(TestAnalyzeOptions options, : JsonConvert.DeserializeObject(File.ReadAllText(options.OutputFilePath)); } - private static void RunMultithreadedAnalyzeCommand(IList files, - bool generateSameOutput, - int expectedResultCode, - int expectedResultCount, - int expectedCacheSize) - { - var testCase = new ResultsCachingTestCase - { - Files = files, - PersistLogFileToDisk = true, - FileSystem = CreateDefaultFileSystemForResultsCaching(files, generateSameOutput) - }; - - var options = new TestAnalyzeOptions - { - OutputFilePath = Guid.NewGuid().ToString(), - TargetFileSpecifiers = new string[] { Guid.NewGuid().ToString() }, - Kind = new List { ResultKind.Fail }, - Level = new List { FailureLevel.Warning, FailureLevel.Error }, - DataToInsert = new OptionallyEmittedData[] { OptionallyEmittedData.Hashes } - }; - - try - { - TestRule.s_testRuleBehaviors = testCase.TestRuleBehaviors.AccessibleOutsideOfContextOnly(); - - var command = new TestMultithreadedAnalyzeCommand(testCase.FileSystem) - { - DefaultPluginAssemblies = new Assembly[] { typeof(AnalyzeCommandBaseTests).Assembly } - }; - - HashUtilities.FileSystem = testCase.FileSystem; - command.Run(options).Should().Be(expectedResultCode); - - SarifLog sarifLog = JsonConvert.DeserializeObject(File.ReadAllText(options.OutputFilePath)); - sarifLog.Runs[0].Results.Count.Should().Be(expectedResultCount); - command._analysisLoggerCache.Count.Should().Be(expectedCacheSize); - } - finally - { - TestRule.s_testRuleBehaviors = TestRuleBehaviors.None; - } - } - private static SarifLog ConvertConsoleOutputToSarifLog(string consoleOutput) { var sb = new StringBuilder(); diff --git a/src/Test.UnitTests.Sarif/Writers/SarifLoggerTests.cs b/src/Test.UnitTests.Sarif/Writers/SarifLoggerTests.cs index 3ac9e85dd..a0ffd73a0 100644 --- a/src/Test.UnitTests.Sarif/Writers/SarifLoggerTests.cs +++ b/src/Test.UnitTests.Sarif/Writers/SarifLoggerTests.cs @@ -335,7 +335,23 @@ public void SarifLogger_WritesFileData() levels: new List { FailureLevel.Warning, FailureLevel.Error }, kinds: new List { ResultKind.Fail })) { - LogSimpleResult(sarifLogger); + var rule = new ReportingDescriptor { Id = "RuleId" }; + Result result = CreateSimpleResult(rule); + result.Locations = new List + { + new Location + { + PhysicalLocation = new PhysicalLocation + { + ArtifactLocation = new ArtifactLocation + { + Uri = new Uri(file) + } + } + } + }; + + LogSimpleResult(sarifLogger, rule, result); } } @@ -348,55 +364,6 @@ public void SarifLogger_WritesFileData() sarifLog.Runs[0].Artifacts[0].Hashes["sha-256"].Should().Be("0953D7B3ADA7FED683680D2107EE517A9DBEC2D0AF7594A91F058D104B7A2AEB"); } - [Fact] - public void SarifLogger_WritesFileContents_EvenWhenLocationUsesUriBaseId() - { - var sb = new StringBuilder(); - - // Create a temporary file whose extension signals that it is textual. - // This ensures that the ArtifactContents.Text property, rather than - // the Binary property, is populated, so the test of the Text property - // at the end will work. - using (var tempFile = new TempFile(".txt")) - { - string tempFilePath = tempFile.Name; - - File.WriteAllText(tempFilePath, "#include \"windows.h\";"); - - var run = new Run - { - // To get text contents, we also need to specify an encoding that - // Encoding.GetEncoding() will accept. - DefaultEncoding = "UTF-8" - }; - - var analysisTargets = new List - { - tempFilePath - }; - - using (var textWriter = new StringWriter(sb)) - { - // Create a logger that inserts artifact contents. - using (_ = new SarifLogger( - textWriter, - run: run, - analysisTargets: analysisTargets, - dataToInsert: OptionallyEmittedData.TextFiles, - levels: new List { FailureLevel.Warning, FailureLevel.Error }, - kinds: new List { ResultKind.Fail })) - { - } - - // The logger should have populated the artifact contents. - string logText = sb.ToString(); - SarifLog sarifLog = JsonConvert.DeserializeObject(logText); - - sarifLog.Runs[0].Artifacts[0].Contents?.Text.Should().NotBeNullOrEmpty(); - } - } - } - [Fact] public void SarifLogger_WritesFileContentsForAnalysisTargets() { @@ -501,7 +468,23 @@ public void SarifLogger_WritesFileDataWithUnrecognizedEncoding() levels: new List { FailureLevel.Warning, FailureLevel.Error }, kinds: new List { ResultKind.Fail })) { - LogSimpleResult(sarifLogger); + var rule = new ReportingDescriptor { Id = "RuleId" }; + Result result = CreateSimpleResult(rule); + result.Locations = new List + { + new Location + { + PhysicalLocation = new PhysicalLocation + { + ArtifactLocation = new ArtifactLocation + { + Uri = new Uri(file) + } + } + } + }; + + LogSimpleResult(sarifLogger, rule, result); } } @@ -910,7 +893,7 @@ public void SarifLogger_EnhancesRunWithAdditionalAnalysisTargets() // The logger should have merged the analysis targets into the existing Artifacts array. IList artifacts = sarifLog.Runs[0].Artifacts; - artifacts.Count.Should().Be(4); + artifacts.Count.Should().Be(2); } [Fact] @@ -953,6 +936,11 @@ private void LogSimpleResult(SarifLogger sarifLogger) sarifLogger.Log(rule, CreateSimpleResult(rule)); } + private void LogSimpleResult(SarifLogger sarifLogger, ReportingDescriptor rule, Result result) + { + sarifLogger.Log(rule, result); + } + private Result CreateSimpleResult(ReportingDescriptor rule) { return new Result