fix: Simplified IFileSystem (#3641)

bitsandfoxes · web-flow · commit 7da3cf3b3e18 · 2024-10-02T01:32:08.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,7 +14,7 @@
 
 ### Features
 
-- Added a flag to options `DisableFileWrite` to allow users to opt-out of all file writing operations. Note that toggling this will affect features such as offline caching and auto-session tracking and release health as these rely on some file persistency ([#3614](https://github.com/getsentry/sentry-dotnet/pull/3614))
+- Added a flag to options `DisableFileWrite` to allow users to opt-out of all file writing operations. Note that toggling this will affect features such as offline caching and auto-session tracking and release health as these rely on some file persistency ([#3614](https://github.com/getsentry/sentry-dotnet/pull/3614), [#3641](https://github.com/getsentry/sentry-dotnet/pull/3641))
 
 ### Dependencies
 
diff --git a/src/Sentry/GlobalSessionManager.cs b/src/Sentry/GlobalSessionManager.cs
@@ -51,29 +51,42 @@ private void PersistSession(SessionUpdate update, DateTimeOffset? pauseTimestamp
             return;
         }
 
+        if (_options.DisableFileWrite)
+        {
+            _options.LogInfo("File write has been disabled via the options. Skipping persisting session.");
+            return;
+        }
+
         try
         {
             _options.LogDebug("Creating persistence directory for session file at '{0}'.", _persistenceDirectoryPath);
 
-            var result = _options.FileSystem.CreateDirectory(_persistenceDirectoryPath);
-            if (result is not FileOperationResult.Success)
+            if (!_options.FileSystem.CreateDirectory(_persistenceDirectoryPath))
             {
-                if (result is FileOperationResult.Disabled)
-                {
-                    _options.LogInfo("Persistent directory for session file has not been created. File-write has been disabled via the options.");
-                }
-                else
-                {
-                    _options.LogError("Failed to create persistent directory for session file.");
-                }
-
+                _options.LogError("Failed to create persistent directory for session file.");
                 return;
             }
 
             var filePath = Path.Combine(_persistenceDirectoryPath, PersistedSessionFileName);
 
             var persistedSessionUpdate = new PersistedSessionUpdate(update, pauseTimestamp);
-            persistedSessionUpdate.WriteToFile(_options.FileSystem, filePath, _options.DiagnosticLogger);
+            if (!_options.FileSystem.CreateFileForWriting(filePath, out var file))
+            {
+                _options.LogError("Failed to persist session file.");
+                return;
+            }
+
+            using var writer = new Utf8JsonWriter(file);
+
+            try
+            {
+                persistedSessionUpdate.WriteTo(writer, _options.DiagnosticLogger);
+                writer.Flush();
+            }
+            finally
+            {
+                file.Dispose();
+            }
 
             _options.LogDebug("Persisted session to a file '{0}'.", filePath);
         }
@@ -91,6 +104,12 @@ private void DeletePersistedSession()
             return;
         }
 
+        if (_options.DisableFileWrite)
+        {
+            _options.LogInfo("File write has been disabled via the options. Skipping deletion of persisted session files.");
+            return;
+        }
+
         var filePath = Path.Combine(_persistenceDirectoryPath, PersistedSessionFileName);
         try
         {
@@ -108,7 +127,11 @@ private void DeletePersistedSession()
                 }
             }
 
-            _options.FileSystem.DeleteFile(filePath);
+            if (!_options.FileSystem.DeleteFile(filePath))
+            {
+                _options.LogError("Failed to delete persisted session file.");
+                return;
+            }
 
             _options.LogInfo("Deleted persisted session file '{0}'.", filePath);
         }
diff --git a/src/Sentry/Http/HttpTransportBase.cs b/src/Sentry/Http/HttpTransportBase.cs
@@ -383,20 +383,24 @@ private void HandleFailure(HttpResponseMessage response, Envelope envelope)
                     persistLargeEnvelopePathEnvVar,
                     destinationDirectory);
 
+                if (_options.DisableFileWrite)
+                {
+                    _options.LogInfo("File write has been disabled via the options. Skipping persisting envelope.");
+                    return;
+                }
+
                 var destination = Path.Combine(destinationDirectory, "envelope_too_large",
                     (eventId ?? SentryId.Create()).ToString());
 
-                var createDirectoryResult = _options.FileSystem.CreateDirectory(Path.GetDirectoryName(destination)!);
-                if (createDirectoryResult is not FileOperationResult.Success)
+                if (!_options.FileSystem.CreateDirectory(Path.GetDirectoryName(destination)!))
                 {
-                    _options.LogError("Failed to create directory to store the envelope: {0}", createDirectoryResult);
+                    _options.LogError("Failed to create directory to store the envelope.");
                     return;
                 }
 
-                var result = _options.FileSystem.CreateFileForWriting(destination, out var envelopeFile);
-                if (result is not FileOperationResult.Success)
+                if (!_options.FileSystem.CreateFileForWriting(destination, out var envelopeFile))
                 {
-                    _options.LogError("Failed to create envelope file: {0}", result);
+                    _options.LogError("Failed to create envelope file.");
                     return;
                 }
 
@@ -449,20 +453,24 @@ private async Task HandleFailureAsync(HttpResponseMessage response, Envelope env
                     persistLargeEnvelopePathEnvVar,
                     destinationDirectory);
 
+                if (_options.DisableFileWrite)
+                {
+                    _options.LogInfo("File write has been disabled via the options. Skipping persisting envelope.");
+                    return;
+                }
+
                 var destination = Path.Combine(destinationDirectory, "envelope_too_large",
                     (eventId ?? SentryId.Create()).ToString());
 
-                var createDirectoryResult = _options.FileSystem.CreateDirectory(Path.GetDirectoryName(destination)!);
-                if (createDirectoryResult is not FileOperationResult.Success)
+                if (!_options.FileSystem.CreateDirectory(Path.GetDirectoryName(destination)!))
                 {
-                    _options.LogError("Failed to create directory to store the envelope: {0}", createDirectoryResult);
+                    _options.LogError("Failed to create directory to store the envelope.");
                     return;
                 }
 
-                var result = _options.FileSystem.CreateFileForWriting(destination, out var envelopeFile);
-                if (result is not FileOperationResult.Success)
+                if (!_options.FileSystem.CreateFileForWriting(destination, out var envelopeFile))
                 {
-                    _options.LogError("Failed to create envelope file: {0}", result);
+                    _options.LogError("Failed to create envelope file.");
                     return;
                 }
 
diff --git a/src/Sentry/ISentryJsonSerializable.cs b/src/Sentry/ISentryJsonSerializable.cs
@@ -17,21 +17,3 @@ public interface ISentryJsonSerializable
     /// </remarks>
     void WriteTo(Utf8JsonWriter writer, IDiagnosticLogger? logger);
 }
