-
Notifications
You must be signed in to change notification settings - Fork 91
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.
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
Fix NullReferenceException
in WorkItemFiler when all results baseline state are unchange/absent/updated
#2412
Changes from 14 commits
03fb5eb
e2ce908
d39dc31
bb7d051
80d790a
9665225
8beb6dc
8349513
44b4fe2
5ebe0c8
9456fb9
9618143
f14d972
d7e093d
516bdf7
c58b2f9
8eb2f43
479801d
716ee18
91184b8
489e1ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,11 @@ public static class SarifWorkItemsExtensions | |
{ | ||
public static bool ShouldBeFiled(this Result result) | ||
{ | ||
if (result == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, 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 public static partial class StringNormalizationExtensions
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
using System; | ||
using System.Collections.Generic; | ||
using System.Collections.ObjectModel; | ||
using System.IO; | ||
using System.Text; | ||
|
||
using FluentAssertions; | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
private readonly string testSarifFilePath = Path.Combine(Directory.GetCurrentDirectory(), "Test", "Data", "File{0}.sarif"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
[Fact] | ||
public void SarifWorkItemExtensions_CreateWorkItemTitle_HandlesSingleResultWithRuleIdOnly() | ||
{ | ||
|
@@ -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"; | ||
|
||
|
@@ -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 | ||
|
||
|
@@ -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[] | ||
|
@@ -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[] | ||
|
There was a problem hiding this comment.
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?