dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.27k stars 4.73k forks source link

[API Proposal]: Add *Try*ResolveLinkTarget to FileSystemInfo #56256

Closed jozkee closed 3 years ago

jozkee commented 3 years ago

Background and motivation

After adding the symbolic Link APIs (https://github.com/dotnet/runtime/issues/24271) I have found myself trying to safely retrieve a link target as a one-step operation. As of today, the only way you can call ResolveLinkTarget without an exception being thrown is by first calling LinkTarget property as a gurading step that verify that the target is accessible, see: https://github.com/dotnet/runtime/blob/877af80cfc288927153d58182dba6e824f4753ce/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs#L52-L57

Doing this has its own problems since above code involves two sys-calls and the target might no longer be available after the first call completes e.g: it might be deleted.

API Proposal

namespace System.IO
{
     public class FileSystemInfo
     {
          public FileSystemInfo? ResolveLinkTarget(bool returnFinalTarget); // Already existing.
          public bool TryResolveLinkTarget([NotNullWhen(true)] out FileSystemInfo? returnFinalTarget, bool returnFinalTarget); // New API.
     }
}

API Usage

FIleSystemInfo info = new DirectoryInfo(@"C:\foo");

// Ensure we have the directory and not a link.
if (directoryInfo.TryResolveLinkTarget(out FileSystemInfo? target, returnFinalTarget: true))
{
    info = target;
}

Console.WriteLine(info.LastWriteTimeUtc);

Risks

None that I could think of.

ghost commented 3 years ago

Tagging subscribers to this area: @dotnet/area-system-io See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation After adding the symbolic Link APIs (https://github.com/dotnet/runtime/issues/24271) I have found myself trying to safely retrieve a link target as a one-step operation. As of today, the only way you can call `ResolveLinkTarget` without an exception being thrown is by first calling LinkTarget property as a gurading step that verify that the target is accessible, see: https://github.com/dotnet/runtime/blob/877af80cfc288927153d58182dba6e824f4753ce/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/FileSystemInfoHelper.cs#L52-L57 Doing this has its own problems since above code involves two sys-calls and the target might no longer be available after the first call completes e.g: it might be deleted. ### API Proposal ```cs namespace System.IO { public class FileSystemInfo { public FileSystemInfo? ResolveLinkTarget(bool returnFinalTarget); public bool ResolveLinkTarget(out FileSystemInfo returnFinalTarget, bool returnFinalTarget); } } ``` ### API Usage ```C# FIleSystemInfo info = new DirectoryInfo(@"C:\foo"); // Ensure we have the directory and not a link. if (directoryInfo.TryResolveLinkTarget(out FileSystemInfo target, returnFinalTarget: true)) { info = target; } Console.WriteLine(info.LastWriteTimeUtc); ``` ### Risks None that I could think of.
Author: Jozkee
Assignees: -
Labels: `api-suggestion`, `area-System.IO`
Milestone: -
jozkee commented 3 years ago

cc @tmds @iSazonov @mklement0 @carlossanlop @adamsitnik and @ericstj.

jeffhandley commented 3 years ago

This seems logical to me to add. I would support adding this in RC1 as it's based on usage/feedback on the new APIs introduced in Preview 7.

tmds commented 3 years ago

I think https://github.com/dotnet/runtime/issues/52908 covers this use-case.

ResolveLinkTarget(returnFinalTarget) provides 3 functions.

With false it returns FileInfo for the direct target of the link. I think this is not a common use-case.

With true it returns FileInfo for the final target of the link by stepping through the links. More use-case centric APIs are:

iSazonov commented 3 years ago

I'd first consider public class SymbolicLink and combine in it all APIs for symlink manipulating. Enhancements of FileInfo/DirectoryInfo are increasingly confusing to consumers whether operations are performed on a link itself or a link target. As for other link types I believe they are too specific to be added in generic API. They are Windows only and for enumerating we could simply publicly expose ReparseTags property in FileSystemInfo struct. This fully covers enumerating scenarios. Other new APIs for generic links are under big question for me, I think we have no need to add its.

adamsitnik commented 3 years ago

This seems logical to me to add.

+1

One more thing that we should consider would be forking PowerShell repo, bumping SDK to latest version and using the new APIs for symboliclink-related logic. If we find something missing, add new APIs similar to what @Jozkee is proposing here

tmds commented 3 years ago

I'd first consider public class SymbolicLink and combine in it all APIs for symlink manipulating. Enhancements of FileInfo/DirectoryInfo are increasingly confusing to consumers whether operations are performed on a link itself or a link target. As for other link types I believe they are too specific to be added in generic API. They are Windows only and for enumerating we could simply publicly expose ReparseTags property in FileSystemInfo struct. This fully covers enumerating scenarios. Other new APIs for generic links are under big question for me, I think we have no need to add its.

An IsLink probably is still desired to disambiguate (https://github.com/dotnet/runtime/issues/53577)?

My preference is to remove ResolveLinkTarget and TryResolveLinkTarget and work out https://github.com/dotnet/runtime/issues/52908 instead. These methods do a bunch of readlink system calls for something that can be done with a single stat.

For the LinkTarget property that was added in https://github.com/dotnet/runtime/issues/24271. This is not at the same level as the other properties. I had preferred it would be a method. It could be on File/Directory( or SymbolicLink).

    public static class File
    {
        public static FileSystemInfo CreateSymbolicLink(string path, string pathToTarget);
        public static string? ReadSymbolicLinkTarget(string linkPath);
    }
    public static class Directory
    {
        public static FileSystemInfo CreateSymbolicLink(string path, string pathToTarget);
        public static string? ReadSymbolicLinkTarget(string linkPath);
    }
jozkee commented 3 years ago

One more thing that we should consider would be forking PowerShell repo, bumping SDK to latest version and using the new APIs for symboliclink-related logic.

@adamsitnik it is a good idea. Symlink APIs are shipping in preview7 so I think we need to wait for it to be publicly available in order for PS Core to bump to that version.

Enhancements of FileInfo/DirectoryInfo are increasingly confusing to consumers whether operations are performed on a link itself or a link target.

@iSazonov yes, there is work to do in clarifying what you should expect when handling symlinks.

My preference is to remove ResolveLinkTarget and TryResolveLinkTarget and work out #52908 instead.

@tmds It is true that https://github.com/dotnet/runtime/issues/52908 suits better the needs of https://github.com/dotnet/runtime/issues/36091, however I don't think we should add an API just because it fixes better one single scenario; I think we can have the organic alternative (CreateSymbolicLink & ResolveLinkTarget are more oriented to be an entry point for link handling and I think there's value in that) and the perf-oriented alternative, and if perf is the goal, I think we could squeeze a bit more by doing https://github.com/dotnet/runtime/issues/52908#issuecomment-885866027 instead.

Whith above said I don't think we should remove ResolveLinkTarget in favor of https://github.com/dotnet/runtime/issues/52908.

These methods do a bunch of readlink system calls for something that can be done with a single stat.

Do you think there could be a way to optimize that? e.g use realpath for returnFinalTarget = true.

For the LinkTarget property that was added in #24271. This is not at the same level as the other properties. I had preferred it would be a method. It could be on File/Directory( or SymbolicLink).

If there is people asking for a static version of it we can consider adding it, but I would prefer if folks could fiddle with what we currently have and take action based on feedback.

cc @carlossanlop

jozkee commented 3 years ago

Given that there is no precedence of having Try* APIs in File[Info]/Directory[Info] I think we should not land this API in 6.0 and wait for user feedback. Will move this proposal to Future.

In addition we can educate users to be aware of possible exceptions in symlink APIs and its proper handling (through documentation). I sent a PR for fixing the scenario that motivated this proposal https://github.com/dotnet/runtime/pull/56915 and seems that handling the FileNotFoundException is good enough.

iSazonov commented 3 years ago

One more thing that we should consider would be forking PowerShell repo, bumping SDK to latest version and using the new APIs for symboliclink-related logic.

@adamsitnik it is a good idea. Symlink APIs are shipping in preview7 so I think we need to wait for it to be publicly available in order for PS Core to bump to that version.

The PR (to move to .Net 6.0 Preview7) is already pulled in PS repo. Will be merged in days.

tmds commented 3 years ago

@tmds It is true that #52908 suits better the needs of #36091, however I don't think we should add an API just because it fixes better one single scenario

fwiw, I think it fits better for any scenario that involves [Try]ResolveLinkTarget.

iSazonov commented 3 years ago

https://github.com/PowerShell/PowerShell/pull/15648 - all PowerShell tests was passed on RC1.

iSazonov commented 3 years ago

https://github.com/PowerShell/PowerShell/pull/15648 was merged

jozkee commented 3 years ago

We may not need to add this after all, will close for now.