Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value #8080

vexx32 · 2018-10-19T03:21:45Z

PR Summary

Test-Path is expected to be almost exclusively a True/False response cmdlet, and the cases where it may error or return an unexpected result are a few too many at present (see above issues). This PR allows $null / empty value(s) to be passed to Test-Path without throwing parameter binding exceptions. It also adds additional logic to Test-Path such that passing it $null, '', or ' ' (or any pure whitespace string) returns $false instead of throwing an error or returning $true on a "path" that is nothing but whitespace.

New behaviour:

PS> Test-Path ' '
False
PS> Test-Path ''
False
PS> Test-Path @()
False

Error conditions:

PS> Test-Path $null
PS> Test-Path $null, $null

These are non-terminating errors.

A small handful of tests predicated on Test-Path failing in these cases have been updated to simply transform the error into terminating via -ErrorAction, as it appears that the important part of the test was not Test-Path itself, but throwing a terminating error in specific code strictures.

Tests for the additional behaviours have also been added.

PR Checklist

…ition

iSazonov · 2018-10-19T12:14:19Z

src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs

            CmdletProviderContext currentContext = CmdletProviderContext;

-            foreach (string path in _paths)
+            if (_paths != null && _paths.Length != 0)


We usually use short code path:

Suggested change

if (_paths != null && _paths.Length != 0)

if (_paths == null || _paths.Length == 0)

{

return false;

}

This is in the ProcessRecord() method, so returning false is not an option. I suppose WriteObject(false); return; is the most appropriate short-cut method here. Thank you!

iSazonov · 2018-10-19T12:23:47Z

@SteveL-MSFT Is this a breaking change?

SteveL-MSFT · 2018-10-24T22:23:32Z

@PowerShell/powershell-committee reviewed this, we believe the correct behavior is to accept null/nullcollection, but return non-terminating error at runtime (instead of exception or $false)

iSazonov · 2018-10-25T03:50:49Z

@vexx32 Please continue and open new issue in PowerShell-Docs.

$false will be output if it encounters a non-null but empty string.

vexx32 · 2018-10-25T04:19:12Z

Added non-terminating errors for the following cases, per PowerShell committee recommendation:

Null collection handed to Test-Path (e.g., Test-Path $null)
Collection of null items handed to Test-Path (e.g., Test-Path $null, $null)

$false will be output if:

Path is invalid (same as existing behaviour)
Path is empty or pure whitespace.

Docs issue: MicrosoftDocs/PowerShell-Docs#3165

iSazonov · 2018-10-25T05:05:59Z

@vexx32 Please add tests for new behaviors.

src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs

iSazonov · 2018-10-25T18:00:18Z

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Path.Tests.ps1

+
+    It 'Should write a non-terminating error when given a null path' {
+        { Test-Path -Path $null -ErrorAction Stop }             | Should -Throw -ErrorId 'NullPathNotPermitted'
+        { Test-Path -Path $null -ErrorAction SilentlyContinue } | Should -Not -Throw


Seems we should remove this. I am not sure that it tests "non-terminating"

That's what I thought, too. Surprisingly, though, -ErrorAction SilentlyContinue will still trigger a | should -throw if and only if the error is terminating to start with (either throw or ThrowTerminatingError())

@indented-automation had to explain that one to me. :)

@adityapatwardhan @JamesWTruher @SteveL-MSFT Can we use the pattern to test non-terminating errors?

Since this change is explicitly to be a non-terminating error, this pattern seems fine. But should probably be followed up with -ErrorAction Stop to verify the non-terminating error is correct.

I'm not sure I follow? I don't see a need to double-check it with -ErrorAction Stop

I believe we need to add a comment to describe the pattern.

iSazonov · 2018-10-25T18:00:48Z

test/powershell/Modules/Microsoft.PowerShell.Management/Test-Path.Tests.ps1

