Write proper error messages for Get-Command ' ' #13564

jakekerr · 2020-09-02T19:23:38Z

PR Summary

Fix #13342. Checks if _commandName is empty or null in the DoPowerShellRelativePathLookup() method. Removes a Dbg.Assert() that checks if the name is empty or null. The assertion fails because _commandName is trimmed in setupPathSearcher().

PR Context

After this change Get-Command ' ' will return a CommandNotFoundException instead of a IndexOutOfRangeException. The Dbg.Assert was removed because according to Issue #10165 whitespace should be a valid argument for Get-Command.

PR Checklist

SteveL-MSFT

Please add test cases

ghost · 2020-09-04T04:16:52Z

All CLA requirements met.

jakekerr · 2020-09-04T04:35:05Z

I see now that an alternate and maybe cleaner solution would involve changing only this line _commandName = _commandName.TrimEnd(Utils.Separators.PathSearchTrimEnd); to create a new variable which is only used for the setupPathSearcher() routine. Creating a new variable for path searching may be more desirable than altering the existing _commandName string. If _commandName doesn't get trimmed we don't ever have to worry about the IndexOutOfRangeException being thrown.

iSazonov · 2020-09-04T05:40:08Z

src/System.Management.Automation/engine/CommandSearcher.cs

                // Home Path:           "~\command.exe"
                // Drive Relative Path: "\Users\User\AppData\Local\Temp\command.exe"
-                if (_commandName[0] == '.' || _commandName[0] == '~' || _commandName[0] == '\\')
+                if (!string.IsNullOrEmpty(_commandName) && (_commandName[0] == '.' || _commandName[0] == '~' || _commandName[0] == '\\'))


_commandName is defined as non-nullable so we can do not check null.
Also it is on a hot path and we could optimize the code a bit. I suggest to add && _commandName.Length != 0 in if above, define char firstChar = _commandName[0] and use the firstChar in the line.

@iSazonov just to confirm. Is this along the lines of what you're suggesting?
char firstChar = _commandName[0];
if (_commandName.Length != 0 && (firstChar == '.' || firstChar == '~' || firstChar == '\\'))

I agree it would be more optimized but the issue of this PR (Get-Command ' ' returns the wrong exception) wouldn't be fixed. Sorry I may be misunderstanding. I'm still learning.

