File I/O Abstraction (Part I) #15568
Conversation
|
Test this change out locally with the following install scripts (Action run 11845655422) VSCode
Azure CLI
|
Dotnet Test Results 78 files - 30 78 suites - 30 29m 31s ⏱️ - 10m 29s Results for commit 0d25ac3. ± Comparison against base commit 4a382ed. This pull request removes 1841 and adds 659 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="System.IO.Abstractions" Version="21.0.29" /> |
There was a problem hiding this comment.
Is this to facilitate the refactor, or something we see sticking around long-term?
There was a problem hiding this comment.
System.IO.Abstraction will still be around and used for mocking the file system in tests. However, I plan to restrict references to the namespace within Bicep.Core.
| { | ||
| public static class GlobalSettings | ||
| { | ||
| public static bool PathCaseSensitive { get; set; } = RuntimeInformation.IsOSPlatform(OSPlatform.Linux); |
There was a problem hiding this comment.
Is this the same as the .NET implementation? AFAIK there are exceptions to this - e.g. you can choose either case-sensitive or case-insensitive on OSX.
There was a problem hiding this comment.
This is same as Omnisharp's DocumentUri implementation. System.Uri doesn’t implement IEquatable, and that's why we have custom path comparers in Bicep.Core in a few places, which is not ideal. Speaking of exceptions, technically, case-insensitive and case-senstitive file systems can exist on all OS platforms, depending on what file system the user has chosen to use. However, there’s no reliable way to detect it. Given these limitations, I believe a heuristic based on the OS platform is the most practical solution.
There was a problem hiding this comment.
You can make portions of paths case-sensitive in NTFS, too, so getting this perfect is a mug's game.
When do we need to know if a path is case sensitive or not? Presumably this is handled by the OS when we try to open the file.
There was a problem hiding this comment.
We need to address path case sensitivity when resolving module paths during module instantiation to prevent duplicate models for the same source file.
| public bool Equals(ResourceIdentifier other) => | ||
| string.Equals(Scheme, other.Scheme, StringComparison.Ordinal) && | ||
| string.Equals(Authority, other.Authority, StringComparison.Ordinal) && | ||
| string.Equals(Path, other.Path, GlobalSettings.PathComparison); |
There was a problem hiding this comment.
Shouldn't the behavior between OSes be the same unless scheme is file?
Even this could be tricky - I wonder if it's worth making the OS-specific behavior ultra-clear and e.g. making a specific method for this, rather than baking it into the default equality comparer?
There was a problem hiding this comment.
Ah good catch. Let me fix it.
There was a problem hiding this comment.
I wonder if it's worth making the OS-specific behavior ultra-clear and e.g. making a specific method for this, rather than baking it into the default equality comparer.
It's a long story...but that's what I implemented originally where I put the logic in Bicep.Core.FileSystem (this is essentially what rfc8089 section 2 suggests). While this design was conceptually better because it avoided leaking abstractions, it ironically ended up complicating the code by a lot. It also made it more difficult to replace System.Uri in Bicep.Core and eliminate PathHelper.PathComparer.
Additionally, I later discovered that MockFileSystem accounts for the OS platform, which would make the test code more complex if we followed this approach.
This current solution is definitely not perfect, but it's a compromise that simplifies the implementation. I'd be open to revisiting the design once the entire migration is complete. For example, there might be an opportunity to get it right if BicepSourceFile doesn’t hold a direct reference to ResourceIdentifier but instead uses a IFileHandle.
| { | ||
| IDirectoryHandle GetParent(); | ||
|
|
||
| Stream OpenRead(); |
There was a problem hiding this comment.
@anthony-c-martin I looked into using BinaryData as the return type. While I appreciate its cleaner abstraction compared to Stream, I have some performance concerns since it always copies the data into a memory stream when created from a stream. I think it might be better to implement extension methods for reading and writing with BinaryData.
This is the first PR in a series of changes to introduce a file I/O abstraction layer to the Bicep project. Currently,
Bicep.Coreheavily relies on the largeIFileSysteminterface,System.Uri. This tight coupling introduces several challenges, such as making it difficult for the Bicep resource provider to supply a mock implementation ofIFileSystem. Additionally, it prevents support for VS Code virtual workspaces becauseSystem.Uriis surprisingly not fully RFC-compliant (see: #11467).To address these issues, a higher-level abstraction for file I/O is required. Given the broad scope of this refactoring, this PR focuses on laying the groundwork by introducing the new abstraction layer and using it to load configuration. Subsequent PRs will replace the usage of
IFileSystemandSystem.Urithroughout Bicep.Core and tidy up theIFileResolverinterface with the new APIs.Key Changes in this PR
The core of this PR is the introduction of a new
Bicep.IOproject. This project defines the new file I/O abstraction layer and includes:Bicep.IO.Abstractiontypes:IFileExplorer: Encapsulates directory and file discovery.IFileHandle: A pointer to a file, abstracting operations like reading or writing.IDirectoryHandle: A pointer to a directory for managing directory traversal.ResourceIdentifier: An RFC3986 compliant URI (without Query and Fragment) which is going to replaceSystem.UriinBicep.Core.Bicep.IO.FileSystemtypes:IFileSystem.The ultimate objective is to phase out direct usage of
Bicep.IO.FileSystemwithinBicep.Coreand restrict it to referencing onlyBicep.IO.AbstractionviaBannedSymbols.txt. This design ensures cleaner separation of concerns and enables better testing, security, and support for virtual environments.Below is an architecture diagram illustrating the new design and how the layers interact:
Microsoft Reviewers: Open in CodeFlow