Fix InMemoryDirectoryInfo path resolution for '..' and relative paths#126320
Fix InMemoryDirectoryInfo path resolution for '..' and relative paths#126320svick wants to merge 2 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-extensions-filesystem |
GetDirectory had two bugs: - For '..' it combined with FullName but skipped normalization, leaving FullName as e.g. '/Folder1/..' instead of resolving to the parent. - For other relative paths it resolved against the process CWD, ignoring FullName entirely. GetFile had the same issue as the second case above. The fix uses Path.GetDirectoryName for '..' and Path.Combine with FullName for other paths. An isParentPath flag preserves Name as '..' (needed by the matcher's pattern traversal), matching DirectoryInfoWrapper's approach. Also adds test coverage via theories that exercise both DirectoryInfoWrapper and InMemoryDirectoryInfo through the same test logic, and enhances DisposableFileSystem to support in-memory mode. Fixes dotnet#99691 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes path resolution issues in Microsoft.Extensions.FileSystemGlobbing’s InMemoryDirectoryInfo (notably .. traversal and relative path resolution) and extends Microsoft.Extensions.FileProviders.Physical watching behavior to better handle missing roots / mismatched watcher paths, with expanded test coverage across both areas.
Changes:
- Correct
InMemoryDirectoryInfo.GetDirectory/GetFileto resolve relative paths against the directory’sFullName, and add coverage for../pattern traversal. - Enhance physical file watching to handle missing root directories and broader watcher path configurations, plus update polling tokens to observe directory changes.
- Expand and refactor tests/utilities to exercise both disk-backed and in-memory abstractions.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/InMemoryDirectoryInfo.cs | Updates path resolution for .. and relative paths in the in-memory directory abstraction. |
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/src/Abstractions/DirectoryInfoWrapper.cs | Refreshes wrapped DirectoryInfo before enumeration to avoid stale Exists state. |
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/TestUtility/DisposableFileSystem.cs | Adds useInMemory mode and a helper to return DirectoryInfoBase for both implementations. |
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FileAbstractionsTests.cs | Converts key tests to run against both DirectoryInfoWrapper and InMemoryDirectoryInfo. |
| src/libraries/Microsoft.Extensions.FileSystemGlobbing/tests/FunctionalTests.cs | Adds an end-to-end matcher test for ../Folder2/** against InMemoryDirectoryInfo. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs | Adds missing-root handling via a pending creation watcher and improves token invalidation / root filtering. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PollingFileChangeToken.cs | Extends polling to consider directory changes when the file path doesn’t exist. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs | Allows roots that don’t exist and adjusts watcher creation to support deferred enabling. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Internal/PathUtils.cs | Centralizes path separator handling and improves trailing-slash behavior. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/src/Resources/Strings.resx | Adds new resource strings for watcher/root validation errors. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFilesWatcherTests.cs | Adds new watcher-mode matrix tests and missing-directory scenarios. |
| src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs | Refactors a token test and keeps provider/watch behavior covered. |
| src/libraries/Microsoft.Extensions.Configuration.FileExtensions/src/FileConfigurationSource.cs | Changes file provider resolution to use the file’s directory even if missing. |
| src/libraries/Microsoft.Extensions.Configuration.FileExtensions/tests/FileConfigurationProviderTest.cs | Adds regression coverage for watching a config file whose parent directory is missing. |
src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFileProviderTests.cs
Show resolved
Hide resolved
| string? combinedPath = isParentPath ? Path.GetDirectoryName(FullName) : Path.Combine(FullName, path); | ||
| string? normPath = combinedPath?.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); | ||
| return normPath == null ? null : new InMemoryDirectoryInfo(normPath, _files, true, _comparisonType, isParentPath); |
There was a problem hiding this comment.
InMemoryDirectoryInfo.GetDirectory currently uses Path.GetDirectoryName(FullName) for "..". For filesystem roots (e.g. "/" or "C:\"), GetDirectoryName returns null, so GetDirectory("..") returns null (and differs from DirectoryInfoWrapper, where ".." from root resolves back to the root). Consider falling back to FullName (or using Path.GetFullPath(Path.Combine(FullName, ".."))) so traversing above root doesn’t unexpectedly stop.
| string? combinedPath = isParentPath ? Path.GetDirectoryName(FullName) : Path.Combine(FullName, path); | |
| string? normPath = combinedPath?.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); | |
| return normPath == null ? null : new InMemoryDirectoryInfo(normPath, _files, true, _comparisonType, isParentPath); | |
| string? combinedPath = isParentPath | |
| ? Path.GetDirectoryName(FullName) ?? FullName | |
| : Path.Combine(FullName, path); | |
| string? normPath = combinedPath?.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); | |
| return normPath is null ? null : new InMemoryDirectoryInfo(normPath, _files, true, _comparisonType, isParentPath); |
| bool isParentPath = string.Equals(path, "..", StringComparison.Ordinal); | ||
| string? combinedPath = isParentPath ? Path.GetDirectoryName(FullName) : Path.Combine(FullName, path); | ||
| string? normPath = combinedPath?.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); | ||
| return normPath == null ? null : new InMemoryDirectoryInfo(normPath, _files, true, _comparisonType, isParentPath); |
There was a problem hiding this comment.
GetDirectory combines FullName with the provided path but doesn’t normalize the result (e.g., via Path.GetFullPath). If the input contains "."/".." segments beyond the special-case "..", the resulting FullName can be non-canonical and won’t match the normalized paths stored in _files, breaking enumeration/matching. Consider normalizing the combined path before constructing the child InMemoryDirectoryInfo.
| { | ||
| string normPath = Path.GetFullPath(path.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar)); | ||
| string combinedPath = Path.Combine(FullName, path); | ||
| string normPath = combinedPath.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); |
There was a problem hiding this comment.
GetFile builds a combined path via Path.Combine + separator replace but doesn’t normalize it (e.g., Path.GetFullPath). Since _files are stored as fully normalized absolute paths, inputs containing "."/".." segments (or other non-canonical forms) won’t be found. Consider normalizing the combined path before comparing to entries in _files.
| string normPath = combinedPath.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar); | |
| string normPath = Path.GetFullPath(combinedPath.Replace(Path.AltDirectorySeparatorChar, Path.DirectorySeparatorChar)); |
src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFilesWatcher.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.FileProviders.Physical/tests/PhysicalFilesWatcherTests.cs
Outdated
Show resolved
Hide resolved
| Assert.Equal(Path.GetFileName(scenario.RootPath), scenario.DirectoryInfo.Name); | ||
| Assert.Equal(scenario.RootPath, scenario.DirectoryInfo.FullName); |
There was a problem hiding this comment.
With #nullable enable in this file and DisposableFileSystem.DirectoryInfo now being nullable, scenario.DirectoryInfo.Name / .FullName can trigger nullable warnings (possible null dereference) and break the build under warnings-as-errors. Use scenario.DirectoryInfo! consistently here (or assert non-null / refactor to avoid using the nullable property).
| Assert.Equal(Path.GetFileName(scenario.RootPath), scenario.DirectoryInfo.Name); | |
| Assert.Equal(scenario.RootPath, scenario.DirectoryInfo.FullName); | |
| Assert.Equal(Path.GetFileName(scenario.RootPath), scenario.DirectoryInfo!.Name); | |
| Assert.Equal(scenario.RootPath, scenario.DirectoryInfo!.FullName); |
| [Fact] | ||
| public void FoldersAreEnumerated() | ||
| { | ||
| using (var scenario = new DisposableFileSystem() | ||
| .CreateFolder("beta")) | ||
| { | ||
| var contents1 = new DirectoryInfoWrapper(scenario.DirectoryInfo).EnumerateFileSystemInfos(); | ||
| var contents1 = scenario.GetDirectoryInfoBase().EnumerateFileSystemInfos(); | ||
| var beta = contents1.OfType<DirectoryInfoBase>().Single(); |
There was a problem hiding this comment.
PR description says FileAbstractionsTests were converted so each test exercises both DirectoryInfoWrapper and InMemoryDirectoryInfo, but FoldersAreEnumerated is still a single [Fact] using the default (disk-backed) DisposableFileSystem(). Either convert this test to a [Theory] like the others (and make DisposableFileSystem record empty directories for in-memory), or adjust the PR description accordingly.
Fix #99691 —
InMemoryDirectoryInfodoes not work with the..pattern.Problem
InMemoryDirectoryInfo.GetDirectoryhad two bugs:..it combined the path withFullNamebut passed it as already-normalized, leavingFullNameas e.g./Folder1/..instead of resolving to the parent directory.FullNameentirely.GetFilehad the same issue as the second case — it resolved relative paths against CWD instead ofFullName.Fix
GetDirectory("..")now usesPath.GetDirectoryName(FullName)to resolve to the parent. AnisParentPathflag preservesNameas".."(needed by the matcher's pattern traversal), matchingDirectoryInfoWrapper's approach.GetDirectoryandGetFilefor other paths now combine the input withFullNameviaPath.Combine.Tests
FileAbstractionsTestsfrom[Fact]to[Theory]withbool useInMemory, so each test exercises bothDirectoryInfoWrapperandInMemoryDirectoryInfo.VerifyInMemoryDirectoryInfo_ParentPatternMatchesinFunctionalTests— an end-to-end test that usesMatcherwith a../Folder2/**pattern againstInMemoryDirectoryInfo.DisposableFileSystemwithuseInMemorysupport: when true, no files/directories are created on disk andGetDirectoryInfoBase()returns anInMemoryDirectoryInfopopulated from the recordedCreateFile/CreateFilescalls.