Skip to content

Commit

Permalink
fix: deleting a file with open file handles behaves consistently to t…
Browse files Browse the repository at this point in the history
…he real file system (#179)

This takes into account the following issues on
[System.IO.Abstractions](https://github.com/TestableIO/System.IO.Abstractions):
- TestableIO/System.IO.Abstractions#894
- TestableIO/System.IO.Abstractions#756
  • Loading branch information
vbreuss authored Nov 2, 2022
1 parent 5516a14 commit dc3c79e
Show file tree
Hide file tree
Showing 10 changed files with 205 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ internal interface IStorageAccessHandle : IDisposable
/// </summary>
FileAccess Access { get; }

/// <summary>
/// Flag indicating, if the access handle resulted from a deletion request.
/// </summary>
bool DeleteAccess { get; }

/// <summary>
/// The <see cref="FileShare" /> that this access handle allows.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@ internal interface IStorageContainer : IFileSystemExtensionPoint,
/// </summary>
/// <returns>An <see cref="IStorageAccessHandle" /> that is used to release the access lock on dispose.</returns>
IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
bool ignoreMetadataError = true);
bool deleteAccess = false,
bool ignoreMetadataErrors = true);

/// <summary>
/// Writes the <paramref name="bytes" /> to the <see cref="IFileInfo" />.
Expand Down
31 changes: 22 additions & 9 deletions Source/Testably.Abstractions.Testing/Storage/InMemoryContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,10 @@ public void Encrypt()
/// <inheritdoc cref="IStorageContainer.GetBytes()" />
public byte[] GetBytes() => _bytes;

/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool)" />
/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool)" />
public IStorageAccessHandle RequestAccess(FileAccess access, FileShare share,
bool ignoreMetadataError = true)
bool deleteAccess = false,
bool ignoreMetadataErrors = true)
{
if (_location.Drive == null)
{
Expand All @@ -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;
}
Expand Down Expand Up @@ -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<Guid, FileHandle> fileHandle in _fileHandles)
{
if (!fileHandle.Value.GrantAccess(access, share))
if (!fileHandle.Value.GrantAccess(access, share, deleteAccess))
{
return false;
}
Expand Down Expand Up @@ -293,10 +294,11 @@ private sealed class FileHandle : IStorageAccessHandle
private readonly Action<Guid> _releaseCallback;

public FileHandle(Guid key, Action<Guid> releaseCallback, FileAccess access,
FileShare share)
FileShare share, bool deleteAccess)
{
_releaseCallback = releaseCallback;
Access = access;
DeleteAccess = deleteAccess;
Share = Execute.OnWindows(
() => share,
() => share == FileShare.None
Expand All @@ -311,6 +313,9 @@ public FileHandle(Guid key, Action<Guid> releaseCallback, FileAccess access,
/// <inheritdoc cref="IStorageAccessHandle.Access" />
public FileAccess Access { get; }

/// <inheritdoc cref="IStorageAccessHandle.DeleteAccess" />
public bool DeleteAccess { get; }

/// <inheritdoc cref="IStorageAccessHandle.Share" />
public FileShare Share { get; }

Expand All @@ -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);
}

/// <inheritdoc cref="object.ToString()" />
public override string ToString()
=> $"{Access} | {Share}";

private static bool CheckAccessWithShare(FileAccess access, FileShare share)
{
switch (access)
Expand Down
19 changes: 12 additions & 7 deletions Source/Testably.Abstractions.Testing/Storage/InMemoryStorage.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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))
Expand Down
13 changes: 9 additions & 4 deletions Source/Testably.Abstractions.Testing/Storage/NullContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,11 @@ public void Encrypt()
public byte[] GetBytes()
=> Array.Empty<byte>();

/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool)" />
/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool)" />
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);

/// <inheritdoc cref="IStorageContainer.WriteBytes(byte[])" />
public void WriteBytes(byte[] bytes)
Expand All @@ -99,17 +100,21 @@ 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

/// <inheritdoc cref="IStorageAccessHandle.Access" />
public FileAccess Access { get; }

/// <inheritdoc cref="IStorageAccessHandle.DeleteAccess" />
public bool DeleteAccess { get; }

/// <inheritdoc cref="IStorageAccessHandle.Share" />
public FileShare Share { get; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ namespace Testably.Abstractions.Testing.Tests.TestHelpers;
/// A <see cref="IStorageContainer" /> for testing purposes.
/// <para />
/// Set <see cref="IsLocked" /> to <see langword="true" /> to simulate a locked file
/// (<see cref="RequestAccess(FileAccess, FileShare, bool)" /> throws an <see cref="IOException" />).
/// (<see cref="RequestAccess(FileAccess, FileShare, bool, bool)" /> throws an <see cref="IOException" />).
/// </summary>
internal sealed class LockableContainer : IStorageContainer
{
/// <summary>
/// Simulate a locked file, if set to <see langword="true" />.<br />
/// In this case <see cref="RequestAccess(FileAccess, FileShare, bool)" /> throws an <see cref="IOException" />,
/// In this case <see cref="RequestAccess(FileAccess, FileShare, bool, bool)" /> throws an <see cref="IOException" />,
/// otherwise it will succeed.
/// </summary>
public bool IsLocked { get; set; }
Expand Down Expand Up @@ -90,16 +90,17 @@ public void Encrypt()
public byte[] GetBytes()
=> _bytes;

/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool)" />
/// <inheritdoc cref="IStorageContainer.RequestAccess(FileAccess, FileShare, bool, bool)" />
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);
}

/// <inheritdoc cref="IStorageContainer.WriteBytes(byte[])" />
Expand All @@ -110,17 +111,21 @@ 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

/// <inheritdoc cref="IStorageAccessHandle.Access" />
public FileAccess Access { get; }

/// <inheritdoc cref="IStorageAccessHandle.DeleteAccess" />
public bool DeleteAccess { get; }

/// <inheritdoc cref="IStorageAccessHandle.Access" />
public FileShare Share { get; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<IOException>()
.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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<IOException>()
.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(
Expand Down
39 changes: 39 additions & 0 deletions Tests/Testably.Abstractions.Tests/FileSystem/File/DeleteTests.cs
Original file line number Diff line number Diff line change
@@ -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<TFileSystem>
: FileSystemTestBase<TFileSystem>
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<IOException>()
.Which.Message.Should()
.Contain($"{filename}'");
FileSystem.File.Exists(filename).Should().BeTrue();
}
else
{
exception.Should().BeNull();
FileSystem.File.Exists(filename).Should().BeFalse();
}
}
}
Loading

0 comments on commit dc3c79e

Please sign in to comment.