Use type conversion in `SSHConnection` hashtables when value doesn't match expected type by SeeminglyScience · Pull Request #10720 · PowerShell/PowerShell · GitHub

PR Summary

Use LanguagePrimitives.ConvertTo when reading a hashtable provided to the SSHConnection parameter.

PR Context

Currently if the hashtable value does not match the expected type exactly, an exception is thrown stating that the value was null.

Fixes #10687

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, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not Work in Progress.
- If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
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
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
  - Impact on PowerShell Editor Services which is used in the PowerShell extension for VSCode (which runs in a different PS Host).
  - Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
  - Impact on PSScriptAnalyzer (which provides linting & formatting in the editor extensions).
  - Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

Use LanguagePrimitives.ConvertTo in GetSSHConnectionStringParameter and GetSSHConnectionIntParameter.

Please add tests.

ConvertTo() can throw PSInvalidCastException but we should keep PSArgumentException.

Is there a circumstance where converting to string will throw?

Yes, it is not common but possible (if someone creates ToString() method which throw).

The same.

Please add tests.

Any thoughts on that? If I try to test that the conversion works then we'll hit the interactive SSH prompt. I could give it a bad port and/or hostname, but that'll add ~5 seconds per test. If that's acceptable I'll add a few of those.

I'd expect 2 tests for these two code paths modified.
Please use Feature and Slow tags in the tests.

Ilya (@iSazonov) I've added tests and normalized the exception type. In the tests I used port 49151 since it's reserved and shouldn't be in use, but that was the result of a quick search so I'm open to suggestions.

It is out of the PR but I'd prefer to see more useful message.

Patrick Meinecke (@SeeminglyScience) Thanks for your contribution!

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

…match expected type (PowerShell#10720) Before the change if the hashtable value does not match the expected type exactly, an exception is thrown stating that the value was null.

Allow conversion in SSH Connection hashtables

eeb6694

Use LanguagePrimitives.ConvertTo in GetSSHConnectionStringParameter and GetSSHConnectionIntParameter.

Rain Sallow (/u/ta11ow) (vexx32) approved these changes Oct 6, 2019

View reviewed changes

Ilya (iSazonov) reviewed Oct 7, 2019

View reviewed changes

Add tests and preserve exception type

34d5f63

Ilya (iSazonov) reviewed Oct 11, 2019

View reviewed changes

Ilya (iSazonov) approved these changes Oct 11, 2019

View reviewed changes

Ilya (iSazonov) requested review from Paul Higinbotham (PaulHigin) and Steve Lee (SteveL-MSFT) October 14, 2019 16:47

Ilya (iSazonov) self-assigned this Oct 14, 2019

Ilya (iSazonov) added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Oct 14, 2019

Ilya (iSazonov) added this to the 7.0.0-preview.5 milestone Oct 14, 2019

Steve Lee (SteveL-MSFT) approved these changes Oct 14, 2019

View reviewed changes

Paul Higinbotham (PaulHigin) approved these changes Oct 15, 2019

View reviewed changes

Ilya (iSazonov) merged commit beb8b44 into PowerShell:master Oct 15, 2019

Patrick Meinecke (SeeminglyScience) deleted the fix-ssh-connection-hashtable-psobject branch October 15, 2019 18:29

ghost mentioned this pull request Oct 23, 2019

Invoke-Command -SSHConnection breaks if the HostName string is [psobject]-wrapped #10687

Closed

Use type conversion in `SSHConnection` hashtables when value doesn't match expected type by SeeminglyScience · Pull Request #10720 · PowerShell/PowerShell · GitHub

Uh oh!

Use type conversion in SSHConnection hashtables when value doesn't match expected type - #10720

Use type conversion in SSHConnection hashtables when value doesn't match expected type#10720
Ilya (iSazonov) merged 2 commits into
PowerShell:masterfrom
SeeminglyScience:fix-ssh-connection-hashtable-psobject

Patrick Meinecke (SeeminglyScience) commented Oct 6, 2019

Uh oh!

Ilya (iSazonov) left a comment

Uh oh!

Ilya (iSazonov) Oct 7, 2019

Uh oh!

Patrick Meinecke (SeeminglyScience) Oct 7, 2019

Uh oh!

Ilya (iSazonov) Oct 7, 2019

Uh oh!

Ilya (iSazonov) Oct 7, 2019

Uh oh!

Patrick Meinecke (SeeminglyScience) commented Oct 7, 2019

Uh oh!

Ilya (iSazonov) commented Oct 7, 2019 •
edited
Loading

Uh oh!

Uh oh!

Patrick Meinecke (SeeminglyScience) commented Oct 11, 2019

Uh oh!

Ilya (iSazonov) Oct 11, 2019

Uh oh!

Ilya (iSazonov) commented Oct 15, 2019

Uh oh!

Deleted user (ghost) commented Oct 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

                       private static string GetSSHConnectionStringParameter(object param)
                       {
-                          if (param is string paramValue && !string.IsNullOrEmpty(paramValue))
+                          string paramValue = LanguagePrimitives.ConvertTo<string>(param);

                           }
-                          throw new PSArgumentException(RemotingErrorIdStrings.InvalidSSHConnectionParameter);
+                          return LanguagePrimitives.ConvertTo<int>(param);

+                  It "<testName>" -TestCases $TestCasesSSHConnection {
+                      param($scriptBlock)
+                      { & $scriptBlock } | Should -Throw -ErrorId '2100,PSSessionOpenFailed'

Use type conversion in `SSHConnection` hashtables when value doesn't match expected type by SeeminglyScience · Pull Request #10720 · PowerShell/PowerShell · GitHub

Uh oh!

Use type conversion in SSHConnection hashtables when value doesn't match expected type - #10720

Conversation

Patrick Meinecke (SeeminglyScience) commented Oct 6, 2019

PR Summary

PR Context

PR Checklist

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Patrick Meinecke (SeeminglyScience) Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Oct 7, 2019

Choose a reason for hiding this comment

Uh oh!

Patrick Meinecke (SeeminglyScience) commented Oct 7, 2019

Uh oh!

Ilya (iSazonov) commented Oct 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Patrick Meinecke (SeeminglyScience) commented Oct 11, 2019

Uh oh!

Ilya (iSazonov) Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) commented Oct 15, 2019

Uh oh!

Deleted user (ghost) commented Oct 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ilya (iSazonov) commented Oct 7, 2019 •
edited
Loading