From dc3c79e70809dad9028907dd4c6a3b642bfc18dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Valentin=20Breu=C3=9F?= Date: Wed, 2 Nov 2022 11:42:10 +0100 Subject: [PATCH] fix: deleting a file with open file handles behaves consistently to the real file system (#179) This takes into account the following issues on [System.IO.Abstractions](https://github.com/TestableIO/System.IO.Abstractions): - https://github.com/TestableIO/System.IO.Abstractions/issues/894 - https://github.com/TestableIO/System.IO.Abstractions/issues/756 --- .../Storage/IStorageAccessHandle.cs | 5 +++ .../Storage/IStorageContainer.cs | 3 +- .../Storage/InMemoryContainer.cs | 31 +++++++++----- .../Storage/InMemoryStorage.cs | 19 +++++---- .../Storage/NullContainer.cs | 13 ++++-- .../TestHelpers/LockableContainer.cs | 17 +++++--- .../FileSystem/Directory/DeleteTests.cs | 32 +++++++++++++++ .../FileSystem/DirectoryInfo/DeleteTests.cs | 33 +++++++++++++++ .../FileSystem/File/DeleteTests.cs | 39 ++++++++++++++++++ .../FileSystem/FileInfo/DeleteTests.cs | 40 +++++++++++++++++++ 10 files changed, 205 insertions(+), 27 deletions(-) create mode 100644 Tests/Testably.Abstractions.Tests/FileSystem/File/DeleteTests.cs create mode 100644 Tests/Testably.Abstractions.Tests/FileSystem/FileInfo/DeleteTests.cs diff --git a/Source/Testably.Abstractions.Testing/Storage/IStorageAccessHandle.cs b/Source/Testably.Abstractions.Testing/Storage/IStorageAccessHandle.cs index 1866687b..0f4008e2 100644 --- a/Source/Testably.Abstractions.Testing/Storage/IStorageAccessHandle.cs +++ b/Source/Testably.Abstractions.Testing/Storage/IStorageAccessHandle.cs @@ -13,6 +13,11 @@ internal interface IStorageAccessHandle : IDisposable /// FileAccess Access { get; } + /// + /// Flag indicating, if the access handle resulted from a deletion request. + /// + bool DeleteAccess { get; } + /// /// The that this access handle allows. /// diff --git a/Source/Testably.Abstractions.Testing/Storage/IStorageContainer.cs b/Source/Testably.Abstractions.Testing/Storage/IStorageContainer.cs index a07e7def..91061a71 100644 --- a/Source/Testably.Abstractions.Testing/Storage/IStorageContainer.cs +++ b/Source/Testably.Abstractions.Testing/Storage/IStorageContainer.cs @@ -75,7 +75,8 @@ internal interface IStorageContainer : IFileSystemExtensionPoint, /// /// An that is used to release the access lock on dispose. IStorageAccessHandle RequestAccess(FileAccess access, FileShare share, - bool ignoreMetadataError = true); + bool deleteAccess = false, + bool ignoreMetadataErrors = true); /// /// Writes the to the . diff --git a/Source/Testably.Abstractions.Testing/Storage/InMemoryContainer.cs b/Source/Testably.Abstractions.Testing/Storage/InMemoryContainer.cs index ffe0ab22..035a4f5e 100644 --- a/Source/Testably.Abstractions.Testing/Storage/InMemoryContainer.cs +++ b/Source/Testably.Abstractions.Testing/Storage/InMemoryContainer.cs @@ -131,9 +131,10 @@ public void Encrypt() /// public byte[] GetBytes() => _bytes; - /// + /// public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share, - bool ignoreMetadataError = true) + bool deleteAccess = false, + bool ignoreMetadataErrors = true) { if (_location.Drive == null) { @@ -146,13 +147,13 @@ public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share, } Execute.OnWindowsIf( - !ignoreMetadataError && Attributes.HasFlag(FileAttributes.ReadOnly), + !ignoreMetadataErrors && Attributes.HasFlag(FileAttributes.ReadOnly), () => throw ExceptionFactory.AccessToPathDenied()); - if (CanGetAccess(access, share)) + if (CanGetAccess(access, share, deleteAccess)) { Guid guid = Guid.NewGuid(); - FileHandle fileHandle = new(guid, ReleaseAccess, access, share); + FileHandle fileHandle = new(guid, ReleaseAccess, access, share, deleteAccess); _fileHandles.TryAdd(guid, fileHandle); return fileHandle; } @@ -238,11 +239,11 @@ internal FileAttributes AdjustAttributes(FileAttributes attributes) return attributes; } - private bool CanGetAccess(FileAccess access, FileShare share) + private bool CanGetAccess(FileAccess access, FileShare share, bool deleteAccess) { foreach (KeyValuePair fileHandle in _fileHandles) { - if (!fileHandle.Value.GrantAccess(access, share)) + if (!fileHandle.Value.GrantAccess(access, share, deleteAccess)) { return false; } @@ -293,10 +294,11 @@ private sealed class FileHandle : IStorageAccessHandle private readonly Action _releaseCallback; public FileHandle(Guid key, Action releaseCallback, FileAccess access, - FileShare share) + FileShare share, bool deleteAccess) { _releaseCallback = releaseCallback; Access = access; + DeleteAccess = deleteAccess; Share = Execute.OnWindows( () => share, () => share == FileShare.None @@ -311,6 +313,9 @@ public FileHandle(Guid key, Action releaseCallback, FileAccess access, /// public FileAccess Access { get; } + /// + public bool DeleteAccess { get; } + /// public FileShare Share { get; } @@ -322,16 +327,24 @@ public void Dispose() #endregion - public bool GrantAccess(FileAccess access, FileShare share) + public bool GrantAccess(FileAccess access, FileShare share, bool deleteAccess) { FileShare usedShare = share; Execute.NotOnWindows(() => usedShare = FileShare.ReadWrite); + if (deleteAccess && !Execute.IsWindows) + { + return true; + } return CheckAccessWithShare(access, Share) && CheckAccessWithShare(Access, usedShare); } + /// + public override string ToString() + => $"{Access} | {Share}"; + private static bool CheckAccessWithShare(FileAccess access, FileShare share) { switch (access) diff --git a/Source/Testably.Abstractions.Testing/Storage/InMemoryStorage.cs b/Source/Testably.Abstractions.Testing/Storage/InMemoryStorage.cs index 09e78900..e08b738d 100644 --- a/Source/Testably.Abstractions.Testing/Storage/InMemoryStorage.cs +++ b/Source/Testably.Abstractions.Testing/Storage/InMemoryStorage.cs @@ -126,12 +126,17 @@ public bool DeleteContainer(IStorageLocation location, bool recursive = false) _fileSystem.ChangeHandler.NotifyPendingChange(WatcherChangeTypes.Deleted, container.Type, notifyFilters, location); - if (_containers.TryRemove(location, out IStorageContainer? removed)) + + CheckGrantAccess(location, container); + using (container.RequestAccess(FileAccess.Write, FileShare.ReadWrite, deleteAccess: true)) { - removed.ClearBytes(); - _fileSystem.ChangeHandler.NotifyCompletedChange(fileSystemChange); - AdjustParentDirectoryTimes(location); - return true; + if (_containers.TryRemove(location, out IStorageContainer? removed)) + { + removed.ClearBytes(); + _fileSystem.ChangeHandler.NotifyCompletedChange(fileSystemChange); + AdjustParentDirectoryTimes(location); + return true; + } } return false; @@ -360,10 +365,10 @@ public IStorageContainer GetOrCreateContainer( CheckGrantAccess(source, sourceContainer); CheckGrantAccess(destination, destinationContainer); using (_ = sourceContainer.RequestAccess(FileAccess.ReadWrite, FileShare.None, - ignoreMetadataErrors)) + ignoreMetadataErrors: ignoreMetadataErrors)) { using (_ = destinationContainer.RequestAccess(FileAccess.ReadWrite, - FileShare.None, ignoreMetadataErrors)) + FileShare.None, ignoreMetadataErrors: ignoreMetadataErrors)) { if (_containers.TryRemove(destination, out IStorageContainer? existingDestinationContainer)) diff --git a/Source/Testably.Abstractions.Testing/Storage/NullContainer.cs b/Source/Testably.Abstractions.Testing/Storage/NullContainer.cs index a8e2ad35..f1379163 100644 --- a/Source/Testably.Abstractions.Testing/Storage/NullContainer.cs +++ b/Source/Testably.Abstractions.Testing/Storage/NullContainer.cs @@ -81,10 +81,11 @@ public void Encrypt() public byte[] GetBytes() => Array.Empty(); - /// + /// public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share, - bool ignoreMetadataError = true) - => new NullStorageAccessHandle(access, share); + bool deleteAccess = false, + bool ignoreMetadataErrors = true) + => new NullStorageAccessHandle(access, share, deleteAccess); /// public void WriteBytes(byte[] bytes) @@ -99,10 +100,11 @@ internal static IStorageContainer New(MockFileSystem fileSystem) private sealed class NullStorageAccessHandle : IStorageAccessHandle { - public NullStorageAccessHandle(FileAccess access, FileShare share) + public NullStorageAccessHandle(FileAccess access, FileShare share, bool deleteAccess) { Access = access; Share = share; + DeleteAccess = deleteAccess; } #region IStorageAccessHandle Members @@ -110,6 +112,9 @@ public NullStorageAccessHandle(FileAccess access, FileShare share) /// public FileAccess Access { get; } + /// + public bool DeleteAccess { get; } + /// public FileShare Share { get; } diff --git a/Tests/Testably.Abstractions.Testing.Tests/TestHelpers/LockableContainer.cs b/Tests/Testably.Abstractions.Testing.Tests/TestHelpers/LockableContainer.cs index 6c0d8c47..a6b95ee0 100644 --- a/Tests/Testably.Abstractions.Testing.Tests/TestHelpers/LockableContainer.cs +++ b/Tests/Testably.Abstractions.Testing.Tests/TestHelpers/LockableContainer.cs @@ -12,13 +12,13 @@ namespace Testably.Abstractions.Testing.Tests.TestHelpers; /// A for testing purposes. /// /// Set to to simulate a locked file -/// ( throws an ). +/// ( throws an ). /// internal sealed class LockableContainer : IStorageContainer { /// /// Simulate a locked file, if set to .
- /// In this case throws an , + /// In this case throws an , /// otherwise it will succeed. /// public bool IsLocked { get; set; } @@ -90,16 +90,17 @@ public void Encrypt() public byte[] GetBytes() => _bytes; - /// + /// public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share, - bool ignoreMetadataError = true) + bool deleteAccess = false, + bool ignoreMetadataErrors = true) { if (IsLocked) { throw ExceptionFactory.ProcessCannotAccessTheFile(""); } - return new AccessHandle(access, share); + return new AccessHandle(access, share, deleteAccess); } /// @@ -110,10 +111,11 @@ public void WriteBytes(byte[] bytes) private class AccessHandle : IStorageAccessHandle { - public AccessHandle(FileAccess access, FileShare share) + public AccessHandle(FileAccess access, FileShare share, bool deleteAccess) { Access = access; Share = share; + DeleteAccess = deleteAccess; } #region IStorageAccessHandle Members @@ -121,6 +123,9 @@ public AccessHandle(FileAccess access, FileShare share) /// public FileAccess Access { get; } + /// + public bool DeleteAccess { get; } + /// public FileShare Share { get; } diff --git a/Tests/Testably.Abstractions.Tests/FileSystem/Directory/DeleteTests.cs b/Tests/Testably.Abstractions.Tests/FileSystem/Directory/DeleteTests.cs index 08351745..e42be2c8 100644 --- a/Tests/Testably.Abstractions.Tests/FileSystem/Directory/DeleteTests.cs +++ b/Tests/Testably.Abstractions.Tests/FileSystem/Directory/DeleteTests.cs @@ -52,6 +52,38 @@ public void Delete_Recursive_MissingDirectory_ShouldDeleteDirectory( .Be($"Could not find a part of the path '{expectedPath}'."); } + [SkippableTheory] + [AutoData] + public void Delete_Recursive_WithOpenFile_ShouldThrowIOException( + string path, string filename) + { + FileSystem.Initialize() + .WithSubdirectory(path); + string filePath = FileSystem.Path.Combine(path, filename); + FileSystemStream openFile = FileSystem.File.OpenWrite(filePath); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + Exception? exception = Record.Exception(() => + { + FileSystem.Directory.Delete(path, true); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + }); + + if (Test.RunsOnWindows) + { + exception.Should().BeOfType() + .Which.Message.Should() + .Contain($"{filename}'"); + FileSystem.File.Exists(filePath).Should().BeTrue(); + } + else + { + exception.Should().BeNull(); + FileSystem.File.Exists(filePath).Should().BeFalse(); + } + } + [SkippableTheory] [AutoData] public void diff --git a/Tests/Testably.Abstractions.Tests/FileSystem/DirectoryInfo/DeleteTests.cs b/Tests/Testably.Abstractions.Tests/FileSystem/DirectoryInfo/DeleteTests.cs index 91258409..d3a84d05 100644 --- a/Tests/Testably.Abstractions.Tests/FileSystem/DirectoryInfo/DeleteTests.cs +++ b/Tests/Testably.Abstractions.Tests/FileSystem/DirectoryInfo/DeleteTests.cs @@ -25,6 +25,39 @@ public void Delete_MissingDirectory_ShouldDeleteDirectory(string path) .Be($"Could not find a part of the path '{sut.FullName}'."); } + [SkippableTheory] + [AutoData] + public void Delete_Recursive_WithOpenFile_ShouldThrowIOException( + string path, string filename) + { + FileSystem.Initialize() + .WithSubdirectory(path); + string filePath = FileSystem.Path.Combine(path, filename); + FileSystemStream openFile = FileSystem.File.OpenWrite(filePath); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + IDirectoryInfo sut = FileSystem.DirectoryInfo.New(path); + Exception? exception = Record.Exception(() => + { + sut.Delete(true); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + }); + + if (Test.RunsOnWindows) + { + exception.Should().BeOfType() + .Which.Message.Should() + .Contain($"{filename}'"); + FileSystem.File.Exists(filePath).Should().BeTrue(); + } + else + { + exception.Should().BeNull(); + FileSystem.File.Exists(filePath).Should().BeFalse(); + } + } + [SkippableTheory] [AutoData] public void Delete_Recursive_WithSubdirectory_ShouldDeleteDirectoryWithContent( diff --git a/Tests/Testably.Abstractions.Tests/FileSystem/File/DeleteTests.cs b/Tests/Testably.Abstractions.Tests/FileSystem/File/DeleteTests.cs new file mode 100644 index 00000000..6badf3b1 --- /dev/null +++ b/Tests/Testably.Abstractions.Tests/FileSystem/File/DeleteTests.cs @@ -0,0 +1,39 @@ +using System.IO; +using Testably.Abstractions.FileSystem; + +namespace Testably.Abstractions.Tests.FileSystem.File; + +// ReSharper disable once PartialTypeWithSinglePart +public abstract partial class DeleteTests + : FileSystemTestBase + where TFileSystem : IFileSystem +{ + [SkippableTheory] + [AutoData] + public void Delete_WithOpenFile_ShouldThrowIOException(string filename) + { + FileSystem.Initialize(); + FileSystemStream openFile = FileSystem.File.OpenWrite(filename); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + Exception? exception = Record.Exception(() => + { + FileSystem.File.Delete(filename); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + }); + + if (Test.RunsOnWindows) + { + exception.Should().BeOfType() + .Which.Message.Should() + .Contain($"{filename}'"); + FileSystem.File.Exists(filename).Should().BeTrue(); + } + else + { + exception.Should().BeNull(); + FileSystem.File.Exists(filename).Should().BeFalse(); + } + } +} \ No newline at end of file diff --git a/Tests/Testably.Abstractions.Tests/FileSystem/FileInfo/DeleteTests.cs b/Tests/Testably.Abstractions.Tests/FileSystem/FileInfo/DeleteTests.cs new file mode 100644 index 00000000..8f11fe75 --- /dev/null +++ b/Tests/Testably.Abstractions.Tests/FileSystem/FileInfo/DeleteTests.cs @@ -0,0 +1,40 @@ +using System.IO; +using Testably.Abstractions.FileSystem; + +namespace Testably.Abstractions.Tests.FileSystem.FileInfo; + +// ReSharper disable once PartialTypeWithSinglePart +public abstract partial class DeleteTests + : FileSystemTestBase + where TFileSystem : IFileSystem +{ + [SkippableTheory] + [AutoData] + public void Delete_WithOpenFile_ShouldThrowIOException(string filename) + { + FileSystem.Initialize(); + FileSystemStream openFile = FileSystem.File.OpenWrite(filename); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + IFileInfo sut = FileSystem.FileInfo.New(filename); + Exception? exception = Record.Exception(() => + { + sut.Delete(); + openFile.Write(new byte[] { 0 }, 0, 1); + openFile.Flush(); + }); + + if (Test.RunsOnWindows) + { + exception.Should().BeOfType() + .Which.Message.Should() + .Contain($"{filename}'"); + FileSystem.File.Exists(filename).Should().BeTrue(); + } + else + { + exception.Should().BeNull(); + FileSystem.File.Exists(filename).Should().BeFalse(); + } + } +} \ No newline at end of file