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

yongyan-gh · 2021-11-30T23:02:35Z

Description:

BUG: Got NullReferenceException when WorkItemFiler tries to file a work item for the result that has baseline state should be ignored.

Fixes:

Handle null results case in SarifLog CreateWorkItemDescription extension method, which happens if all results are not eligible for filing work item. result.ShouldBeFiled() == false
In WorkItemFiler.FileWorkItems(), do not create work item if all the results of target Sarif Log are not eligible for filing a work item.

Currently the result is treated as it should not be file as work item if

Its baseline State is one of Unchanged, Absent, Updated
It's suppressed

lgtm-com · 2021-11-30T23:25:37Z

This pull request introduces 1 alert when merging d1928a1 into 678e283 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

In reply to: 983119894

update comments Update comments

yongyan-gh · 2021-12-02T01:26:31Z

the issue has been resolved in commits after this.

In reply to: 983119894

shaopeng-gh · 2021-12-07T00:57:11Z

src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemExtensionsTests.cs

+        public void SarifWorkItemExtensions_CreateWorkItemDescription_ResultsShouldNotBeFiled()
+        {
+            string toolName = "TestToolName";
+            string firstLocation = @"C:\Test\Data\File{0}sarif";


@"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

eddynaka · 2021-12-08T19:05:46Z

src/Sarif.WorkItems/SarifWorkItemsExtensions.cs

@@ -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;


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

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

eddynaka · 2021-12-08T19:06:02Z

                       .Count();

you can replace where for count and do thee unwrap/wrap as well

In reply to: 989110458

Refers to: src/Sarif.WorkItems/SarifWorkItemsExtensions.cs:166 in 9665225. [](commit_id = 9665225, deletion_comment = False)

shaopeng-gh · 2022-01-03T19:02:47Z

src/Sarif.WorkItems/SarifWorkItemFiler.cs

@@ -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)


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

removed in latest change

shaopeng-gh · 2022-01-04T02:07:10Z

src/Sarif.WorkItems/SarifWorkItemFiler.cs

@@ -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)


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

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

shaopeng-gh

michaelcfanning · 2022-01-04T23:21:10Z

@marmegh @EasyRhinoMSFT

michaelcfanning · 2022-01-04T23:22:53Z

src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemFilerTests.cs

+            context.SetProperty(ExpectedWorkItemsCount, numberOfResults);
+            context.SetProperty(ExpectedFilingResult, FilingResult.None);
+
+            TestWorkItemFiler(sarifLog, context, true);


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

michaelcfanning · 2022-01-04T23:24:35Z

src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemFilerTests.cs

+            context.SetProperty(ExpectedWorkItemsCount, numberOfResults);
+            context.SetProperty(ExpectedFilingResult, FilingResult.None);
+
+            TestWorkItemFiler(sarifLog, context, true);


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

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

michaelcfanning · 2022-01-04T23:28:02Z

src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemExtensionsTests.cs

@@ -17,6 +18,8 @@
 {
    public class SarifWorkItemExtensionsTests
    {
+        private readonly string testSarifFilePath = Path.Combine(Directory.GetCurrentDirectory(), "Test", "Data", "File{0}.sarif");


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

michaelcfanning · 2022-01-04T23:29:42Z

src/Sarif.WorkItems/SarifWorkItemFiler.cs

@@ -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)


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?

michaelcfanning · 2022-01-04T23:31:00Z

                        if (updatedSarifWorkItemModel == null)

Notice this 'return null' code path has good logging.

Refers to: src/Sarif.WorkItems/SarifWorkItemFiler.cs:329 in 5ebe0c8. [](commit_id = 5ebe0c8, deletion_comment = False)

michaelcfanning · 2022-01-04T23:32:11Z

src/Sarif.WorkItems/SarifWorkItemFiler.cs

@@ -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)


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)?

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.

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?

michaelcfanning · 2022-01-04T23:39:18Z

src/Sarif.WorkItems/SarifWorkItemsExtensions.cs

@@ -12,6 +12,11 @@ public static class SarifWorkItemsExtensions
    {
        public static bool ShouldBeFiled(this Result result)
        {
+            if (result == null)


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? :)

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()

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); }

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()

…/microsoft/sarif-sdk into users/yongyan-gh/fileWorkItemFix

