Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from Nov 21, 2018

Conversation

@vexx32
Copy link
Contributor

commented Oct 19, 2018

PR Summary

Fix #5717.
Fix #8076.

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

@@ -156,49 +162,60 @@ protected override void ProcessRecord()
{
CmdletProviderContext currentContext = CmdletProviderContext;

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 19, 2018

Collaborator

We usually use short code path:

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

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 19, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Collaborator

commented Oct 19, 2018

@SteveL-MSFT Is this a breaking change?

@vexx32 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

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

@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

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

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

@iSazonov iSazonov self-assigned this Oct 25, 2018

@vexx32 vexx32 referenced this pull request Oct 25, 2018
1 of 8 tasks complete
Add non-terminating errors for null input
$false will be output if it encounters a non-null but empty string.
@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

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

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

$false will be output if:

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

Docs issue: MicrosoftDocs/PowerShell-Docs#3165

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 25, 2018

@vexx32 Please add tests for new behaviors.

vexx32 added some commits Oct 25, 2018


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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 25, 2018

Collaborator

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

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 25, 2018

Author Contributor

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. :)

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 25, 2018

Collaborator

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

This comment has been minimized.

Copy link
@SteveL-MSFT

SteveL-MSFT Oct 25, 2018

Member

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.

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 25, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 26, 2018

Collaborator

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

}

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

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 25, 2018

Collaborator

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

This comment has been minimized.

Copy link
@vexx32

vexx32 Oct 30, 2018

Author Contributor

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

vexx32 added some commits Oct 30, 2018

Add new tests
Add comment to confusing but necessary test pattern
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Nov 6, 2018

@vexx32 Could you please continue?

Update: Sorry, seems my comments was addressed.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 14, 2018

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

vexx32 added some commits Nov 16, 2018

@iSazonov iSazonov added the CL-General label Nov 16, 2018

@vexx32

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2018

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2018

Restart MacOs CI.

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

8 checks passed

CodeFactor 2 issues fixed. 1 issue found.
Details
PowerShell-CI-linux #PR-8080-20181116.01 succeeded
Details
PowerShell-CI-macos #PR-8080-20181116.02 succeeded
Details
PowerShell-CI-spelling #PR-8080-20181116.01 succeeded
Details
PowerShell-CI-windows #PR-8080-20181116.01 succeeded
Details
WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

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

@vexx32 vexx32 deleted the vexx32:Test-Path/AllowNull branch Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.