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

Violation of the (great) separation of concerns design principle in Join-Path #4386

Open
schittli opened this issue Jul 28, 2017 · 14 comments

Comments

@schittli
Copy link

commented Jul 28, 2017

Good evening

it's really hard to complain this issue because it's a discussion about the most basic design principles we know and love snce many years:

Functions (in PowerShell commands) should never do things which the user does not expect.
Such things are well known as side-effects and we had to fight with them 20 years ago as Spaghetti code was widely used.

Therefore it hurts to see that those Problems are comming back.

The concrete problem:
Join-Path does no longer solve the problem which we expect (join path items),
but it also checks if a drive exists.

The essence behind this "logic" is that no one is joining path items with non existing drive letters. Of course, this is wrong.
But to get a consitent logic, the next version of Join-Path must automatically create any non-exsting path - it's the same logic: no one is joining path items with a non existing path.

Beside the inconsistent logic, we have more very annoying side effects:

  • Existing Code no longer works
  • Users are searching for functions which are solvong the problem to join path items
  • If someone is writing a function which is just joining path items - how should it get called?
    Join-Path-Without-Test?
    maybe Just-Join-Path?
    Of course, it must be called Join-Path.

There are many better options to handle the Problem, for example:

  • Add a Parameter (e.g. -AssertDrive and -EnforceDir), but don't change the default logic because it is already perfect
  • Create another function, e.g. Join-Path-Assert
  • Pass the result of Join-Path to a function which is testing if the drive exists

Kind regards,
Thomas

Steps to reproduce

# If the Drive Q: does not exists:
PS C:\Users\schittli> Join-Path 'q:\' 'test'
Join-Path : Cannot find drive. A drive with the name 'q' does not exist.

Expected behavior

Join-Path ist doing what we expect: join path Elements. Nothing else.

Actual behavior

Join-Path has an enforced side-effect and it does break the separation of concerns design principle

Environment data

> $PSVersionTable
Name                           Value
----                           -----
PSVersion                      5.1.14393.1480
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.14393.1480
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Jul 29, 2017

Would a -force be an adequate solution?

@briantist

This comment has been minimized.

Copy link

commented Apr 17, 2018

@SteveL-MSFT could you explain in detail?

Do you mean that you would now need to use Join-Path -Force if your drive doesn't exist?
Do you mean that Join-Path -Force would do the drive check, and you would restore the original (much better) behavior?

Because the first option sucks; this is a pretty big breaking change that most affects the people who were trying to do the "Right Thing" by not hardcoding \, with the goal of supporting future platforms.

Besides that aspect, it's not at all intuitive that a function that joins paths should check whether part of it actually exists.

The .Net framework for example didn't change this to check for existence: [System.IO.Path]::Combine('Q:','One','Two')

I really hope this gets restored.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

When did this behavior change? Trying to determine how impactful such a breaking change would be where we either default to checking or default to not checking.

@johlju

This comment has been minimized.

Copy link

commented Apr 18, 2018

For both PowerShell 6.0.2 and in Windows Powershell (tested 5.1.14409.1005, 5.1.16299.251, and 5.1.17134.1)

This does work:

  • Join-Path -Path 'DOMAIN' -ChildPath 'John'
  • Join-Path -Path '\\dummy\resource' -ChildPath 'Folder'
  • Split-Path -Path 'DOMAIN\John' -Leaf
  • Split-Path -Path 'Y:\Folder' -Leaf
  • Split-Path -Path '\\dummy\resource\Folder' -Leaf

This does not work:

  • Join-Path -Path 'Y:\' -ChildPath 'Folder'
    • Error: Join-Path : Cannot find drive. A drive with the name 'Y' does not exist.

Since this is in both editions (correct me if I'm wrong) there would be a breaking change if Join-Path suddenly didn't throw an error. So a parameter to override that behavior could be added which would tell Join-Path not to validate a rooted path. User could then instead accomplished that with Resolve-Path.

Note: Didn't have a Windows PowerShell 5.1.14393 to test with.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Apr 18, 2018

@johlju thanks for the data. However, some of the discussion above implies that this behavior of checking if the path is valid is a regression from previous behavior and I'm trying to understand which version of PowerShell had the different behavior. Trying to understand if the discussion is putting behavior back to what it was or requesting a breaking change. This is why I suggested above to perhaps use -Force (or perhaps more specifically something like -NoValidate) to enable the new desired behavior without a breaking change.

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2018

It may be a breaking change... but I think this is a bug..,, and I don't think fixing broken behavior as a result of a bug is really a breaking change

https://github.com/PowerShell/PowerShell/blob/master/docs/dev-process/breaking-change-contract.md#acceptable-changes

Any existing behavior that results in an error message generally may be changed to provide new functionality.

This is the relevant code:

// Ignore the result. Just using this to get the providerId and drive
Globber.GetProviderPath(parent, context, out provider, out drive);
if (drive == null && isProviderQualified)
{
drive = provider.HiddenDrive;
}
context.Drive = drive;

Globber.GetProviderPath() is where the exception is thrown. But, after where the exception is thrown, there is logic to check for a null drive. This along with the comment about ignoring the result indicates to me that this should be wrapped in a try/catch. If I'm interpreting this right, it seems the intent was to just use the GetProvderPath() to try to get the provider and drive if possible, If not, it falls back to the current provider and drive.

I propose that Globber.GetProviderPath() be wrapped in a throwaway try/catch

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Apr 18, 2018

Sadly, I tried this and a few other things and it turns out I was wrong. ☹️
This would require some more in-depth debugging.

@PetSerAl

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

As far as I understand, the problem is because PowerShell does not know how to join paths. It need to redirect that operation to target provider. But if you specify non-existent drive, then which provider should it use? For example Alias, Environment, Function and Variable providers does not support navigation and produce different result from what it would for FileSystem provider:

PS> Join-Path alias:\asd fgh
Alias:\fgh
PS> Join-Path env:\asd fgh
Env:\fgh
PS> Join-Path function:\asd fgh
Function:\fgh
PS> Join-Path variable:\asd fgh
Variable:\fgh
@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

@joeyaiello

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

This came up in documentation discussions, I would naively propose that, if the PSDrive can't be resolved, that we fall back to default filesystem path separators for the given platform.

This would fall in line with what we do with non-drive Join-Path calls:

join-path 'foo' 'bar'
foo\bar

Thoughts?

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2018

I think this is a decent assumption that the filesystem is the target for a non existent drive.

Just as an alternative solution to consider: Using the current location's provider.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

@markekraus's suggestion to use the current location provider makes sense to me

@markekraus

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2018

I will add that my proposal is how join-path works today.

Set-Location AD:\
join-path 'foo' 'bar'

result:

bar,foo

Since the ActiveDirectory provider uses , for the path separator.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

@PowerShell/powershell-committee reviewed this. Expectation is that if given a drive that exists, it should use that provider's path separator. If given a drive does not exist, then it will default to the current provider's path separator. An additional -Provider parameter may be added to allow specifying an explicit provider (this should be a separate issue).

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