Skip to content

Commit

Permalink
address feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@joem-msft
joem-msft committed Jun 25, 2024
1 parent 04c74b0 commit 6525724
Show file tree
Hide file tree
Showing 6 changed files with 258 additions and 85 deletions.
12 changes: 4 additions & 8 deletions src/Persistence.Tests/MsApp/MsappArchiveSaveTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ public void Msapp_ShouldSave_Screen(string screenName, string screenEntryName, s
msappValidation.App.Should().BeNull();
msappValidation.CanonicalEntries.Count.Should().Be(2);
msappValidation.DoesEntryExist(screenEntryName).Should().BeTrue();
var screenEntry = msappValidation.CanonicalEntries[MsappArchive.NormalizePath(screenEntryName)];
screenEntry.Should().NotBeNull();
using var streamReader = new StreamReader(msappValidation.GetRequiredEntry(screenEntryName).Open());
var yaml = streamReader.ReadToEnd().NormalizeNewlines();
var expectedYaml = File.ReadAllText(expectedYamlPath).NormalizeNewlines();
Expand Down Expand Up @@ -70,17 +68,15 @@ public void Msapp_ShouldSave_Screen_With_Control(string screenName, string contr
msappValidation.CanonicalEntries.Count.Should().Be(2);

// Validate screen
var screenEntry = msappValidation.CanonicalEntries[MsappArchive.NormalizePath(screenEntryName)];
screenEntry.Should().NotBeNull();
msappValidation.DoesEntryExist(screenEntryName).Should().BeTrue();
using var streamReader = new StreamReader(msappValidation.GetRequiredEntry(screenEntryName).Open());
var yaml = streamReader.ReadToEnd().NormalizeNewlines();
var expectedYaml = File.ReadAllText(expectedYamlPath).NormalizeNewlines();
yaml.Should().Be(expectedYaml);

// Validate editor state
if (msappValidation.CanonicalEntries.TryGetValue(MsappArchive.NormalizePath(editorStateName), out var editorStateEntry))
if (msappValidation.DoesEntryExist(editorStateName))
{
editorStateEntry.Should().NotBeNull();
using var editorStateReader = new StreamReader(msappValidation.GetRequiredEntry(editorStateName).Open());
var json = editorStateReader.ReadToEnd().ReplaceLineEndings();
var expectedJson = File.ReadAllText(expectedJsonPath).ReplaceLineEndings().TrimEnd();
Expand Down Expand Up @@ -113,7 +109,7 @@ public void Msapp_ShouldSave_App(string screenName)
msappValidation.App!.Screens.Count.Should().Be(1);
msappValidation.App.Screens.Single().Name.Should().Be(screenName);
msappValidation.App.Name.Should().Be(App.ControlName);
msappValidation.CanonicalEntries.Keys.Should().Contain(MsappArchive.NormalizePath(MsappArchive.HeaderFileName));
msappValidation.DoesEntryExist(MsappArchive.HeaderFileName).Should().BeTrue();
}

[TestMethod]
Expand All @@ -136,6 +132,6 @@ public void Msapp_ShouldSave_WithUniqueName()
}

msappArchive.CanonicalEntries.Count.Should().Be(sameNames.Length);
msappArchive.CanonicalEntries.Keys.Should().Contain(MsappArchive.NormalizePath(Path.Combine(MsappArchive.Directories.Src, @$"SameName{sameNames.Length + 1}{MsappArchive.YamlPaFileExtension}")));
msappArchive.DoesEntryExist(Path.Combine(MsappArchive.Directories.Src, @$"SameName{sameNames.Length + 1}.pa.yaml")).Should().BeTrue();
}
}
191 changes: 167 additions & 24 deletions src/Persistence.Tests/MsApp/MsappArchiveTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,7 @@ public void AddEntryTests(string[] entries, string[] expectedEntries)
msappArchive.CanonicalEntries.Count.Should().Be(entries.Length);
foreach (var expectedEntry in expectedEntries)
{
msappArchive.CanonicalEntries.ContainsKey(expectedEntry)
.Should()
.BeTrue($"Expected entry {expectedEntry} to exist in the archive");
msappArchive.DoesEntryExist(expectedEntry).Should().BeTrue();
msappArchive.DoesEntryExist(expectedEntry).Should().BeTrue($"Expected entry {expectedEntry} to exist in the archive");
}

// Get the required entry should throw if it doesn't exist
Expand Down Expand Up @@ -141,6 +138,31 @@ public void Msapp_ShouldHave_Screens(string testDirectory, int allEntriesCount,
var screen = msappArchive.App.Screens.Single(c => c.Name == topLevelControlName);
}