+    }
+
+    It 'Should write a non-terminating error when given a null path' {
+        { Test-Path -Path $null -ErrorAction Stop }             | Should -Throw -ErrorId 'NullPathNotPermitted'


I believe we should tests here Test-Path $null, $null;.

Done. Also had to modify some ErrorPosition tests as they were causing infinite loops when the cmldet simply wrote a non-terminating error.

Add comment to confusing but necessary test pattern

iSazonov · 2018-10-31T04:30:02Z

@PowerShell/powershell-committee reviewed this, we believe the correct behavior is to accept null/nullcollection, but return non-terminating error at runtime (instead of exception or $false)

We'll get $false without -ErrorAction Stop - yes?

vexx32 · 2018-10-31T04:34:40Z

We'll get a non-terminating error per the recommendation. In terms of output this will effectively be treated as $null in expressions and statements; in most reasonable circumstances this will be coerced to$falsein a statement that is expecting a boolean conditional, yes.

Would explicitly outputting $false along with the error(s) be preferable?

iSazonov · 2018-11-06T06:28:56Z

@vexx32 Could you please continue?

Update: Sorry, seems my comments was addressed.

iSazonov · 2018-11-14T05:16:53Z

@SteveL-MSFT @adityapatwardhan Could you please review the small PR?

src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs

vexx32 · 2018-11-16T18:13:31Z

MacOS CI failure appears to be temporary and no related to the PR as far as I can see.

iSazonov · 2018-11-16T19:24:03Z

Restart MacOs CI.

…ath value (PowerShell#8080)

ZenoArrow · 2019-11-05T10:26:41Z

Is this change due to be released in PowerShell 7? I haven't seen it in the release notes of the previews released to date, just wanted to check that it's still on course to be included.

iSazonov · 2019-11-05T11:06:23Z

@ZenoArrow The fix is in current 7.0 Preview5 and will be in 7.0 GA too.
Also the fix was mentioned in CHANGELOG.md.

ZenoArrow · 2019-11-05T18:51:46Z

@iSazonov
Thanks for the heads up about the changelog. Referencing that file...
"Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value (#8080) (Thanks @vexx32!)"

The behaviour seems to be fixed for empty path, but the behaviour is different from what I'd expect for $null values...

Am I missing something?

vexx32 · 2019-11-05T23:58:47Z

@ZenoArrow apologies! Looks like I neglected to update the PR description properly. The PS committee's decision was that we should maintain the error for $null values. I didn't agree, but that is that they decided and so that is how it ended up being implemented.

If you disagree with their decision there (I'm right there with you really) I'd recommend opening an issue so that they can see that the behaviour isn't as desired and we can have them reconvene and reassess. 🙂

/Cc @SteveL-MSFT

ZenoArrow · 2019-11-06T00:20:16Z

@vexx32 Thanks for the heads up. I can raise a new issue, but it'd effectively just be a duplicate of the issue raised by @KevinMarquette last year...
#8076

Regarding returning the non-terminating error, I understand this to a degree, but what I can't understand is why Test-Path can't return more than just the error. Correct me if I'm wrong, but usually error stream output doesn't interfere with the standard output, so in my mind there's no reason not to return both (an error message to the error stream and a "False" value to the standard stream).

vexx32 · 2019-11-06T00:58:56Z

It certainly could! But it's not strictly necessary. If you're using Test-Path as a conditional in most cases the non-respose will be treated as $false.

For example:

if (Test-Path $null) { "true!" } Else { "false!" }

ZenoArrow · 2019-11-06T15:06:55Z

@vexx32 Good to know. In that case the current improvements to Test-Path should cover what I was hoping for.

vexx32 added 2 commits October 18, 2018 22:48

Add empty / null path handling to Test-Path

1313095

Update tests to fail correctly under the sole remaining erroring cond…

7d5735a

…ition

vexx32 requested review from adityapatwardhan and daxian-dbw as code owners October 19, 2018 03:21

iSazonov reviewed Oct 19, 2018

View reviewed changes

Use quick-exit code paths instead of lengthy if/else

2f41783

SteveL-MSFT added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 19, 2018

vexx32 changed the title ~~Add empty / null path handling to Test-Path~~ Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value Oct 19, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Oct 24, 2018

iSazonov self-assigned this Oct 25, 2018

vexx32 mentioned this pull request Oct 25, 2018

Test-Path: Update documentation MicrosoftDocs/PowerShell-Docs#3165

Closed

8 tasks

Add non-terminating errors for null input

44417f5

$false will be output if it encounters a non-null but empty string.

vexx32 added 4 commits October 25, 2018 10:48

Reformat test file for readability

63e1b8f

Use more descriptive error ID

d793658

Add Tests for new behaviour

794e78b

Remove unnecessary blank line per StyleCop

cae5294

iSazonov reviewed Oct 25, 2018

View reviewed changes

vexx32 added 5 commits October 30, 2018 16:30

Add new tests

60810ca

Add comment to confusing but necessary test pattern

Clarify it name

e731fab

Fix endless loop caused by change of error type in tests

2008e51

Fix FullyQualifiedErrorID

28cc91f

Consolidate ErrorIDs to match

0be7bf1

iSazonov approved these changes Oct 31, 2018

View reviewed changes

SteveL-MSFT requested changes Nov 14, 2018

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Outdated Show resolved Hide resolved

src/Microsoft.PowerShell.Commands.Management/commands/management/PingPathCommand.cs Outdated Show resolved Hide resolved

vexx32 added 2 commits November 15, 2018 21:34

Add .resx file for TestPath

890fcb1

Use resource file for error string

cddfb79

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 16, 2018

SteveL-MSFT approved these changes Nov 20, 2018

View reviewed changes

iSazonov merged commit 775552c into PowerShell:master Nov 21, 2018

iSazonov pushed a commit to iSazonov/PowerShell that referenced this pull request Nov 29, 2018

Test-Path: Return $false when given an empty or $null -Path/-LiteralP…

6317858

…ath value (PowerShell#8080)

vexx32 deleted the Test-Path/AllowNull branch January 22, 2019 13:19

kilasuit mentioned this pull request Oct 22, 2024

test-path $null throws error #24468

Closed

5 tasks

-            if (_paths != null && _paths.Length != 0)
+            if (_paths == null || _paths.Length == 0)
+            {
+                return false;
+            }

Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value #8080

Test-Path: Return $false when given an empty or $null -Path/-LiteralPath value #8080

Uh oh!

Conversation

vexx32 commented Oct 19, 2018 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 19, 2018

Uh oh!

SteveL-MSFT commented Oct 24, 2018

Uh oh!

iSazonov commented Oct 25, 2018

Uh oh!

vexx32 commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 25, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Oct 31, 2018

Uh oh!

vexx32 commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Nov 14, 2018

Uh oh!

Uh oh!

Uh oh!

vexx32 commented Nov 16, 2018

Uh oh!

iSazonov commented Nov 16, 2018

Uh oh!

ZenoArrow commented Nov 5, 2019

Uh oh!

iSazonov commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZenoArrow commented Nov 5, 2019

Uh oh!

vexx32 commented Nov 5, 2019

Uh oh!

ZenoArrow commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vexx32 commented Nov 6, 2019

Uh oh!

ZenoArrow commented Nov 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vexx32 commented Oct 19, 2018 •

edited by iSazonov

Loading

vexx32 commented Oct 25, 2018 •

edited

Loading

vexx32 commented Oct 31, 2018 •

edited

Loading

iSazonov commented Nov 6, 2018 •

edited

Loading

iSazonov commented Nov 5, 2019 •

edited

Loading

ZenoArrow commented Nov 6, 2019 •

edited

Loading