-
Notifications
You must be signed in to change notification settings - Fork 93
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 ArgumentNullException
when PropertiesDictionary
is used and comparer is null
#2482
Fix ArgumentNullException
when PropertiesDictionary
is used and comparer is null
#2482
Conversation
ArgumentNullException
when PropertiesDictionary
is used and comparer is null
@@ -10,6 +10,7 @@ | |||
|
|||
<PropertyGroup> | |||
<TargetFrameworks>netcoreapp3.1</TargetFrameworks> | |||
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net48</TargetFrameworks> |
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.
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.
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.
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.
Also included a simple and focused test to prevent us from facing this issue again.
@@ -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) |
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.
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.
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.
…://github.com/microsoft/sarif-sdk into users/ednakamu/fixing-argument-null-exception
src/ReleaseHistory.md
Outdated
@@ -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) |
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.
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.
Fixes #2481
This change will fix the ArgumentNullException when passing a null comparer to the ConcurrentDictionary when using the TypedPropertiesDictionary.
The issue:
sarif-sdk/src/Sarif.WorkItems/SarifWorkItemContext.cs
Lines 12 to 22 in ea44355
sarif-sdk/src/Sarif/PropertiesDictionary.cs
Lines 20 to 31 in ea44355
sarif-sdk/src/Sarif/TypedPropertiesDictionary.cs
Lines 16 to 31 in 503fe29
With that, on (3), the second constructor will receive a null comparer, generating the ArgumentNullException.