dotnet / runtime

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

Replace System.IO.File / System.IO.Directory with interfaces #29328

Open FrankyBoy opened 5 years ago

FrankyBoy commented 5 years ago

Sorry if this already exists but I couldn't find anything. The only thing remotely similar is IFileProvider which does not offer write support in any way and read support only through streams.

I think it would be really helpful to have replacements for the static methods in System.IO.File and System.IO.Directory (and potentially others) with an interface that can be injected via DI. Right now I find that on each project that somehow touches files I have some IFileHandler or similar which allows for basic file handling (read/write) without making my code untestable by unit tests. I think this would make much more sense to be included in the standard itself.

Also, including such an interface would allow libraries that may need file access to not do so directly. I.e. library can just depend on IFileHandler and the library user can implement their own functionality on top of that (maybe they actually want their "files" to go to a centralized DB instead of the filesystem?).

rashadrivera commented 5 years ago

This is a bad idea since Microsoft's code must be a closed system so no hackers can DI inject into a fully developed system. In addition, if MS was to adopt such a practice, it would undermine the integrity of what the community relies on and inevitably lead to bugs related to an otherwise reliable method of accessing files and directories.

You are free to create a middle-ware layer in where you code gains File and Directory functionality and where you DI inject what you want.

FrankyBoy commented 5 years ago

@rashadrivera based on that logic you should not use DI for anything. Also note that this is about the .net Standard, not an actual implementation like Core or Framework, and does not make any suggestions if an implementation should use DI for these things internally.

terrajobst commented 5 years ago

Providing an abstraction has come up before, usually in the context of mocking. I'm very skeptical that this would work for file IO as we can't really version interfaces.

However, an abstract class would probably work. We might end up doing something like this when we're adding support for web assembly. /cc @JeremyKuhne @danroth27

rashadrivera commented 5 years ago

@terrajobst,

Concerning changing File and Directory to interface: that makes sense and was not something I considered. Thanks for the enlightenment.

Concerning the notion of implementing a DI layer for File & Directory: are you saying that your (or the .NET Foundation) may actually consider making these classes depend on a DI framework (or a built in like implementation)? Does it not seem like a risky proposition to allow a core capability (such as these), to be bypassed from the application that is implementing them? With that said, consider this: Any application that uses File or Directory are currently confident that any exception is likely due to the implementation; not these classes. Now if a DI behavior was added, and the implementer was to use a database as the file repository (such as suggested), then the integrity of File and Directory throughout the rest of the application is subject to potential bugs introduced by the DI implementation; which will cast doubt on the stability of File and Directory. From an architectural stand-point, it makes more sense that the implementer develop their own abstraction layer in where they can implement any file repository they please around DI or whatever. This is my point and why I'm asserting that a DI implementation would be dangerous for File and Directory (or any other .NET features that are built around established, fundamental standards and practices). I retract my comment about hacking as that is a heavy, security concept that is not easily express via this medium.

rashadrivera commented 5 years ago

@FrankyBoy,

Please see my expanded explanation to my aforementioned comment. Also, what would be the overall "value added" to the .NET community if these static function where implemented via DI as you suggested? OR, am I misunderstanding your initial comment in some way?

Lastly, would it not make sense to define an interface with methods that match the File and Directory signatures? How would that not work for a DI implementation?

FrankyBoy commented 5 years ago

@rashadrivera you are still mixing specification and implementation. You are assuming that if such an interface would be introduced in the standard so that consumers can use it, internal functionality in the implementation would be switched to that functionality. This is simply not the case.

Lastly, would it not make sense to define an interface with methods that match the File and Directory signatures?

that is literally what I'm suggesting.

svick commented 5 years ago

@terrajobst

I'm very skeptical that this would work for file IO as we can't really version interfaces.

Not even with default interface methods?

JeremyKuhne commented 4 years ago

@terrajobst can you elaborate on https://github.com/dotnet/runtime/issues/29328#issuecomment-490699364 in regards to interface versioning and default interface methods?

terrajobst commented 4 years ago

Default interface methods (DIMs) aren't free of versioning problems either. Yes, we can add members that are defaulted but we need to be careful that we don't do that in cases where a shared diamond exist where the runtime can't decide which DIM to use. When that happens, the program will crash at the call site, similar to how it would fail if you'd add an interface method without updating all implementers.

Furthermore, not all languages support DIMs. We (BCL) think we need a bit more time before we can bet new features on something like DIMs. My question is: why would this have to be an interface in the first place? This isn't a scenario where multiple inheritance is needed. An abstract base class would work just fine IMHO.