diff --git a/Fuchs.Tests/ReminderServiceStoreFileTests.cs b/Fuchs.Tests/ReminderServiceStoreFileTests.cs new file mode 100644 index 0000000..901c399 --- /dev/null +++ b/Fuchs.Tests/ReminderServiceStoreFileTests.cs @@ -0,0 +1,125 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Fuchs.intranet; +using Fuchs.Notifications; +using Fuchs.Services; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging; +using MigraDoc.DocumentObjectModel; +using Moq; +using OCORE.security; +using Xunit; + +namespace Fuchs.Tests; + +/// +/// Reminder counterpart of : every failure branch of +/// must be logged individually and +/// must not reach the blob upload or the ReminderFileCreated event (ADR 0025). +/// +public class ReminderServiceStoreFileTests +{ + private sealed class RecordingLogger : ILogger + { + public sealed record Entry(LogLevel Level, Exception? Exception, string Message); + public readonly List Entries = new(); + public IDisposable? BeginScope(TState state) where TState : notnull => null; + public bool IsEnabled(LogLevel logLevel) => true; + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, + Func formatter) => + Entries.Add(new Entry(logLevel, exception, formatter(state, exception))); + } + + private const string UnreachableConnectionString = + "Server=127.0.0.1,1;Database=fuchs;User Id=u;Password=p;Connect Timeout=1;TrustServerCertificate=True"; + + private static (ReminderService svc, RecordingLogger log, Mock blob, Mock events) + MakeService(Mock pdf, string? connectionString = null) + { + var log = new RecordingLogger(); + var blob = new Mock(); + var events = new Mock(); + var config = new ConfigurationBuilder().AddInMemoryCollection(new Dictionary + { + ["ConnectionStrings:fuchs_fds_ConnectionString"] = connectionString + }).Build(); + var svc = new ReminderService(new Fuchs_intranet(config), pdf.Object, blob.Object, events.Object, log); + return (svc, log, blob, events); + } + + private static FdsReminderData Reminder() => new() + { + ReminderRegistration = new OCORE.OCORE_dictionaries.GenericObjectDictionary(new Dictionary + { + ["Id"] = "M1ABC", + ["subject"] = "Zahlungserinnerung", + ["IsFinal"] = true + }), + IsDraft = false + }; + + private static Mock PdfReturning(byte[] bytes) + { + var pdf = new Mock(); + pdf.Setup(p => p.WriteLetterAsync(It.IsAny(), It.IsAny())) + .ReturnsAsync(new Document()); + pdf.Setup(p => p.DocToPdfBytes(It.IsAny())).Returns(bytes); + return pdf; + } + + private static void AssertNothingPersisted(Mock blob, Mock events) + { + blob.Verify(b => b.UploadReminderPdfAsync(It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny>(), It.IsAny()), Times.Never); + events.Verify(e => e.ReminderFileCreatedAsync(It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task StoreReminderDocumentFileAsync_RenderProducesZeroBytes_ReturnsEmptyAndLogsError() + { + var (svc, log, blob, events) = MakeService(PdfReturning(Array.Empty())); + + var result = await svc.StoreReminderDocumentFileAsync(Reminder(), false, "user1", new DatabaseSecurity("user1", null)); + + Assert.Empty(result); + var entry = Assert.Single(log.Entries, e => e.Level == LogLevel.Error); + Assert.Null(entry.Exception); + Assert.Contains("produced 0 bytes", entry.Message); + Assert.Contains("M1ABC", entry.Message); + AssertNothingPersisted(blob, events); + } + + [Fact] + public async Task StoreReminderDocumentFileAsync_RenderThrows_ReturnsEmptyAndLogsExceptionOnce() + { + var pdf = new Mock(); + var boom = new InvalidOperationException("render exploded"); + pdf.Setup(p => p.WriteLetterAsync(It.IsAny(), It.IsAny())).ThrowsAsync(boom); + var (svc, log, blob, events) = MakeService(pdf); + + var result = await svc.StoreReminderDocumentFileAsync(Reminder(), false, "user1", new DatabaseSecurity("user1", null)); + + Assert.Empty(result); + var entry = Assert.Single(log.Entries, e => e.Level == LogLevel.Error); + Assert.Same(boom, entry.Exception); + Assert.Contains("render failed", entry.Message); + AssertNothingPersisted(blob, events); + } + + [Fact] + public async Task StoreReminderDocumentFileAsync_RenderOkButDbStoreFails_ReturnsEmptyAndSkipsBlob() + { + var (svc, log, blob, events) = MakeService(PdfReturning(new byte[] { 0x25, 0x50, 0x44, 0x46 }), UnreachableConnectionString); + + var result = await svc.StoreReminderDocumentFileAsync(Reminder(), false, "user1", new DatabaseSecurity("user1", null)); + + Assert.Empty(result); + Assert.DoesNotContain(log.Entries, e => e.Message.Contains("produced 0 bytes")); + Assert.DoesNotContain(log.Entries, e => e.Message.Contains("render failed")); + AssertNothingPersisted(blob, events); + } +} diff --git a/Fuchs.Tests/ReportDocFileNameTests.cs b/Fuchs.Tests/ReportDocFileNameTests.cs new file mode 100644 index 0000000..03146ec --- /dev/null +++ b/Fuchs.Tests/ReportDocFileNameTests.cs @@ -0,0 +1,33 @@ +using Fuchs.Controllers; +using Xunit; + +namespace Fuchs.Tests; + +/// +/// names a regenerated invoice document +/// (inv/rdoc, ADR 0025). DocumentName comes from the user-typed invoice title, so it must survive +/// path separators and editor markup without truncating or breaking the download filename. +/// +public class ReportDocFileNameTests +{ + [Theory] + [InlineData("Rechnung R2026-0258.pdf", "Rechnung R2026-0258.pdf")] + [InlineData("Wartung 2025/26 R2026-0001.pdf", "Wartung 2025_26 R2026-0001.pdf")] + [InlineData("Angebot A\\B: \"x\" |z?*.pdf", "Angebot A_B_ _x_ _y__z__.pdf")] + [InlineData("

