Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix NullReferenceException in WorkItemFiler when all results baseline state are unchange/absent/updated #2412

Merged
merged 21 commits into from
Feb 25, 2022
Merged

Fix NullReferenceException in WorkItemFiler when all results baseline state are unchange/absent/updated #2412

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
03fb5eb
Fix null reference exception
yongyan-gh Nov 30, 2021
e2ce908
Do not file work item if all results should not be filed
yongyan-gh Nov 30, 2021
d39dc31
Update release history
yongyan-gh Dec 1, 2021
bb7d051
update release history
yongyan-gh Dec 2, 2021
80d790a
Fix the test so it covers changes correctly
yongyan-gh Dec 2, 2021
9665225
Use portable path instead of DOS path in test cases
yongyan-gh Dec 7, 2021
8beb6dc
address PR feedbacks
yongyan-gh Dec 8, 2021
8349513
update Linq query to use null-conditional operator ?.
yongyan-gh Dec 21, 2021
44b4fe2
Add null check for ShouldBeFiled() extension method
yongyan-gh Jan 4, 2022
5ebe0c8
Merge branch 'main' into users/yongyan-gh/fileWorkItemFix
yongyan-gh Jan 4, 2022
9456fb9
Address code review feedbacks
yongyan-gh Jan 6, 2022
9618143
Merge branch 'users/yongyan-gh/fileWorkItemFix' of https://github.com…
yongyan-gh Jan 6, 2022
f14d972
Fix test case
yongyan-gh Jan 6, 2022
d7e093d
Merge branch 'main' into users/yongyan-gh/fileWorkItemFix
yongyan-gh Feb 8, 2022
516bdf7
Addressing review issues
yongyan-gh Feb 16, 2022
c58b2f9
Merge branch 'main' into users/yongyan-gh/fileWorkItemFix
yongyan-gh Feb 16, 2022
8eb2f43
Merge from main
yongyan-gh Feb 23, 2022
479801d
merge from main branch
yongyan-gh Feb 23, 2022
716ee18
Add more test coverages
yongyan-gh Feb 23, 2022
91184b8
Fix dotnet format issues
yongyan-gh Feb 23, 2022
489e1ad
Merge branch 'main' into users/yongyan-gh/fileWorkItemFix
yongyan-gh Feb 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Unreleased

