Handle null reference exception in CsvCommands.cs: ConvertPSObjectToCSV by mikkas456 · Pull Request #26144 · PowerShell/PowerShell
PR Summary
This PR handles a null reference exception in the method ConvertPSObjectToCSV in the file CsvCommands.cs. This fixes a crash that would occur when passing a hash table with a null value to Export-Csv and ConvertTo-Csv. This PR also adds several test cases to Export-Csv.Tests.ps1 and ConvertTo-Csv.Tests.ps1, which will test if Export-Csv and ConvertTo-Csv can properly handle hash tables.
PR Context
Closes #25836
PR Checklist
- PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
- Summarized changes
- Make sure all
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header - This PR is ready to merge. If this PR is a work in progress, please open this as a Draft Pull Request and mark it as Ready to Review when it is ready to merge.
- Breaking changes
- None
- OR
- Experimental feature(s) needed
- Experimental feature name(s):
- User-facing changes
- Not Applicable
- OR
- Documentation needed
- Issue filed:
- Testing - New and feature
- N/A or can only be tested interactively
- OR
- Make sure you've added a new test if existing tests do not effectively test the code changed
@microsoft-github-policy-service agree
Comment on lines +1047 to +1055
| //Handle null reference exception for existing property with null value | ||
| if (dictionary[propertyName] == null) | ||
| { | ||
| value = null; | ||
| } | ||
| else | ||
| { | ||
| value = dictionary[propertyName].ToString(); | ||
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify the code?
| //Handle null reference exception for existing property with null value | |
| if (dictionary[propertyName] == null) | |
| { | |
| value = null; | |
| } | |
| else | |
| { | |
| value = dictionary[propertyName].ToString(); | |
| } | |
| value = dictionary[propertyName]?.ToString(); |
Also please remove obvious comment.
Comment on lines +240 to +243
| It 'should convert hashtable with null values without error'{ | ||
| { $TestHashTable | ConvertTo-Csv } | Should -Not -Throw | ||
| } | ||
|
|
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the test since next one does the check.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I have addressed these issues in another commit.
label
/azp run PowerShell-Windows-Packaging-CI, PowerShell-CI-linux-packaging
Azure Pipelines could not run because the pipeline triggers exclude this branch/path.
| if (dictionary.Contains(propertyName)) | ||
| { | ||
| value = dictionary[propertyName].ToString(); | ||
| value = dictionary[propertyName]?.ToString(); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if value is null here, we should try GetToStringValueForProperty?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you know how to reproduce a scenario where this could be happen please open new issue.
@PowerShell/powershell-maintainers I don't think we can bring this to 7.6 and therefore not 7.5. removing the backport label unless we get more feedback.
