timothyandClaude Fable 5.1
e7f794057d
Three of the four findings standing on the 2026-09-05 16:24 review verdict, which the branch had not answered. The count cap is the blocking one. The three id-list validators took whatever the request carried, so the only bound on `graphicsElementIds`/`watermarkIds` was the Kestrel body cap -- a transport limit, not a collection limit. The earlier disposition deferred it to #917 on the grounds that `ApplyUpdateRequest` reconciles the same list uncapped anyway; that is true and does not answer the ask, because the reconcile is downstream of a validator that can refuse the request outright. One shared `Validators.IdsMustExist` now carries the cap for all three, counted on the RAW list before `Distinct` (a million copies of one id costs the same to parse and materialize whatever the distinct count is) and before any database work. The same helper is where the field name and the diagnostic cap now live. The 422 said "Graphics element(s) do not exist: 999" without naming which request field carried the 999, and echoed every rejected id -- an oversized request answered with an oversized response. Both fixed once, in the shared place, so the three sites cannot drift. `Kind` moves into `GraphicsElementDefaults.IsOnNowNext`. The seeder required `Kind == Text` and the API's `builtIn` did not, so an Image row at the exact seeded path was `builtIn:true` on the wire while `GetBuiltInElementId` refused to treat it as the built-in element -- two sites disagreeing about one row, which is the shape #568 exists to close. Identity is now one predicate applied whole at both sites; the seeder's SQL `Kind` filter is gone rather than kept as a duplicate, since a duplicate guard would mask the predicate's own clause. Also the fourth finding, the check-then-write race: `RefreshGraphicsElements` can delete a validated element between `Validate` and `SaveChangesAsync`, handing the join insert the FK violation the validator exists to prevent. A transaction does not close it -- neither provider locks rows the validator merely read -- so both handlers catch `DbUpdateException`, re-ask the existence question on a fresh context, and return the validator's own 422 when an id has since gone; anything else keeps its own exception. Foreign keys are off in `InMemoryTvContext`, so the trigger is simulated by an armed save-failure interceptor while the recovery itself runs against real post-delete state. Refs #568 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
53 lines
3.2 KiB
C#
53 lines
3.2 KiB
C#
using System.IO;
|
|
using ErsatzTV.Core.Domain;
|
|
|
|
namespace ErsatzTV.Core.Graphics;
|
|
|
|
public static class GraphicsElementDefaults
|
|
{
|
|
// Built-in "On Now / Next" text element filename -- a component of OnNowNextSeededPath below,
|
|
// never itself an identity check (#568: a filename-only comparison is folder-agnostic).
|
|
public const string OnNowNextFileName = "on-now-next.yml";
|
|
|
|
// Display name only. Never use it for identity -- that is IsOnNowNext below (#67 / #74 / #568).
|
|
public const string OnNowNextName = "On Now / Next";
|
|
|
|
// The full path the seeder writes the built-in template to (GraphicsElementSeeder.SeedOnNowNext
|
|
// builds `target` the same way). A `builtIn` discriminator must match THIS, not
|
|
// `Path.GetFileName(...) == OnNowNextFileName` -- a filename-only comparison is folder-agnostic:
|
|
// a user element named exactly `on-now-next.yml` in any of the other four template folders
|
|
// (image/motion/subtitle/script) would also report `builtIn:true` (#568). `Kind == Text` alone
|
|
// does not close this either, since a second text template could share the filename in principle.
|
|
public static string OnNowNextSeededPath =>
|
|
Path.Combine(FileSystemLayout.GraphicsElementsTextTemplatesFolder, OnNowNextFileName);
|
|
|
|
/// <summary>
|
|
/// The one identity test for the built-in On Now / Next element: the exact file the seeder
|
|
/// wrote, at the exact path it wrote it to, of the kind it wrote it as. Ordinal on purpose,
|
|
/// and so case-sensitive on purpose.
|
|
/// </summary>
|
|
/// <remarks>
|
|
/// <para>
|
|
/// <c>Kind</c> is part of the identity rather than a second test any caller may add or
|
|
/// skip: the seeder's own "does the built-in row exist yet?" check resolves through this
|
|
/// predicate, so a row of another kind at the seeded path answering yes would suppress
|
|
/// the Text row every consumer resolves. A caller applying only the path half would
|
|
/// report that wrong-kind row as the built-in element while the seeder refused to treat
|
|
/// it as one -- the two sites disagreeing about the same row, which is the defect this
|
|
/// predicate exists to make impossible (#568).
|
|
/// </para>
|
|
/// <para>
|
|
/// Callers compare in memory rather than in a <c>Where</c> clause, because in SQL the
|
|
/// answer would be the PROVIDER's to give: <c>GraphicsElement.Path</c> takes no explicit
|
|
/// collation (<c>TvContext.OnModelCreating</c> pins one only on the listed name/title
|
|
/// columns), so SQLite compares it case-sensitively while MySQL uses the server default,
|
|
/// which is normally case-INsensitive. Evaluating one discriminator site in SQL and the
|
|
/// other in memory would let the two disagree on MySQL alone. The SQLite test suite
|
|
/// cannot tell the two apart -- BINARY collation and an ordinal comparison agree on
|
|
/// every input -- so this is held by keeping the comparison out of SQL, not by a test.
|
|
/// </para>
|
|
/// </remarks>
|
|
public static bool IsOnNowNext(string path, GraphicsElementKind kind) =>
|
|
kind == GraphicsElementKind.Text && string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal);
|
|
}
|