address feedback and cross plat tests

src/Persistence.Tests/MsApp/MsappArchiveTests.cs

@@ -193,7 +193,7 @@ public void DoesEntryExistWorksWithNewEntriesCreated()
     }
 
     [TestMethod]
-    public void TryGenerateUniqueEntryPathTests()
+    public void GenerateUniqueEntryPathTests()
     {
         // Setup test archive with a couple entries in it already
         using var archiveMemStream = new MemoryStream();
@@ -204,33 +204,45 @@ public void TryGenerateUniqueEntryPathTests()
         archive.CreateEntry("dir1/entryD.pa.yaml");
 
         //
-        string? actualEntryPath;
-        archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("entryA1.pa.yaml");
-        archive.TryGenerateUniqueEntryPath(null, "entryC", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("entryC.pa.yaml");
-        archive.TryGenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("dir1\\entryA.pa.yaml");
-        archive.TryGenerateUniqueEntryPath("dir1", "entryC", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("dir1\\entryC1.pa.yaml");
+        archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA1.pa.yaml");
+        archive.GenerateUniqueEntryPath(null, "entryC", ".pa.yaml").Should().Be("entryC.pa.yaml");
+        archive.GenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml").Should().Be("dir1/entryA.pa.yaml");
+        archive.GenerateUniqueEntryPath("dir1", "entryC", ".pa.yaml").Should().Be("dir1/entryC1.pa.yaml");
 
         // Verify repeated calls will keep incrementing the suffix
-        archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("entryA1.pa.yaml");
+        var actualEntryPath = archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA1.pa.yaml").And.Subject;
         archive.CreateEntry(actualEntryPath!);
 
-        archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("entryA2.pa.yaml");
+        actualEntryPath = archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA2.pa.yaml").And.Subject;
         archive.CreateEntry(actualEntryPath!);
 
-        archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath).Should().BeTrue();
-        actualEntryPath.Should().Be("entryA3.pa.yaml");
+        actualEntryPath = archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA3.pa.yaml").And.Subject;
 
         // Verify when using a custom separator
-        archive.TryGenerateUniqueEntryPath(null, "entryA", ".pa.yaml", out actualEntryPath, uniqueSuffixSeparator: "_").Should().BeTrue();
-        actualEntryPath.Should().Be("entryA_1.pa.yaml");
-        archive.TryGenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml", out actualEntryPath, uniqueSuffixSeparator: "_").Should().BeTrue();
-        actualEntryPath.Should().Be("dir1\\entryA.pa.yaml");
+        archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml", uniqueSuffixSeparator: "_").Should().Be("entryA_1.pa.yaml");
+        archive.GenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml", uniqueSuffixSeparator: "_").Should().Be("dir1/entryA.pa.yaml");
+    }
+
+    [TestMethod]
+    public void GenerateUniqueEntryPathReturnsNormalizedPathsTests()
+    {
+        // Setup test archive with a couple entries in it already
+        using var archiveMemStream = new MemoryStream();
+        using var archive = new MsappArchive(archiveMemStream, ZipArchiveMode.Create, _mockYamlSerializationFactory.Object);
+        archive.CreateEntry("entryA.pa.yaml");
+        archive.CreateEntry("dir1/entryA.pa.yaml");
+        archive.CreateEntry("dir1/dir2/entryA.pa.yaml");
+
+        // when entry already unique
+        archive.GenerateUniqueEntryPath(null, "entryC", ".pa.yaml").Should().Be("entryC.pa.yaml");
+        archive.GenerateUniqueEntryPath(@"/dir1\", "entryC", ".pa.yaml").Should().Be("dir1/entryC.pa.yaml");
+        archive.GenerateUniqueEntryPath(@"  \dir1\dir2\", "entryC", ".pa.yaml").Should().Be("dir1/dir2/entryC.pa.yaml");
+
+        // when unique entry generated
+        archive.GenerateUniqueEntryPath(null, "entryA", ".pa.yaml").Should().Be("entryA1.pa.yaml");
+        archive.GenerateUniqueEntryPath("dir1", "entryA", ".pa.yaml").Should().Be("dir1/entryA1.pa.yaml");
+        archive.GenerateUniqueEntryPath(@"/dir1\", "entryA", ".pa.yaml").Should().Be("dir1/entryA1.pa.yaml");
+        archive.GenerateUniqueEntryPath(@"  \dir1\dir2\", "entryA", ".pa.yaml").Should().Be("dir1/dir2/entryA1.pa.yaml");
     }
 
     [TestMethod]

src/Persistence.Tests/Persistence.Tests.csproj

@@ -31,7 +31,7 @@
         <PackageReference Include="MSTest" Version="$(MSTest)" />
         <PackageReference Include="coverlet.collector" Version="$(CoverletCollectorVersion)" />
         <PackageReference Include="FluentAssertions" Version="$(FluentAssertionsVersion)" />
-        <PackageReference Include="Moq" Version="4.16.0" />
+        <PackageReference Include="Moq" Version="$(MoqVersion)" />
     </ItemGroup>
 
     <ItemGroup Label="Global usings">

src/Persistence/MsApp/IMsappArchive.cs

@@ -76,14 +76,13 @@ public interface IMsappArchive : IDisposable
     /// <param name="fileNameNoExtension">The name of the file, without an extension. This file name should already have been made 'safe' by the caller. If needed, use <see cref="MsappArchive.TryMakeSafeForEntryPathSegment"/> to make it safe.</param>
     /// <param name="extension">The file extension to add for the file path or null for no extension. Note: This should already contain only safe chars, as it is expected to not contain customer data.</param>
     /// <param name="uniqueSuffixSeparator">The string added just before the unique file number and extension. Default is empty string.</param>
-    /// <param name="entryPath">The entry path which is unique in the specified <paramref name="directory"/>.</param>
-    /// <returns>A value indicating whether a unique entry was able to be created. If true, then <paramref name="entryPath"/> will contain the unique path.</returns>
+    /// <returns>The entry path which is unique in the specified <paramref name="directory"/>.</returns>
     /// <exception cref="ArgumentException">directory is empty or whitespace.</exception>
-    bool TryGenerateUniqueEntryPath(
+    /// <exception cref="InvalidOperationException">A unique filename entry could not be generated.</exception>
+    string GenerateUniqueEntryPath(
         string? directory,
         string fileNameNoExtension,
         string? extension,
-        [NotNullWhen(true)] out string? entryPath,
         string uniqueSuffixSeparator = "");
 
     /// <summary>

src/Persistence/MsApp/MsappArchive.cs

@@ -33,6 +33,8 @@ public static class Directories
         public const string Resources = "Resources";
     }
 
+    private const char EntryPathSeparator = '/';
+
     public const string MsappFileExtension = ".msapp";
     public const string YamlFileExtension = ".yaml";
     public const string YamlPaFileExtension = ".pa.yaml";
@@ -361,19 +363,19 @@ public T Deserialize<T>(ZipArchiveEntry archiveEntry) where T : Control
     /// <returns></returns>
     public IEnumerable<ZipArchiveEntry> GetDirectoryEntries(string directoryName, string? extension = null, bool recursive = true)
     {
-        directoryName = NormalizePath(directoryName).TrimEnd('/');
+        directoryName = NormalizePath(directoryName).TrimEnd(EntryPathSeparator);
 
         foreach (var entry in CanonicalEntries)
         {
             // Do not return directories which some zip implementations include as entries
-            if (entry.Key.EndsWith('/'))
+            if (entry.Key.EndsWith(EntryPathSeparator))
                 continue;
 
-            if (directoryName != string.Empty && !entry.Key.StartsWith(directoryName + '/', StringComparison.InvariantCulture))
+            if (directoryName != string.Empty && !entry.Key.StartsWith(directoryName + EntryPathSeparator, StringComparison.InvariantCulture))
                 continue;
 
             // If not recursive, skip subdirectories
-            if (!recursive && entry.Key.IndexOf('/', directoryName.Length == 0 ? 0 : directoryName.Length + 1) > 0)
+            if (!recursive && entry.Key.IndexOf(EntryPathSeparator, directoryName.Length == 0 ? 0 : directoryName.Length + 1) > 0)
                 continue;
 
             if (extension != null && !entry.Key.EndsWith(extension, StringComparison.OrdinalIgnoreCase))
@@ -458,46 +460,47 @@ private string GetSafeEntryPath(string directory, string name, string extension)
         if (!TryMakeSafeForEntryPathSegment(name, out var safeName, unsafeCharReplacementText: ""))
             throw new ArgumentException("Control name is not valid.", nameof(name));
 
-        if (!TryGenerateUniqueEntryPath(directory.WhiteSpaceToNull(), safeName, extension, out var entryPath))
-        {
-            throw new InvalidOperationException("Failed to find a unique name for the control.");
-        }
-
-        return entryPath;
+        return GenerateUniqueEntryPath(directory.WhiteSpaceToNull(), safeName, extension);
     }
 
     /// <inheritdoc/>
-    public bool TryGenerateUniqueEntryPath(
+    public string GenerateUniqueEntryPath(
         string? directory,
         string fileNameNoExtension,
         string? extension,
-        [NotNullWhen(true)] out string? entryPath,
         string uniqueSuffixSeparator = "")
     {
         if (directory != null && string.IsNullOrWhiteSpace(directory))
         {
             throw new ArgumentException("The directory can be null, but cannot be empty or whitespace only.", nameof(directory));
         }
-        _ = !string.IsNullOrEmpty(fileNameNoExtension) ? fileNameNoExtension : throw new ArgumentNullException(nameof(fileNameNoExtension));
+        _ = fileNameNoExtension ?? throw new ArgumentNullException(nameof(fileNameNoExtension));
+        if (!IsSafeForEntryPathSegment(fileNameNoExtension))
+        {
+            throw new ArgumentException($"The {nameof(fileNameNoExtension)} must be safe for use as an entry path segment. Prevalidate using {nameof(TryMakeSafeForEntryPathSegment)} first.", nameof(fileNameNoExtension));
+        }
+        if (extension != null && !IsSafeForEntryPathSegment(extension))
+        {
+            throw new ArgumentException("The extension can be null, but cannot be empty or whitespace only, and must be a valid entry path segment.", nameof(directory));
+        }
 
-        var entryPathPrefix = directory == null ? fileNameNoExtension : Path.Combine(directory, fileNameNoExtension);
+        var entryPathPrefix = $"{NormalizeDirectoryEntryPath(directory)}{fileNameNoExtension}";
 
         // First see if we can use the name as is
-        entryPath = $"{entryPathPrefix}{extension}";
+        var entryPath = $"{entryPathPrefix}{extension}";
         if (!DoesEntryExist(entryPath))
-            return true;
+            return NormalizeEntryPath(entryPath);
 
         // If file with the same name already exists, add a number to the end of the name
         entryPathPrefix += uniqueSuffixSeparator;
         for (var i = 1; i < int.MaxValue; i++)
         {
             entryPath = $"{entryPathPrefix}{i}{extension}";
             if (!DoesEntryExist(entryPath))
-                return true;
+                return NormalizeEntryPath(entryPath);
         }
 
-        entryPath = null;
-        return false;
+        throw new InvalidOperationException("Failed to generate a unique name.");
     }
 
     public void Save()
@@ -533,7 +536,68 @@ public static string NormalizePath(string path)
         if (string.IsNullOrWhiteSpace(path))
             return string.Empty;
 
-        return path.Trim().Replace('\\', '/').TrimStart('/').ToLowerInvariant();
+        return path.Trim().Replace('\\', EntryPathSeparator).TrimStart(EntryPathSeparator).ToLowerInvariant();
+    }
+
+    /// <summary>
+    /// Normalizes an entry path to a valid FullPath of the entry.<br/>
+    /// Valid normalized entry paths do not contain leading or trailing slashes, and use forward slashes as directory separators.<br/>
+    /// </summary>
+    /// <param name="entryPath">The path to normalize.</param>
+    /// <returns>The normalized entry path.</returns>
+    /// <exception cref="ArgumentNullException"><paramref name="entryPath"/> is null</exception>
+    /// <exception cref="ArgumentException"><paramref name="entryPath"/> cannot be normalized as it resulted in an empty path, which is invalid.</exception>
+    public static string NormalizeEntryPath(string entryPath)
+    {
+        _ = entryPath ?? throw new ArgumentNullException(nameof(entryPath));
+
+        if (!TryNormalizeEntryPath(entryPath, out var normalizedPath))
+            throw new ArgumentException("Zip entry path cannot be normalized, as it results in an empty or whitespace path, which is invalid.", nameof(entryPath));
+
+        return normalizedPath;
+    }
+
+    /// <summary>
+    /// Attempts to normalize an entry path to a valid FullPath of the entry.<br/>
+    /// Valid normalized entry paths do not contain leading or trailing slashes, and use forward slashes as directory separators.<br/>
+    /// </summary>
+    /// <returns>true if the path was successfully normalized; otherwise false, indicating the normalized path resulted in an empty string which is an invalid entry path.</returns>
+    public static bool TryNormalizeEntryPath(string entryPath, [NotNullWhen(true)] out string? normalizedPath)
+    {
+        _ = entryPath ?? throw new ArgumentNullException(nameof(entryPath));
+
+        normalizedPath = entryPath.Trim().Replace('\\', EntryPathSeparator).TrimStart(EntryPathSeparator);
+        if (string.IsNullOrWhiteSpace(normalizedPath))
+        {
+            normalizedPath = null;
+            return false;
+        }
+
+        return true;
+    }
+
+    /// <summary>
+    /// Normalizes a directory path so it can be used as a prefix for entry paths.<br/>
+    /// Normalized directory paths are different than entry paths.
+    /// Directory paths end with '/' unless they are at the root.
+    /// </summary>
+    /// <returns>Empty string for root directory or a path that ends with '/' for a sub-directory.</returns>
+    public static string NormalizeDirectoryEntryPath(string? directoryPath)
+    {
+        // we assume that each segment of the path is already valid (i.e. doesn't have leading/trailing whitespace)
+        // Callers should ensure they enter valid path segments. We can add additional validation if this becomes a problem.
+
+        if (string.IsNullOrEmpty(directoryPath))
+            return string.Empty;
+
+        var normalizedPath = directoryPath
+            .Replace('\\', EntryPathSeparator)
+            .Trim(EntryPathSeparator); // we trim the ends of all '/' chars so that we can be sure to add only a single one at the end
+
+        if (normalizedPath.Length == 0)
+            return string.Empty;
+
+        return normalizedPath + EntryPathSeparator;
     }
 
     /// <summary>
@@ -560,6 +624,21 @@ public static bool TryMakeSafeForEntryPathSegment(
         return safeName != null;
     }
 
+    /// <summary>
+    /// Used to verify that a name is safe for use as a single path segment for an entry.
+    /// Directory separator chars are not allowed in a path segment.
+    /// </summary>
+    /// <param name="name">The proposed path segment name.</param>
+    /// <returns>false when <paramref name="name"/> is null, empty, whitespace only, has leading or trailing whitespace, contains path separator chars or contains any other invalid chars.</returns>
+    public static bool IsSafeForEntryPathSegment(string name)
+    {
+        _ = name ?? throw new ArgumentNullException(nameof(name));
+
+        return !string.IsNullOrWhiteSpace(name)
+            && !UnsafeFileNameCharactersRegex().IsMatch(name)
+            && name.Trim() == name; // No leading or trailing whitespace
+    }
+
     /// <summary>
     /// Regular expression that matches any characters that are unsafe for entry filenames.<br/>
     /// Note: we don't allow any sort of directory separator chars for filenames to remove cross-platform issues.

src/Versions.props

@@ -10,5 +10,8 @@
         <FluentAssertionsVersion>6.12.0</FluentAssertionsVersion>
         <MinVerVersion>4.3.0</MinVerVersion>
         <MicrosoftExtensionsLoggingVersion>7.0.0</MicrosoftExtensionsLoggingVersion>
+
+        <!-- WARNING: Do not update Moq library past 4.20.0 due to privacy issues: https://github.com/devlooped/moq/issues/1372 -->
+        <MoqVersion>4.16.0</MoqVersion>
     </PropertyGroup>
 </Project>