Improve FileSettingsStore exception handling

Author: menees

src/Directory.Build.props

@@ -27,7 +27,7 @@
 		<_SkipUpgradeNetAnalyzersNuGetWarning>true</_SkipUpgradeNetAnalyzersNuGetWarning>
 
 		<!-- Make the assembly, file, and NuGet package versions the same. -->
-		<Version>5.1.1</Version>
+		<Version>5.1.2</Version>
 	</PropertyGroup>
 
 	<PropertyGroup Condition="'$(Configuration)'=='Debug'">

src/Menees.Common/Exceptions.cs

@@ -5,7 +5,9 @@ namespace Menees
 	using System;
 	using System.Collections.Generic;
 	using System.Diagnostics;
+	using System.IO;
 	using System.Linq;
+	using System.Security;
 	using System.Text;
 	using Menees.Diagnostics;
 
@@ -228,6 +230,17 @@ public static string GetMessage(Exception ex)
 			return result;
 		}
 
+		/// <summary>
+		/// Gets whether an exception is an <see cref="IOException"/>, <see cref="SecurityException"/>,
+		/// or <see cref="UnauthorizedAccessException"/>.
+		/// </summary>
+		/// <param name="ex">The exception to check.</param>
+		public static bool IsAccessException(Exception ex)
+		{
+			bool result = ex != null && (ex is IOException || ex is UnauthorizedAccessException || ex is SecurityException);
+			return result;
+		}
+
 		#endregion
 
 		#region Private Methods

src/Menees.Common/FileSettingsStore.cs

@@ -10,6 +10,7 @@ namespace Menees
 	using System.Linq;
 	using System.Security;
 	using System.Text;
+	using System.Xml;
 	using System.Xml.Linq;
 
 	#endregion
@@ -30,11 +31,30 @@ public FileSettingsStore()
 			{
 				// Make sure the file exists and isn't zero length.  If a process was killed while saving data,
 				// then the settings store file may be empty.  If so, we'll pretend it isn't there.
-				FileInfo file = new(fileName);
-				if (file.Exists && file.Length > 0)
+				FileInfo fileInfo = new(fileName);
+				if (fileInfo.Exists && fileInfo.Length > 0)
 				{
-					this.root = XElement.Load(fileName);
-					break;
+					try
+					{
+						// Only read settings from a file that we have *write* access to so if we're running from a remote read-only file share,
+						// then it will load a local saved file (e.g., from AppData) instead of loading one from the remote read-only share.
+						using (FileStream stream = new(fileName, FileMode.Open, FileAccess.ReadWrite, FileShare.Read))
+						{
+							this.root = XElement.Load(stream);
+						}
+
+						break;
+					}
+					catch (XmlException ex)
+					{
+						Log.Warning(typeof(FileSettingsStore), $"Unable to load the user settings from {fileName}.", ex);
+					}
+					catch (Exception ex) when (Exceptions.IsAccessException(ex))
+					{
+						// Ignore the file if we don't have both read and write access to it. If we're running off a read-only share,
+						// we know Save() can't write back to that location, so we shouldn't read in from that location on startup.
+						Log.Debug(typeof(FileSettingsStore), $"Unable to get read/write access to {fileName}.", ex);
+					}
 				}
 			}
 
@@ -85,28 +105,15 @@ public void Save()
 					File.SetAttributes(fileName, File.GetAttributes(fileName) | FileAttributes.Hidden);
 					break;
 				}
