Skip to content

Commit

Permalink
Fix NullReferenceException in WorkItemFiler when all results baseli…
Browse files Browse the repository at this point in the history 
…ne state are unchange/absent/updated (#2412)

* Fix null reference exception

* Do not file work item if all results should not be filed

update comments

Update comments

* Update release history

* update release history

* Fix the test so it covers changes correctly

* Use portable path instead of DOS path in test cases

* address PR feedbacks

* update Linq query to use null-conditional operator ?.

* Add null check for ShouldBeFiled() extension method

* Address code review feedbacks

* Fix test case

* Addressing review issues

* merge from main branch

* Add more test coverages

* Fix dotnet format issues
  • Loading branch information
@yongyan-gh
yongyan-gh committed Feb 25, 2022
1 parent 25b7e57 commit 1b7d87c
Show file tree
Hide file tree
Showing 7 changed files with 563 additions and 124 deletions.
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* BUGFIX: Fix `ArgumentException` when `--recurse` is enabled and two file target specifiers generates the same file path. [#2438](https://github.com/microsoft/sarif-sdk/pull/2438)
* BUGFIX: Fix 'InvalidOperationException' with message `Collection was modified; enumeration operation may not execute` in `MultithreadedAnalyzeCommandBase`, which is raised when analyzing with the `--hashes` switch. [#2447](https://github.com/microsoft/sarif-sdk/pull/2447)
* BUGFIX: Fix `Merge` command produces empty SARIF file in Linux when providing file name only without path. [#2408](https://github.com/microsoft/sarif-sdk/pull/2408)
* BUGFIX: Fix `NullReferenceException` when filing work item with a SARIF file which has no filable results. [#2412](https://github.com/microsoft/sarif-sdk/pull/2412)

## **v2.4.12** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.12) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.12) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.12) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.12) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.12)

Expand Down
53 changes: 31 additions & 22 deletions src/Sarif.WorkItems/SarifWorkItemFiler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ public class SarifWorkItemFiler : IDisposable
private readonly object m_syncRoot = new object();
private FilingClient m_filingClient = null;

internal static readonly string s_multipleLocationsTextPattern = "{0} (+{1} locations)";

/// <summary>
/// Initializes a new instance of the <see cref="SarifWorkItemFiler"> class.</see>
/// </summary>
Expand Down Expand Up @@ -199,17 +201,17 @@ public virtual IReadOnlyList<SarifLog> SplitLogFile(SarifLog sarifLog)

Logger.LogInformation($"Splitting strategy - {splittingStrategy}");

if (splittingStrategy == SplittingStrategy.None)
{
return new[] { sarifLog };
}

PartitionFunction<string> partitionFunction = null;

Stopwatch splittingStopwatch = Stopwatch.StartNew();

switch (splittingStrategy)
{
case SplittingStrategy.None:
{
partitionFunction = (result) => result.ShouldBeFiled() ? "Include" : null;
break;
}
case SplittingStrategy.PerRun:
{
partitionFunction = (result) => result.ShouldBeFiled() ? "Include" : null;
Expand Down Expand Up @@ -332,6 +334,13 @@ public SarifWorkItemModel FileWorkItemInternal(SarifLog sarifLog, SarifWorkItemC
}
}

if (string.IsNullOrWhiteSpace(sarifWorkItemModel.Title) || string.IsNullOrWhiteSpace(sarifWorkItemModel.BodyOrDescription))
{
this.Logger.LogWarning("Attempt to call work item client with invalid work item values.", logId);
this.LogMetricsForProcessedModel(sarifLog, sarifWorkItemModel, FilingResult.Canceled);
return null;
}

Task<IEnumerable<WorkItemModel>> task = filingClient.FileWorkItems(new[] { sarifWorkItemModel });
task.Wait();
this.FiledWorkItems.AddRange(task.Result);
Expand Down Expand Up @@ -387,23 +396,23 @@ private void LogMetricsForProcessedModel(SarifLog sarifLog, SarifWorkItemModel s
: "";

var workItemMetrics = new Dictionary<string, object>
{
{ "LogId", logId },
{ "WorkItemModelId", sarifWorkItemModel.Id },
{ nameof(sarifWorkItemModel.Area), sarifWorkItemModel.Area },
{ nameof(sarifWorkItemModel.BodyOrDescription), sarifWorkItemModel.BodyOrDescription },
{ "FilingResult", filingResult.ToString() },
{ nameof(sarifWorkItemModel.CommentOrDiscussion), sarifWorkItemModel.CommentOrDiscussion },
{ nameof(sarifWorkItemModel.HtmlUri), sarifWorkItemModel.HtmlUri },
{ nameof(sarifWorkItemModel.Iteration), sarifWorkItemModel.Iteration },
{ "LabelsOrTags", tags },
{ "LocationUri", uris },
{ nameof(sarifWorkItemModel.Milestone), sarifWorkItemModel.Milestone },
{ nameof(sarifWorkItemModel.OwnerOrAccount), sarifWorkItemModel.OwnerOrAccount },
{ nameof(sarifWorkItemModel.RepositoryOrProject), sarifWorkItemModel.RepositoryOrProject },
{ nameof(sarifWorkItemModel.Title), sarifWorkItemModel.Title },
{ nameof(sarifWorkItemModel.Uri), sarifWorkItemModel.Uri },
};
{
{ "LogId", logId },
{ "WorkItemModelId", sarifWorkItemModel.Id },
{ nameof(sarifWorkItemModel.Area), sarifWorkItemModel.Area },
{ nameof(sarifWorkItemModel.BodyOrDescription), sarifWorkItemModel.BodyOrDescription },
{ "FilingResult", filingResult.ToString() },
{ nameof(sarifWorkItemModel.CommentOrDiscussion), sarifWorkItemModel.CommentOrDiscussion },
{ nameof(sarifWorkItemModel.HtmlUri), sarifWorkItemModel.HtmlUri },
{ nameof(sarifWorkItemModel.Iteration), sarifWorkItemModel.Iteration },
{ "LabelsOrTags", tags },
{ "LocationUri", uris },
{ nameof(sarifWorkItemModel.Milestone), sarifWorkItemModel.Milestone },
{ nameof(sarifWorkItemModel.OwnerOrAccount), sarifWorkItemModel.OwnerOrAccount },
{ nameof(sarifWorkItemModel.RepositoryOrProject), sarifWorkItemModel.RepositoryOrProject },
{ nameof(sarifWorkItemModel.Title), sarifWorkItemModel.Title },
{ nameof(sarifWorkItemModel.Uri), sarifWorkItemModel.Uri },
};

foreach (string key in additionalCustomDimensions.Keys)
{
Expand Down
13 changes: 7 additions & 6 deletions src/Sarif.WorkItems/SarifWorkItemModel.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,14 @@ public class SarifWorkItemModel : WorkItemModel<SarifWorkItemContext>
}
}

bool? resultContainsWorkItemUri = sarifLog.Runs?.Any(run => run.Results.Any(result => result.WorkItemUris?.Count > 0));
if (resultContainsWorkItemUri == true)
foreach (Run run in sarifLog.Runs)
{
Uri workItemUri = sarifLog.Runs?.Where(run => run.Results != null && run.Results.Any(result => result.WorkItemUris?.Count > 0)).LastOrDefault()
.Results.Where(result => result.WorkItemUris != null && result.WorkItemUris.Count > 0).LastOrDefault()
.WorkItemUris.LastOrDefault();
this.Uri = workItemUri;
Result resultContainsWorkItemUri = run.Results.LastOrDefault(result => result.WorkItemUris?.Count > 0);
if (resultContainsWorkItemUri != null)
{
this.Uri = resultContainsWorkItemUri.WorkItemUris.LastOrDefault();
break;
}
}

// Shared GitHub/Azure DevOps concepts
Expand Down
47 changes: 30 additions & 17 deletions src/Sarif.WorkItems/SarifWorkItemsExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
using System.IO;
using System.Linq;

using Microsoft.VisualStudio.Services.Common;

namespace Microsoft.CodeAnalysis.Sarif.WorkItems
{
public static class SarifWorkItemsExtensions
Expand Down Expand Up @@ -143,33 +145,44 @@ public static int GetAggregateFilableResultsCount(this SarifLog log)

public static string CreateWorkItemDescription(this SarifLog log, SarifWorkItemContext context)
{
int totalResults = log.GetAggregateFilableResultsCount();
int totalResults = 0;
int artifactLocationCount = 0;
Uri firstDetectedLocation = null;

IEnumerable<Result> results = log?.Runs?.SelectMany(run => run.Results.Where(r => r.ShouldBeFiled()));

foreach (Result result in results.AsEmptyIfNull())
{
totalResults++;
foreach (Location location in result.Locations.AsEmptyIfNull())
{
firstDetectedLocation ??= location.PhysicalLocation?.ArtifactLocation?.Uri;
artifactLocationCount += location.PhysicalLocation?.ArtifactLocation?.Uri != null ? 1 : 0;
}
}

if (totalResults == 0)
{
return null;
}

List<string> toolNames = log.GetToolNames();
string phrasedToolNames = toolNames.ToAndPhrase();
string multipleToolsFooter = toolNames.Count > 1 ? WorkItemsResources.MultipleToolsFooter : string.Empty;

IEnumerable<Result> results = log?.Runs?[0]?.Results.Where(r => r.ShouldBeFiled());
Uri runRepositoryUri = log?.Runs.FirstOrDefault()?.VersionControlProvenance?.FirstOrDefault().RepositoryUri;
Uri detectionLocationUri = !string.IsNullOrEmpty(runRepositoryUri?.OriginalString) ?
runRepositoryUri :
results?.FirstOrDefault().Locations?[0].PhysicalLocation?.ArtifactLocation?.Uri;
Uri detectionLocationUri = !string.IsNullOrEmpty(runRepositoryUri?.OriginalString)
? runRepositoryUri
: firstDetectedLocation;

string detectionLocation = (detectionLocationUri?.IsAbsoluteUri == true && detectionLocationUri?.Scheme == "https")
? context.CreateLinkText(detectionLocationUri.OriginalString, detectionLocationUri?.OriginalString)
: detectionLocationUri?.OriginalString;

int locCount = results == null ? 0 :
results
.Where(r => r.Locations != null)
.SelectMany(r => r.Locations)
.Where(l => l.PhysicalLocation != null && l.PhysicalLocation.ArtifactLocation != null && l.PhysicalLocation.ArtifactLocation.Uri != null)
.Count();

if (locCount > 1)
{
int additionalLocations = locCount - 1;
detectionLocation = $"{detectionLocation} (+{additionalLocations} locations)";
}
// "{0} (+{1} locations)"
detectionLocation = artifactLocationCount > 1
? string.Format(SarifWorkItemFiler.s_multipleLocationsTextPattern, detectionLocation, artifactLocationCount - 1)
: detectionLocation;

// This work item contains {0} {1} issue(s) detected in {2}{3}. Click the 'Scans' tab to review results.
string description =
Expand Down
Loading

0 comments on commit 1b7d87c

Please sign in to comment.