SaveFileAsync: write hidden files again, check the SaveFilesAsync folder; test on Windows

.github/workflows/build.yml

@@ -46,8 +46,9 @@ jobs:
         run: dotnet build ConvertApi.sln -c Release --no-restore -p:ContinuousIntegrationBuild=true
 
       # ConvertApi.Tests serves results from a fake HTTP client, so the tests need no network and no API token.
+      # TreatNoTestsAsError fails the step when no test ran, for example after a test adapter stops finding them.
       - name: Test
-        run: dotnet test ConvertApi.Tests/ConvertApi.Tests.csproj -c Release --no-build
+        run: dotnet test ConvertApi.Tests/ConvertApi.Tests.csproj -c Release --no-build -- RunConfiguration.TreatNoTestsAsError=true
 
       # A pre-release version that sorts below any real release and names the run, so a package
       # downloaded from this run can be tried out but never mistaken for a published one.

CHANGELOG.md

@@ -18,14 +18,26 @@ The format is based on Keep a Changelog and this project adheres to Semantic Ver
 
 ### Fixed
 - `SaveFileAsync` and `SaveFilesAsync` replace an existing file instead of writing over its start. They
-  opened it with `FileInfo.OpenWrite`, which does not truncate, so a result saved over a longer file kept
-  that file's trailing bytes and came out corrupt: a 146,734-byte PDF saved over a 400,000-byte file stayed
-  400,000 bytes. Every earlier version has this bug.
+  open it with `FileInfo.OpenWrite`, which does not truncate, and did not cut it to the new length, so a
+  result saved over a longer file kept that file's trailing bytes and came out corrupt: a 146,734-byte PDF
+  saved over a 400,000-byte file stayed 400,000 bytes. Every version since 2.0.0 (2018) has this bug.
+  Hidden and system files on Windows are still written, as before.
 
 ### Changed
 - `SaveFilesAsync(outputDirectory)` creates the folder if it does not exist, instead of throwing
-  `DirectoryNotFoundException`. An empty path still means the current directory.
-- Unit tests (`ConvertApi.Tests`, NUnit) run in the `build` check of every pull request.
+  `DirectoryNotFoundException`. An empty path still means the current directory, and an empty result list
+  still saves nothing and creates no folder.
+- `SaveFilesAsync` checks the folder before downloading anything:
+  - A folder that cannot be created now throws before the download, with the exception
+    `Directory.CreateDirectory` throws, for example `UnauthorizedAccessException` (which is not an
+    `IOException`) when the parent folder is read-only. 3.2.2 downloaded the first file and then threw
+    `DirectoryNotFoundException`.
+  - It throws `ArgumentException` when the path is blank, is an existing file, or looks like a result file
+    name, such as `SaveFilesAsync("result.pdf")` for PDF results, which would otherwise create a folder named
+    `result.pdf`. To save one result as a file, call `SaveFileAsync`. To use a new folder whose name has an
+    extension, end the path with a slash: `SaveFilesAsync("archive.pdf/")`.
+- Unit tests (`ConvertApi.Tests`, NUnit) run in the `build` check of every pull request. The check fails if
+  no test runs.
 - The package readme on nuget.org is the repository's `README.md`; the separate `ConvertApi/docs/README.md`
   had drifted and is removed. The publish workflow tags the packed copy's links `utm_source=nuget`.
 - README: a quick start, C# task sections, and snippets that run as copied on Windows, macOS and Linux.

ConvertApi.Tests/SaveFileTests.cs

@@ -1,26 +1,38 @@
 using System.Net;
+using System.Runtime.InteropServices;
 using ConvertApiDotNet.Interface;
 using ConvertApiDotNet.Model;
 
 namespace ConvertApiDotNet.Tests;
 
 // SaveFileAsync and SaveFilesAsync download through the static ConvertApi.GetClient(). Each test installs a
 // fake client that serves the result files from memory, so the tests make no network calls, and they must not