eddynaka

michaelcfanning · 2022-02-09T22:13:55Z

src/Sarif.WorkItems/SarifWorkItemFiler.cs

                    PartitionFunction<string> partitionFunction = null;

                    Stopwatch splittingStopwatch = Stopwatch.StartNew();

                    switch (splittingStrategy)
                    {
+                        case SplittingStrategy.None:
+                        {
+                            // get ride of results should not be filed


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?

michaelcfanning · 2022-02-09T22:27:32Z

src/Sarif.WorkItems/SarifWorkItemsExtensions.cs

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


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.

michaelcfanning · 2022-02-09T22:30:12Z

src/Sarif.WorkItems/SarifWorkItemsExtensions.cs

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


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).

michaelcfanning · 2022-02-09T22:33:02Z

src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemExtensionsTests.cs

@@ -17,6 +18,9 @@
 {
    public class SarifWorkItemExtensionsTests
    {
+        // Use portable file path so the tests can run on windows and other platforms.


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

What's the utility of this comment? #Closed

michaelcfanning · 2022-02-09T22:34:42Z

src/Test.UnitTests.Sarif.WorkItems/SarifWorkItemExtensionsTests.cs

@@ -17,6 +18,9 @@
 {
    public class SarifWorkItemExtensionsTests
    {
+        // Use portable file path so the tests can run on windows and other platforms.
+        private readonly string testSarifFilePath = Path.Combine(Directory.GetCurrentDirectory(), "Test", "Data", "File{0}.sarif");


testSarifFilePath

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

yongyan-gh · 2022-02-23T07:08:14Z

Test coverage data:

michaelcfanning

Fix null reference exception

03fb5eb

yongyan-gh requested review from michaelcfanning, eddynaka and shaopeng-gh November 30, 2021 23:51

yongyan-gh changed the title ~~WorkItemFiler fix~~ Fix NullReferenceException in WorkItemFiler when all results baseline state are unchange/absent/updated Nov 30, 2021

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

e2ce908

update comments Update comments

yongyan-gh force-pushed the users/yongyan-gh/fileWorkItemFix branch from fdf350f to e2ce908 Compare December 1, 2021 00:24

Update release history

d39dc31

yongyan-gh marked this pull request as ready for review December 2, 2021 00:06

yongyan-gh added 2 commits December 1, 2021 16:37

update release history

bb7d051

Fix the test so it covers changes correctly

80d790a

shaopeng-gh reviewed Dec 7, 2021

View reviewed changes

Use portable path instead of DOS path in test cases

9665225

eddynaka reviewed Dec 8, 2021

View reviewed changes

yongyan-gh added 2 commits December 8, 2021 13:01

address PR feedbacks

8beb6dc

update Linq query to use null-conditional operator ?.

8349513

shaopeng-gh reviewed Jan 3, 2022

View reviewed changes

shaopeng-gh reviewed Jan 4, 2022

View reviewed changes

shaopeng-gh approved these changes Jan 4, 2022

View reviewed changes

yongyan-gh changed the title ~~Fix NullReferenceException in WorkItemFiler when all results baseline state are unchange/absent/updated~~ Fix NullReferenceException in WorkItemFiler when all results baseline state are unchange/absent/updated Jan 4, 2022

yongyan-gh added 2 commits January 4, 2022 12:36

Add null check for ShouldBeFiled() extension method

44b4fe2

Merge branch 'main' into users/yongyan-gh/fileWorkItemFix

5ebe0c8

michaelcfanning reviewed Jan 4, 2022

View reviewed changes

yongyan-gh added 3 commits January 5, 2022 17:12

Address code review feedbacks

9456fb9

Merge branch 'users/yongyan-gh/fileWorkItemFix' of https://github.com…

9618143

…/microsoft/sarif-sdk into users/yongyan-gh/fileWorkItemFix

Fix test case

f14d972

eddynaka approved these changes Jan 7, 2022

View reviewed changes

Merge branch 'main' into users/yongyan-gh/fileWorkItemFix

d7e093d

michaelcfanning reviewed Feb 9, 2022

View reviewed changes

yongyan-gh added 5 commits February 16, 2022 02:07

Addressing review issues

516bdf7

Merge branch 'main' into users/yongyan-gh/fileWorkItemFix

c58b2f9

Merge from main

8eb2f43

merge from main branch

479801d

Add more test coverages

716ee18

Fix dotnet format issues

91184b8

michaelcfanning approved these changes Feb 25, 2022

View reviewed changes

michaelcfanning enabled auto-merge (squash) February 25, 2022 21:47

Merge branch 'main' into users/yongyan-gh/fileWorkItemFix

489e1ad

michaelcfanning merged commit 1b7d87c into main Feb 25, 2022

michaelcfanning deleted the users/yongyan-gh/fileWorkItemFix branch February 25, 2022 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yongyan-gh commented Nov 30, 2021 •

edited

Loading

lgtm-com bot commented Nov 30, 2021 •

edited by yongyan-gh

Loading

yongyan-gh commented Dec 2, 2021

shaopeng-gh Dec 7, 2021 •

edited

Loading

eddynaka Dec 8, 2021 •

edited

Loading

eddynaka commented Dec 8, 2021 •

edited by yongyan-gh

Loading

shaopeng-gh Jan 3, 2022 •

edited

Loading

yongyan-gh Jan 6, 2022

shaopeng-gh Jan 4, 2022 •

edited

Loading

yongyan-gh Jan 4, 2022

shaopeng-gh left a comment

michaelcfanning commented Jan 4, 2022

michaelcfanning Jan 4, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jan 4, 2022 •

edited by yongyan-gh

Loading

yongyan-gh Jan 6, 2022

michaelcfanning Jan 4, 2022 •

edited by eddynaka

Loading

michaelcfanning Jan 4, 2022 •

edited

Loading

michaelcfanning commented Jan 4, 2022

michaelcfanning Jan 4, 2022

yongyan-gh Jan 5, 2022

yongyan-gh Jan 6, 2022

michaelcfanning Jan 4, 2022

yongyan-gh Jan 5, 2022

shaopeng-gh Jan 5, 2022

yongyan-gh Jan 6, 2022

eddynaka left a comment

michaelcfanning Feb 9, 2022

michaelcfanning Feb 9, 2022

michaelcfanning Feb 9, 2022

michaelcfanning Feb 9, 2022 •

edited by eddynaka

Loading

michaelcfanning Feb 9, 2022 •

edited by eddynaka

Loading

yongyan-gh commented Feb 23, 2022

michaelcfanning left a comment

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

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

Conversation

yongyan-gh commented Nov 30, 2021 • edited Loading

Description:

Fixes:

lgtm-com bot commented Nov 30, 2021 • edited by yongyan-gh Loading

yongyan-gh commented Dec 2, 2021

shaopeng-gh Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

eddynaka Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

eddynaka commented Dec 8, 2021 • edited by yongyan-gh Loading

shaopeng-gh Jan 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaopeng-gh left a comment

Choose a reason for hiding this comment

michaelcfanning commented Jan 4, 2022

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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Jan 4, 2022 • edited by eddynaka Loading

Choose a reason for hiding this comment

michaelcfanning Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

michaelcfanning commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddynaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelcfanning Feb 9, 2022 • edited by eddynaka Loading

Choose a reason for hiding this comment

michaelcfanning Feb 9, 2022 • edited by eddynaka Loading

Choose a reason for hiding this comment

yongyan-gh commented Feb 23, 2022

michaelcfanning left a comment

Choose a reason for hiding this comment

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

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

yongyan-gh commented Nov 30, 2021 •

edited

Loading

lgtm-com bot commented Nov 30, 2021 •

edited by yongyan-gh

Loading

shaopeng-gh Dec 7, 2021 •

edited

Loading

eddynaka Dec 8, 2021 •

edited

Loading

eddynaka commented Dec 8, 2021 •

edited by yongyan-gh

Loading

shaopeng-gh Jan 3, 2022 •

edited

Loading

shaopeng-gh Jan 4, 2022 •

edited

Loading

michaelcfanning Jan 4, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jan 4, 2022 •

edited by yongyan-gh

Loading

michaelcfanning Jan 4, 2022 •

edited by eddynaka

Loading

michaelcfanning Jan 4, 2022 •

edited

Loading

michaelcfanning Feb 9, 2022 •

edited by eddynaka

Loading

michaelcfanning Feb 9, 2022 •

edited by eddynaka

Loading