Rechnung R2026-0002.pdf

", "Rechnung R2026-0002.pdf")] + [InlineData(" Rechnung R2026-0003.pdf ", "Rechnung R2026-0003.pdf")] + public void SafeDocumentFileName_ReplacesInvalidCharsAndStripsMarkup(string documentName, string expected) + { + Assert.Equal(expected, IntranetController.SafeDocumentFileName(documentName, "Rechnung_X.pdf")); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData("

")] + public void SafeDocumentFileName_EmptyDocumentName_UsesFallback(string? documentName) + { + Assert.Equal("Rechnung_W7VAL.pdf", IntranetController.SafeDocumentFileName(documentName!, "Rechnung_W7VAL.pdf")); + } +} diff --git a/Fuchs/Controllers/IntranetController.Invoices2.cs b/Fuchs/Controllers/IntranetController.Invoices2.cs index 2bdd220..ad02628 100644 --- a/Fuchs/Controllers/IntranetController.Invoices2.cs +++ b/Fuchs/Controllers/IntranetController.Invoices2.cs @@ -293,7 +293,13 @@ public partial class IntranetController _logger.LogDebug("HandleReportDoc reportid={ReportId} typ={Typ} user={User}", reportid, Form("typ"), UserAccountID); byte[]? content = null; var file = _mfr.GetReportDoc(ref content, reportid); - if (file == null || content == null || content.Length == 0) + string fileName, mimeType; + if (file != null && content is { Length: > 0 }) + { + fileName = file.Name; + mimeType = file.MimeType(); + } + else { // Regenerate-on-miss (ADR 0025). Finalised, non-external only: fds__setInvoiceFile // writes WHERE [file] IS NULL, so persisting a draft render here would permanently @@ -310,18 +316,32 @@ public partial class IntranetController _logger.LogWarning("HandleReportDoc: document not found reportid={ReportId} user={User}", reportid, UserAccountID); return StatusCode(404, new { error = "Dokument wurde nicht gefunden" }); } - string regeneratedName = fdInv.InvoiceRegistration?.getString("DocumentName") - .ne($"Rechnung_{fdInv.Id}.pdf") ?? $"Rechnung_{reportid}.pdf"; - file = new FileInfo(Path.Combine(Path.GetTempPath(), regeneratedName)); + fileName = SafeDocumentFileName( + fdInv.InvoiceRegistration?.getString("DocumentName") ?? "", $"Rechnung_{fdInv.Id.ne(reportid)}.pdf"); + mimeType = "application/pdf"; } _logger.LogDebug("HandleReportDoc found reportid={ReportId} fileName={FileName} mimeType={MimeType} size={Size} user={User}", - reportid, file.Name, file.MimeType(), content?.Length ?? 0, UserAccountID); + reportid, fileName, mimeType, content.Length, UserAccountID); if (Form("typ") != "img") - return await FileContentResultAsync(content!, file.MimeType(), file.Name); + return await FileContentResultAsync(content, mimeType, fileName); return await PdfPreviewAsync("inv/rdoc", reportid, async () => await JSONAsync(new { id = reportid, img = await BuildPdfImageArray(content!) })); } + /// + /// Download filename for a regenerated document. DocumentName is derived from the user-typed + /// invoice title, so it can carry path separators ("Wartung 2025/26") and stray editor markup; + /// mirrors the cleanup FdsMfr.GetReportDoc applies to stored documents. + /// + internal static string SafeDocumentFileName(string documentName, string fallback) + { + string name = (documentName ?? "").Replace("

", "").Replace("