[TestMethod]
public void ZipArchiveEntryPathTests()
{
using var stream = new MemoryStream();
using (var zipArchiveWrite = new ZipArchive(stream, ZipArchiveMode.Create, leaveOpen: true))
{
var entry = zipArchiveWrite.CreateEntry("dir/file1.txt");
entry.Name.Should().Be("file1.txt");

zipArchiveWrite.CreateEntry("/dir/file2.txt");
zipArchiveWrite.CreateEntry(@"\dir\file3.txt");
entry = zipArchiveWrite.CreateEntry("dir/");
entry.Name.Should().Be("");
}

stream.Position = 0;
using var zipArchiveRead = new ZipArchive(stream, ZipArchiveMode.Read);
zipArchiveRead.Entries.Select(e => e.FullName).Should().BeEquivalentTo([
"dir/file1.txt",
"/dir/file2.txt",
@"\dir\file3.txt",
"dir/",
], "ZipArchive entry paths are not normalized and assumed to be correct for the current OS");
}

[TestMethod]
public void DoesEntryExistTests()
{
Expand Down Expand Up @@ -193,7 +215,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();
Expand All @@ -204,33 +226,91 @@ 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(Path.Combine("dir1", "entryA.pa.yaml"));
archive.GenerateUniqueEntryPath("dir1", "entryC", ".pa.yaml").Should().Be(Path.Combine("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(Path.Combine("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(Path.Combine("dir1", "entryC.pa.yaml"));
archive.GenerateUniqueEntryPath(@"\dir1/dir2\", "entryC", ".pa.yaml").Should().Be(Path.Combine("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(Path.Combine("dir1", "entryA1.pa.yaml"));
archive.GenerateUniqueEntryPath(@"/dir1\", "entryA", ".pa.yaml").Should().Be(Path.Combine("dir1", "entryA1.pa.yaml"));
archive.GenerateUniqueEntryPath(@"\dir1/dir2\", "entryA", ".pa.yaml").Should().Be(Path.Combine("dir1", "dir2", "entryA1.pa.yaml"));
}

[TestMethod]
public void NormalizeDirectoryEntryPathTests()
{
// Root paths:
MsappArchive.NormalizeDirectoryEntryPath(null).Should().Be(string.Empty, "Normalized directory entry paths are used to compose full paths, and we don't want to return null");
MsappArchive.NormalizeDirectoryEntryPath(string.Empty).Should().Be(string.Empty, "Empty string should be returned as is");
MsappArchive.NormalizeDirectoryEntryPath("/").Should().Be(string.Empty, "Root directory should be normalized to empty string");
MsappArchive.NormalizeDirectoryEntryPath("\\").Should().Be(string.Empty, "Root directory should be normalized to empty string");

var expectedDir1 = $"dir1{Path.DirectorySeparatorChar}";
var expectedDir1Dir2 = $"dir1{Path.DirectorySeparatorChar}dir2{Path.DirectorySeparatorChar}";

// Single directory:
MsappArchive.NormalizeDirectoryEntryPath(@"dir1").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"dir1/").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"/dir1").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"/dir1/").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"dir1\").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"\dir1").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"\dir1\").Should().Be(expectedDir1);

// Multiple directories:
MsappArchive.NormalizeDirectoryEntryPath(@"dir1/dir2").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"dir1/dir2/").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"/dir1/dir2").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"/dir1/dir2/").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"dir1\dir2").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"dir1\dir2\").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"\dir1\dir2").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"\dir1\dir2\").Should().Be(expectedDir1Dir2);

// middle directory separator chars are consolidated to one:
MsappArchive.NormalizeDirectoryEntryPath(@"//dir1//").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"\\dir1\\").Should().Be(expectedDir1);
MsappArchive.NormalizeDirectoryEntryPath(@"//dir1/dir2//").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"\\dir1\dir2\\").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"//dir1///dir2//").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"\\dir1\\\dir2\\").Should().Be(expectedDir1Dir2);
MsappArchive.NormalizeDirectoryEntryPath(@"\/dir1/\dir2/\").Should().Be(expectedDir1Dir2);

// When path segment names have leading/trailing whitespace, they are currently preserved:
MsappArchive.NormalizeDirectoryEntryPath(@" \/ dir1 /\ dir2 /\ ")
.Should().Be($" {Path.DirectorySeparatorChar} dir1 {Path.DirectorySeparatorChar} dir2 {Path.DirectorySeparatorChar} {Path.DirectorySeparatorChar}",
"currently, normalization doesn't 'trim' path segment names");
}

