New-PSSessionConfigurationFile - improve value handling #26643
Conversation
Improves the parameter handling to properly unwrap PSObject wrapped string or Hashtable values when encountered in object[] based parameters. The existing code just checked for the string or Hashtable types without considering whether they were wrapped by a PSObject. This also improves the error message to include the correct member name on an error as before it was hardcoded to `ModulesToImport`.
There was a problem hiding this comment.
Pull request overview
This PR improves parameter handling in New-PSSessionConfigurationFile to properly unwrap PSObject-wrapped string or hashtable values in object[] based parameters (ModulesToImport, VisibleCmdlets, VisibleFunctions, etc.). Previously, the cmdlet would fail when encountering PSObject-wrapped strings (e.g., 'Get-Help' | Write-Output), and error messages incorrectly always referenced 'ModulesToImport' regardless of which parameter failed validation.
Key Changes:
- Adds
PSObject.Base()call to unwrap PSObject values before type checking - Updates error messages to include the correct parameter name by passing
configKeyto utility methods - Changes from
HashtabletoIDictionaryfor broader dictionary type support
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/System.Management.Automation/engine/remoting/commands/NewPSSessionConfigurationFile.cs | Updated CombineHashTableOrStringArray and GetVisibilityDefault methods to accept configKey parameter for accurate error messages; added PSObject.Base() to unwrap values; changed type check from Hashtable to IDictionary |
| test/powershell/Modules/Microsoft.PowerShell.Core/PSSessionConfiguration.Tests.ps1 | Added three new tests: handling PSObject-wrapped strings, handling dictionary types, and verifying correct error message member names |
Comments suppressed due to low confidence (1)
src/System.Management.Automation/engine/remoting/commands/NewPSSessionConfigurationFile.cs:2131
- The handling of empty or null strings in the updated code differs from the original behavior, which could lead to incorrect comma separators in the output.
In the original code, when a string was empty/null, it would throw an error (since it would try to cast to Hashtable and fail). However, the new code skips empty strings (due to the !string.IsNullOrEmpty(strVal) check) but still appends a comma separator afterwards. This can result in extra commas in the output when empty strings are in the array.
For example, with input @('Cmdlet1', '', 'Cmdlet2'), the output would be: 'Cmdlet1', , 'Cmdlet2' (note the double comma).
This differs from the CombineStringArray method (lines 2083-2101) which has similar logic but handles the comma separator inside the null check, avoiding this issue.
Consider moving the comma separator logic inside the string check block, or adding similar logic for the IDictionary case to maintain consistency.
if (value is string strVal && !string.IsNullOrEmpty(strVal))
{
sb.Append(QuoteName(strVal));
}
else if (value is IDictionary dictVal)
{
sb.Append(CombineHashtable(dictVal, writer));
}
else
{
string message = StringUtil.Format(RemotingErrorIdStrings.DISCTypeMustBeStringOrHashtableArray,
configKey);
PSArgumentException e = new PSArgumentException(message);
caller.ThrowTerminatingError(e.ErrorRecord);
}
if (i < (values.Length - 1))
{
sb.Append(", ");
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
PR Summary
Improves the parameter handling to properly unwrap PSObject wrapped string or Hashtable values when encountered in object[] based parameters. The existing code just checked for the string or Hashtable types without considering whether they were wrapped by a PSObject.
This also improves the error message to include the correct member name on an error as before it was hardcoded to
ModulesToImport.PR Context
The existing cmdlet fails to deal with a string that might be wrapped as a
PSObjectfor any parameters that are typed asobject[]. This includes the parametersModulesToImport,VisibleCmdlets, andVisibleFunctions. For example running the following results in an errorNot only is this a string and should be valid but the error points to the wrong member
ModulesToImportrather thanVisibleCmdlets.This PR fixes the logic to unwrap and
PSObjectvalue but also fixes the error message to contain the correct member.There is most likely other improvements that could be done to handle nested values as the checks for string vs hashtable/IDictionary are not really consistent but that can be done in follow up PRs if required.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header