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>
4.0 KiB
status, date, applyTo, supersededBy
| status | date | applyTo | supersededBy | |||||
|---|---|---|---|---|---|---|---|---|
| Accepted | 2026-09-16 |
|
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 (
IsExternalfalse — 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__setInvoiceFilewritesWHERE [file] IS NULLandfds__setInvoiceFinalnever 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!IsDraftgate 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__setInvoiceFiletimeout that caused the miss. The client preview timeout ($inv.pdfPreviewTimeout) must stay generous (120 s). inv/rdocstill serves external (MFR) documents purely fromfds__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/idocalready had. - Regenerate drafts too, to mirror
req/idoc's render path.req/idocrenders a draft on the fly without storing it; persisting a draft throughfds__setInvoiceFileis what causes the permanent-draft defect above. - Return 404 and rely on the
_BackupAndClearInvoiceFilerecovery links. That proc is a manual-only maintenance tool, and theinv/rdoc?create=1link it prints was never implemented on that endpoint.