Plugin options system improvements (#411)

* Plugin options system improvements

* Docstrings

Author: mslukebo

src/Microsoft.Performance.SDK.Runtime.Tests/Options/PluginOptionsSystemTests.cs

@@ -8,7 +8,9 @@
 using Microsoft.Performance.SDK.Options;
 using Microsoft.Performance.SDK.Processing;
 using Microsoft.Performance.SDK.Runtime.Options;
+using Microsoft.Performance.SDK.Runtime.Options.Serialization;
 using Microsoft.Performance.SDK.Runtime.Options.Serialization.DTO;
+using Microsoft.Performance.SDK.Runtime.Options.Serialization.Loading;
 using Microsoft.Performance.SDK.Tests.Options;
 using Microsoft.Performance.Testing;
 using Microsoft.Performance.Testing.SDK;
@@ -111,6 +113,18 @@ await sut.Saver.TrySaveAsync(new PluginOptionsDto()
         Assert.AreEqual(expectedValue, newDto.BooleanOptions.First(o => o.Guid == optionGuid).Value);
     }
 
+    [TestMethod]
+    public async Task TrySaveCurrentRegistry_LoaderFails_FailsToSave()
+    {
+        var sut = new PluginOptionsSystem(
+            new NullPluginOptionsLoader(),
+            new InMemoryPluginOptionsDtoRepository(),
+            new PluginOptionsRegistry(Logger.Null));
+
+        sut.RegisterOptionsFrom(new StubProcessingSource(TestPluginOption.FieldOption("foo")));
+        Assert.IsFalse(await sut.TrySaveCurrentRegistry());
+    }
+
     private PluginOptionsSystem CreateSut(params IProcessingSource[] processingSources)
     {
         var sut = PluginOptionsSystem.CreateUnsaved((_) => new NullLogger());

src/Microsoft.Performance.SDK.Runtime.Tests/Options/StreamPluginOptionsLoaderTests.cs

@@ -14,7 +14,7 @@
 namespace Microsoft.Performance.SDK.Runtime.Tests.Options;
 
 /// <summary>
-///     Tests for the <see cref="StreamPluginOptionsLoader"/>.
+///     Tests for the <see cref="StreamPluginOptionsLoader{T}"/>.
 /// </summary>
 [TestClass]
 [UnitTest]
@@ -53,7 +53,7 @@ public async Task LoadedDto_ContainsCorrectValues()
 
     /// <summary>
     ///     Asserts that the <see cref="Stream"/> used to load the <see cref="PluginOptionsDto"/> is closed after loading
-    ///     if <see cref="StreamPluginOptionsLoader"/> is configured to do so.
+    ///     if <see cref="StreamPluginOptionsLoader{T}"/> is configured to do so.
     /// </summary>
     [TestMethod]
     public async Task CloseStreamOnRead_ClosesStream()
@@ -71,7 +71,7 @@ public async Task CloseStreamOnRead_ClosesStream()
 
     /// <summary>
     ///     Asserts that the <see cref="Stream"/> used to load the <see cref="PluginOptionsDto"/> is not closed after loading
-    ///     if <see cref="StreamPluginOptionsLoader"/> is not configured to do so.
+    ///     if <see cref="StreamPluginOptionsLoader{T}"/> is not configured to do so.
     /// </summary>
     [TestMethod]
     public async Task DoNotCloseStreamOnRead_DoesNotCloseStream()
@@ -87,20 +87,37 @@ public async Task DoNotCloseStreamOnRead_DoesNotCloseStream()
         Assert.IsTrue(stream.CanWrite);
     }
 
+    [TestMethod]
+    public async Task EmptyStream_ReturnsEmptyDto()
+    {
+        using MemoryStream stream = new MemoryStream();
+        var sut = new TestStreamPluginOptionsLoader(stream, false);
+
+        var loadedDto = await sut.TryLoadAsync();
+
+        Assert.IsNotNull(loadedDto);
+        Assert.IsTrue(loadedDto.IsEmpty());
+    }
+
     private sealed class TestStreamPluginOptionsLoader
-        : StreamPluginOptionsLoader
+        : StreamPluginOptionsLoader<MemoryStream>
     {
-        private readonly Stream stream;
+        private readonly MemoryStream memoryStream;
 
-        public TestStreamPluginOptionsLoader(MemoryStream stream, bool closeStreamOnRead)
+        public TestStreamPluginOptionsLoader(MemoryStream memoryStream, bool closeStreamOnRead)
             : base(closeStreamOnRead, Logger.Null)
         {
-            this.stream = stream;
+            this.memoryStream = memoryStream;
+        }
+
+        protected override MemoryStream GetStream()
+        {
+            return this.memoryStream;
         }
 
-        protected override Stream GetStream()
+        protected override bool HasContent(MemoryStream stream)
         {
-            return this.stream;
+            return stream.Length > 0;
         }
     }
 }