(!string.IsNullOrEmpty(_commandName)

You added the check to fix the issue. But _commandName is annotated as non-nullable and we can exclude null check and have _commandName.Length != 0. And the check should be in previous if condition (before char firstChar = _commandName[0]).

iSazonov · 2020-09-04T05:41:26Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1

+        It "Returns CommandNotFoundException if a single space is used" {
+            {Get-Command ' ' -ErrorAction Stop} | Should -Throw -ErrorId "CommandNotFoundException,Microsoft.PowerShell.Commands.GetCommandCommand"
+        }


For consistency we could add tests for null and empty arguments. Please use -TestCases.

iSazonov · 2020-09-04T05:47:30Z

Creating a new variable for path searching may be more desirable than altering the existing _commandName string.

I think it makes no sense to create new variable - this complicates code and don't protect underlying internal methods.

iSazonov · 2020-09-07T03:09:49Z

src/System.Management.Automation/engine/CommandSearcher.cs

+                //char firstChar = _commandName[0];
+                if (_commandName.Length != 0) 


Please remove commented code and move the condition in line 1104 to line 1090.

Oh yeah thanks. Can't believe I missed that.

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1

Co-authored-by: Ilya <darpa@yandex.ru>

…master

jakekerr · 2020-09-14T19:36:01Z

@SteveL-MSFT what do you think of the added test cases? Thanks.

ghost · 2020-09-22T02:00:08Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

jakekerr · 2020-11-02T16:10:24Z

@adityapatwardhan I don't mean to bother you but I just wanted to know if there's anything else I can do to help get this PR approved? Thanks.

adityapatwardhan · 2020-11-02T16:36:14Z

Currently working on some high priority items. I will be reviewing this after. Thank you for your patience.

ghost · 2020-11-10T02:00:08Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

adityapatwardhan · 2020-11-13T22:32:13Z

@SteveL-MSFT can you update your review

ghost · 2020-11-21T02:00:12Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

iSazonov · 2021-01-10T13:03:50Z

@jakekerr Thanks for your contribution!

ghost · 2021-02-12T02:06:52Z

🎉v7.2.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

jakekerr added 2 commits August 27, 2020 10:53

Remove an unnecessary dbg.assert()

abcfb59

Add IsNullOrEmpty check to relative path lookup.

ed7328b

ghost assigned adityapatwardhan Sep 2, 2020

SteveL-MSFT requested changes Sep 3, 2020

View reviewed changes

ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 3, 2020

Add test case for single whitespace name parameter

e451c53

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Sep 4, 2020

iSazonov reviewed Sep 4, 2020

View reviewed changes

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 4, 2020

jakekerr added 3 commits September 6, 2020 13:12

Check for _commandName length in setupPathSearcher

934e214

Added tests for whitespace, empty and null names

1d5c83c

Remove commented out code.

033e766

iSazonov reviewed Sep 7, 2020

View reviewed changes

Moved _commandName length conditional check.

58f63e0

jakekerr requested a review from SteveL-MSFT September 7, 2020 03:24

iSazonov reviewed Sep 7, 2020

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/Get-Command.Tests.ps1 Outdated Show resolved Hide resolved

Remove unused variable.

a8cbd6e

iSazonov reviewed Sep 8, 2020

View reviewed changes

jakekerr and others added 4 commits September 7, 2020 21:32

Remove extra empty line.

2014b42

Co-authored-by: Ilya <darpa@yandex.ru>

use Name variable for headers and value for tests.

d262767

Capitalized Name header and put it in single quote

df6bda7

Merge branch 'master' of https://github.com/jackerr3/PowerShell into …

b571656

…master

iSazonov approved these changes Sep 8, 2020

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Sep 22, 2020

ghost removed the Review - Needed The PR is being reviewed label Nov 2, 2020

ghost added the Review - Needed The PR is being reviewed label Nov 10, 2020

ghost removed the Review - Needed The PR is being reviewed label Nov 13, 2020

ghost added the Review - Needed The PR is being reviewed label Nov 21, 2020

SteveL-MSFT approved these changes Dec 15, 2020

View reviewed changes

iSazonov merged commit d3f7ba9 into PowerShell:master Jan 10, 2021

ghost removed the Review - Needed The PR is being reviewed label Jan 10, 2021

iSazonov assigned iSazonov and unassigned adityapatwardhan Jan 10, 2021

iSazonov added this to the 7.2.0-preview.3 milestone Jan 10, 2021

ghost mentioned this pull request Feb 12, 2021

Get-Command fails with unexpected error when given a blank command-name argument #13342

Closed

		//char firstChar = _commandName[0];
		if (_commandName.Length != 0)

Write proper error messages for Get-Command ' ' #13564

Write proper error messages for Get-Command ' ' #13564

Uh oh!

Conversation

jakekerr commented Sep 2, 2020 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 4, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakekerr commented Sep 4, 2020

Uh oh!

iSazonov Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakekerr Sep 5, 2020

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 6, 2020

Choose a reason for hiding this comment

Uh oh!

iSazonov Sep 4, 2020

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Sep 4, 2020

Uh oh!

iSazonov Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

jakekerr Sep 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jakekerr commented Sep 14, 2020

Uh oh!

ghost commented Sep 22, 2020

Uh oh!

jakekerr commented Nov 2, 2020

Uh oh!

adityapatwardhan commented Nov 2, 2020

Uh oh!

ghost commented Nov 10, 2020

Uh oh!

adityapatwardhan commented Nov 13, 2020

Uh oh!

ghost commented Nov 21, 2020

Uh oh!

iSazonov commented Jan 10, 2021

Uh oh!

ghost commented Feb 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakekerr commented Sep 2, 2020 •

edited by iSazonov

Loading

ghost commented Sep 4, 2020 •

edited by ghost

Loading

iSazonov Sep 4, 2020 •

edited

Loading