* BREAKING: Fix `InvalidOperationException` when using PropertiesDictionary in a multithreaded application, and remove `[Serializable]` from it. Now use of BinaryFormatter on it will result in `SerializationException`: Type `PropertiesDictionary` is not marked as serializable. [#2415](https://github.com/microsoft/sarif-sdk/pull/2415)
* BUGFIX: Fix `NullReferenceException` when filing work item using a SARIF file which contains results with baseline state. [#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
11 changes: 6 additions & 5 deletions src/Sarif.WorkItems/SarifWorkItemFiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,17 +199,18 @@ 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:
{
// get ride of results should not be filed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment. First, it does not follow convention, starting with a capital letter, ending with a period. It doesn't read very well grammatically. Not sure it provides useful additional information. Finally, this line of code is identical to other lines in this file, why are they commented? If they don't need to be, why is this line commented?

partitionFunction = (result) => result.ShouldBeFiled() ? "Include" : null;
break;
}
case SplittingStrategy.PerRun:
{
partitionFunction = (result) => result.ShouldBeFiled() ? "Include" : null;
Expand Down
22 changes: 13 additions & 9 deletions src/Sarif.WorkItems/SarifWorkItemsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ public static class SarifWorkItemsExtensions
{
public static bool ShouldBeFiled(this Result result)
{
if (result == null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

result

Why did we add this? Why wouldn't a null dereference be the expected condition?

i.e., why do we require

myObject?.ThisIsAnActualMethod()

to protect against a null variable but

myObject.ThisIsAnExtensionMethod()

should work when myObject is null?

I am asking a sincere question. But I'm not sure this change is a good design pattern, Shaopeng, what's your argument for it? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it depends on which one should be responsible for the null check, the caller or the extension method.

If the caller should handle null value then we don't need this check inside extension method.

myObject?.ThisIsAnExtensionMethod()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, my thought was similar to the IDictionary cast that we were looking at last PR, either a helper already did that for you or need to cast to IDictionary everywhere it is used,
in this case if the extension method did the check for you,
or where you call the extension method.
only differ when user forgot to check null before call this extension seeing an unhandled exception because we call result.BaselineState at the first line, the other case is no exception returns false.

A quick find reference I see about 10 usages of this method.

In .net source code they also do null check at the first line of the
extension methods, like below, our logic doe not need to throw:

public static partial class StringNormalizationExtensions
{
public static bool IsNormalized(this string strInput, NormalizationForm normalizationForm)
{
if (strInput == null)
{
throw new ArgumentNullException(nameof(strInput));
}

        return strInput.IsNormalized(normalizationForm);
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the way how .net handles in the extension method. The extension method will have similar behavior to instance method and can be protected by ? operation:

myObject?.ThisIsAnExtensionMethod()

{
throw new ArgumentNullException(nameof(result));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing this, because it is additional work and suggests a general pattern we should adhere that doesn't seem to have good ROI.

if (result.BaselineState == BaselineState.Absent ||
result.BaselineState == BaselineState.Updated ||
result.BaselineState == BaselineState.Unchanged)
Expand Down Expand Up @@ -150,20 +155,19 @@ public static string CreateWorkItemDescription(this SarifLog log, SarifWorkItemC

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
: results?.FirstOrDefault()?.Locations?[0].PhysicalLocation?.ArtifactLocation?.Uri;

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();
int locCount = results == null
? 0
: results.Where(r => r.Locations != null)
.SelectMany(r => r.Locations)
.Count(l => l.PhysicalLocation?.ArtifactLocation?.Uri != null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my personal opinion is this code isn't super readable and i avoid linq statements that potentially entail multiple iterations over a collection (in a pathological case).


if (locCount > 1)
{
Expand Down
92 changes: 87 additions & 5 deletions src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemExtensionsTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.IO;
using System.Text;

using FluentAssertions;
Expand All @@ -17,6 +18,9 @@ namespace Microsoft.CodeAnalysis.Sarif.WorkItems
{
public class SarifWorkItemExtensionsTests
{
// Use portable file path so the tests can run on windows and other platforms.
Copy link
Member

@michaelcfanning michaelcfanning Feb 9, 2022 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Use portable file path so the tests can run on windows and other platforms.

What's the utility of this comment? #Closed

private readonly string testSarifFilePath = Path.Combine(Directory.GetCurrentDirectory(), "Test", "Data", "File{0}.sarif");
Copy link
Member

@michaelcfanning michaelcfanning Jan 4, 2022 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readonly

Please add a comment here explaining that we create the test file path in this way so that we don't use a single, platform-specific file path rendering (i.e., we'll get Windows paths on Windows and Linux running xplat). #Closed

Copy link
Member

@michaelcfanning michaelcfanning Feb 9, 2022 •
edited by eddynaka
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testSarifFilePath

please make this static and give it an s_ prefix #Closed


[Fact]
public void SarifWorkItemExtensions_CreateWorkItemTitle_HandlesSingleResultWithRuleIdOnly()
{
Expand Down Expand Up @@ -245,7 +249,7 @@ public void SarifWorkItemExtensions_CreateWorkItemDescription_SingleResultMultip
public void SarifWorkItemExtensions_CreateWorkItemDescription_MultipleResults()
{
string toolName = "TestToolName";
string firstLocation = @"C:\Test\Data\File{0}sarif";
string firstLocation = testSarifFilePath;
int numOfResult = 15;
string additionLocationCount = "14";

Expand All @@ -271,11 +275,54 @@ public void SarifWorkItemExtensions_CreateWorkItemDescription_MultipleResults()
description.Should().BeEquivalentTo($"This work item contains {numOfResult} '{toolName}' issue(s) detected in {string.Format(firstLocation, 1)} (+{additionLocationCount} locations). Click the 'Scans' tab to review results.");
}

[Fact]
public void SarifWorkItemExtensions_CreateWorkItemDescription_ResultsShouldNotBeFiled()
{
string toolName = "TestToolName";
string firstLocation = testSarifFilePath;
int numOfResult = 8;
int expectedNumOfResult = 0;

int index = 1;
SarifLog sarifLog = TestData.CreateSimpleLogWithRules(0, numOfResult);
sarifLog.Runs[0].Results.ForEach(r => r.Locations
= new[]
{
new Location
{
PhysicalLocation = new PhysicalLocation
{
ArtifactLocation = new ArtifactLocation
{
Uri = new Uri(string.Format(firstLocation, index++))
}
}
}
});

sarifLog.Runs[0].Results.ForEach(r => r.BaselineState = BaselineState.Unchanged);
string description = sarifLog.CreateWorkItemDescription(new SarifWorkItemContext() { CurrentProvider = Microsoft.WorkItems.FilingClient.SourceControlProvider.AzureDevOps });

description.Should().BeEquivalentTo($"This work item contains {expectedNumOfResult} '{toolName}' issue(s) detected in . Click the 'Scans' tab to review results.");
}

[Fact]
public void SarifWorkItemExtensions_CreateWorkItemDescription_WithoutResults()
{
int numOfResult = 0;

SarifLog sarifLog = TestData.CreateSimpleLogWithRules(0, numOfResult);

string description = sarifLog.CreateWorkItemDescription(new SarifWorkItemContext() { CurrentProvider = Microsoft.WorkItems.FilingClient.SourceControlProvider.AzureDevOps });

description.Should().BeEquivalentTo($"This work item contains {numOfResult} '' issue(s) detected in . Click the 'Scans' tab to review results.");
}

[Fact]
public void SarifWorkItemExtensions_CreateWorkItemDescription_MultipleResultsMultipleLocations()
{
string toolName = "TestToolName";
string firstLocation = @"C:\Test\Data\File{0}sarif";
string firstLocation = testSarifFilePath;
int numOfResult = 15;
string additionLocationCount = "29"; // 15 results each results have 2 locations

Expand Down Expand Up @@ -315,9 +362,9 @@ public void SarifWorkItemExtensions_CreateWorkItemDescription_MultipleResultsMul
public void SarifWorkItemExtensions_CreateWorkItemDescription_SingleResultWithMultipleArtifacts()
{
string toolName = "TestToolName";
string firstLocation = @"C:\Test\Data\File1.sarif";
string secondLocation = @"C:\Test\Data\File2.sarif";
string thirdLocation = @"C:\Test\Data\File3.sarif";
string firstLocation = string.Format(testSarifFilePath, 1);
string secondLocation = string.Format(testSarifFilePath, 2);
string thirdLocation = string.Format(testSarifFilePath, 3);

SarifLog sarifLog = TestData.CreateSimpleLogWithRules(0, 1);
sarifLog.Runs[0].Results[0].Locations = new[]
Expand Down Expand Up @@ -384,6 +431,41 @@ public void SarifWorkItemExtensions_CreateWorkItemDescription_SingleResultWithMu
description.Should().BeEquivalentTo($"This work item contains 1 '{toolName}' issue(s) detected in {firstLocation}. Click the 'Scans' tab to review results.");
}

[Fact]
public void SarifWorkItemExtensions_ShouldBeFiledTests()
{
var testCases = new[]
{
new { Result = new Result { Kind = ResultKind.Pass }, Expected = false },
new { Result = new Result { Kind = ResultKind.Fail }, Expected = true },
new { Result = new Result { Kind = ResultKind.Open }, Expected = true },
new { Result = new Result { Kind = ResultKind.Review }, Expected = true },
new { Result = new Result { Kind = ResultKind.Informational }, Expected = false },
new { Result = new Result { Kind = ResultKind.None }, Expected = false },
new { Result = new Result { Kind = ResultKind.NotApplicable }, Expected = false },
new { Result = new Result { Kind = ResultKind.Fail, BaselineState = BaselineState.Absent }, Expected = false },
new { Result = new Result { Kind = ResultKind.Open, BaselineState = BaselineState.Updated }, Expected = false },
new { Result = new Result { Kind = ResultKind.Review, BaselineState = BaselineState.Unchanged }, Expected = false },
new { Result = new Result { Kind = ResultKind.Fail, BaselineState = BaselineState.New }, Expected = true },
new { Result = new Result { Kind = ResultKind.Fail, BaselineState = BaselineState.None }, Expected = true },
new { Result = new Result { Kind = ResultKind.Fail, Suppressions = new [] { new Suppression { Status = SuppressionStatus.Accepted } } }, Expected = false },
new { Result = new Result { Kind = ResultKind.Fail, Suppressions = new [] { new Suppression { Status = SuppressionStatus.Rejected } } }, Expected = false },
new { Result = new Result { Kind = ResultKind.Fail, Suppressions = new Suppression[] { } }, Expected = true },
new { Result = new Result { Kind = ResultKind.Fail, Suppressions = new [] { new Suppression { Status = SuppressionStatus.Rejected }, new Suppression { Status = SuppressionStatus.UnderReview } } }, Expected = false },
};

foreach (var test in testCases)
{
bool actual = test.Result.ShouldBeFiled();
actual.Should().Be(actual);
}

// test case for null result
Result nullResult = null;
Assert.Throws<ArgumentNullException>(() => nullResult.ShouldBeFiled());
(nullResult?.ShouldBeFiled()).Should().BeNull();
}

private static readonly string ToolName = Guid.NewGuid().ToString();

public Tuple<string, Result>[] ResultsWithVariousRuleExpressions = new[]
Expand Down
Loading