-
-internal static class JsonSerializableExtensions
-{
-    public static void WriteToFile(this ISentryJsonSerializable serializable, IFileSystem fileSystem, string filePath, IDiagnosticLogger? logger)
-    {
-        var result = fileSystem.CreateFileForWriting(filePath, out var file);
-        if (result is not FileOperationResult.Success)
-        {
-            return;
-        }
-
-        using var writer = new Utf8JsonWriter(file);
-
-        serializable.WriteTo(writer, logger);
-        writer.Flush();
-        file.Dispose();
-    }
-}
diff --git a/src/Sentry/Internal/FileSystemBase.cs b/src/Sentry/Internal/FileSystemBase.cs
@@ -20,10 +20,10 @@ public IEnumerable<string> EnumerateFiles(string path, string searchPattern, Sea
 
     public Stream OpenFileForReading(string path) => File.OpenRead(path);
 
-    public abstract FileOperationResult CreateDirectory(string path);
-    public abstract FileOperationResult DeleteDirectory(string path, bool recursive = false);
-    public abstract FileOperationResult CreateFileForWriting(string path, out Stream fileStream);
-    public abstract FileOperationResult WriteAllTextToFile(string path, string contents);
-    public abstract FileOperationResult MoveFile(string sourceFileName, string destFileName, bool overwrite = false);
-    public abstract FileOperationResult DeleteFile(string path);
+    public abstract bool CreateDirectory(string path);
+    public abstract bool DeleteDirectory(string path, bool recursive = false);
+    public abstract bool CreateFileForWriting(string path, out Stream fileStream);
+    public abstract bool WriteAllTextToFile(string path, string contents);
+    public abstract bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false);
+    public abstract bool DeleteFile(string path);
 }
