fix: Simplified IFileSystem
#3641
Conversation
|
|
||
| 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(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is only used once in the GlobalSessionsManager. Success/Failure is getting handled there and does not need to be abstracted away into an extension.
|
Follow up to: |
| 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. |
There was a problem hiding this comment.
This is a good call out. Adding a sexton like or (can't recall the options) above methods in the interface with a snippet might make it even more obvious
| @@ -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. | |||
There was a problem hiding this comment.
this won't show up in the IDE at call site. Needs
There was a problem hiding this comment.
These interfaces and classes are all internal, so it looks like the only place to document this is in some remarks or something on SentryOptions.DisableFileWrite.
@bitsandfoxes can you provide a bit more context around when/why that's not possible? |
Because the caller typically would like to log whether the failed write attempt failed because of a failure or because writing has been disabled. So I ended up with constructs like this all over the place. So I could not escape checking for |
TLDR;
The responsibility to check whether a system is allowed to write to disk or not falls onto the system itself. The
ReadOnlyFileSystemis only there to prevent regressions and crashes on platforms like the Switch.Any attempts to write to disk and fail can now simply logged as
Errorbecause that's what they are.Context
The idea was to have
IFileSystembe the abstraction layer and be swapped betweenReadOnlyandReadWriteand have it be clever enough to log whether the operation failed or was disallowed.Having it smart enough is not really feasable. The caller is still interested whether it failed or was disallowed for logging purposes. Having the caller simply attempt to write to disk and then not care whether it was allowed to do so is not possible.
So we end up checking the success/failure/disallowed with every call. That is noisy. So we're checking whether we're allowed to write to disk before even attempting. This making the
FileOperationResultpointless. We can just log and skip when we know we can't write to disk in the first place.