diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index db8573c..754bd02 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -424,13 +424,25 @@ be applied. **Now:** EC writes a complete temporary file beside the destination and replaces the old one only after that write succeeds. Converted files and recovery sidecars -already worked this way; all four saved-file types now use the same mechanism, -`AtomicArtifactFile`. +already worked this way; the four saved-file types listed above now use the same +mechanism, `AtomicArtifactFile`. The recovery sidecar keeps its own writer, which also reads back and verifies what it wrote - more than the shared one does, and not worth reducing. The same change closed EC-16. +**Later correction:** the two GUI exports, the text list and the CSV report, were +missed and still opened the chosen file directly, so a failed write could erase a +previous report. They now write through `AtomicArtifactFile` as well. This is folded +into BL-24 because it completes the same fix rather than adding a mechanism. + +`GuiExportWriteTests` pin the new behaviour: a failed write leaves the previous +report byte-for-byte and no staging file. They never ran against the original code, +because the method they call did not exist; with the direct write re-created behind +that method, the mid-write failure tests fail. A read-only report or a link is +refused rather than replaced, which a direct write would not have done either. The +save dialog around the write is not exercised by tests. + ### BL-25 **EC and LineEndingNormalizer both verify that conversion preserved the content, diff --git a/sources/EncodingChecker.Tests/GuiExportWriteTests.cs b/sources/EncodingChecker.Tests/GuiExportWriteTests.cs new file mode 100644 index 0000000..b802449 --- /dev/null +++ b/sources/EncodingChecker.Tests/GuiExportWriteTests.cs @@ -0,0 +1,266 @@ +using System.Diagnostics; +using System.Text; + +namespace EncodingChecker.Tests; + +/// +/// The GUI's text and CSV exports never destroy an existing report by failing to write its +/// replacement. +/// +/// +/// Both export commands write through , which stages the +/// new report in a temporary file and installs it only when it is complete. A failed write must +/// leave the previous report byte-for-byte as it was and no staging file beside it. A read-only +/// report or a link is refused instead of being replaced. The cases use the encodings and content +/// writers the commands themselves use; the save dialog around the write is not exercised. +/// +public sealed class GuiExportWriteTests : IDisposable +{ + private static readonly byte[] Bom = [0xEF, 0xBB, 0xBF]; + + private readonly string _root = + Directory.CreateTempSubdirectory("ec_export_").FullName; + + private string ReportPath => Path.Combine(_root, "report.out"); + + public void Dispose() + { + try + { + // A report left read-only by a case would otherwise stop the folder being deleted. + if (File.Exists(ReportPath)) + File.SetAttributes(ReportPath, FileAttributes.Normal); + + Directory.Delete(_root, recursive: true); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // A leftover temp directory must not fail the test run. + } + } + + private static Encoding EncodingFor(bool csv) => + csv ? ConversionReport.CsvFileEncoding : MainForm.TextExportEncoding; + + private static ConversionReportEntry Row() => new() + { + FilePath = @"C:\scan\a.txt", + SourceEncoding = "utf-8", + TargetEncoding = "utf-8", + }; + + // The content each command writes. + private static Action Content(bool csv) => writer => + { + if (csv) + ConversionReport.WriteCsv([Row()], writer); + else + MainForm.WriteTextExport([("utf-8", @"C:\scan", "a.txt")], writer); + }; + + // What the file must hold: the byte-order mark, then exactly the text the writer produces. + private static byte[] ExpectedBytes(bool csv) + { + using var text = new StringWriter(); + + if (csv) + ConversionReport.WriteCsv([Row()], text); + else + MainForm.WriteTextExport([("utf-8", @"C:\scan", "a.txt")], text); + + return [.. Bom, .. new UTF8Encoding(false).GetBytes(text.ToString())]; + } + + private void AssertNoStagingFileLeft() + { + Assert.DoesNotContain( + Directory.EnumerateFiles(_root), + f => f.EndsWith(EncodingConverter.TempFileSuffix, StringComparison.Ordinal)); + } + + [Fact] + public void TheTextExportListsTheCharsetThenTheFullPath() + { + // The line format is what other tools read back, so it is pinned outside the helper. + using var text = new StringWriter(); + MainForm.WriteTextExport([("utf-8", @"C:\scan", "a.txt")], text); + + Assert.Equal("utf-8\tC:\\scan\\a.txt" + Environment.NewLine, text.ToString()); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void ANewReportIsWrittenWithItsByteOrderMark(bool csv) + { + string? error = MainForm.WriteExportFile(ReportPath, EncodingFor(csv), Content(csv)); + + Assert.Null(error); + Assert.Equal(ExpectedBytes(csv), File.ReadAllBytes(ReportPath)); + AssertNoStagingFileLeft(); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void AnExistingReportIsReplacedByTheCompleteNewOne(bool csv) + { + File.WriteAllText(ReportPath, "the previous report"); + + string? error = MainForm.WriteExportFile(ReportPath, EncodingFor(csv), Content(csv)); + + Assert.Null(error); + Assert.Equal(ExpectedBytes(csv), File.ReadAllBytes(ReportPath)); + AssertNoStagingFileLeft(); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void AWriteThatFailsPartWayLeavesThePreviousReportUntouched(bool csv) + { + byte[] previous = Encoding.UTF8.GetBytes("the previous report\r\n"); + File.WriteAllBytes(ReportPath, previous); + + string? error = MainForm.WriteExportFile( + ReportPath, + EncodingFor(csv), + writer => + { + // A real prefix reaches the disk before the failure. + Content(csv)(writer); + writer.Flush(); + + throw new IOException("the disk is full"); + }); + + Assert.Equal("the disk is full", error); + Assert.Equal(previous, File.ReadAllBytes(ReportPath)); + AssertNoStagingFileLeft(); + } + + [Fact] + public void AFailedWriteOfANewReportLeavesNothingBehind() + { + string? error = MainForm.WriteExportFile( + ReportPath, + EncodingFor(csv: true), + writer => + { + Content(csv: true)(writer); + writer.Flush(); + + throw new IOException("the disk is full"); + }); + + Assert.Equal("the disk is full", error); + Assert.Empty(Directory.EnumerateFiles(_root)); + } + + [Fact] + public void AReportHeldOpenByAnotherProgramIsNotReplacedAndTheReasonIsReturned() + { + byte[] previous = Encoding.UTF8.GetBytes("the previous report\r\n"); + File.WriteAllBytes(ReportPath, previous); + + string? error; + + using (new FileStream(ReportPath, FileMode.Open, FileAccess.Read, FileShare.None)) + { + error = MainForm.WriteExportFile( + ReportPath, EncodingFor(csv: false), Content(csv: false)); + } + + Assert.False(string.IsNullOrEmpty(error)); + Assert.Equal(previous, File.ReadAllBytes(ReportPath)); + AssertNoStagingFileLeft(); + } + + [Fact] + public void ADestinationInAFolderThatDoesNotExistIsAnErrorAndCreatesNothing() + { + string missing = Path.Combine(_root, "no-such-folder", "report.out"); + + string? error = MainForm.WriteExportFile( + missing, EncodingFor(csv: true), Content(csv: true)); + + Assert.False(string.IsNullOrEmpty(error)); + Assert.Empty(Directory.EnumerateFileSystemEntries(_root)); + } + + [Fact] + public void AReadOnlyReportIsRefusedNotOverwritten() + { + // A direct write failed on a read-only file; the atomic install would have cleared the + // flag and replaced it, so the refusal is what keeps that protection. + byte[] previous = Encoding.UTF8.GetBytes("the previous report\r\n"); + File.WriteAllBytes(ReportPath, previous); + File.SetAttributes(ReportPath, FileAttributes.ReadOnly); + + string? error = MainForm.WriteExportFile( + ReportPath, EncodingFor(csv: true), Content(csv: true)); + + Assert.NotNull(error); + Assert.Contains("read-only", error); + Assert.Equal(previous, File.ReadAllBytes(ReportPath)); + Assert.True(File.GetAttributes(ReportPath).HasFlag(FileAttributes.ReadOnly)); + AssertNoStagingFileLeft(); + } + + [Fact] + public void ALinkAsTheDestinationIsRefusedNotReplaced() + { + // A junction is the link a test can create without special privileges; a file symlink + // carries the same ReparsePoint attribute the refusal checks. + string target = Path.Combine(_root, "target"); + Directory.CreateDirectory(target); + + string link = Path.Combine(_root, "link"); + Assert.True( + RunCmd($"mklink /J \"{link}\" \"{target}\"") && Directory.Exists(link), + "The junction fixture could not be created."); + + try + { + string? error = MainForm.WriteExportFile( + link, EncodingFor(csv: true), Content(csv: true)); + + Assert.NotNull(error); + Assert.Contains("is a link", error); + + // The link is still a link and nothing was written through it. + Assert.True(File.GetAttributes(link).HasFlag(FileAttributes.ReparsePoint)); + Assert.Empty(Directory.EnumerateFileSystemEntries(target)); + } + finally + { + Assert.True(RunCmd($"rmdir \"{link}\"")); + } + } + + private static bool RunCmd(string command) + { + using Process? process = Process.Start(new ProcessStartInfo("cmd.exe", $"/c {command}") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + }); + + if (process is null) + return false; + + _ = process.StandardOutput.ReadToEndAsync(); + _ = process.StandardError.ReadToEndAsync(); + + if (!process.WaitForExit(10000)) + { + process.Kill(entireProcessTree: true); + + return false; + } + + return process.ExitCode == 0; + } +} diff --git a/sources/EncodingChecker/MainForm.Export.cs b/sources/EncodingChecker/MainForm.Export.cs index d7d9dfd..396695d 100644 --- a/sources/EncodingChecker/MainForm.Export.cs +++ b/sources/EncodingChecker/MainForm.Export.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Text; using System.Windows.Forms; @@ -20,26 +21,34 @@ private void OnExport(object? sender, EventArgs e) title: @"Export to a Text File", filter: @"Text files (*.txt)|*.txt", defaultFileName: "Encoding.txt", - encoding: new UTF8Encoding(true), + encoding: TextExportEncoding, failureMessage: "Failed to export the report: {0}", - write: writer => - { - foreach (ListViewItem item in lstResults.CheckedItems) - { - string charset = item.SubItems[ResultsColumnCharset].Text; - string fileName = item.SubItems[ResultsColumnFileName].Text; - string directory = item.SubItems[ResultsColumnDirectory].Text; - - writer.WriteLine("{0}\t{1}\\{2}", charset, directory, fileName); - } - }); + write: writer => WriteTextExport( + lstResults.CheckedItems.Cast().Select(item => + ( + item.SubItems[ResultsColumnCharset].Text, + item.SubItems[ResultsColumnDirectory].Text, + item.SubItems[ResultsColumnFileName].Text + )), + writer)); + } + + /// UTF-8 with a BOM, so the text list opens correctly in Notepad. + internal static readonly Encoding TextExportEncoding = new UTF8Encoding(true); + + /// One tab-separated line per file: its charset, then its full path. + internal static void WriteTextExport( + IEnumerable<(string Charset, string Directory, string FileName)> rows, + TextWriter writer) + { + foreach ((string charset, string directory, string fileName) in rows) + writer.WriteLine("{0}\t{1}\\{2}", charset, directory, fileName); } /// - /// Shared shape for the two exports whose failure is an I/O exception. The journal - /// export is deliberately not routed through this: its Save method reports failure - /// by returning an error string, and forcing that into a thrown exception here would - /// manufacture one that was never raised. + /// Shared shape for the text and CSV exports. The journal export is deliberately not + /// routed through this: its Save method already writes atomically and reports failure by + /// returning an error string. /// private void ExportToFile( string title, @@ -60,15 +69,59 @@ private void ExportToFile( if (saveFileDialog.ShowDialog(this) != DialogResult.OK) return; - try + string? error = WriteExportFile(saveFileDialog.FileName, encoding, write); + + if (error is not null) + ShowWarning(failureMessage, error); + } + + /// + /// Writes an export without replacing an existing report until the new one is complete, + /// so a failed write leaves the previous report as it was. + /// + /// + /// A read-only report or a link is refused rather than replaced: the atomic install would + /// clear the read-only flag or swap the link for a regular file, where a direct write + /// would have failed or written through. A fault of an argument or invalid-operation kind + /// raised by is reported as a failed export. + /// + /// on success; otherwise, why the write failed. + internal static string? WriteExportFile( + string path, Encoding encoding, Action write) + { + if (RefusalForExistingDestination(path) is { } refusal) + return refusal; + + return AtomicArtifactFile.Write(path, stream => { - using var writer = new StreamWriter(saveFileDialog.FileName, false, encoding); + using var writer = new StreamWriter(stream, encoding, leaveOpen: true); write(writer); + }); + } + + private static string? RefusalForExistingDestination(string path) + { + FileAttributes attributes; + + try + { + attributes = File.GetAttributes(path); } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + catch (Exception ex) when ( + ex is IOException or UnauthorizedAccessException or ArgumentException + or NotSupportedException) { - ShowWarning(failureMessage, ex.Message); + // Not there yet, or not readable; the write reports its own failure. + return null; } + + if ((attributes & FileAttributes.ReparsePoint) != 0) + return $"'{path}' is a link. Choose a regular file for the report."; + + if ((attributes & FileAttributes.ReadOnly) != 0) + return $"'{path}' is read-only."; + + return null; } private void OnExportResultsOpening(object? sender, EventArgs e)