-// run in parallel with other tests that install a client.
+// run in parallel with other tests that install a client. Installing it also sets the static ConvertApi.ApiToken,
+// so both are put back after each test.
 [NonParallelizable]
 public class SaveFileTests
 {
     private string _dir = "";
+    private string? _savedToken;
+    private IConvertApiHttpClient? _savedClient;
+    private static int _requests;
 
     [SetUp]
-    public void CreateTempDirectory()
+    public void SetUp()
     {
+        _savedToken = ConvertApi.ApiToken;
+        _savedClient = ConvertApi.GetClient();
+        _requests = 0;
         _dir = Path.Combine(Path.GetTempPath(), "ConvertApi.Tests-" + Guid.NewGuid().ToString("N"));
         Directory.CreateDirectory(_dir);
     }
 
     [TearDown]
-    public void DeleteTempDirectory() => Directory.Delete(_dir, recursive: true);
+    public void TearDown()
+    {
+        _ = new ConvertApi(_savedToken!, _savedClient!);
+        Directory.Delete(_dir, recursive: true);
+    }
 
     [Test]
     public async Task SaveFileAsync_ReplacesALongerExistingFile()
@@ -92,6 +104,133 @@ public async Task SaveFilesAsync_EmptyFolderMeansTheCurrentDirectory()
         Assert.That(File.ReadAllBytes(Path.Combine(_dir, "document.pdf")), Is.EqualTo(content));
     }
 
