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 ArgumentNullException when PropertiesDictionary is used and comparer is null #2482

Merged
merged 11 commits into from
Apr 26, 2022
Merged

Fix ArgumentNullException when PropertiesDictionary is used and comparer is null #2482

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
861f25e
Defaulting comparer when its null
eddynaka Apr 26, 2022
78eb504
Updating condition to work on windows and linux
eddynaka Apr 26, 2022
78702d2
Removing converters and library from the change
eddynaka Apr 26, 2022
475f412
Updating to net48
eddynaka Apr 26, 2022
e7f37a1
Updating driver only
eddynaka Apr 26, 2022
92203a4
Merge branch 'main' into users/ednakamu/fixing-argument-null-exception
eddynaka Apr 26, 2022
4626612
updating release history
eddynaka Apr 26, 2022
2a33c2a
Merge branch 'main' into users/ednakamu/fixing-argument-null-exception
eddynaka Apr 26, 2022
f2b735d
Adding tests
eddynaka Apr 26, 2022
8d0b303
Merge branch 'users/ednakamu/fixing-argument-null-exception' of https…
eddynaka Apr 26, 2022
b0ac0c6
Updating release history
eddynaka Apr 26, 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
2 changes: 2 additions & 0 deletions src/ReleaseHistory.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# SARIF Package Release History (SDK, Driver, Converters, and Multitool)

## Unreleased

* BUGFIX: Fix `ArgumentNullException` when `PropertiesDictionary` is used and comparer is null. [#2482](https://github.com/microsoft/sarif-sdk/pull/2482)
Copy link
Member

Choose a reason for hiding this comment

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

used

'is instantiated with a null comparer.'

* BUGFIX: Fix `UnhandledEngineException` when target path does not exist for multithreaded application by validating directories as is done for singlethreaded analysis. [#2461](https://github.com/microsoft/sarif-sdk/pull/2461)

## **v2.4.14** [Sdk](https://www.nuget.org/packages/Sarif.Sdk/2.4.14) | [Driver](https://www.nuget.org/packages/Sarif.Driver/2.4.14) | [Converters](https://www.nuget.org/packages/Sarif.Converters/2.4.14) | [Multitool](https://www.nuget.org/packages/Sarif.Multitool/2.4.14) | [Multitool Library](https://www.nuget.org/packages/Sarif.Multitool.Library/2.4.14)
Expand Down
2 changes: 1 addition & 1 deletion src/Sarif/TypedPropertiesDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public TypedPropertiesDictionary() : this(null, StringComparer.Ordinal)
{
}

public TypedPropertiesDictionary(PropertiesDictionary initializer, IEqualityComparer<string> comparer) : base(comparer)
public TypedPropertiesDictionary(PropertiesDictionary initializer, IEqualityComparer<string> comparer) : base(comparer ?? StringComparer.Ordinal)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

StringComparer

As I mentioned in the description, when this is instantiated by the SarifWorkItemContext, the comparer will be null.

So, to use the same strategy as the constructor above, I'm defaulting the comparer to StringComparer.Ordinal.

Copy link
Member

Choose a reason for hiding this comment

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

Yes! Good fix. Did you confirm that this is the behavior for Dictionary<string, T> by examining the reference implementation? Ideally, we would match the behavior of that concrete type.

{
if (initializer != null)
{
Expand Down
1 change: 1 addition & 0 deletions src/Test.UnitTests.Sarif.WorkItems/Test.UnitTests.Sarif.WorkItems.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

<PropertyGroup>
<TargetFrameworks>netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net48</TargetFrameworks>
<IsPackable>false</IsPackable>
<IsTestProject>true</IsTestProject>
<RootNamespace>Test.UnitTests.Sarif.WorkItems</RootNamespace>
Expand Down
18 changes: 18 additions & 0 deletions src/Test.UnitTests.Sarif/PropertiesDictionaryTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,24 @@ public void PropertiesDictionary_TestCacheDescription()
PropertiesDictionary_TestCacheDescription_Helper(GeneratePropertiesDictionaryProperty, PROPERTIES_NONDEFAULT);
}

[Fact]
public void PropertiesDictionary_ShouldNotThrownException_WhenComparerIsNull()
{
var initializer = new PropertiesDictionary();

Exception exception = Record.Exception(() =>
{
new PropertiesDictionary(initializer);
});
exception.Should().BeNull();

exception = Record.Exception(() =>
{
new PropertiesDictionary(initializer, comparer: null);
});
exception.Should().BeNull();
}

private void PropertiesDictionary_TestCacheDescription_Helper(Func<int, string, IOption> GeneratePropertyMethod, object value)
{
string textLoaded = string.Empty;
Expand Down
1 change: 1 addition & 0 deletions src/Test.UnitTests.WorkItems/Test.UnitTests.WorkItems.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

<PropertyGroup>
<TargetFrameworks>netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net48</TargetFrameworks>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

net48

To simulate the issue, we need to change to targetFramework 48. It will happen every time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a side note, I also tried to add net48 for all the other test projects to enhance our testing capabilities BUT the pipeline is timing out. So, we have to investigate why is that happening.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also included a simple and focused test to prevent us from facing this issue again.

<IsPackable>false</IsPackable>
<IsTestProject>true</IsTestProject>
</PropertyGroup>
Expand Down