fix: detect no-op document stores via OUTPUT rowset; sanitise regenerated filename
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>
This commit is contained in:
@@ -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<byte>();
|
||||
if (setResult.AffectedRows == 0)
|
||||
if (!string.IsNullOrEmpty(stored.Exception)) return Array.Empty<byte>();
|
||||
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);
|
||||
|
||||
@@ -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<byte>();
|
||||
if (setResult.AffectedRows == 0)
|
||||
if (!string.IsNullOrEmpty(stored.Exception)) return Array.Empty<byte>();
|
||||
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);
|
||||
|
||||
Reference in New Issue
Block a user