StefanandClaude Opus 5
4cac109283
Follow-up to 99bf533 from code review:
- The AffectedRows == 0 guard in Store{Invoice,Reminder}DocumentFileAsync
was dead: fds__setInvoiceFile / fds__setReminderFile run SET NOCOUNT ON,
so ExecuteNonQuery returns -1 whether or not the UPDATE ... WHERE [file]
IS NULL matched. Both procs already return the updated Id through their
OUTPUT clause, so read that result set (getSQLDatatable_async) instead:
empty rowset = no-op store, logged, and the blob upload / FileCreated
event are skipped rather than reporting unpersisted bytes as success.
- inv/rdoc's regenerate path built the download name from the raw
DocumentName; a user-typed title with '/' truncated FileInfo.Name.
SafeDocumentFileName strips editor markup and every invalid filename
char, and the path no longer goes through FileInfo at all.
- Reminder counterpart tests for the store-file failure branches.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
80 lines
4.0 KiB
Markdown
80 lines
4.0 KiB
Markdown
---
|
|
status: Accepted
|
|
date: 2026-09-16
|
|
applyTo:
|
|
- "Fuchs/Controllers/IntranetController.Invoices*.cs"
|
|
- "Fuchs/Controllers/IntranetController.Requests.cs"
|
|
- "Fuchs/Services/InvoiceService.cs"
|
|
- "Fuchs_Database/dbo/Stored Procedures/fds__getReportDocument.sql"
|
|
- "Fuchs_Database/dbo/Stored Procedures/fds__setInvoiceFile.sql"
|
|
supersededBy: ""
|
|
---
|
|
|
|
# 0025 — Invoice document endpoints regenerate a missing final PDF on read
|
|
|
|
## Context
|
|
|
|
A finalised invoice's PDF is rendered exactly once, at finalisation
|
|
(`req/sconf` → `StoreInvoiceDocumentFileAsync` → `fds__setInvoiceFile`), and
|
|
stored in `fds__invoices.[file]` (primary) plus blob storage (archive). If that
|
|
single write fails — observed on 2026-09-16 for `W7VAL`/`R2026-0258`, where the
|
|
`fds__setInvoiceFile` call timed out under DB load — the invoice is left
|
|
finalised with `[file] = NULL` and nothing ever retries.
|
|
|
|
The read endpoints then disagreed: `req/idoc` regenerated a missing file
|
|
(a fallback carried over from the legacy VB `fuchs_ocms_intranet_requests.vb`
|
|
"idoc" case), while `inv/rdoc` — the endpoint the invoice list's preview icon
|
|
actually calls — read `fds__getReportDocument` and returned a permanent 404.
|
|
|
|
## Decision
|
|
|
|
Both invoice document read endpoints (`req/idoc`, `inv/rdoc`) regenerate and
|
|
persist the PDF via `StoreInvoiceDocumentFileAsync` when no stored bytes exist,
|
|
**only** when the invoice is
|
|
|
|
- **finalised** (`IsFinal`, i.e. `!FdsInvoiceData.IsDraft`), and
|
|
- **not external** (`IsExternal` false — an MFR-sourced document has no
|
|
template to regenerate from; a missing MFR file stays a 404).
|
|
|
|
A read is therefore allowed to write `[file]`, upload the blob and emit
|
|
`InvoiceFileCreated` — the same side effects finalisation would have had.
|
|
|
|
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 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
|
|
|
|
- **Drafts are never persisted by a read.** `fds__setInvoiceFile` writes
|
|
`WHERE [file] IS NULL` and `fds__setInvoiceFinal` never clears `[file]`; a
|
|
draft render stored early would make the later final write a no-op and the
|
|
invoice would carry the watermarked draft PDF forever. The `!IsDraft` gate is
|
|
load-bearing — do not relax it.
|
|
- The regenerated PDF is rendered from the invoice's *current* data. That is
|
|
acceptable because a finalised invoice is immutable in the editor; it is not
|
|
acceptable for anything mutable.
|
|
- A read on a cold miss now costs a render plus a DB write, so it is exposed
|
|
to the same `fds__setInvoiceFile` timeout that caused the miss. The client
|
|
preview timeout (`$inv.pdfPreviewTimeout`) must stay generous (120 s).
|
|
- `inv/rdoc` still serves external (MFR) documents purely from
|
|
`fds__getReportDocument`; regeneration never applies to them.
|
|
|
|
## Alternatives considered
|
|
|
|
- **Retry at finalisation instead of on read.** Would cover the store failure
|
|
but not an already-damaged row; regeneration on read heals both and matches
|
|
the behaviour `req/idoc` already had.
|
|
- **Regenerate drafts too, to mirror `req/idoc`'s render path.** `req/idoc`
|
|
*renders* a draft on the fly without storing it; persisting a draft through
|
|
`fds__setInvoiceFile` is what causes the permanent-draft defect above.
|
|
- **Return 404 and rely on the `_BackupAndClearInvoiceFile` recovery links.**
|
|
That proc is a manual-only maintenance tool, and the `inv/rdoc?create=1`
|
|
link it prints was never implemented on that endpoint.
|