", "").Trim(); + if (name.Length == 0) name = fallback; + foreach (char c in Path.GetInvalidFileNameChars()) + name = name.Replace(c, '_'); + return name.Length == 0 ? fallback : name; + } + private async Task HandleReportDocByName(string fn, string id, string code) { if (!HasForm("name")) { _logger.LogWarning("HandleReportDocByName: missing 'name' form field user={User}", UserAccountID); return BadRequest400(); } diff --git a/Fuchs/Docs/Decisions/0025-invoice-document-regenerate-on-miss.md b/Fuchs/Docs/Decisions/0025-invoice-document-regenerate-on-miss.md index 8e0abd8..d7575c0 100644 --- a/Fuchs/Docs/Decisions/0025-invoice-document-regenerate-on-miss.md +++ b/Fuchs/Docs/Decisions/0025-invoice-document-regenerate-on-miss.md @@ -42,12 +42,13 @@ A read is therefore allowed to write `[file]`, upload the blob and emit Every failure inside `StoreInvoiceDocumentFileAsync`/`StoreReminderDocumentFileAsync` (render threw, render produced no bytes, DB write threw, DB write matched 0 rows) is logged at Error level individually, because the method signals all -four to callers with the same empty array. The 0-rows case needed switching -from `setSQLValue_async` (reports success once `ExecuteNonQueryAsync` -completes without throwing, regardless of rows affected) to -`setSQLValue_async_result`, whose `AffectedRows` makes a no-op `UPDATE ... -WHERE [file] IS NULL` distinguishable from an actual write — this is what -made the original incident's failure invisible to every log. +four to callers with the same empty array. The 0-rows case is detected via +the procs' existing `OUTPUT inserted.[Id]` result set (read with +`getSQLDatatable_async`; empty ⇒ the `UPDATE ... WHERE [file] IS NULL` +matched nothing). Rows-affected is **not** usable for this: both procs run +`SET NOCOUNT ON`, so `ExecuteNonQuery` returns -1 whether or not the +`UPDATE` matched, which is why `setSQLValue_async` could never distinguish +a no-op from a write and the original incident was invisible to every log. ## Consequences diff --git a/Fuchs/Services/InvoiceService.cs b/Fuchs/Services/InvoiceService.cs index 4758fb1..ce849eb 100644 --- a/Fuchs/Services/InvoiceService.cs +++ b/Fuchs/Services/InvoiceService.cs @@ -447,17 +447,16 @@ public class InvoiceService : IInvoiceService SQL_VarChar("@Id", invoice.Id), new("@file", SqlDbType.VarBinary) { Value = ba } }; - var setResult = await setSQLValue_async_result( + // fds__setInvoiceFile writes WHERE [file] IS NULL and returns the updated Id via its + // OUTPUT clause, so an empty result set means the UPDATE matched nothing (already stored, + // wrong id, invoice gone) without any SQL error. Rows-affected cannot be used for this: + // the proc runs SET NOCOUNT ON, so ExecuteNonQuery reports -1 either way (ADR 0025). + var stored = await getSQLDatatable_async( "EXECUTE [dbo].[fds__setInvoiceFile] @Id, @file;", Conn, pl, Security: dbSec, options: new FIS_SQLOptions()); - if (!setResult.success) return Array.Empty(); - if (setResult.AffectedRows == 0) + if (!string.IsNullOrEmpty(stored.Exception)) return Array.Empty(); + if (stored.Count == 0) { - // fds__setInvoiceFile writes WHERE [file] IS NULL — 0 rows affected without an - // exception means the UPDATE simply didn't match (already stored, wrong id, or the - // invoice vanished under us), not a SQL failure setSQLValue_async_result would report. - // ExecuteNonQueryAsync returning rows-affected=0 is not an error by itself, so this was - // previously indistinguishable from success — see ADR 0025. _logger.LogError( "StoreInvoiceDocumentFileAsync: fds__setInvoiceFile updated 0 rows for {Id} (file already stored, or invoice missing), user={User}", invoice.Id, userAccountId); diff --git a/Fuchs/Services/ReminderService.cs b/Fuchs/Services/ReminderService.cs index 3c20871..2f3b278 100644 --- a/Fuchs/Services/ReminderService.cs +++ b/Fuchs/Services/ReminderService.cs @@ -158,14 +158,14 @@ public class ReminderService : IReminderService SQL_VarChar("@authuser", userAccountId), new("@file", SqlDbType.VarBinary) { Value = ba } }; - var setResult = await setSQLValue_async_result( + // OUTPUT-clause row count, not rows-affected (SET NOCOUNT ON) — see the matching note in + // InvoiceService.StoreInvoiceDocumentFileAsync / ADR 0025. + var stored = await getSQLDatatable_async( "EXECUTE [dbo].[fds__setReminderFile] @Id, @file;", Conn, pl, Security: dbSec, options: new FIS_SQLOptions()); - if (!setResult.success) return Array.Empty(); - if (setResult.AffectedRows == 0) + if (!string.IsNullOrEmpty(stored.Exception)) return Array.Empty(); + if (stored.Count == 0) { - // fds__setReminderFile writes WHERE [file] IS NULL — see the matching comment/ADR 0025 - // note in InvoiceService.StoreInvoiceDocumentFileAsync. _logger.LogError( "StoreReminderDocumentFileAsync: fds__setReminderFile updated 0 rows for {Id} (file already stored, or reminder missing), user={User}", reminder.Id, userAccountId);