diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 9e0fbcb..fe38590 100644 --- a/.github/workflows/build.yml +++ b/.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. diff --git a/CHANGELOG.md b/CHANGELOG.md index 0859595..fe1a5e7 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/ConvertApi.Tests/SaveFileTests.cs b/ConvertApi.Tests/SaveFileTests.cs index b9420a5..8ec1a81 100644 --- a/ConvertApi.Tests/SaveFileTests.cs +++ b/ConvertApi.Tests/SaveFileTests.cs @@ -1,4 +1,5 @@ using System.Net; +using System.Runtime.InteropServices; using ConvertApiDotNet.Interface; using ConvertApiDotNet.Model; @@ -6,21 +7,32 @@ 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(() => 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(() => 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(() => 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 files) : HttpMessageHand { protected override Task 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}"); diff --git a/ConvertApi/ConvertApiExtension.cs b/ConvertApi/ConvertApiExtension.cs index 92bc164..0a64b93 100644 --- a/ConvertApi/ConvertApiExtension.cs +++ b/ConvertApi/ConvertApiExtension.cs @@ -33,10 +33,16 @@ private static async Task 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 SaveFileAsync(this ConvertApiFile processedFi /// /// Save the files into , 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. /// + /// 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 SaveFileAsync. To use a new folder whose name has an extension, end the path with a slash. public static async Task> SaveFilesAsync(this IEnumerable processedFile, string outputDirectory) { + var files = processedFile.ToList(); + var list = new List(); + // 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(); - 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 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)); + } /// /// Delete files from the ConvertAPI server, and if left, they automatically will be deleted after 3 hours.