diff --git a/src/Sentry/Internal/Http/CachingTransport.cs b/src/Sentry/Internal/Http/CachingTransport.cs
@@ -78,6 +78,10 @@ private CachingTransport(ITransport innerTransport, SentryOptions options, bool
             options.TryGetProcessSpecificCacheDirectoryPath() ??
             throw new InvalidOperationException("Cache directory or DSN is not set.");
 
+        // Sanity check: This should never happen in the first place.
+        // We check for `DisableFileWrite` before creating the CachingTransport.
+        Debug.Assert(!_options.DisableFileWrite);
+
         _processingDirectoryPath = Path.Combine(_isolatedCacheDirectoryPath, ProcessingFolder);
     }
 
@@ -451,8 +455,7 @@ private async Task StoreToCacheAsync(
 
         EnsureFreeSpaceInCache();
 
-        var result = _options.FileSystem.CreateFileForWriting(envelopeFilePath, out var stream);
-        if (result is not FileOperationResult.Success)
+        if (!_options.FileSystem.CreateFileForWriting(envelopeFilePath, out var stream))
         {
             _options.LogDebug("Failed to store to cache.");
             return;
diff --git a/src/Sentry/Internal/IFileSystem.cs b/src/Sentry/Internal/IFileSystem.cs
@@ -1,14 +1,12 @@
 namespace Sentry.Internal;
 
-internal enum FileOperationResult
-{
-    Success,
-    Failure,
-    Disabled
-}
-
 internal interface IFileSystem
 {
+    // Note: You are responsible for handling success/failure when attempting to write to disk.
+    // You are required to check for `Options.FileWriteDisabled` whether you are allowed to call any writing operations.
+    // The options will automatically pick between `ReadOnly` and `ReadAndWrite` to prevent accidental file writing that
+    // could cause crashes on restricted platforms like the Nintendo Switch.
+
     // Note: This is not comprehensive.  If you need other filesystem methods, add to this interface,
     // then implement in both Sentry.Internal.FileSystem and Sentry.Testing.FakeFileSystem.
 
@@ -21,10 +19,10 @@ internal interface IFileSystem
     string? ReadAllTextFromFile(string file);
     Stream OpenFileForReading(string path);
 
-    FileOperationResult CreateDirectory(string path);
-    FileOperationResult DeleteDirectory(string path, bool recursive = false);
-    FileOperationResult CreateFileForWriting(string path, out Stream fileStream);
-    FileOperationResult WriteAllTextToFile(string path, string contents);
-    FileOperationResult MoveFile(string sourceFileName, string destFileName, bool overwrite = false);
-    FileOperationResult DeleteFile(string path);
+    bool CreateDirectory(string path);
+    bool DeleteDirectory(string path, bool recursive = false);
+    bool CreateFileForWriting(string path, out Stream fileStream);
+    bool WriteAllTextToFile(string path, string contents);
+    bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false);
+    bool DeleteFile(string path);
 }
diff --git a/src/Sentry/Internal/InstallationIdHelper.cs b/src/Sentry/Internal/InstallationIdHelper.cs
@@ -60,7 +60,7 @@ internal class InstallationIdHelper(SentryOptions options)
             var directoryPath = Path.Combine(rootPath, "Sentry", options.Dsn!.GetHashString());
             var fileSystem = options.FileSystem;
 
-            if (fileSystem.CreateDirectory(directoryPath) is not FileOperationResult.Success)
+            if (!fileSystem.CreateDirectory(directoryPath))
             {
                 options.LogDebug("Failed to create a directory for installation ID file ({0}).", directoryPath);
                 return null;
@@ -79,7 +79,7 @@ internal class InstallationIdHelper(SentryOptions options)
 
             // Generate new installation ID and store it in a file
             var id = Guid.NewGuid().ToString();
-            if (fileSystem.WriteAllTextToFile(filePath, id) is not FileOperationResult.Success)
+            if (!fileSystem.WriteAllTextToFile(filePath, id))
             {
                 options.LogDebug("Failed to write Installation ID to file ({0}).", filePath);
                 return null;
diff --git a/src/Sentry/Internal/ReadOnlyFilesystem.cs b/src/Sentry/Internal/ReadOnlyFilesystem.cs
@@ -2,20 +2,24 @@ namespace Sentry.Internal;
 
 internal class ReadOnlyFileSystem : FileSystemBase
 {
-    public override FileOperationResult CreateDirectory(string path) => FileOperationResult.Disabled;
+    // Note: You are responsible for handling success/failure when attempting to write to disk.
+    // You are required to check for `Options.FileWriteDisabled` whether you are allowed to call any writing operations.
+    // The options will automatically pick between `ReadOnly` and `ReadAndWrite` to prevent accidental file writing that
+    // could cause crashes on restricted platforms like the Nintendo Switch.
 
-    public override FileOperationResult DeleteDirectory(string path, bool recursive = false) => FileOperationResult.Disabled;
+    public override bool CreateDirectory(string path) => false;
 
-    public override FileOperationResult CreateFileForWriting(string path, out Stream fileStream)
+    public override bool DeleteDirectory(string path, bool recursive = false) => false;
+
+    public override bool CreateFileForWriting(string path, out Stream fileStream)
     {
         fileStream = Stream.Null;
-        return FileOperationResult.Disabled;
+        return false;
     }
 
-    public override FileOperationResult WriteAllTextToFile(string path, string contents) => FileOperationResult.Disabled;
+    public override bool WriteAllTextToFile(string path, string contents) => false;
 
-    public override FileOperationResult MoveFile(string sourceFileName, string destFileName, bool overwrite = false) =>
-        FileOperationResult.Disabled;
+    public override bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false) => false;
 
-    public override FileOperationResult DeleteFile(string path) => FileOperationResult.Disabled;
+    public override bool DeleteFile(string path) => false;
 }
diff --git a/src/Sentry/Internal/ReadWriteFileSystem.cs b/src/Sentry/Internal/ReadWriteFileSystem.cs
@@ -2,31 +2,36 @@ namespace Sentry.Internal;
 
 internal class ReadWriteFileSystem : FileSystemBase
 {
-    public override FileOperationResult CreateDirectory(string path)
+    // Note: You are responsible for handling success/failure when attempting to write to disk.
+    // You are required to check for `Options.FileWriteDisabled` whether you are allowed to call any writing operations.
+    // The options will automatically pick between `ReadOnly` and `ReadAndWrite` to prevent accidental file writing that
+    // could cause crashes on restricted platforms like the Nintendo Switch.
+
+    public override bool CreateDirectory(string path)
     {
         Directory.CreateDirectory(path);
-        return DirectoryExists(path) ? FileOperationResult.Success : FileOperationResult.Failure;
+        return DirectoryExists(path);
     }
 
-    public override FileOperationResult DeleteDirectory(string path, bool recursive = false)
+    public override bool DeleteDirectory(string path, bool recursive = false)
     {
         Directory.Delete(path, recursive);
-        return Directory.Exists(path) ? FileOperationResult.Failure : FileOperationResult.Success;
+        return !Directory.Exists(path);
     }
 
-    public override FileOperationResult CreateFileForWriting(string path, out Stream fileStream)
+    public override bool CreateFileForWriting(string path, out Stream fileStream)
     {
         fileStream = File.Create(path);
-        return FileOperationResult.Success;
+        return true;
     }
 
-    public override FileOperationResult WriteAllTextToFile(string path, string contents)
+    public override bool WriteAllTextToFile(string path, string contents)
     {
         File.WriteAllText(path, contents);
-        return File.Exists(path) ? FileOperationResult.Success : FileOperationResult.Failure;
+        return File.Exists(path);
     }
 
-    public override FileOperationResult MoveFile(string sourceFileName, string destFileName, bool overwrite = false)
+    public override bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false)
     {
 #if NETCOREAPP3_0_OR_GREATER
         File.Move(sourceFileName, destFileName, overwrite);
@@ -44,15 +49,15 @@ public override FileOperationResult MoveFile(string sourceFileName, string destF
 
         if (File.Exists(sourceFileName) || !File.Exists(destFileName))
         {
-            return FileOperationResult.Failure;
+            return false;
         }
 
-        return FileOperationResult.Success;
+        return true;
     }
 
-    public override FileOperationResult DeleteFile(string path)
+    public override bool DeleteFile(string path)
     {
         File.Delete(path);
-        return File.Exists(path) ? FileOperationResult.Failure : FileOperationResult.Success;
+        return !File.Exists(path);
     }
 }
diff --git a/src/Sentry/Internal/SdkComposer.cs b/src/Sentry/Internal/SdkComposer.cs
@@ -32,7 +32,7 @@ private ITransport CreateTransport()
 
             if (_options.DisableFileWrite)
             {
-                _options.LogInfo("File writing is disabled, Skipping caching transport creation.");
+                _options.LogInfo("File write has been disabled via the options. Skipping caching transport creation.");
             }
             else
             {
diff --git a/test/Sentry.Testing/FakeFileSystem.cs b/test/Sentry.Testing/FakeFileSystem.cs
diff --git a/test/Sentry.Tests/ReadOnlyFileSystemTests.cs b/test/Sentry.Tests/ReadOnlyFileSystemTests.cs
diff --git a/test/Sentry.Tests/ReadWriteFileSystemTests.cs b/test/Sentry.Tests/ReadWriteFileSystemTests.cs

Original file line number	Diff line number	Diff line change
`@@ -2,31 +2,36 @@ namespace Sentry.Internal;`
`2`	`2`
`3`	`3`	`internal class ReadWriteFileSystem : FileSystemBase`
`4`	`4`	`{`
`5`		`- public override FileOperationResult CreateDirectory(string path)`
	`5`	`+ // Note: You are responsible for handling success/failure when attempting to write to disk.`
	`6`	+ // You are required to check for `Options.FileWriteDisabled` whether you are allowed to call any writing operations.
	`7`	+ // The options will automatically pick between `ReadOnly` and `ReadAndWrite` to prevent accidental file writing that
	`8`	`+ // could cause crashes on restricted platforms like the Nintendo Switch.`
	`9`	`+`
	`10`	`+ public override bool CreateDirectory(string path)`
`6`	`11`	`{`
`7`	`12`	`Directory.CreateDirectory(path);`
`8`		`- return DirectoryExists(path) ? FileOperationResult.Success : FileOperationResult.Failure;`
	`13`	`+ return DirectoryExists(path);`
`9`	`14`	`}`
`10`	`15`
`11`		`- public override FileOperationResult DeleteDirectory(string path, bool recursive = false)`
	`16`	`+ public override bool DeleteDirectory(string path, bool recursive = false)`
`12`	`17`	`{`
`13`	`18`	`Directory.Delete(path, recursive);`
`14`		`- return Directory.Exists(path) ? FileOperationResult.Failure : FileOperationResult.Success;`
	`19`	`+ return !Directory.Exists(path);`
`15`	`20`	`}`
`16`	`21`
`17`		`- public override FileOperationResult CreateFileForWriting(string path, out Stream fileStream)`
	`22`	`+ public override bool CreateFileForWriting(string path, out Stream fileStream)`
`18`	`23`	`{`
`19`	`24`	`fileStream = File.Create(path);`
`20`		`- return FileOperationResult.Success;`
	`25`	`+ return true;`
`21`	`26`	`}`
`22`	`27`
`23`		`- public override FileOperationResult WriteAllTextToFile(string path, string contents)`
	`28`	`+ public override bool WriteAllTextToFile(string path, string contents)`
`24`	`29`	`{`
`25`	`30`	`File.WriteAllText(path, contents);`
`26`		`- return File.Exists(path) ? FileOperationResult.Success : FileOperationResult.Failure;`
	`31`	`+ return File.Exists(path);`
`27`	`32`	`}`
`28`	`33`
`29`		`- public override FileOperationResult MoveFile(string sourceFileName, string destFileName, bool overwrite = false)`
	`34`	`+ public override bool MoveFile(string sourceFileName, string destFileName, bool overwrite = false)`
`30`	`35`	`{`
`31`	`36`	`#if NETCOREAPP3_0_OR_GREATER`
`32`	`37`	`File.Move(sourceFileName, destFileName, overwrite);`
`@@ -44,15 +49,15 @@ public override FileOperationResult MoveFile(string sourceFileName, string destF`
`44`	`49`
`45`	`50`	`if (File.Exists(sourceFileName) \|\| !File.Exists(destFileName))`
`46`	`51`	`{`
`47`		`- return FileOperationResult.Failure;`
	`52`	`+ return false;`
`48`	`53`	`}`
`49`	`54`
`50`		`- return FileOperationResult.Success;`
	`55`	`+ return true;`
`51`	`56`	`}`
`52`	`57`
`53`		`- public override FileOperationResult DeleteFile(string path)`
	`58`	`+ public override bool DeleteFile(string path)`
`54`	`59`	`{`
`55`	`60`	`File.Delete(path);`
`56`		`- return File.Exists(path) ? FileOperationResult.Failure : FileOperationResult.Success;`
	`61`	`+ return !File.Exists(path);`
`57`	`62`	`}`
`58`	`63`	`}`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ private ITransport CreateTransport()`
`32`	`32`
`33`	`33`	`if (_options.DisableFileWrite)`
`34`	`34`	`{`
`35`		`- _options.LogInfo("File writing is disabled, Skipping caching transport creation.");`
	`35`	`+ _options.LogInfo("File write has been disabled via the options. Skipping caching transport creation.");`
`36`	`36`	`}`
`37`	`37`	`else`
`38`	`38`	`{`