src/Microsoft.Performance.SDK.Runtime/Options/PluginOptionsSystem.cs

@@ -154,6 +154,8 @@ public async Task<bool> TryLoadAsync()
     ///     the <see cref="Saver"/>. Previously saved options, as loaded by the <see cref="Loader"/> during this method call,
     ///     will still be included in the saved options. If an option in the <see cref="Registry"/> was previously saved,
     ///     its value will be updated after the save.
+    ///     If <see cref="Loader"/> fails to load options, the save will fail and this method will have
+    ///     no effect.
     /// </summary>
     /// <returns>
     ///     <c>true</c> if the <see cref="Registry"/> was saved; <c>false</c> otherwise.
@@ -166,6 +168,12 @@ public async Task<bool> TrySaveCurrentRegistry()
     {
         var newDto = optionsRegistryToDtoConverter.ConvertToDto(this.Registry);
         var oldDto = await this.Loader.TryLoadAsync();
+
+        if (oldDto == null)
+        {
+            return false;
+        }
+
         return await this.Saver.TrySaveAsync(oldDto.UpdateTo(newDto));
     }
 }

src/Microsoft.Performance.SDK.Runtime/Options/Serialization/DTO/PluginOptionsDto.cs

@@ -27,6 +27,13 @@ public sealed class PluginOptionsDto
     /// </summary>
     public IReadOnlyCollection<FieldArrayPluginOptionDto> FieldArrayOptions { get; init; } = new List<FieldArrayPluginOptionDto>();
 
+    public bool IsEmpty()
+    {
+        return !this.BooleanOptions.Any() &&
+               !this.FieldOptions.Any() &&
+               !this.FieldArrayOptions.Any();
+    }
+
     /// <summary>
     ///     Merges the values from the given <see cref="PluginOptionsDto"/> into this one, returning the new instance.
     ///     DTOs in <paramref name="newDto"/> that have the same <see cref="PluginOptionDto.Guid"/> as a DTO in this

src/Microsoft.Performance.SDK.Runtime/Options/Serialization/Loading/FilePluginOptionsLoader.cs

@@ -12,10 +12,8 @@ namespace Microsoft.Performance.SDK.Runtime.Options.Serialization.Loading;
 ///     A <see cref="IPluginOptionsLoader"/> that can load a <see cref="PluginOptionsDto"/> instance from a file.
 /// </summary>
 public sealed class FilePluginOptionsLoader
-    : StreamPluginOptionsLoader
+    : StreamPluginOptionsLoader<FileStream>
 {
-    private readonly string filePath;
-
     /// <summary>
     ///     Initializes a new instance of the <see cref="FilePluginOptionsLoader"/> class.
     /// </summary>
@@ -37,17 +35,28 @@ public FilePluginOptionsLoader(string filePath, ILogger logger)
         Guard.NotNullOrWhiteSpace(filePath, nameof(filePath));
         Guard.NotNull(logger, nameof(logger));
 
-        this.filePath = filePath;
+        this.FilePath = filePath;
     }
 
+    /// <summary>
+    ///     Gets the path to the file from which to load options.
+    /// </summary>
+    public string FilePath { get; }
+
     private protected override string GetDeserializeErrorMessage(Exception exception)
     {
-        return $"Failed to load plugin options from '{this.filePath}': {exception.Message}.";
+        return $"Failed to load plugin options from '{this.FilePath}': {exception.Message}.";
+    }
+
+    /// <inheritdoc />
+    protected override FileStream GetStream()
+    {
+        return File.Open(this.FilePath, FileMode.OpenOrCreate, FileAccess.Read, FileShare.Read);
     }
 
     /// <inheritdoc />
