Skip to content

Enhance Remove-Item to work with OneDrive #14902

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.

Sign up for GitHub

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

Merged
rjmholt merged 8 commits into from
Apr 12, 2021
Merged

Enhance Remove-Item to work with OneDrive #14902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
731ab10
Enhance Remove-Item to work with OneDrive
iSazonov Feb 24, 2021
2596388
Fix reparse point check
iSazonov Feb 25, 2021
8c81afe
Add performance optimization
iSazonov Feb 25, 2021
ec6ddd2
Add new test for Get-ChildItem on emulated OneDrive
iSazonov Feb 25, 2021
f339140
Add new test for Remove-Item on emulated OneDrive
iSazonov Feb 25, 2021
cd7f760
Fix tests 2
iSazonov Feb 25, 2021
382d5a6
Cleanups
iSazonov Feb 25, 2021
41df0b6
Address feedback
iSazonov Feb 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Microsoft.PowerShell.Commands.Management/commands/management/Navigation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2699,7 +2699,7 @@ protected override void ProcessRecord()
try
{
System.IO.DirectoryInfo di = new(providerPath);
if (di != null && (di.Attributes & System.IO.FileAttributes.ReparsePoint) != 0)
if (di != null && InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(di))
{
shouldRecurse = false;
treatAsFile = true;
Expand Down
9 changes: 8 additions & 1 deletion src/System.Management.Automation/engine/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1863,7 +1863,7 @@ internal static string GetFormatStyleString(FormatStyle formatStyle)

if (ExperimentalFeature.IsEnabled("PSAnsiRendering"))
{
PSStyle psstyle = PSStyle.Instance;
PSStyle psstyle = PSStyle.Instance;
switch (formatStyle)
{
case FormatStyle.Reset:
Expand Down Expand Up @@ -2099,6 +2099,13 @@ public static class InternalTestHooks

internal static bool ThrowExdevErrorOnMoveDirectory;

// To emulate OneDrive behavior we use the hard-coded symlink.
// If OneDriveTestRecuseOn is false then the symlink works as regular symlink.
// If OneDriveTestRecuseOn is true then we resurce into the symlink as OneDrive should work.
internal static bool OneDriveTestOn;
internal static bool OneDriveTestRecurseOn;
internal static string OneDriveTestSymlinkName = "link-Beta";

/// <summary>This member is used for internal test purposes.</summary>
public static void SetTestHook(string property, object value)
{
Expand Down
72 changes: 54 additions & 18 deletions src/System.Management.Automation/namespaces/FileSystemProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1896,9 +1896,17 @@ private void Dir(
}

bool hidden = false;
bool checkReparsePoint = true;
if (!Force)
{
hidden = (recursiveDirectory.Attributes & FileAttributes.Hidden) != 0;

// Performance optimization.
// Since we have already checked Attributes for Hidden we have already did a p/invoke
// and initialized Attributes property.
// So here we can check for ReparsePoint without new p/invoke.
// If it is not a reparse point we skip one p/invoke in IsReparsePointLikeSymlink() below.
checkReparsePoint = (recursiveDirectory.Attributes & FileAttributes.ReparsePoint) != 0;
}

// if "Hidden" is explicitly specified anywhere in the attribute filter, then override
Expand All @@ -1912,7 +1920,7 @@ private void Dir(
// c) it is not a reparse point with a target (not OneDrive or an AppX link).
if (tracker == null)
{
if (InternalSymbolicLinkLinkCodeMethods.IsReparsePointWithTarget(recursiveDirectory))
if (checkReparsePoint && InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(recursiveDirectory))
{
continue;
}
Expand Down Expand Up @@ -2064,7 +2072,7 @@ string ToModeString(FileSystemInfo fileSystemInfo)
public static string NameString(PSObject instance)
{
return instance?.BaseObject is FileSystemInfo fileInfo
? InternalSymbolicLinkLinkCodeMethods.IsReparsePointWithTarget(fileInfo)
? InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(fileInfo)
? $"{fileInfo.Name} -> {InternalSymbolicLinkLinkCodeMethods.GetTarget(instance)}"
: fileInfo.Name
: string.Empty;
Expand Down Expand Up @@ -3104,22 +3112,31 @@ private void RemoveDirectoryInfoItem(DirectoryInfo directory, bool recurse, bool
continueRemoval = ShouldProcess(directory.FullName, action);
}

if (directory.Attributes.HasFlag(FileAttributes.ReparsePoint))
if (InternalSymbolicLinkLinkCodeMethods.IsReparsePointLikeSymlink(directory))
{
void WriteErrorHelper(Exception exception)
{
WriteError(new ErrorRecord(exception, errorId: "DeleteSymbolicLinkFailed", ErrorCategory.WriteError, directory));
}

try
{
// TODO:
// Different symlinks seem to vary by behavior.
// In particular, OneDrive symlinks won't remove without recurse,
// but the .NET API here does not allow us to distinguish them.
// We may need to revisit using p/Invokes here to get the right behavior
directory.Delete();
if (InternalTestHooks.OneDriveTestOn)
{
WriteErrorHelper(new IOException());
return;
}
else
{
// Name surrogates should just be detached.
directory.Delete();
}
}
catch (Exception e)
{
string error = StringUtil.Format(FileSystemProviderStrings.CannotRemoveItem, directory.FullName, e.Message);
var exception = new IOException(error, e);
WriteError(new ErrorRecord(exception, errorId: "DeleteSymbolicLinkFailed", ErrorCategory.WriteError, directory));
WriteErrorHelper(exception);
}

return;
Expand Down Expand Up @@ -8244,28 +8261,47 @@ internal static bool IsReparsePoint(FileSystemInfo fileInfo)
return fileInfo.Attributes.HasFlag(System.IO.FileAttributes.ReparsePoint);
}

internal static bool IsReparsePointWithTarget(FileSystemInfo fileInfo)
internal static bool IsReparsePointLikeSymlink(FileSystemInfo fileInfo)
{
if (!IsReparsePoint(fileInfo))
#if UNIX
// Reparse point on Unix is a symlink.
return IsReparsePoint(fileInfo);
#else
if (InternalTestHooks.OneDriveTestOn && fileInfo.Name == InternalTestHooks.OneDriveTestSymlinkName)
{
return false;
return !InternalTestHooks.OneDriveTestRecurseOn;
}
#if !UNIX
// It is a reparse point and we should check some reparse point tags.
var data = new WIN32_FIND_DATA();

WIN32_FIND_DATA data = default;
using (var handle = FindFirstFileEx(fileInfo.FullName, FINDEX_INFO_LEVELS.FindExInfoBasic, ref data, FINDEX_SEARCH_OPS.FindExSearchNameMatch, IntPtr.Zero, 0))
{
if (handle.IsInvalid)
{
// If we can not open the file object we assume it's a symlink.
return true;
}

// To exclude one extra p/invoke in some scenarios
// we don't check fileInfo.FileAttributes
const int FILE_ATTRIBUTE_REPARSE_POINT = 0x0400;
if ((data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) == 0)
{
// Not a reparse point.
return false;
}

// The name surrogate bit 0x20000000 is defined in https://docs.microsoft.com/windows/win32/fileio/reparse-point-tags
// Name surrogates (0x20000000) are reparse points that point to other named entities local to the filesystem
// (like symlinks and mount points).
// In the case of OneDrive, they are not name surrogates and would be safe to recurse into.
if (!handle.IsInvalid && (data.dwReserved0 & 0x20000000) == 0 && (data.dwReserved0 != IO_REPARSE_TAG_APPEXECLINK))
if ((data.dwReserved0 & 0x20000000) == 0 && (data.dwReserved0 != IO_REPARSE_TAG_APPEXECLINK))
{
return false;
}
}
#endif

return true;
#endif
}

internal static bool WinIsHardLink(FileSystemInfo fileInfo)
Expand Down
63 changes: 62 additions & 1 deletion test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ Describe "Hard link and symbolic link tests" -Tags "CI", "RequireAdminOnWindows"
$omegaFile1 = Join-Path $omegaDir "OmegaFile1"
$omegaFile2 = Join-Path $omegaDir "OmegaFile2"
$betaDir = Join-Path $alphaDir "sub-Beta"
$betaLink = Join-Path $alphaDir "link-Beta"
$betaLink = Join-Path $alphaDir "link-Beta" # Don't change! The name is hard-coded in PowerShell for OneDrive tests.
$betaFile1 = Join-Path $betaDir "BetaFile1.txt"
$betaFile2 = Join-Path $betaDir "BetaFile2.txt"
$betaFile3 = Join-Path $betaDir "BetaFile3.txt"
Expand Down Expand Up @@ -616,6 +616,31 @@ Describe "Hard link and symbolic link tests" -Tags "CI", "RequireAdminOnWindows"
$ci = Get-ChildItem $alphaLink -Recurse -Name
$ci.Count | Should -BeExactly 7 # returns 10 - unexpectly recurce in link-alpha\link-Beta. See https://github.com/PowerShell/PowerShell/issues/11614
}
It "Get-ChildItem will recurse into emulated OneDrive directory" -Skip:(-not $IsWindows) {
# The test depends on the files created in previous test:
#New-Item -ItemType SymbolicLink -Path $alphaLink -Value $alphaDir
#New-Item -ItemType SymbolicLink -Path $betaLink -Value $betaDir
Comment on lines +621 to +622
Copy link
Collaborator

@rjmholt rjmholt Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove these?

Copy link
Collaborator Author

@iSazonov iSazonov Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep the file style.

Copy link
Collaborator

@rjmholt rjmholt Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file style is to keep commented out lines of code?

Copy link
Collaborator Author

@iSazonov iSazonov Feb 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full text is:

            # The test depends on the files created in previous test:
            #New-Item -ItemType SymbolicLink -Path $alphaLink -Value $alphaDir
            #New-Item -ItemType SymbolicLink -Path $betaLink -Value $betaDir

The same style is in previous test, and two tests above too.
It's not very good that the tests depend on each other, but this has already been.

Copy link
Collaborator

@rjmholt rjmholt Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like that should be in a Context BeforeAll block, and then torn down in an AfterAll block

Copy link
Collaborator Author

@iSazonov iSazonov Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I pointed "It's not very good that the tests depend on each other, but this has already been." I would not want to redo all these tests now. It is better to do in separate PR. See also #14902 (comment)

I opened #15178 for tracking.


[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestOn', $true)
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $false)
Comment on lines +624 to +625
Copy link
Collaborator

@rjmholt rjmholt Apr 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also the kind of thing that would be better in a BeforeEach block in a Context

try
{
# '$betaDir' is a symlink - we don't follow symlinks
# This emulates PowerShell 6.2 and below behavior.
$ci = Get-ChildItem -Path $alphaDir -Recurse
$ci.Count | Should -BeExactly 7

# Now we follow the symlink like on OneDrive.
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $true)
$ci = Get-ChildItem -Path $alphaDir -Recurse
$ci.Count | Should -BeExactly 10
}
finally
{
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $false)
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestOn', $false)
}
}
It "Get-ChildItem will recurse into symlinks given -FollowSymlink, avoiding link loops" {
New-Item -ItemType Directory -Path $gammaDir
New-Item -ItemType SymbolicLink -Path $uponeLink -Value $betaDir
Expand Down Expand Up @@ -737,6 +762,42 @@ Describe "Hard link and symbolic link tests" -Tags "CI", "RequireAdminOnWindows"
$childB.Count | Should -BeExactly $childA.Count
$childB.Name | Should -BeExactly $childA.Name
}
It "Remove-Item will recurse into emulated OneDrive directory" -Skip:(-not $IsWindows) {
$alphaDir = Join-Path $TestDrive "sub-alpha2"
$alphaLink = Join-Path $TestDrive "link-alpha2"
$alphaFile1 = Join-Path $alphaDir "AlphaFile1.txt"
$betaDir = Join-Path $alphaDir "sub-Beta"
$betaLink = Join-Path $alphaDir "link-Beta"
$betaFile1 = Join-Path $betaDir "BetaFile1.txt"

New-Item -ItemType Directory -Path $alphaDir > $null
New-Item -ItemType File -Path $alphaFile1 > $null
New-Item -ItemType Directory -Path $betaDir > $null
New-Item -ItemType File -Path $betaFile1 > $null

New-Item -ItemType SymbolicLink -Path $alphaLink -Value $alphaDir > $null
New-Item -ItemType SymbolicLink -Path $betaLink -Value $betaDir > $null

[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestOn', $true)
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $false)
try
{
# With the test hook turned on we don't remove '$betaDir' symlink.
# This emulates PowerShell 7.1 and below behavior.
{ Remove-Item -Path $betaLink -Recurse -ErrorAction Stop } | Should -Throw -ErrorId "DeleteSymbolicLinkFailed,Microsoft.PowerShell.Commands.RemoveItemCommand"

# Now we emulate OneDrive and follow the symlink like on OneDrive.
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $true)
Remove-Item -Path $betaLink -Recurse
Test-Path -Path $betaLink | Should -BeFalse
}
finally
{
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestRecurseOn', $false)
[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('OneDriveTestOn', $false)
}
}

}
}

Expand Down