Implement IDirectoryContents on PhysicalDirectoryInfo

[manandre]

Closes #86354

Also introduces a refactoring of the internal (but publicly exposed) PhysicalDirectoryContents class, in order to avoid code duplication.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

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

Issue Details

---

Closes #86354

Also introduces a refactoring of the internal (but publicly exposed) PhysicalDirectoryContents class, in order to avoid code duplication.

Author:	manandre
Assignees:	-
Labels:	new-api-needs-documentation, area-Extensions-FileSystem
Milestone:	-

[rameel]

Other providers also have it implemented, that the IFileInfo representing a directory does not implement IDirectoryContents.

[Jozkee]

@rameel can you please elaborate? Are you saying that other IFileInfo implementations that represent a directory need to also be changed?

[Jozkee]

Not related to this change but what's the point of _entries if we assign it every time it will be used?

[rameel]

Yes, I think the same approach should be applied to other providers as well. This would ensure consistent behavior and adhere to the principle of least astonishment.

Since IFileInfo is an abstraction, the code working with IFileInfo should be aware that when IFileInfo.IsDirectory is true, it can iterate through the files in that directory or not. Otherwise, we encounter a case of "leaky abstraction" where the code knows it's working with PhysicalFileProvider, which raises the question of why it needs an abstraction like IFileProvider in the first place.

Currently, the way it's implemented in this pull request, if it keeps as it is, we can't rely on the same code to work correctly when given different implementations of IFileProvider.

public static void Work(IFileProvider provider)

Ideally, I believe this should be reflected in the interface, something like this:

public interface IDirectoryContents : IFileInfo

This way, the code working with IFileProvider will have consistent behavior.

However, this would be a breaking change, and I'm not sure if it's the right approach to introduce it.

But if similar changes are made to other official providers, where IFileInfo.IsDirectory = true also conventionally implements IDirectoryContents, then developers of third-party providers could take the official solution as an example and implement the same in their own providers.

[Jozkee]

@rameel I don't think this is worth introducing a breaking change.

One way in which we could make this work is by introducing public interface IDirectoryInfo : IFileInfo, IDirectoryContents.

IFileInfo is for compatibility. Types like PhysicalDirectoryInfo can be updated to implement IDirectoryInfo instead and implement the GetEnumerator().

Bear in mind that we don't own all types that extend IFileInfo; the respective owners need to be notified of this change in order for them to take action.

cc @vpenades

[rameel]

Sounds good.

[vpenades]

I think the current PR is fine and there's no need for an IDirectoryInfo interface.

I think the best approach to consume these interfaces is this:

1. Check whether the IFileInfo.IsDirectory is true
2. Cast to IDirectoryContents and iterate.

Notice that what really defines an instance as a directory view is the instance implementing IDirectoryContents, having a bool IsDirectory property is just sugar.

Also notice that in more advanced scenarios like some I've seen around, it can happen that an actual file can implement IDirectoryContents, for example, consider a Zip archive, that is a file itself, but contains a full directory tree inside.

Having a decoupled IFileInfo and a IDirectoryContents interfaces allows to combining them in some of these advanced scenarios, that's why I think IDirectoryInfo is not neccesary because it is restricted to a specific scenario (physical direrctories)

[Jozkee]

@rameel I also think the proposed abstraction should suffice for most scenarios.

[Jozkee]

Another option is to reject the proposal, PhysicalDirectoryInfo is not sealed so you can actually achieve content enumeration by doing the following:

class EnumerablePhysicalDirectoryInfo : PhysicalDirectoryInfo, IDirectoryContents
{
    public MyPhysicalDirectoryInfo(DirectoryInfo info) : base(info)
    {
    }

    public IEnumerator<IFileInfo> GetEnumerator()
    {
        throw new NotImplementedException();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        throw new NotImplementedException();
    }
}

@vpenades can you remind me why is it important for you to address this in PhysicalDirectoryInfo?

[vpenades]

@vpenades can you remind me why is it important for you to address this in PhysicalDirectoryInfo?

The whole point is to not have to require using the PhysicalDirectoryInfo directly for directory traversal.

Think about it: if you have access to PhysicalDirectoryInfo , you also have access to System.IO.FileInfo, so you would not need these classes and interfaces in the first place.

Historically there's been a problem with System.IO.FileInfo and DirectoryInfo because they were neither interfaces nor inheritable classes, so it was not possible to use them for anything other than physical drive access.