-				catch (IOException inputOutputEx)
-				{
-					errorLogProperties.Add(fileName, inputOutputEx);
-				}
-				catch (SecurityException securityEx)
+				catch (Exception ex) when (Exceptions.IsAccessException(ex))
 				{
-					errorLogProperties.Add(fileName, securityEx);
-				}
-				catch (UnauthorizedAccessException accessEx)
-				{
-					errorLogProperties.Add(fileName, accessEx);
+					errorLogProperties.Add(fileName, ex);
 				}
 			}
 
 			if (!saved)
 			{
-				const string Message = "Unable to save the user settings to a file store.";
-
-				// Log out the exceptions first since we can't pass those additional
-				// details into the new exception we're throwing.
-				Log.Error(typeof(FileSettingsStore), Message, null, errorLogProperties);
-				throw Exceptions.NewInvalidOperationException(Message);
+				throw Exceptions.NewInvalidOperationException("Unable to save the user settings to a file store.", errorLogProperties);
 			}
 		}
 
@@ -126,7 +133,10 @@ private static IEnumerable<string> GetPotentialFileNames()
 			List<string> result = new();
 
 			// First, try the application's base directory.  That gives the best isolation for side-by-side app usage,
-			// but a non-admin user may not have permissions to write to this directory.
+			// but a non-admin user may not have permissions to write to this directory.  Side-by-side isolation is
+			// important if old and new versions of the software are run on the same machine.  (New versions will
+			// typically be able to read in old settings, but they'll only write out settings in the new format.  After
+			// that "upgrade", the old version wouldn't be able to read the settings in the new format.)
 			string path = Path.Combine(ApplicationInfo.BaseDirectory, fileName);
 			result.Add(path);
 

tests/Menees.Common.Tests/DialogFilterItemTest.cs

@@ -12,15 +12,10 @@ public class DialogFilterItemTest
 		[TestMethod]
 		public void ConstructorTest()
 		{
-			DialogFilterItem item = new("cs");
-			item.ItemName.ShouldContain("C# Source Files", Case.Insensitive);
-			item.Masks.SequenceEqual(new[] { "*.cs" }).ShouldBeTrue();
-			item.ToString().ShouldContain("C# Source Files (*.cs)|*.cs", Case.Insensitive);
-
-			item = new DialogFilterItem("vb", false);
-			item.ItemName.ShouldBe("Visual Basic Source File");
-			item.Masks.SequenceEqual(new[] { "*.vb" }).ShouldBeTrue();
-			item.ToString().ShouldBe("Visual Basic Source File (*.vb)|*.vb");
+			DialogFilterItem item = new DialogFilterItem("exe", false);
+			item.ItemName.ShouldBe("Application", StringCompareShould.IgnoreCase);
+			item.Masks.SequenceEqual(new[] { "*.exe" }).ShouldBeTrue();
+			item.ToString().ShouldBe("Application (*.exe)|*.exe", StringCompareShould.IgnoreCase);
 
 			item = new DialogFilterItem("Text Files", "txt");
 			item.ItemName.ShouldBe("Text Files");

tests/Menees.Common.Tests/ExceptionsTest.cs

@@ -7,6 +7,8 @@
 using Shouldly;
 using System.Diagnostics;
 using System.Collections.Generic;
+using System.IO;
+using System.Security;
 
 namespace Menees.Common.Tests
 {
@@ -124,6 +126,19 @@ public void GetMessageTest()
 			Trace.Write(output);
 		}
 
+		[TestMethod]
+		public void IsAccessExceptionTest()
+		{
+			Exceptions.IsAccessException(new IOException()).ShouldBeTrue();
+			Exceptions.IsAccessException(new SecurityException()).ShouldBeTrue();
+			Exceptions.IsAccessException(new UnauthorizedAccessException()).ShouldBeTrue();
+			Exceptions.IsAccessException(new PathTooLongException()).ShouldBeTrue();
+			Exceptions.IsAccessException(new FileNotFoundException()).ShouldBeTrue();
+
+			Exceptions.IsAccessException(new ArgumentException()).ShouldBeFalse();
+			Exceptions.IsAccessException(new InvalidOperationException()).ShouldBeFalse();
+		}
+
 		private static void CheckMessage(string actualMessage, string expectedMessage)
 		{
 			// .NET Core's AggregateException.Message includes the inner exceptions in the first line of the message.