[TestMethod]
Expand Down Expand Up @@ -282,4 +362,67 @@ public void TryMakeSafeForEntryPathSegmentWithUnderscoreReplacementTests(string
safeName.Should().BeNull();
}
}

[TestMethod]
public void TryMakeSafeForEntryPathSegmentWhereInputContainsPathSeparatorCharsTests()
{
MsappArchive.TryMakeSafeForEntryPathSegment("Foo\\Bar.pa.yaml", out var safeName).Should().BeTrue();
safeName.Should().Be("FooBar.pa.yaml");
MsappArchive.TryMakeSafeForEntryPathSegment("Foo/Bar.pa.yaml", out safeName).Should().BeTrue();
safeName.Should().Be("FooBar.pa.yaml");

// with replacement
MsappArchive.TryMakeSafeForEntryPathSegment("Foo\\Bar.pa.yaml", out safeName, unsafeCharReplacementText: "_").Should().BeTrue();
safeName.Should().Be("Foo_Bar.pa.yaml");
MsappArchive.TryMakeSafeForEntryPathSegment("Foo/Bar.pa.yaml", out safeName, unsafeCharReplacementText: "-").Should().BeTrue();
safeName.Should().Be("Foo-Bar.pa.yaml");
}

[TestMethod]
public void TryMakeSafeForEntryPathSegmentWhereInputContainsInvalidPathCharTests()
{
var invalidChars = Path.GetInvalidPathChars()
.Union(Path.GetInvalidFileNameChars());
foreach (var c in invalidChars)
{
// Default behavior should remove invalid chars
MsappArchive.TryMakeSafeForEntryPathSegment($"Foo{c}Bar.pa.yaml", out var safeName).Should().BeTrue();
safeName.Should().Be("FooBar.pa.yaml");

// Replacement char should be used for invalid chars
MsappArchive.TryMakeSafeForEntryPathSegment($"Foo{c}Bar.pa.yaml", out safeName, unsafeCharReplacementText: "_").Should().BeTrue();
safeName.Should().Be("Foo_Bar.pa.yaml");

// When input results in only whitespace or empty, return value should be false
MsappArchive.TryMakeSafeForEntryPathSegment($"{c}", out _).Should().BeFalse("because safe segment is empty string");
MsappArchive.TryMakeSafeForEntryPathSegment($" {c} ", out _).Should().BeFalse("because safe segment is whitespace");
MsappArchive.TryMakeSafeForEntryPathSegment($"{c} {c}", out _).Should().BeFalse("because safe segment is whitespace");
}
}

[TestMethod]
public void IsSafeForEntryPathSegmentTests()
{
MsappArchive.IsSafeForEntryPathSegment("Foo.pa.yaml").Should().BeTrue();

// Path separator chars should not be used for path segments
MsappArchive.IsSafeForEntryPathSegment("Foo/Bar.pa.yaml").Should().BeFalse("separator chars should not be used for path segments");
MsappArchive.IsSafeForEntryPathSegment("/Foo.pa.yaml").Should().BeFalse("separator chars should not be used for path segments");
MsappArchive.IsSafeForEntryPathSegment("Foo\\Bar.pa.yaml").Should().BeFalse("separator chars should not be used for path segments");
MsappArchive.IsSafeForEntryPathSegment("\\Foo.pa.yaml").Should().BeFalse("separator chars should not be used for path segments");

MsappArchive.IsSafeForEntryPathSegment("Foo/Bar.pa.yaml").Should().BeFalse("separator chars should not be used for path segments");
}

[TestMethod]
public void IsSafeForEntryPathSegmentShouldNotAllowInvalidPathCharsTests()
{
var invalidChars = Path.GetInvalidPathChars()
.Union(Path.GetInvalidFileNameChars());

foreach (var c in invalidChars)
{
MsappArchive.IsSafeForEntryPathSegment($"Foo{c}Bar.pa.yaml").Should().BeFalse($"Invalid char '{c}' should not be allowed for path segments");
}
}
}
2 changes: 1 addition & 1 deletion src/Persistence.Tests/Persistence.Tests.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Expand Down
9 changes: 4 additions & 5 deletions src/Persistence/MsApp/IMsappArchive.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -113,7 +112,7 @@ bool TryGenerateUniqueEntryPath(

/// <summary>
/// Dictionary of all entries in the archive.
/// The keys are normalized paths for the entry computed using <see cref="MsappArchive.NormalizePath"/>.
/// The keys are normalized paths for the entry computed using <see cref="MsappArchive.CanonicalizePath"/>.
/// </summary>
IReadOnlyDictionary<string, ZipArchiveEntry> CanonicalEntries { get; }

Expand Down
Loading

0 comments on commit 6525724

Please sign in to comment.