From 12df2588017b2641fb7dd78774413ecd72716051 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Sun, 20 Sep 2026 01:34:45 +0300 Subject: [PATCH 1/2] Write the GUI text and CSV exports atomically Both exports opened the chosen file directly, which truncates an existing report before the new one is complete. If the write then failed, the warning appeared but the previous report was already gone. The CLI reports, journals, plans and settings already used AtomicArtifactFile; these two paths did not. The exports now write through WriteExportFile, which stages the report in a temporary file and installs it only when it is complete. The save dialog, encodings, BOM policy and row scope are unchanged. The new tests fail a mid-write error against the old behaviour, leaving the previous report untouched and no staging file behind. BL-24 now records that its "all saved files" claim missed these two exports. Co-Authored-By: Claude Sonnet 5 --- docs/DEFECT-BACKLOG.md | 6 + .../GuiExportWriteTests.cs | 156 ++++++++++++++++++ sources/EncodingChecker/MainForm.Export.cs | 31 ++-- 3 files changed, 181 insertions(+), 12 deletions(-) create mode 100644 sources/EncodingChecker.Tests/GuiExportWriteTests.cs diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index db8573c..7ff912e 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -431,6 +431,12 @@ The recovery sidecar keeps its own writer, which also reads back and verifies wh 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 (`GuiExportWriteTests` +fail a mid-write error against the old behaviour and pass now). The save dialog +around that step 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..6c31998 --- /dev/null +++ b/sources/EncodingChecker.Tests/GuiExportWriteTests.cs @@ -0,0 +1,156 @@ +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. The cases run +/// the same encoding and content each command uses; the save dialog around it 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 + { + Directory.Delete(_root, recursive: true); + } + catch (IOException) + { + // Best-effort cleanup. + } + } + + private static Encoding EncodingFor(bool csv) => + csv ? ConversionReport.CsvFileEncoding : new UTF8Encoding(true); + + private static ConversionReportEntry Row() => new() + { + FilePath = @"C:\scan\a.txt", + SourceEncoding = "utf-8", + TargetEncoding = "utf-8", + }; + + // The content each command writes: one line per file for the text export, the CSV report + // for the other. + private static Action Content(bool csv) => writer => + { + if (csv) + ConversionReport.WriteCsv([Row()], writer); + else + writer.WriteLine("utf-8\tC:\\scan\\a.txt"); + }; + + private void AssertNoStagingFileLeft() + { + Assert.DoesNotContain( + Directory.EnumerateFiles(_root), + f => f.EndsWith(EncodingConverter.TempFileSuffix, StringComparison.Ordinal)); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void ANewReportIsWrittenWithItsByteOrderMark(bool csv) + { + string? error = MainForm.WriteExportFile(ReportPath, EncodingFor(csv), Content(csv)); + + Assert.Null(error); + + byte[] written = File.ReadAllBytes(ReportPath); + Assert.Equal(Bom, written[..3]); + Assert.Contains("a.txt", Encoding.UTF8.GetString(written, 3, written.Length - 3)); + 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); + + byte[] written = File.ReadAllBytes(ReportPath); + Assert.Equal(Bom, written[..3]); + Assert.DoesNotContain("previous", Encoding.UTF8.GetString(written)); + 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(); + } +} diff --git a/sources/EncodingChecker/MainForm.Export.cs b/sources/EncodingChecker/MainForm.Export.cs index d7d9dfd..a3ff15c 100644 --- a/sources/EncodingChecker/MainForm.Export.cs +++ b/sources/EncodingChecker/MainForm.Export.cs @@ -36,10 +36,9 @@ private void OnExport(object? sender, EventArgs e) } /// - /// 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,16 +59,24 @@ 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. + /// + /// on success; otherwise, why the write failed. + internal static string? WriteExportFile( + string path, Encoding encoding, Action write) => + AtomicArtifactFile.Write(path, stream => { - using var writer = new StreamWriter(saveFileDialog.FileName, false, encoding); + using var writer = new StreamWriter(stream, encoding, leaveOpen: true); write(writer); - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) - { - ShowWarning(failureMessage, ex.Message); - } - } + }); private void OnExportResultsOpening(object? sender, EventArgs e) { From 8e96451ad19e4ac7dd30491220efb64fe63d01b6 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Sun, 20 Sep 2026 01:58:31 +0300 Subject: [PATCH 2/2] Refuse read-only and linked export destinations; share the text export The atomic install would clear a read-only flag or swap a link for a regular file, where the previous direct write failed or wrote through. WriteExportFile now refuses both with a message, so exports keep that protection. The text export's encoding and line format move into internal members that the handler and the tests both use, so the tests no longer duplicate them and now compare exact bytes. New cases cover a missing folder, a read-only report and a link. BL-24's note is reworded: it says why it is folded into BL-24 and that the tests never ran against the original code, only against the direct write re-created behind the same method. Co-Authored-By: Claude Sonnet 5 --- docs/DEFECT-BACKLOG.md | 16 +- .../GuiExportWriteTests.cs | 142 ++++++++++++++++-- sources/EncodingChecker/MainForm.Export.cs | 74 +++++++-- 3 files changed, 197 insertions(+), 35 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index 7ff912e..754bd02 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -424,8 +424,8 @@ 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 @@ -433,9 +433,15 @@ 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 (`GuiExportWriteTests` -fail a mid-write error against the old behaviour and pass now). The save dialog -around that step is not exercised by tests. +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 diff --git a/sources/EncodingChecker.Tests/GuiExportWriteTests.cs b/sources/EncodingChecker.Tests/GuiExportWriteTests.cs index 6c31998..b802449 100644 --- a/sources/EncodingChecker.Tests/GuiExportWriteTests.cs +++ b/sources/EncodingChecker.Tests/GuiExportWriteTests.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Text; namespace EncodingChecker.Tests; @@ -9,8 +10,9 @@ namespace EncodingChecker.Tests; /// /// 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. The cases run -/// the same encoding and content each command uses; the save dialog around it is not exercised. +/// 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 { @@ -25,16 +27,20 @@ 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 (IOException) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) { - // Best-effort cleanup. + // A leftover temp directory must not fail the test run. } } private static Encoding EncodingFor(bool csv) => - csv ? ConversionReport.CsvFileEncoding : new UTF8Encoding(true); + csv ? ConversionReport.CsvFileEncoding : MainForm.TextExportEncoding; private static ConversionReportEntry Row() => new() { @@ -43,16 +49,28 @@ private static Encoding EncodingFor(bool csv) => TargetEncoding = "utf-8", }; - // The content each command writes: one line per file for the text export, the CSV report - // for the other. + // The content each command writes. private static Action Content(bool csv) => writer => { if (csv) ConversionReport.WriteCsv([Row()], writer); else - writer.WriteLine("utf-8\tC:\\scan\\a.txt"); + 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( @@ -60,6 +78,16 @@ private void AssertNoStagingFileLeft() 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)] @@ -68,10 +96,7 @@ public void ANewReportIsWrittenWithItsByteOrderMark(bool csv) string? error = MainForm.WriteExportFile(ReportPath, EncodingFor(csv), Content(csv)); Assert.Null(error); - - byte[] written = File.ReadAllBytes(ReportPath); - Assert.Equal(Bom, written[..3]); - Assert.Contains("a.txt", Encoding.UTF8.GetString(written, 3, written.Length - 3)); + Assert.Equal(ExpectedBytes(csv), File.ReadAllBytes(ReportPath)); AssertNoStagingFileLeft(); } @@ -85,10 +110,7 @@ public void AnExistingReportIsReplacedByTheCompleteNewOne(bool csv) string? error = MainForm.WriteExportFile(ReportPath, EncodingFor(csv), Content(csv)); Assert.Null(error); - - byte[] written = File.ReadAllBytes(ReportPath); - Assert.Equal(Bom, written[..3]); - Assert.DoesNotContain("previous", Encoding.UTF8.GetString(written)); + Assert.Equal(ExpectedBytes(csv), File.ReadAllBytes(ReportPath)); AssertNoStagingFileLeft(); } @@ -153,4 +175,92 @@ public void AReportHeldOpenByAnotherProgramIsNotReplacedAndTheReasonIsReturned() 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 a3ff15c..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,19 +21,28 @@ 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); } /// @@ -69,14 +79,50 @@ private void ExportToFile( /// 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) => - AtomicArtifactFile.Write(path, stream => + string path, Encoding encoding, Action write) + { + if (RefusalForExistingDestination(path) is { } refusal) + return refusal; + + return AtomicArtifactFile.Write(path, stream => { 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 or ArgumentException + or NotSupportedException) + { + // 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) {