+    [Test]
+    public async Task SaveFileAsync_ReplacesAHiddenFile()
+    {
+        // On Windows, FileMode.Create (CREATE_ALWAYS) refuses an existing hidden or system file, and 3.2.2 wrote it.
+        var path = Path.Combine(_dir, "document.pdf");
+        File.WriteAllBytes(path, Bytes(5_000, seed: 9));
+        File.SetAttributes(path, FileAttributes.Hidden | FileAttributes.System);
+        var content = Bytes(1_000, seed: 10);
+        var file = Serve(("document.pdf", content)).Single();
+
+        await file.SaveFileAsync(path);
+
+        Assert.That(File.ReadAllBytes(path), Is.EqualTo(content));
+    }
+
+    [Test]
+    public async Task SaveFileAsync_ReplacesAShorterFile()
+    {
+        var path = Path.Combine(_dir, "document.pdf");
+        File.WriteAllBytes(path, Bytes(10, seed: 11));
+        var content = Bytes(1_000, seed: 12);
+        var file = Serve(("document.pdf", content)).Single();
+
+        await file.SaveFileAsync(path);
+
+        Assert.That(File.ReadAllBytes(path), Is.EqualTo(content));
+    }
+
+    [Test]
+    public async Task SaveFileAsync_WritesToADeviceThatCannotSeek()
+    {
+        // The length of /dev/null cannot be set, so the save must not try to.
+        Assume.That(RuntimeInformation.IsOSPlatform(OSPlatform.Windows), Is.False, "Unix only");
+        var file = Serve(("document.pdf", Bytes(1_000, seed: 13))).Single();
+
+        var saved = await file.SaveFileAsync("/dev/null");
+
+        Assert.That(saved.FullName, Is.EqualTo("/dev/null"));
+    }
+
+    [TestCase("result.pdf")]
+    [TestCase("RESULT.PDF")]
+    [TestCase("nested/result.pdf")]
+    public async Task SaveFilesAsync_AResultFileNameIsRejected(string name)
+    {
+        var response = new ConvertApiResponse { Files = Serve(("document.pdf", Bytes(1_000, seed: 14))) };
+        var path = Path.Combine(_dir, name);
+
+        var error = await Assert.ThrowsAsync<ArgumentException>(() => response.SaveFilesAsync(path));
+
+        using (Assert.EnterMultipleScope())
+        {
+            Assert.That(error!.ParamName, Is.EqualTo("outputDirectory"));
+            Assert.That(error.Message, Does.Contain("SaveFileAsync"));
+            Assert.That(Directory.Exists(path), Is.False, "no folder is created");
+            Assert.That(_requests, Is.Zero, "nothing is downloaded");
+        }
+    }
+
+    [Test]
+    public async Task SaveFilesAsync_AnExistingFileIsRejected()
+    {
+        var path = Path.Combine(_dir, "notes.txt");
+        File.WriteAllText(path, "keep me");
+        var response = new ConvertApiResponse { Files = Serve(("document.pdf", Bytes(1_000, seed: 15))) };
+
+        await Assert.ThrowsAsync<ArgumentException>(() => response.SaveFilesAsync(path));
+
+        using (Assert.EnterMultipleScope())
+        {
+            Assert.That(File.ReadAllText(path), Is.EqualTo("keep me"));
+            Assert.That(_requests, Is.Zero);
+        }
+    }
+
+    [Test]
+    public async Task SaveFilesAsync_ABlankFolderIsRejected()
+    {
+        var response = new ConvertApiResponse { Files = Serve(("document.pdf", Bytes(1_000, seed: 16))) };
+
+        await Assert.ThrowsAsync<ArgumentException>(() => response.SaveFilesAsync("   "));
+        Assert.That(_requests, Is.Zero);
+    }
+
+    [TestCase("archive.pdf/")]
+    [TestCase("v1.2")]
+    [TestCase("output.d")]
+    public async Task SaveFilesAsync_FolderNamesWithADotAreAccepted(string name)
+    {
+        // A trailing slash says "folder"; an extension that no result has is a folder name too.
+        var content = Bytes(1_000, seed: 17);
+        var response = new ConvertApiResponse { Files = Serve(("document.pdf", content)) };
+        var path = Path.Combine(_dir, name);
+
+        await response.SaveFilesAsync(path);
+
+        Assert.That(File.ReadAllBytes(Path.Combine(path, "document.pdf")), Is.EqualTo(content));
+    }
+
+    [Test]
+    public async Task SaveFilesAsync_AnExistingFolderNamedLikeAFileIsUsed()
+    {
+        var path = Path.Combine(_dir, "archive.pdf");
+        Directory.CreateDirectory(path);
+        var content = Bytes(1_000, seed: 18);
+        var response = new ConvertApiResponse { Files = Serve(("document.pdf", content)) };
+
+        await response.SaveFilesAsync(path);
+
+        Assert.That(File.ReadAllBytes(Path.Combine(path, "document.pdf")), Is.EqualTo(content));
+    }
+
+    [Test]
+    public async Task SaveFilesAsync_NoResultsCreateNoFolder()
+    {
+        var path = Path.Combine(_dir, "output");
+        var response = new ConvertApiResponse { Files = Serve() };
+
+        var saved = await response.SaveFilesAsync(path);
+
+        using (Assert.EnterMultipleScope())
+        {
+            Assert.That(saved, Is.Empty);
+            Assert.That(Directory.Exists(path), Is.False);
+        }
+    }
+
     private static byte[] Bytes(int length, int seed)
     {
         var bytes = new byte[length];
@@ -125,6 +264,7 @@ private sealed class FileServer(Dictionary<Uri, byte[]> files) : HttpMessageHand
     {
         protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
         {
+            Interlocked.Increment(ref _requests);
             if (request.Method != HttpMethod.Get || !files.TryGetValue(request.RequestUri!, out var content))
                 throw new InvalidOperationException($"Unexpected request: {request.Method} {request.RequestUri}");
 

ConvertApi/ConvertApiExtension.cs

@@ -33,10 +33,16 @@ private static async Task<FileInfo> SaveFileAsync(Uri url, string fileName)
             var fileInfo = new FileInfo(fileName);
             using (var readFile = await AsStreamAsync(url))
             {
-                // FileMode.Create truncates an existing file. FileInfo.OpenWrite (FileMode.OpenOrCreate) did not,
-                // so a result saved over a longer file kept that file's trailing bytes and came out corrupt.
-                using (var fileStream = new FileStream(fileInfo.FullName, FileMode.Create, FileAccess.Write, FileShare.None))
+                // OpenWrite (FileMode.OpenOrCreate) does not truncate, so after the copy the file is cut to what was
+                // written: otherwise a result saved over a longer file keeps that file's trailing bytes. FileMode.Create
+                // would truncate, but on Windows it fails on an existing hidden or system file, which OpenWrite writes.
+                // A stream that cannot seek (a device such as /dev/null) has no length to set.
+                using (var fileStream = fileInfo.OpenWrite())
+                {
                     await readFile.CopyToAsync(fileStream);
+                    if (fileStream.CanSeek && fileStream.Length > fileStream.Position)
+                        fileStream.SetLength(fileStream.Position);
+                }
             }
 
             return fileInfo;
@@ -75,22 +81,54 @@ public static async Task<FileInfo> SaveFileAsync(this ConvertApiFile processedFi
 
         /// <summary>
         /// Save the files into <paramref name="outputDirectory"/>, each under its own name, replacing files that
-        /// exist. The folder is created if it does not exist.
+        /// exist. The folder is created if it does not exist. An empty string means the current folder.
         /// </summary>
+        /// <exception cref="ArgumentException"><paramref name="outputDirectory"/> is blank, is an existing file, or
+        /// looks like a result file name (for example "result.pdf" for PDF results). To save one result as a file,
+        /// call <c>SaveFileAsync</c>. To use a new folder whose name has an extension, end the path with a slash.</exception>
         public static async Task<List<FileInfo>> SaveFilesAsync(this IEnumerable<ConvertApiFile> processedFile, string outputDirectory)
         {
+            var files = processedFile.ToList();
+            var list = new List<FileInfo>();
+            // Nothing to save: no folder is created, as in versions before the folder was created.
+            if (files.Count == 0)
+                return list;
+
             // An empty path still means the current directory, as Path.Combine("", name) below always did.
             if (!string.IsNullOrEmpty(outputDirectory))
+            {
+                CheckIsAFolder(outputDirectory, files);
                 Directory.CreateDirectory(outputDirectory);
+            }
 
-            var list = new List<FileInfo>();
-            foreach (var file in processedFile)
+            foreach (var file in files)
             {
                 list.Add(await file.SaveFileAsync(Path.Combine(outputDirectory, file.FileName)));
             }
 
             return list;
         }
+
+        // SaveFilesAsync("result.pdf") would otherwise create a folder named result.pdf, which then blocks
+        // SaveFileAsync("result.pdf"). Checked before anything is downloaded.
+        private static void CheckIsAFolder(string outputDirectory, List<ConvertApiFile> files)
+        {
+            if (string.IsNullOrWhiteSpace(outputDirectory))
+                throw new ArgumentException("The output folder is blank. Pass \"\" or \".\" for the current folder.", nameof(outputDirectory));
+
+            var endsInSeparator = outputDirectory.EndsWith(Path.DirectorySeparatorChar.ToString())
+                                  || outputDirectory.EndsWith(Path.AltDirectorySeparatorChar.ToString());
+            var extension = Path.GetExtension(outputDirectory);
+            var looksLikeAResultFile = !endsInSeparator
+                                       && extension.Length > 1
+                                       && !Directory.Exists(outputDirectory)
+                                       && files.Any(f => string.Equals(Path.GetExtension(f.FileName ?? ""), extension, StringComparison.OrdinalIgnoreCase));
+
+            if (File.Exists(outputDirectory) || looksLikeAResultFile)
+                throw new ArgumentException(
+                    $"SaveFilesAsync saves into a folder, and '{outputDirectory}' is a file name. To save the result as '{outputDirectory}', " +
+                    "call SaveFileAsync. To use a new folder with this name, end the path with a slash.", nameof(outputDirectory));
+        }
 
         /// <summary>
         /// Delete files from the ConvertAPI server, and if left, they automatically will be deleted after 3 hours.