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 5 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
3 changes: 3 additions & 0 deletions src/ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

## **To be released**
* 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)

* FEATURE: `MultithreadCommandBase` will use cache when hashing is enabled. [#2388](https://github.com/microsoft/sarif-sdk/pull/2388)
Expand Down
6 changes: 6 additions & 0 deletions src/Sarif.WorkItems/SarifWorkItemFiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,12 @@ public SarifWorkItemModel FileWorkItemInternal(SarifLog sarifLog, SarifWorkItemC
{
string logId = sarifLog.GetProperty<Guid>(LOGID_PROPERTY_NAME).ToString();

if (sarifLog.Runs?.Any(run => run.Results?.Any(result => result.ShouldBeFiled()) == true) != true)
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jan 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

if (sarifLog.Runs?.Any(run => run.Results?.Any(result => result.ShouldBeFiled()) == true) != true)

was thinking about the case of one run have work item and the other have empty array if this condiftion works.

do we think better to expand the nested condition, easiler to read and maintenance :) #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed in latest change

Copy link
Collaborator

@shaopeng-gh shaopeng-gh Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

do we think also add a safety check in this method as well

public static bool ShouldBeFiled(this Result result)
{
if(result ==null) return false;
......
} #Closed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added the check to ShouldBeFiled() and added test cases.

Copy link
Member

@michaelcfanning michaelcfanning Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

sarifLog

You should add logging to this code path, don't you think? @rtaket, does someone from your team need to review this change as well?

Copy link
Member

Choose a reason for hiding this comment

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

sarifLog

I don't understand why this code path is what we want. You are going to traverse the entire log file, which could be very large. Is that what we want? What is this going to save us in work that we won't perform (when there are no issues to file)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the SARIF log doesn't have any eligible result to file as a work item and it doesn't return null, the following code will create a SarifWorkItemModel:

var sarifWorkItemModel = new SarifWorkItemModel(sarifLog, filingContext);

In SarifWorkItemModel code, it iterates all results and get first result which should be filed, to construct work item title. If none of result is eligible, the SairfWorkItemModel's title will be set to null, which causes NullReferenceException when calling filingClient.FileWorkItems() to file work item.

Another solution I can think is before calling FileWorkItemInternal(), in SplitLogFile method, make sure there is only eligible results in split logs. But seems traverse of the results is still needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the check to SplitLogFile method, this way the sarifLog only includes eligible results to file work item, no need to traverse results. Pls let me know what do you think?

{
// If the sarifLog does not contain result which should be filed as a work item, return null.
return null;
}

// The helper below will initialize the sarif work item model with a copy
// of the root pipeline filing context. This context will then be initialized
// based on the current sarif log file that we're processing.
Expand Down
2 changes: 1 addition & 1 deletion src/Sarif.WorkItems/SarifWorkItemsExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public static string CreateWorkItemDescription(this SarifLog log, SarifWorkItemC
Uri runRepositoryUri = log?.Runs.FirstOrDefault()?.VersionControlProvenance?.FirstOrDefault().RepositoryUri;
Uri detectionLocationUri = !string.IsNullOrEmpty(runRepositoryUri?.OriginalString) ?
runRepositoryUri :
results?.FirstOrDefault().Locations?[0].PhysicalLocation?.ArtifactLocation?.Uri;
results?.FirstOrDefault()?.Locations?[0].PhysicalLocation?.ArtifactLocation?.Uri;
Copy link
Collaborator

@eddynaka eddynaka Dec 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

results?.FirstOrDefault()?.Locations?[0].PhysicalLocation?.ArtifactLocation?.Uri

can u unwrap and wrap making ? and : be in separate lines? #Closed


string detectionLocation = (detectionLocationUri?.IsAbsoluteUri == true && detectionLocationUri?.Scheme == "https")
? context.CreateLinkText(detectionLocationUri.OriginalString, detectionLocationUri?.OriginalString)
Expand Down
43 changes: 43 additions & 0 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 @@ -271,6 +271,49 @@ 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 = @"C:\Test\Data\File{0}sarif";
Copy link
Collaborator

@shaopeng-gh shaopeng-gh Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@"C:\Test\Data\File{0}sarif"

suggest change to not using windows specific format, like:
Path.Combine(Directory.GetCurrentDirectory(), "Test", "Data", "File{0}sarif");
#Closed

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()
{
Expand Down
69 changes: 58 additions & 11 deletions src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemFilerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.CodeAnalysis.Test.Utilities.Sarif;
using Microsoft.CodeAnalysis.WorkItems;
using Microsoft.TeamFoundation.WorkItemTracking.WebApi.Models;
using Microsoft.VisualStudio.Services.Common;
using Microsoft.VisualStudio.Services.WebApi;
using Microsoft.VisualStudio.Services.WebApi.Patch.Json;
using Microsoft.WorkItems;
Expand Down Expand Up @@ -105,6 +106,41 @@ public void WorkItemFiler_ValidatesSarifLogFileLocationArgument()
action.Should().Throw<ArgumentNullException>();
}

[Fact]
public void WorkItemFiler_SarifLogResultsShouldNotBeFiled()
{
SarifLog sarifLog = TestData.CreateSimpleLog();

// Set all results baseline state to 'Unchanged' so no work item should be filed.
sarifLog.Runs.SelectMany(run => run.Results).ForEach(result => result.BaselineState = BaselineState.Unchanged);

SarifWorkItemContext context = CreateAzureDevOpsTestContext();

context.SplittingStrategy = SplittingStrategy.None;

int numberOfResults = 0;
context.SetProperty(ExpectedWorkItemsCount, numberOfResults);
context.SetProperty(ExpectedFilingResult, FilingResult.None);

TestWorkItemFiler(sarifLog, context, true);
Copy link
Member

@michaelcfanning michaelcfanning Jan 4, 2022 •
edited by yongyan-gh
Loading

Choose a reason for hiding this comment

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

true

Is it important for these tests to verify the adoClient == false code path?

I think probably it is important, because we want to ensure the GitHub filer also doesn't file work items?

Or is the relevant code sufficiently covered by the adoClient case? #Resolved

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Parameter adoClient controls which FilingClient mock object it generates, adoClient or githubClient. Added/updated test cases also covers githubClient.

}

[Fact]
public void WorkItemFiler_SarifLogResultsAreEmpty()
{
SarifLog sarifLog = TestData.CreateEmptyRun();

SarifWorkItemContext context = CreateAzureDevOpsTestContext();

context.SplittingStrategy = SplittingStrategy.None;

int numberOfResults = 0;
context.SetProperty(ExpectedWorkItemsCount, numberOfResults);
context.SetProperty(ExpectedFilingResult, FilingResult.None);

TestWorkItemFiler(sarifLog, context, true);
Copy link
Member

@michaelcfanning michaelcfanning Jan 4, 2022 •
edited by yongyan-gh
Loading

Choose a reason for hiding this comment

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

true

As a rule don't embed literals in method calls. What does true here mean? We don't know.

Instead, qualify the literal with the parameter name. If the parameter name is good, the code will be cleaner:

TestWorkItemFiler(sarifLog, context, adoClient: true) #Resolved

}

private void TestWorkItemFiler(SarifLog sarifLog, SarifWorkItemContext context, bool adoClient)
{
// ONE. Create test data that the low-level ADO client mocks
Expand Down Expand Up @@ -162,14 +198,17 @@ private void TestWorkItemFiler(SarifLog sarifLog, SarifWorkItemContext context,
CreateWorkItemCalled.Should().Be(expectedWorkItemsCount);
CreateAttachmentCount.Should().Be(adoClient ? expectedWorkItemsCount : 0);

// default value is FilingResult.Succeeded
FilingResult expectedFilingResult = context.GetProperty(ExpectedFilingResult);

// This property is a naive mechanism to ensure that the code
// executed comprehensively (i.e., that execution was not limited
// due to unhandled exceptions). This is required because we have
// not really implemented a proper async API with appropriate handling
// for exceptions and other negative conditions. I wouldn't expect this
// little helper to survive but it closes the loop for the current
// rudimentary in-flight implementation.
filer.FilingResult.Should().Be(FilingResult.Succeeded);
filer.FilingResult.Should().Be(expectedFilingResult);

filer.FiledWorkItems.Count.Should().Be(expectedWorkItemsCount);

Expand All @@ -188,20 +227,23 @@ private void TestWorkItemFiler(SarifLog sarifLog, SarifWorkItemContext context,
//
updatedSarifLog.Should().NotBeEquivalentTo(sarifLog);

foreach (Run run in updatedSarifLog.Runs)
if (expectedWorkItemsCount > 0)
{
foreach (Result result in run.Results)
foreach (Run run in updatedSarifLog.Runs)
{
result.WorkItemUris.Should().NotBeNull();
result.WorkItemUris.Count.Should().Be(1);
result.WorkItemUris[0].Should().Be(bugHtmlUri);
foreach (Result result in run.Results)
{
result.WorkItemUris.Should().NotBeNull();
result.WorkItemUris.Count.Should().Be(1);
result.WorkItemUris[0].Should().Be(bugHtmlUri);

result.TryGetProperty(SarifWorkItemFiler.PROGRAMMABLE_URIS_PROPERTY_NAME, out List<Uri> programmableUris)
.Should().BeTrue();
result.TryGetProperty(SarifWorkItemFiler.PROGRAMMABLE_URIS_PROPERTY_NAME, out List<Uri> programmableUris)
.Should().BeTrue();

programmableUris.Should().NotBeNull();
programmableUris.Count.Should().Be(1);
programmableUris[0].Should().Be(bugUri);
programmableUris.Should().NotBeNull();
programmableUris.Count.Should().Be(1);
programmableUris[0].Should().Be(bugUri);
}
}
}
}
Expand Down Expand Up @@ -374,5 +416,10 @@ private static SarifWorkItemContext CreateAzureDevOpsTestContext()
new PerLanguageOption<int>(
"Extensibility", nameof(ExpectedWorkItemsCount),
defaultValue: () => { return 1; });

internal static PerLanguageOption<FilingResult> ExpectedFilingResult { get; } =
new PerLanguageOption<FilingResult>(
"Extensibility", nameof(ExpectedFilingResult),
defaultValue: () => { return FilingResult.Succeeded; });
}
}