-    protected override Stream GetStream()
+    protected override bool HasContent(FileStream stream)
     {
-        return File.Open(this.filePath, FileMode.OpenOrCreate, FileAccess.Read, FileShare.Read);
+        return stream.Length > 0;
     }
 }

src/Microsoft.Performance.SDK.Runtime/Options/Serialization/Loading/StreamPluginOptionsLoader.cs

@@ -13,14 +13,18 @@ namespace Microsoft.Performance.SDK.Runtime.Options.Serialization.Loading;
 /// <summary>
 ///     Base class for <see cref="IPluginOptionsLoader"/> classes which load from a stream.
 /// </summary>
-public abstract class StreamPluginOptionsLoader
+/// <typeparam name="T">
+///     The type of the stream from which to load.
+/// </typeparam>
+public abstract class StreamPluginOptionsLoader<T>
     : IPluginOptionsLoader
+    where T : Stream
 {
     private readonly bool closeStreamOnRead;
     private readonly ILogger logger;
 
     /// <summary>
-    ///     Initializes a new instance of the <see cref="StreamPluginOptionsLoader"/> class.
+    ///     Initializes a new instance of the <see cref="StreamPluginOptionsLoader{T}"/> class.
     /// </summary>
     /// <param name="closeStreamOnRead">
     ///     Whether to dispose of the stream returned by <see cref="GetStream"/> at the end of a call to
@@ -45,6 +49,11 @@ public async Task<PluginOptionsDto> TryLoadAsync()
         var stream = GetStream();
         try
         {
+            if (!HasContent(stream))
+            {
+                return new PluginOptionsDto();
+            }
+
             return await JsonSerializer.DeserializeAsync<PluginOptionsDto>(stream, jsonSerializerOptions);
         }
         catch (Exception e)
@@ -72,5 +81,16 @@ private protected virtual string GetDeserializeErrorMessage(Exception exception)
     /// <returns>
     ///     The <see cref="Stream"/> from which to load the <see cref="PluginOptionsDto"/>.
     /// </returns>
-    protected abstract Stream GetStream();
+    protected abstract T GetStream();
+
+    /// <summary>
+    ///     Determines whether <paramref name="stream"/> has content.
+    /// </summary>
+    /// <param name="stream">
+    ///     The <see cref="Stream"/> of type <typeparamref name="T"/> to check.
+    /// </param>
+    /// <returns>
+    ///     Whether <paramref name="stream"/> has content.
+    /// </returns>
+    protected abstract bool HasContent(T stream);
 }

src/Microsoft.Performance.SDK.Runtime/Options/Serialization/Saving/FilePluginOptionsSaver.cs

@@ -14,8 +14,6 @@ namespace Microsoft.Performance.SDK.Runtime.Options.Serialization.Saving;
 public sealed class FilePluginOptionsSaver
     : StreamPluginOptionsSaver
 {
-    private readonly string filePath;
-
     /// <summary>
     ///     Initializes a new instance of the <see cref="FilePluginOptionsSaver"/> class.
     /// </summary>
@@ -37,17 +35,22 @@ public FilePluginOptionsSaver(string filePath, ILogger logger)
         Guard.NotNullOrWhiteSpace(filePath, nameof(filePath));
         Guard.NotNull(logger, nameof(logger));
 
-        this.filePath = filePath;
+        this.FilePath = filePath;
     }
 
+    /// <summary>
+    ///     Gets the path to the file to which options will be saved.
+    /// </summary>
+    public string FilePath { get; }
+
     private protected override string GetSerializeErrorMessage(Exception exception)
     {
-        return $"Failed to save plugin options to '{this.filePath}': {exception.Message}.";
+        return $"Failed to save plugin options to '{this.FilePath}': {exception.Message}.";
     }
 
     /// <inheritdoc />
     protected override Stream GetStream()
     {
-        return File.Open(this.filePath, FileMode.Create, FileAccess.Write, FileShare.None);
+        return File.Open(this.FilePath, FileMode.Create, FileAccess.Write, FileShare.None);
     }
 }