The new IFileInfo interface, as I understand it, was introduced to have a set of building blocks so you can create a directory system pattern on stores other than a physical drive system... but for the concept to work, you need to do things so consumers only use the interfaces and not the underlaying implementing classes. In fact, if I would be me, I would have made physical file and physical directory classes internal, because they're just a specific implementation of the public interfaces.

In fact, trying to solve issues like these by requiring to use the implementing class is big anti-pattern.

[Jozkee]

But as I shown in #91024 (comment) the building blocks for directory enumeration are already there. So we are really not achieving much in this PR, don't you think?

[vpenades]

But as I shown in #91024 (comment) the building blocks for directory enumeration are already there. So we are really not achieving much in this PR, don't you think?

Let's say I write a library that is able to display a directory tree in WPF, and I want it to work on physical drives, emulated physical drives in memory, directories inside archives, remote directories over a REST API or whatever crazy directory tree system I want to traverse.

Lets call the library WPFVirtualDirectoryTreeViewer, or in my case, using Avalonia: InteropTypes.IO.FileProviders.Avalonia

First thing: which packages should this library reference? since it's a generic directory tree traversing library, it only requires Microsoft.Extensions.FileProviders.Abstractions and nothing else, right?

So I could use the control like this:

WPFDirectoryTreePanel.Root = (IFileProvider)new PhysicalFileProvider("c:\\");

but also like this:

WPFDirectoryTreePanel.Root = (IFileProvider)new VirtualFileProvider(...);
WPFDirectoryTreePanel.Root = (IFileProvider)new ArchiveFileProvider("archive.ZIP");

Notice that the WPF control is not referencing Microsoft.Extensions.FileProviders.Physical, And in fact it should not require it at all because only by using the interface methods, it should be able to traverse the whole tree on its own, without having any knowledge of the underlaying implementation, which excludes any knowledge of PhysicalDirectoryInfo existence.

If traversing trees with the interface APIs is limited to only the first directory level, then the whole IFileInfo abstraction is flawed, because to develop such a tree view control, you should not only require to implement a specific hack for PhysicalDirectoryInfo, but for all IFileInfo implementations out there.

[vpenades]

Doing a search, I've found other people has reached the same conclusion that an instance representing a directory must implement both IFileInfo and IDirectoryContents

• TestDirectoryContent
• PeerDirectoryProvider
• StageProviderDirectory
• InMemoryDirectory

[Jozkee]

Notice that the WPF control is not referencing Microsoft.Extensions.FileProviders.Physical

Yes, I understand, I'm not trying to force WPFVirtualDirectoryTreeViewer to take a dependency on Microsoft.Extensions.FileProviders.Physical, I was making the point that such library can handle directory enumeration today by checking if the IFIleInfo in turn also implements IDirectoryContents, that is not blocked on this change.

Doing a search, I've found other people has reached the same conclusion that an instance representing a directory must implement both IFileInfo and IDirectoryContents

That is good data that supports that this change will leave us in a better state, thanks.

[rameel]

But what about other providers?

[Jozkee]

@rameel could you please file issues on the repos where you think other IFileInfos should implement IDirectoryContents?

[vpenades]

About other providers, I was considering .Embedded , but I can't find it anymore, it's been removed? Anyway that provider is a bit special since embedded resources use the "." dot as separator, which is a bit tricky to use when using with files with multi dotted extensions...

[Jozkee]

Embedded file provider is still there https://learn.microsoft.com/dotnet/api/microsoft.extensions.fileproviders.embeddedfileprovider. It only contains a EmbeddedResourceFileInfo though.

[rameel]

There is a ManifestEmbeddedFileProvider that returns a ManifestDirectoryInfo that does not implement IDirectoryContents. As part of the fix for the PhysicalFileProvider, I think this should also be addressed.

I have looked at the implementations of other providers we have, and no changes are needed there because:

1. EmbeddedFileProvider provides only a flat list of files.
2. CompositeFileProvider does not have its own implementations of IFileInfo and returns what the internal providers return.

[Jozkee]

@rameel thanks for checking, would you like to send a PR to address ManifestDirectoryInfo? I missed it because I wrongfully checked public types only.

[rameel]

I apologize, I was sure that ManifestEmbeddedFileProvider was in the current repository, but it is actually in ASP.NET Core. I will try to send a PR in the coming weekend.

[rameel]

I have submitted a PR #50586