From 900dac983214e81e1c9a7b006f89a3312b50fd39 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 08:33:18 +0200 Subject: [PATCH 01/19] fix(568): reject unknown channel graphicsElementIds with 422; discriminate builtIn by seeded path not filename UpdateChannelHandler.Validate never checked incoming graphicsElementIds against GraphicsElements, so PUT /api/v1/channels/{id} with a non-existent id hit FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId at SaveChangesAsync and surfaced as an unhandled 500. Add GraphicsElementIdsMustExist, following the existing FFmpegProfileMustExist/WatermarkMustExist/FillerPresetMustExist shape, so an unknown id now returns 422 for parity with every other FK field on this full-replace DTO. GetAllGraphicsElementsForApiHandler and GraphicsElementSeeder.GetBuiltInElementId keyed builtIn off Path.GetFileName(e.Path) == OnNowNextFileName -- folder-agnostic, so a user element named exactly on-now-next.yml in any other template folder would also report builtIn:true. Both now compare against GraphicsElementDefaults.OnNowNextSeededPath, the full path the seeder actually writes to. Follow-up from the #74 whole-branch review (2026-07-22), deferred as data-safe/not SPA-reachable. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Channels/Commands/UpdateChannelHandler.cs | 37 +++++++++++++-- .../GetAllGraphicsElementsForApiHandler.cs | 2 +- .../Graphics/GraphicsElementDefaults.cs | 11 +++++ .../Graphics/GraphicsElementSeeder.cs | 29 ++++++------ ...reateChannelDefaultGraphicsElementTests.cs | 2 +- .../CreateChannelFromLineupHandlerTests.cs | 2 +- .../UpdateChannelGraphicsElementsTests.cs | 45 +++++++++++++++++++ .../Graphics/GraphicsElementHandlerTests.cs | 39 ++++++++++++++++ .../GraphicsElementDefaultAttachTests.cs | 27 ++++++++++- 9 files changed, 171 insertions(+), 23 deletions(-) diff --git a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs index 294a3736b..5d5964514 100644 --- a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs @@ -229,14 +229,15 @@ public class UpdateChannelHandler( .Apply((_, _, _, _, _) => channel); // combine the page-only Group rule plus the FK existence checks (FFmpeg profile / watermark / - // fallback filler) with the channel validation; splitting keeps tuple arity within - // LanguageExt's supported applicative range while still accumulating all errors + // fallback filler / graphics elements) with the channel validation; splitting keeps tuple + // arity within LanguageExt's supported applicative range while still accumulating all errors return (ValidateGroup(request.Group), await FFmpegProfileMustExist(dbContext, request, cancellationToken), await WatermarkMustExist(dbContext, request, cancellationToken), await FillerPresetMustExist(dbContext, request, cancellationToken), + await GraphicsElementIdsMustExist(dbContext, request, cancellationToken), channelValidation) - .Apply((_, _, _, _, c) => c); + .Apply((_, _, _, _, _, c) => c); } private static async Task> FFmpegProfileMustExist( @@ -295,6 +296,36 @@ public class UpdateChannelHandler( return BaseError.New($"Fallback filler {request.FallbackFillerId} does not exist."); } + // The reconcile in ApplyUpdateRequest blindly Adds a ChannelGraphicsElement for every incoming + // id; an id with no matching GraphicsElement row would otherwise hit + // FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId at SaveChangesAsync and surface as + // an unhandled 500 (there is no global exception filter). Reject it here instead, for parity + // with every other FK field on this full-replace DTO (#568). + private static async Task> GraphicsElementIdsMustExist( + TvContext dbContext, + UpdateChannel request, + CancellationToken cancellationToken) + { + List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; + if (requested.Count == 0) + { + return Unit.Default; + } + + List existingIds = await dbContext.GraphicsElements + .Where(e => requested.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(cancellationToken); + + List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); + if (missingIds.Count == 0) + { + return Unit.Default; + } + + return BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}"); + } + private static async Task> MirrorSourceMustBeValid( TvContext dbContext, UpdateChannel request, diff --git a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs index b98f56e22..da7899685 100644 --- a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs +++ b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs @@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory db .Select(e => new { Vm = ProjectToViewModel(e), - BuiltIn = Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName + BuiltIn = e.Path == GraphicsElementDefaults.OnNowNextSeededPath }) .OrderBy(x => x.Vm.Name == x.Vm.FileName) .ThenBy(x => x.Vm.Name) diff --git a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs index 6b5c6f778..042a65e61 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -1,3 +1,5 @@ +using System.IO; + namespace ErsatzTV.Core.Graphics; public static class GraphicsElementDefaults @@ -7,4 +9,13 @@ public static class GraphicsElementDefaults // Display name only. Never use it for identity -- that is the filename above (#67 / #74). public const string OnNowNextName = "On Now / Next"; + + // The full path the seeder writes the built-in template to. A `builtIn` discriminator must + // match THIS, not `Path.GetFileName(...) == OnNowNextFileName` -- a filename-only comparison is + // case-sensitive-by-accident and 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); } diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index 98067ebb7..5152371c8 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -246,29 +246,26 @@ public static class GraphicsElementSeeder } /// - /// Identity is the filename, never the user-editable Name (the #67 lesson carried into #74). - /// The Kind is part of it: the five template folders are separate namespaces, so an unrelated - /// image/motion/subtitle/script element may legitimately be named `on-now-next.yml` too, and - /// filename alone would hand back whichever row the unordered query happened to return first. + /// Identity is the full seeded path, never the user-editable Name (the #67 lesson carried into + /// #74) and never the bare filename (#568: filename-only matching is folder-agnostic, so a user + /// element named exactly `on-now-next.yml` in a different template folder would also match). + /// The Kind filter stays as belt-and-braces since `OnNowNextSeededPath` is itself a Text-folder + /// path. /// public static async Task> GetBuiltInElementId( TvContext context, CancellationToken cancellationToken) { - List<(int Id, string Path)> candidates = await context.GraphicsElements + List matches = await context.GraphicsElements .Where(e => e.Kind == GraphicsElementKind.Text) - .Select(e => new { e.Id, e.Path }) - .ToListAsync(cancellationToken) - .Map(rows => rows.Select(r => (r.Id, r.Path)).ToList()); + .Where(e => e.Path == GraphicsElementDefaults.OnNowNextSeededPath) + .Select(e => e.Id) + .OrderBy(id => id) + .ToListAsync(cancellationToken); - var matches = candidates - .Where(c => System.IO.Path.GetFileName(c.Path) == GraphicsElementDefaults.OnNowNextFileName) - .OrderBy(c => c.Id) - .ToList(); - - // Lowest id wins if two text templates somehow share the filename, so the choice is stable - // across restarts rather than dependent on query order. - return matches.Count == 0 ? Option.None : matches[0].Id; + // Lowest id wins if two rows somehow share the seeded path, so the choice is stable across + // restarts rather than dependent on query order. + return matches.Count == 0 ? Option.None : matches[0]; } private static async Task UpgradeUnmodifiedTemplate( diff --git a/ErsatzTV.Tests/Application/Channels/CreateChannelDefaultGraphicsElementTests.cs b/ErsatzTV.Tests/Application/Channels/CreateChannelDefaultGraphicsElementTests.cs index 1769def48..5740ab1ab 100644 --- a/ErsatzTV.Tests/Application/Channels/CreateChannelDefaultGraphicsElementTests.cs +++ b/ErsatzTV.Tests/Application/Channels/CreateChannelDefaultGraphicsElementTests.cs @@ -26,7 +26,7 @@ public class CreateChannelDefaultGraphicsElementTests : ChannelHandlerTestBase await using TvContext context = Db.CreateContext(); var element = new GraphicsElement { - Path = $"/templates/text/{GraphicsElementDefaults.OnNowNextFileName}", + Path = GraphicsElementDefaults.OnNowNextSeededPath, Kind = GraphicsElementKind.Text }; diff --git a/ErsatzTV.Tests/Application/Channels/CreateChannelFromLineupHandlerTests.cs b/ErsatzTV.Tests/Application/Channels/CreateChannelFromLineupHandlerTests.cs index 430d15008..ae9350bac 100644 --- a/ErsatzTV.Tests/Application/Channels/CreateChannelFromLineupHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Channels/CreateChannelFromLineupHandlerTests.cs @@ -1020,7 +1020,7 @@ public class CreateChannelFromLineupHandlerTests await using TvContext context = _db.CreateContext(); var element = new GraphicsElement { - Path = $"/templates/text/{GraphicsElementDefaults.OnNowNextFileName}", + Path = GraphicsElementDefaults.OnNowNextSeededPath, Kind = GraphicsElementKind.Text }; diff --git a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs index f99159c1e..bb981fde0 100644 --- a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs @@ -1,6 +1,7 @@ using ErsatzTV.Application.Channels; using ErsatzTV.Core; using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Errors; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; using LanguageExt; @@ -15,6 +16,9 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase { private UpdateChannelHandler MakeHandler() => new(Worker, Db.Factory, SearchTargets, RemoteLogoCacher); + private static BaseError LeftOf(Either either) => + either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); + private async Task<(int ElementAId, int ElementBId)> SeedGraphicsElements() { await using TvContext context = Db.CreateContext(); @@ -73,4 +77,45 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase reloaded.ChannelGraphicsElements.ShouldBeEmpty(); } } + + // #568: an unknown graphicsElementIds entry used to reach ApplyUpdateRequest unchecked, which + // blindly Adds a ChannelGraphicsElement and lets SaveChangesAsync hit + // FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId -> unhandled 500. Reddens if the + // GraphicsElementIdsMustExist validator alone is removed from UpdateChannelHandler.Validate. + [Test] + public async Task Should_Reject_Unknown_GraphicsElementId_With_422_Not_500() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: [999]), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.ShouldNotBeOfType(); + error.Value.ShouldContain("999"); + + // no partial write: the channel keeps no graphics element association + await using TvContext context = Db.CreateContext(); + Channel reloaded = await context.Channels.Include(c => c.ChannelGraphicsElements) + .SingleAsync(c => c.Id == channel.Id); + reloaded.ChannelGraphicsElements.ShouldBeEmpty(); + } + + [Test] + public async Task Should_Reject_When_One_Of_Several_GraphicsElementIds_Is_Unknown() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId, 12345]), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.ShouldNotBeOfType(); + error.Value.ShouldContain("12345"); + } } diff --git a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs index d68fa1ca0..1d1500efb 100644 --- a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs @@ -1,6 +1,8 @@ using ErsatzTV.Application.Graphics; +using ErsatzTV.Core; using ErsatzTV.Core.Api.Graphics; using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Graphics; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; using NUnit.Framework; @@ -62,6 +64,43 @@ public class GraphicsElementHandlerTests result.ShouldBeEmpty(); } + // #568: the discriminator used to be Path.GetFileName(e.Path) == OnNowNextFileName, which is + // folder-agnostic -- a user element named exactly "on-now-next.yml" outside the seeded text + // template folder would also report builtIn:true. Reddens if the fix (compare the full seeded + // path) is reverted to a filename-only comparison. + [Test] + public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn() + { + string userElementPath = System.IO.Path.Combine("/config/graphics-elements/image", GraphicsElementDefaults.OnNowNextFileName); + await SeedElement(1, userElementPath, GraphicsElementKind.Image, string.Empty); + + var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); + + List result = + await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None); + + result.Count.ShouldBe(1); + result[0].BuiltIn.ShouldBeFalse(); + } + + [Test] + public async Task GetAllGraphicsElementsForApi_Should_Mark_The_Seeded_Path_As_BuiltIn() + { + await SeedElement( + 1, + GraphicsElementDefaults.OnNowNextSeededPath, + GraphicsElementKind.Text, + GraphicsElementDefaults.OnNowNextName); + + var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); + + List result = + await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None); + + result.Count.ShouldBe(1); + result[0].BuiltIn.ShouldBeTrue(); + } + private async Task SeedElement(int id, string path, GraphicsElementKind kind, string name) { await using TvContext context = _db.CreateContext(); diff --git a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index f06c129bf..5ed293f28 100644 --- a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs +++ b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs @@ -35,7 +35,7 @@ public class GraphicsElementDefaultAttachTests { var element = new GraphicsElement { - Path = $"/templates/text/{GraphicsElementDefaults.OnNowNextFileName}", + Path = GraphicsElementDefaults.OnNowNextSeededPath, Name = "On Now / Next", Kind = GraphicsElementKind.Text }; @@ -189,6 +189,31 @@ public class GraphicsElementDefaultAttachTests (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); } + // #568: filename-only matching was folder-agnostic -- a user Text element named exactly + // "on-now-next.yml" outside the seeded text-template folder used to also count as built-in. + // Same Kind as the real seeded row, deliberately a different folder, so this only reddens if + // GetBuiltInElementId goes back to comparing Path.GetFileName(...) instead of the full path. + [Test] + public async Task Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder() + { + await using TvContext context = _db.CreateContext(); + await context.GraphicsElements.AddAsync( + new GraphicsElement + { + Path = System.IO.Path.Combine( + "/config/graphics-elements/text/some-subfolder", + GraphicsElementDefaults.OnNowNextFileName), + Kind = GraphicsElementKind.Text + }); + await context.SaveChangesAsync(); + + Channel channel = await SeedChannel(context, "1"); + + await GraphicsElementSeeder.AttachOnNowNextByDefault(context, CancellationToken.None); + + (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); + } + // The marker is permanent, so writing it with nothing resolved would strand every channel. Stay // armed instead and pick the work up once the element exists. [Test] From c84bdd2427a4d8e72ffa3ef135af0b0264685563 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 08:34:07 +0200 Subject: [PATCH 02/19] docs(568): record the full-seeded-path builtIn discriminator in graphics-elements.md Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- docs/graphics-elements.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 2e1bd9d06..62de5d63f 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -121,8 +121,12 @@ never overwrites an operator's file. Two rules govern it after that: - Updating the shipped default → `graphics.seeded-template-upgrade-by-fingerprint`. - It is attached to channels by default → `graphics.on-now-next-on-by-default`. -Identity is the **filename** (`GraphicsElementDefaults.OnNowNextFileName`), never the editable `Name`; -`GraphicsElementResponseModel.BuiltIn` is derived from it server-side so the SPA never name-matches. +Identity is the **full seeded path** (`GraphicsElementDefaults.OnNowNextSeededPath`), never the editable +`Name` and never the bare filename (`OnNowNextFileName`) alone — a filename-only, folder-agnostic +comparison let a user element named exactly `on-now-next.yml` in a different template folder also +report `builtIn:true` (#568). `GraphicsElementResponseModel.BuiltIn` and +`GraphicsElementSeeder.GetBuiltInElementId` both compare against the full path server-side so the SPA +never name-matches. ## Tests From 352305ade8afb0215d1be721518e8fdc914e00dd Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 09:36:43 +0200 Subject: [PATCH 03/19] fix(568): sync docs/comments to the seeded-path builtIn discriminator, harden UpdateDecoHandler's twin FK ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round on #568 found the branch changed builtIn identity from a bare filename comparison to the full seeded path (GraphicsElementDefaults. OnNowNextSeededPath) but left several places still asserting the old rule: - docs/decisions/records/graphics/channel-level-attachment.md and on-now-next-on-by-default.md (both status: active) still described a filename-only match; corrected in place and cross-referenced. - docs/api-conventions.md §8 quoted the retired `Path.GetFileName(element.Path) == OnNowNextFileName` expression verbatim; replaced with the current OnNowNextSeededPath comparison and a note on the UpdateChannelHandler 422 hardening. - Three in-code comments (GraphicsElementDefaults.cs, GraphicsElementSeeder.cs, ChannelGraphicsDefaults.cs) still said "identity is the filename". - docs/graphics-elements.md's mutation-coverage table (row 10, row 18) named clauses that no longer exist or no longer redden any test post-#568; re-measured directly (removing the seeded-path check reddens Ignores_A_Non_Built_In_Element_With_A_Different_Filename and Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder; removing the Kind==Text filter alone reddens nothing, so it moves to the "known clauses with no red" list with that measurement dated). Also closed the should-fix twin: UpdateDecoHandler's graphicsElementIds and watermarkIds are top-level ReplaceDecoRequest fields in the same position as UpdateChannelRequest.graphicsElementIds (not the deep-FK-in-a-nested-list carve-out), and the reconcile in ApplyUpdateRequest blindly Added a join row for any incoming id -- the identical FK-constraint-to-500 defect #568 fixed on the channel path. Added GraphicsElementIdsMustExist/WatermarkIdsMustExist validators mirroring UpdateChannelHandler's, pinned by UpdateDecoGraphicsElementsTests (reddens when either validator alone is removed -- verified). Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Channels/ChannelGraphicsDefaults.cs | 3 +- .../Scheduling/Commands/UpdateDecoHandler.cs | 56 +++++- .../Graphics/GraphicsElementDefaults.cs | 6 +- .../Graphics/GraphicsElementSeeder.cs | 5 +- .../UpdateDecoGraphicsElementsTests.cs | 173 ++++++++++++++++++ docs/api-conventions.md | 17 +- .../graphics/channel-level-attachment.md | 7 +- .../graphics/on-now-next-on-by-default.md | 6 +- docs/graphics-elements.md | 12 +- 9 files changed, 266 insertions(+), 19 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs diff --git a/ErsatzTV.Application/Channels/ChannelGraphicsDefaults.cs b/ErsatzTV.Application/Channels/ChannelGraphicsDefaults.cs index f9d519e17..4f729443e 100644 --- a/ErsatzTV.Application/Channels/ChannelGraphicsDefaults.cs +++ b/ErsatzTV.Application/Channels/ChannelGraphicsDefaults.cs @@ -23,7 +23,8 @@ public static class ChannelGraphicsDefaults { // HLS Direct is skipped because ErsatzTV is not transcoding there -- there is no frame // pipeline to draw into, and the editor disables the toggle for the same reason. Identity is - // the element's filename, never its user-editable Name (the #67 lesson). + // the element's full seeded path (`GraphicsElementDefaults.OnNowNextSeededPath`), never its + // user-editable Name (the #67 lesson, sharpened from filename to full path by #568). if (channel.StreamingMode is StreamingMode.HttpLiveStreamingDirect) { return; diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index dd88739a3..d639f8f99 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -218,8 +218,60 @@ public class UpdateDecoHandler( UpdateDeco request, CancellationToken cancellationToken) => (await DecoMustExist(dbContext, request, cancellationToken), await ValidateDecoName(dbContext, request), - ValidateBreakContent(request)) - .Apply((deco, _, _) => deco); + ValidateBreakContent(request), + await WatermarkIdsMustExist(dbContext, request, cancellationToken), + await GraphicsElementIdsMustExist(dbContext, request, cancellationToken)) + .Apply((deco, _, _, _, _) => deco); + + // Mirrors UpdateChannelHandler.GraphicsElementIdsMustExist (#568): the reconcile in + // ApplyUpdateRequest blindly Adds a DecoWatermark/DecoGraphicsElement for every incoming id, and + // an id with no matching row hits the FK constraint at SaveChangesAsync and surfaces as an + // unhandled 500 (there is no global exception filter). These are top-level fields on + // ReplaceDecoRequest, the same position as graphicsElementIds on UpdateChannelRequest -- not the + // "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md. + private static async Task> WatermarkIdsMustExist( + TvContext dbContext, + UpdateDeco request, + CancellationToken cancellationToken) + { + List requested = request.WatermarkIds?.Distinct().ToList() ?? []; + if (requested.Count == 0) + { + return Unit.Default; + } + + List existingIds = await dbContext.ChannelWatermarks + .Where(w => requested.Contains(w.Id)) + .Select(w => w.Id) + .ToListAsync(cancellationToken); + + List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); + return missingIds.Count == 0 + ? Unit.Default + : BaseError.New($"Watermark(s) do not exist: {string.Join(", ", missingIds)}"); + } + + private static async Task> GraphicsElementIdsMustExist( + TvContext dbContext, + UpdateDeco request, + CancellationToken cancellationToken) + { + List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; + if (requested.Count == 0) + { + return Unit.Default; + } + + List existingIds = await dbContext.GraphicsElements + .Where(e => requested.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(cancellationToken); + + List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); + return missingIds.Count == 0 + ? Unit.Default + : BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}"); + } private static Task> DecoMustExist( TvContext dbContext, diff --git a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs index 042a65e61..aefa0977d 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -4,10 +4,12 @@ namespace ErsatzTV.Core.Graphics; public static class GraphicsElementDefaults { - // Built-in "On Now / Next" text element; identity is by filename, never by user-editable Name. + // Built-in "On Now / Next" text element filename -- a component of OnNowNextSeededPath below, + // never itself an identity check (a filename-only comparison is case-sensitive-by-accident and + // folder-agnostic; see OnNowNextSeededPath, #568). public const string OnNowNextFileName = "on-now-next.yml"; - // Display name only. Never use it for identity -- that is the filename above (#67 / #74). + // Display name only. Never use it for identity -- that is OnNowNextSeededPath below (#67 / #74 / #568). public const string OnNowNextName = "On Now / Next"; // The full path the seeder writes the built-in template to. A `builtIn` discriminator must diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index 5152371c8..8fcd8022c 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -179,8 +179,9 @@ public static class GraphicsElementSeeder return; } - // Name is display-only (identity is the filename), but leaving it null sorts the built-in - // element into the unnamed bucket at the bottom of the SPA list until the first refresh. + // Name is display-only (identity is the full seeded path, `target` above -- #568), but + // leaving it null sorts the built-in element into the unnamed bucket at the bottom of the + // SPA list until the first refresh. await context.GraphicsElements.AddAsync( new Core.Domain.GraphicsElement { diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs new file mode 100644 index 000000000..42830f234 --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -0,0 +1,173 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Scheduling; + +/// +/// #568: the same full-replace-DTO FK hardening applied to UpdateChannelHandler's +/// graphicsElementIds also closes the identical twin defect in UpdateDecoHandler -- both +/// graphicsElementIds and watermarkIds are top-level ReplaceDecoRequest fields (not the "deep FK +/// ids nested inside item-list request bodies" carve-out in api-conventions.md), and the +/// reconcile in ApplyUpdateRequest blindly Adds a join row for every incoming id, so an unknown +/// id used to hit the FK constraint at SaveChangesAsync and surface as an unhandled 500. +/// +[TestFixture] +public class UpdateDecoGraphicsElementsTests +{ + private InMemoryTvContext _db = null!; + private ChannelWriter _channel = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _channel = Substitute.For>(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private static bool IsLeft(Either result) => result.Match(Right: _ => false, Left: _ => true); + + private async Task SeedDeco() + { + await using TvContext context = _db.CreateContext(); + context.Decos.Add( + new Deco + { + Id = 1, + DecoGroupId = 1, + Name = "D", + BreakContent = [], + DecoWatermarks = [], + DecoGraphicsElements = [] + }); + await context.SaveChangesAsync(); + } + + private static UpdateDeco MakeUpdate( + List graphicsElementIds = null, + List watermarkIds = null) => + new( + 1, + 1, + "D", + DecoMode.Inherit, + watermarkIds ?? [], + false, + graphicsElementIds is null ? DecoMode.Inherit : DecoMode.Override, + graphicsElementIds ?? [], + false, + DecoMode.Inherit, + [], + DecoMode.Inherit, + CollectionType.Collection, + null, + null, + null, + null, + false, + DecoMode.Inherit, + CollectionType.Collection, + null, + null, + null, + null); + + // Reddens if UpdateDecoHandler.GraphicsElementIdsMustExist alone is removed from Validate. + [Test] + public async Task Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementIds: [999]), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("999"); + + // no partial write: the deco keeps no graphics element association + await using TvContext context = _db.CreateContext(); + Deco reloaded = await context.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1); + reloaded.DecoGraphicsElements.ShouldBeEmpty(); + } + + [Test] + public async Task Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + new UpdateDeco( + 1, + 1, + "D", + DecoMode.Override, + [999], + false, + DecoMode.Inherit, + [], + false, + DecoMode.Inherit, + [], + DecoMode.Inherit, + CollectionType.Collection, + null, + null, + null, + null, + false, + DecoMode.Inherit, + CollectionType.Collection, + null, + null, + null, + null), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("999"); + } + + [Test] + public async Task Should_Accept_A_Known_GraphicsElementId() + { + await SeedDeco(); + + int elementId; + await using (TvContext context = _db.CreateContext()) + { + var element = new GraphicsElement { Path = "element-a.yml" }; + context.GraphicsElements.Add(element); + await context.SaveChangesAsync(); + elementId = element.Id; + } + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementIds: [elementId]), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + await using TvContext reload = _db.CreateContext(); + Deco reloaded = await reload.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1); + reloaded.DecoGraphicsElements.Select(x => x.GraphicsElementId).ShouldBe(new[] { elementId }); + } +} diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 8c2c61100..83b6126cf 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -935,11 +935,18 @@ for items that have no group, so the SPA can render an "ungrouped" bucket concept elsewhere, this is the established pattern to follow — but be aware it means `Id` is not a reliable real-entity id for those synthetic rows. -**Channel graphics (issue #74)**: `ChannelDetailResponseModel`/`UpdateChannelRequest` carry -`graphicsElementIds` (the channel's attached `GraphicsElement` ids, reconciled add/remove on PUT via -`Channel.ChannelGraphicsElements`), and `GraphicsElementResponseModel` exposes a server-derived -`builtIn` (`Path.GetFileName(element.Path) == GraphicsElementDefaults.OnNowNextFileName`) — never -client-settable. +**Channel graphics (issue #74, hardened #568)**: `ChannelDetailResponseModel`/`UpdateChannelRequest` +carry `graphicsElementIds` (the channel's attached `GraphicsElement` ids, reconciled add/remove on PUT +via `Channel.ChannelGraphicsElements`); `UpdateChannelHandler.Validate` rejects any id not present in +`GraphicsElements` with 422 (previously an unhandled 500 from the FK constraint — #568), matching +every other field on this full-replace DTO. The identical shape existed on `PUT /api/v1/decos/{id}` +(`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the deep-FK-in-a- +nested-list carve-out below) and is hardened the same way by `UpdateDecoHandler.Validate` +(`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). `GraphicsElementResponseModel` exposes +a server-derived +`builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as `element.Path == +GraphicsElementDefaults.OnNowNextSeededPath` (the full seeded path, not the bare filename — a +filename-only match was case-sensitive-by-accident and folder-agnostic, #568) — never client-settable. ## 9. Authentication — session-or-key posture (fail-closed) diff --git a/docs/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index f25a2b216..33bb41a10 100644 --- a/docs/decisions/records/graphics/channel-level-attachment.md +++ b/docs/decisions/records/graphics/channel-level-attachment.md @@ -6,7 +6,7 @@ since: '2026-07-22' supersedes: none superseded-by: none rule: A channel can attach `GraphicsElement`s directly via a new `ChannelGraphicsElement` join table (a base layer under deco/playout-item elements), and a built-in text element (`on-now-next.yml`) is seeded once per database so the On Now/Next overlay works out of the box. -signals: 'ChannelGraphicsElement, Channel graphics attachment, GraphicsElementSelector base layer, on-now-next seeded element, GraphicsElementDefaults.OnNowNextFileName, builtIn discriminator · paths: `ErsatzTV.Core/Domain/ChannelGraphicsElement.cs`, `ErsatzTV.Core/FFmpeg/GraphicsElementSelector.cs`, `ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs`, `ConfigElementKey.GraphicsOnNowNextSeeded`, `GraphicsElementResponseModel.BuiltIn` · issues: #74' +signals: 'ChannelGraphicsElement, Channel graphics attachment, GraphicsElementSelector base layer, on-now-next seeded element, GraphicsElementDefaults.OnNowNextFileName, GraphicsElementDefaults.OnNowNextSeededPath, builtIn discriminator · paths: `ErsatzTV.Core/Domain/ChannelGraphicsElement.cs`, `ErsatzTV.Core/FFmpeg/GraphicsElementSelector.cs`, `ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs`, `ConfigElementKey.GraphicsOnNowNextSeeded`, `GraphicsElementResponseModel.BuiltIn` · issues: #74, #568' --- #74 asked for a transient "On Now / Next" text bug burned onto the transcoded stream at each @@ -40,8 +40,11 @@ four joins (composite key `{ChannelId, GraphicsElementId}`), added via a dual-pr preset). - The API needed a way for the SPA to find the built-in element without a fragile name-match — the direct #67 lesson (`WatermarkResponseModel.imageSource`). `GraphicsElementResponseModel` gained a - server-derived `BuiltIn` bool, computed by comparing the row's `Path` filename to + server-derived `BuiltIn` bool, computed by comparing the row's `Path` to `GraphicsElementDefaults.OnNowNextFileName` rather than trusting the element's editable `Name`. + (#568 sharpened this from a bare-filename comparison to the full seeded path, + `GraphicsElementDefaults.OnNowNextSeededPath` — a filename-only match was case-sensitive-by-accident + and folder-agnostic; see `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`.) - The channel editor's Branding-tab "Show On Now / Next overlay" switch follows the exact pattern of the existing logo-bug toggle: on adds the built-in element's id to `graphicsElementIds`, off removes it; disabled (with an explanatory caption) when the channel is HLS-Direct. diff --git a/docs/decisions/records/graphics/on-now-next-on-by-default.md b/docs/decisions/records/graphics/on-now-next-on-by-default.md index f925cd269..d60d72d55 100644 --- a/docs/decisions/records/graphics/on-now-next-on-by-default.md +++ b/docs/decisions/records/graphics/on-now-next-on-by-default.md @@ -60,8 +60,10 @@ is simply absent, indistinguishable from never having enabled it — so the one- re-attaches it. That is inherent to "enable it on all channels by default" rather than a defect. The never-re-attach guarantee therefore holds *from the marker onwards*, not across the upgrade boundary. -Identity is the element's **filename** (`GraphicsElementDefaults.OnNowNextFileName`), never the -user-editable `Name` — the #67 lesson carried through #74. +Identity is the element's **full seeded path** (`GraphicsElementDefaults.OnNowNextSeededPath`), never +the user-editable `Name` — the #67 lesson carried through #74, sharpened from a bare filename to the +full path by #568 (a filename-only match was case-sensitive-by-accident and folder-agnostic; see +`graphics.channel-level-attachment` and `docs/graphics-elements.md`). **HLS Direct is excluded at both sites.** ErsatzTV is not transcoding there, so `GraphicsElementSelector` returns empty and the editor disables the toggle; an attachment would be inert while still reading as diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 62de5d63f..64aba0534 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -181,7 +181,14 @@ a test that cannot fail: - the inner `catch` around that cleanup delete — it stops a failing delete from replacing the exception being unwound (which C# otherwise does, and which would downgrade a real cancellation to a swallowed warning). `MockFileSystem` does not propagate an interceptor throw raised during the - delete, so the clause is correct by language semantics but not reachable from a test here. + delete, so the clause is correct by language semantics but not reachable from a test here; +- the `Kind == GraphicsElementKind.Text` filter in `GetBuiltInElementId` (#568) — belt-and-braces + since `OnNowNextSeededPath` is itself a text-template-folder path, so the `Path` equality clause + above (row 10) always excludes a wrong-`Kind` row first. Measured 2026-09-05: dropping the `Kind` + filter alone and running every test that calls `GetBuiltInElementId` or `ChannelGraphicsDefaults` + (`GraphicsElementDefaultAttachTests`, 11 tests, the only file referencing either), nothing reddens — + `Ignores_A_Same_Named_Element_Of_A_Different_Kind` included, because that test's element path never + equals the seeded path regardless of `Kind`. Rows are written per clause, not per block: a row naming a whole `if` or a whole style-merge block hides which individual fields inside it are actually reachable from a test. @@ -197,7 +204,7 @@ hides which individual fields inside it are actually reachable from a test. | 7 | line-ending normalisation dropped from Normalize | `Upgrades_An_Untouched_Previous_Default_With_Windows_Line_Endings` | | 8 | HLS-Direct exclusion removed from the backfill | `Skips_Hls_Direct_Channels_Where_The_Overlay_Cannot_Render` | | 9 | already-attached filter removed | `Does_Not_Duplicate_An_Existing_Attachment`
`While_Armed_A_Restored_Element_Is_Attached_To_Every_Eligible_Channel` | -| 10 | filename check removed from the built-in lookup | `Ignores_A_Non_Built_In_Element_With_A_Different_Filename` | +| 10 | seeded-path check removed from the built-in lookup (#568: `Path == OnNowNextSeededPath`, the full path, not a bare-filename match) | `Ignores_A_Non_Built_In_Element_With_A_Different_Filename`
`Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder` | | 11 | graphics.on_now_next_default_attached guard never fires | `Does_Not_Re_Attach_After_An_Operator_Clears_It` | | 12 | create-time `ChannelGraphicsDefaults.Attach` call removed from `CreateChannelHandler` | `Attaches_The_Built_In_Element_To_A_New_Channel` | | 13 | HLS-Direct skip removed from the create path | `Leaves_An_Hls_Direct_Channel_Alone_Because_Nothing_Can_Render_There`
`Should_Not_Attach_The_Overlay_To_An_Hls_Direct_Channel` | @@ -205,7 +212,6 @@ hides which individual fields inside it are actually reachable from a test. | 15 | FitTextBlock drops HaloColor again | `The_Scale_Path_Preserves_Halo_Blur`
`The_Scale_Path_Preserves_The_Halo` | | 16 | unrounded inset subtracted from the budget | `Fractional_Padding_Still_Respects_Width_Percent` | | 17 | inset clamp removed | `An_Oversized_Padding_Is_Clamped_To_The_Largest_That_Fits` | -| 18 | Kind == Text filter dropped from the lookup | `Ignores_A_Same_Named_Element_Of_A_Different_Kind` | | 19 | Sanitize passes values through unchecked | `Non_Finite_And_Absurd_Box_Values_Do_Not_Corrupt_The_Geometry` | | 20 | the BOX itself is no longer clamped (only insetPixels) | `An_Oversized_Border_Is_Clamped_And_Does_Not_Flood_The_Element` | | 21 | duplicate guard removed from EnsureBuiltInElementRow | `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | From 131c63f7f4dffc2d585a374f15e8f899e397a5a2 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 10:58:46 +0200 Subject: [PATCH 04/19] fix(568): one ordinal identity predicate for the built-in element, applied in memory at both sites The branch moved the `builtIn` discriminator from a bare filename to the full seeded path, but split how the two sites evaluate it: the API handler compares in memory (ordinal) while GetBuiltInElementId's new `.Where(e => e.Path == OnNowNextSeededPath)` compares in SQL. GraphicsElement.Path takes no explicit collation -- TvContext.OnModelCreating pins one only on the listed name/title columns -- so SQLite answers that case-sensitively and MySQL uses the server default, which is normally case-INsensitive. On MySQL the two discriminators could therefore disagree about the same row: AttachOnNowNextByDefault would resolve a case-variant user element as the built-in one while the API reported builtIn:false for it. Collapse both onto GraphicsElementDefaults.IsOnNowNext, ordinal, applied in memory. GetBuiltInElementId goes back to loading the Text candidates and filtering in memory (the shape it had before this branch), keeping only the `Kind` enum filter in SQL. The prose claimed more than the code did. "A filename-only comparison is case-sensitive-by-accident" appeared in four places as a defect the full-path fix removed; a full-path comparison is exactly as case-sensitive, so the clause said nothing and implied a fix that had not happened. Case sensitivity is now deliberate and stated as such -- the built-in element is the exact file the seeder wrote, at the exact path it wrote it to -- and the reason the comparison is kept out of SQL is recorded where the predicate lives. docs/decisions/records/graphics/channel-level-attachment.md said BuiltIn was "computed by comparing the row's `Path` to GraphicsElementDefaults. OnNowNextFileName", which was true of neither the pre-#568 rule (filename to filename) nor the current one; an active record resolved by key now states the current predicate in its own sentence rather than in a parenthetical. Two tests pin the ordinal rule against a loosening to OrdinalIgnoreCase, one per site. Measured: OrdinalIgnoreCase reddens exactly GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn and Ignores_A_Case_Variant_Of_The_Seeded_Path, 2 failed / 77 passed of the 79 graphics tests. They do NOT pin provider independence -- under SQLite's BINARY collation an equivalent SQL comparison answers identically, so no test in this suite can distinguish the two. That is stated at each site rather than left for a reader to assume the tests cover it. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../GetAllGraphicsElementsForApiHandler.cs | 2 +- .../Graphics/GraphicsElementDefaults.cs | 35 ++++++++++++++----- .../Graphics/GraphicsElementSeeder.cs | 19 +++++++--- .../Graphics/GraphicsElementHandlerTests.cs | 25 +++++++++++++ .../GraphicsElementDefaultAttachTests.cs | 26 ++++++++++++++ docs/api-conventions.md | 7 ++-- .../graphics/channel-level-attachment.md | 10 +++--- .../graphics/on-now-next-on-by-default.md | 8 ++--- docs/graphics-elements.md | 20 +++++++---- 9 files changed, 119 insertions(+), 33 deletions(-) diff --git a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs index da7899685..78e1e710a 100644 --- a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs +++ b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs @@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory db .Select(e => new { Vm = ProjectToViewModel(e), - BuiltIn = e.Path == GraphicsElementDefaults.OnNowNextSeededPath + BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path) }) .OrderBy(x => x.Vm.Name == x.Vm.FileName) .ThenBy(x => x.Vm.Name) diff --git a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs index aefa0977d..7b32e85c1 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -5,19 +5,36 @@ 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 (a filename-only comparison is case-sensitive-by-accident and - // folder-agnostic; see OnNowNextSeededPath, #568). + // 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 OnNowNextSeededPath below (#67 / #74 / #568). + // 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. A `builtIn` discriminator must - // match THIS, not `Path.GetFileName(...) == OnNowNextFileName` -- a filename-only comparison is - // case-sensitive-by-accident and 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. + // 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); + + /// + /// The one identity test for the built-in On Now / Next element. Ordinal on purpose, and so + /// case-sensitive on purpose: the built-in element is the exact file the seeder wrote, at + /// the exact path it wrote it to. + /// + /// + /// Callers compare in memory rather than in a Where clause, because in SQL the answer + /// would be the PROVIDER's to give: GraphicsElement.Path takes no explicit collation + /// (TvContext.OnModelCreating 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. + /// + public static bool IsOnNowNext(string path) => + string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal); } diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index 8fcd8022c..338f0be84 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -253,16 +253,27 @@ public static class GraphicsElementSeeder /// The Kind filter stays as belt-and-braces since `OnNowNextSeededPath` is itself a Text-folder /// path. /// + /// + /// The path comparison is in memory rather + /// than a Where clause: in SQL the match would be the provider's collation to decide, and + /// this site would then disagree with the API's `builtIn` (which compares in memory) on MySQL. + /// The Kind filter stays in SQL because it is an enum, not a string. + /// public static async Task> GetBuiltInElementId( TvContext context, CancellationToken cancellationToken) { - List matches = await context.GraphicsElements + List<(int Id, string Path)> candidates = await context.GraphicsElements .Where(e => e.Kind == GraphicsElementKind.Text) - .Where(e => e.Path == GraphicsElementDefaults.OnNowNextSeededPath) - .Select(e => e.Id) + .Select(e => new { e.Id, e.Path }) + .ToListAsync(cancellationToken) + .Map(rows => rows.Select(r => (r.Id, r.Path)).ToList()); + + List matches = candidates + .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path)) + .Select(c => c.Id) .OrderBy(id => id) - .ToListAsync(cancellationToken); + .ToList(); // Lowest id wins if two rows somehow share the seeded path, so the choice is stable across // restarts rather than dependent on query order. diff --git a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs index 1d1500efb..ff0a8c540 100644 --- a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs @@ -101,6 +101,31 @@ public class GraphicsElementHandlerTests result[0].BuiltIn.ShouldBeTrue(); } + // #568: identity is GraphicsElementDefaults.IsOnNowNext, an ORDINAL comparison, so a row whose + // path differs from the seeded one only in case is a different element. Reddens if that + // comparison is loosened to OrdinalIgnoreCase. It does NOT pin provider independence: under + // SQLite's BINARY collation a `Where(e => e.Path == ...)` in SQL answers identically, which is + // why the comparison is kept in memory rather than pinned here (see IsOnNowNext's remarks). + [Test] + public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn() + { + await SeedElement(1, CaseVariantOfSeededPath(), GraphicsElementKind.Text, string.Empty); + + var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); + + List result = + await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None); + + result.Count.ShouldBe(1); + result[0].BuiltIn.ShouldBeFalse(); + } + + // The seeded path with only the FILENAME's case changed -- same folder, same spelling. + private static string CaseVariantOfSeededPath() => + System.IO.Path.Combine( + System.IO.Path.GetDirectoryName(GraphicsElementDefaults.OnNowNextSeededPath)!, + GraphicsElementDefaults.OnNowNextFileName.ToUpperInvariant()); + private async Task SeedElement(int id, string path, GraphicsElementKind kind, string name) { await using TvContext context = _db.CreateContext(); diff --git a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index 5ed293f28..abf0e88f5 100644 --- a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs +++ b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs @@ -214,6 +214,32 @@ public class GraphicsElementDefaultAttachTests (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); } + // #568: GetBuiltInElementId resolves through GraphicsElementDefaults.IsOnNowNext, an ORDINAL + // comparison, so a Text row in the seeded folder whose filename differs only in case is a + // different element. Reddens if that comparison is loosened to OrdinalIgnoreCase. It does NOT + // pin provider independence -- under SQLite's BINARY collation an equivalent SQL `Where` answers + // identically; that is held by keeping the comparison in memory (see IsOnNowNext's remarks). + [Test] + public async Task Ignores_A_Case_Variant_Of_The_Seeded_Path() + { + await using TvContext context = _db.CreateContext(); + await context.GraphicsElements.AddAsync( + new GraphicsElement + { + Path = System.IO.Path.Combine( + System.IO.Path.GetDirectoryName(GraphicsElementDefaults.OnNowNextSeededPath)!, + GraphicsElementDefaults.OnNowNextFileName.ToUpperInvariant()), + Kind = GraphicsElementKind.Text + }); + await context.SaveChangesAsync(); + + Channel channel = await SeedChannel(context, "1"); + + await GraphicsElementSeeder.AttachOnNowNextByDefault(context, CancellationToken.None); + + (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); + } + // The marker is permanent, so writing it with nothing resolved would strand every channel. Stay // armed instead and pick the work up once the element exists. [Test] diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 83b6126cf..5835e5766 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -943,10 +943,9 @@ every other field on this full-replace DTO. The identical shape existed on `PUT (`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the deep-FK-in-a- nested-list carve-out below) and is hardened the same way by `UpdateDecoHandler.Validate` (`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). `GraphicsElementResponseModel` exposes -a server-derived -`builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as `element.Path == -GraphicsElementDefaults.OnNowNextSeededPath` (the full seeded path, not the bare filename — a -filename-only match was case-sensitive-by-accident and folder-agnostic, #568) — never client-settable. +a server-derived `builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as +`GraphicsElementDefaults.IsOnNowNext(element.Path)` — ordinal equality against the full seeded path, +not the bare filename, which was folder-agnostic (#568) — never client-settable. ## 9. Authentication — session-or-key posture (fail-closed) diff --git a/docs/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index 33bb41a10..6eb717e8c 100644 --- a/docs/decisions/records/graphics/channel-level-attachment.md +++ b/docs/decisions/records/graphics/channel-level-attachment.md @@ -40,11 +40,11 @@ four joins (composite key `{ChannelId, GraphicsElementId}`), added via a dual-pr preset). - The API needed a way for the SPA to find the built-in element without a fragile name-match — the direct #67 lesson (`WatermarkResponseModel.imageSource`). `GraphicsElementResponseModel` gained a - server-derived `BuiltIn` bool, computed by comparing the row's `Path` to - `GraphicsElementDefaults.OnNowNextFileName` rather than trusting the element's editable `Name`. - (#568 sharpened this from a bare-filename comparison to the full seeded path, - `GraphicsElementDefaults.OnNowNextSeededPath` — a filename-only match was case-sensitive-by-accident - and folder-agnostic; see `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`.) + server-derived `BuiltIn` bool, computed from the row's `Path` rather than the element's editable + `Name`. The test is `GraphicsElementDefaults.IsOnNowNext` — ordinal equality against the full + seeded path. #568 sharpened it from the original bare-filename comparison, which was + folder-agnostic: a user element named exactly `on-now-next.yml` in another template folder also + reported `builtIn:true`. See `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`. - The channel editor's Branding-tab "Show On Now / Next overlay" switch follows the exact pattern of the existing logo-bug toggle: on adds the built-in element's id to `graphicsElementIds`, off removes it; disabled (with an explanatory caption) when the channel is HLS-Direct. diff --git a/docs/decisions/records/graphics/on-now-next-on-by-default.md b/docs/decisions/records/graphics/on-now-next-on-by-default.md index d60d72d55..5279b03b5 100644 --- a/docs/decisions/records/graphics/on-now-next-on-by-default.md +++ b/docs/decisions/records/graphics/on-now-next-on-by-default.md @@ -60,10 +60,10 @@ is simply absent, indistinguishable from never having enabled it — so the one- re-attaches it. That is inherent to "enable it on all channels by default" rather than a defect. The never-re-attach guarantee therefore holds *from the marker onwards*, not across the upgrade boundary. -Identity is the element's **full seeded path** (`GraphicsElementDefaults.OnNowNextSeededPath`), never -the user-editable `Name` — the #67 lesson carried through #74, sharpened from a bare filename to the -full path by #568 (a filename-only match was case-sensitive-by-accident and folder-agnostic; see -`graphics.channel-level-attachment` and `docs/graphics-elements.md`). +Identity is the element's **full seeded path** — `GraphicsElementDefaults.IsOnNowNext`, ordinal +equality against `OnNowNextSeededPath` — never the user-editable `Name`: the #67 lesson carried +through #74, sharpened from a bare filename to the full path by #568 because a filename-only match +was folder-agnostic (see `graphics.channel-level-attachment` and `docs/graphics-elements.md`). **HLS Direct is excluded at both sites.** ErsatzTV is not transcoding there, so `GraphicsElementSelector` returns empty and the editor disables the toggle; an attachment would be inert while still reading as diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 64aba0534..2cfbc641d 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -121,12 +121,20 @@ never overwrites an operator's file. Two rules govern it after that: - Updating the shipped default → `graphics.seeded-template-upgrade-by-fingerprint`. - It is attached to channels by default → `graphics.on-now-next-on-by-default`. -Identity is the **full seeded path** (`GraphicsElementDefaults.OnNowNextSeededPath`), never the editable -`Name` and never the bare filename (`OnNowNextFileName`) alone — a filename-only, folder-agnostic -comparison let a user element named exactly `on-now-next.yml` in a different template folder also -report `builtIn:true` (#568). `GraphicsElementResponseModel.BuiltIn` and -`GraphicsElementSeeder.GetBuiltInElementId` both compare against the full path server-side so the SPA -never name-matches. +Identity is the **full seeded path**, never the editable `Name` and never the bare filename +(`OnNowNextFileName`) alone — a filename-only, folder-agnostic comparison let a user element named +exactly `on-now-next.yml` in a different template folder also report `builtIn:true` (#568). Both +`GraphicsElementResponseModel.BuiltIn` and `GraphicsElementSeeder.GetBuiltInElementId` resolve it +through the one predicate, `GraphicsElementDefaults.IsOnNowNext`, so the SPA never name-matches. + +`IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` +clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` +pins one only on the listed name/title columns), so a SQL `Path ==` comparison is case-sensitive +under SQLite and normally case-INsensitive under MySQL. Evaluating one of the two discriminator +sites in SQL and the other in memory is what would let them disagree, on MySQL only. The SQLite test +suite cannot distinguish the two — 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; what the case-variant tests in +the table below pin is the ordinal rule itself, against a loosening to `OrdinalIgnoreCase`. ## Tests From ae314aa5eadfd7d4f44c4093e8736444d7b70ca9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 11:04:44 +0200 Subject: [PATCH 05/19] docs(568): re-measure the built-in lookup's mutation rows against the code as it now stands The mutation-coverage table's claims were measured against an earlier shape of GetBuiltInElementId and are re-taken here, because the lookup changed twice on this branch (filename -> seeded path, then SQL -> in-memory IsOnNowNext) and a claim about which tests a mutation reddens does not survive either move on its own. Measured 2026-09-05, each mutation applied alone to the committed tree: - Row 10, the seeded-path filter removed: 3 red, not the 2 the row listed. Ignores_A_Case_Variant_Of_The_Seeded_Path joins the two already named, because without the filter every Text row resolves as the built-in one. - Row 24 is new: IsOnNowNext loosened from Ordinal to OrdinalIgnoreCase reddens exactly the two case-variant tests, 2 failed / 77 passed. One row covers both discriminator sites because they now share the predicate. - The Kind==Text filter's "no red" bullet is re-measured across the WHOLE ErsatzTV.Tests project -- 2121 passed, 6 skipped, 0 failed -- rather than the 11 tests of the one file that names GetBuiltInElementId. ChannelGraphicsDefaults reaches the lookup from the channel-create handlers as well, so the narrower population could not have seen a red there. The conclusion is unchanged; what changes is that it is now measured over the population that could falsify it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- docs/graphics-elements.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 2cfbc641d..7f7ad5858 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -191,12 +191,14 @@ a test that cannot fail: swallowed warning). `MockFileSystem` does not propagate an interceptor throw raised during the delete, so the clause is correct by language semantics but not reachable from a test here; - the `Kind == GraphicsElementKind.Text` filter in `GetBuiltInElementId` (#568) — belt-and-braces - since `OnNowNextSeededPath` is itself a text-template-folder path, so the `Path` equality clause - above (row 10) always excludes a wrong-`Kind` row first. Measured 2026-09-05: dropping the `Kind` - filter alone and running every test that calls `GetBuiltInElementId` or `ChannelGraphicsDefaults` - (`GraphicsElementDefaultAttachTests`, 11 tests, the only file referencing either), nothing reddens — - `Ignores_A_Same_Named_Element_Of_A_Different_Kind` included, because that test's element path never - equals the seeded path regardless of `Kind`. + since `OnNowNextSeededPath` is itself a text-template-folder path, so the `IsOnNowNext` clause + (row 10) always excludes a wrong-`Kind` row first. Measured 2026-09-05: with the `Kind` filter + dropped, the WHOLE `ErsatzTV.Tests` suite stays green — 2121 passed, 6 skipped, 0 failed. The + whole project rather than a filtered subset on purpose: `ChannelGraphicsDefaults` reaches this + lookup from the channel-create handlers too, so a population scoped to the tests that name + `GetBuiltInElementId` would have missed them. `Ignores_A_Same_Named_Element_Of_A_Different_Kind` + stays green for the same reason as the rest — its element's path never equals the seeded path, + regardless of `Kind`. Rows are written per clause, not per block: a row naming a whole `if` or a whole style-merge block hides which individual fields inside it are actually reachable from a test. @@ -212,7 +214,7 @@ hides which individual fields inside it are actually reachable from a test. | 7 | line-ending normalisation dropped from Normalize | `Upgrades_An_Untouched_Previous_Default_With_Windows_Line_Endings` | | 8 | HLS-Direct exclusion removed from the backfill | `Skips_Hls_Direct_Channels_Where_The_Overlay_Cannot_Render` | | 9 | already-attached filter removed | `Does_Not_Duplicate_An_Existing_Attachment`
`While_Armed_A_Restored_Element_Is_Attached_To_Every_Eligible_Channel` | -| 10 | seeded-path check removed from the built-in lookup (#568: `Path == OnNowNextSeededPath`, the full path, not a bare-filename match) | `Ignores_A_Non_Built_In_Element_With_A_Different_Filename`
`Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder` | +| 10 | seeded-path check removed from the built-in lookup (#568: the `IsOnNowNext` filter, the full path, not a bare-filename match) | `Ignores_A_Case_Variant_Of_The_Seeded_Path`
`Ignores_A_Non_Built_In_Element_With_A_Different_Filename`
`Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder` | | 11 | graphics.on_now_next_default_attached guard never fires | `Does_Not_Re_Attach_After_An_Operator_Clears_It` | | 12 | create-time `ChannelGraphicsDefaults.Attach` call removed from `CreateChannelHandler` | `Attaches_The_Built_In_Element_To_A_New_Channel` | | 13 | HLS-Direct skip removed from the create path | `Leaves_An_Hls_Direct_Channel_Alone_Because_Nothing_Can_Render_There`
`Should_Not_Attach_The_Overlay_To_An_Hls_Direct_Channel` | @@ -225,6 +227,7 @@ hides which individual fields inside it are actually reachable from a test. | 21 | duplicate guard removed from EnsureBuiltInElementRow | `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | | 22 | EnsureBuiltInElementRow removed from the ALREADY-SEEDED branch | `An_Already_Seeded_Install_Missing_Its_Element_Row_Gets_One`
`Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | | 23 | non-atomic in-place write restored | `A_Failed_Replace_After_A_Complete_Temp_Write_Leaves_The_Original_Intact`
`A_Failed_Write_Leaves_The_Original_Template_Intact` | +| 24 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | | 24 | marker written even with nothing resolved | `Stays_Armed_When_There_Is_No_Built_In_Element_To_Attach`
`While_Armed_A_Restored_Element_Is_Attached_To_Every_Eligible_Channel` | | 25 | the atomic replace becomes a non-atomic copy | `A_Failed_Replace_After_A_Complete_Temp_Write_Leaves_The_Original_Intact` | | 26 | the frame cap on the inset is disarmed | `Non_Finite_And_Absurd_Box_Values_Do_Not_Corrupt_The_Geometry`
`The_Inset_Is_Capped_Against_The_Frame_Even_With_No_Width_Percent` | From 0e92c147a4dcf115a19d3eebd20ef46e875f8ee7 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 12:23:58 +0200 Subject: [PATCH 06/19] fix(568): a deco validator may only reject ids the apply path would consume Review found the new UpdateDecoHandler FK validators ran unconditionally while ApplyUpdateRequest reads either id list ONLY under DecoMode.Override or Merge -- under Inherit/Disable it Clear()s the join and ignores the field. So the branch turned a previously-succeeding save into a 422 over ids that were about to be discarded, and the SPA reaches that shape: DecosScreen's toReplaceRequest sends watermarkIds/graphicsElementIds from the draft whatever the mode selector says, while the picker itself is disabled off-Override. RefreshGraphicsElementsHandler deletes rows whose template file is gone (cascading the join away), so a stale editor draft could be locked out of saving a deco back to Inherit, with a 422 naming an element the disabled UI does not even show. Measured before the fix on the review's E2E instance: PUT /api/v1/decos/1 with graphicsElementsMode=Inherit and graphicsElementIds=[999] returned 422 "Graphics element(s) do not exist: 999". The mode predicate is now named once per collection -- ConsumesWatermarkIds / ConsumesGraphicsElementIds -- and read by both the apply path and its validator, rather than the apply path holding one copy and the validator implying another. A second copy is what let the two disagree in the first place. Two tests pin the gate, one per collection, each reddening when its guard alone is removed: Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It Measured 2026-09-05, each guard removed alone from the committed tree: 1 failed / 4 passed, and the failure is exactly the test named for that guard. Both assert the apply-path outcome as well as the accept, so a validator that stopped rejecting for some other reason would not satisfy them. Refs #568 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Scheduling/Commands/UpdateDecoHandler.cs | 27 ++++++- .../UpdateDecoGraphicsElementsTests.cs | 80 ++++++++++++++++--- docs/api-conventions.md | 6 +- 3 files changed, 99 insertions(+), 14 deletions(-) diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index d639f8f99..db8e558b7 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -31,7 +31,7 @@ public class UpdateDecoHandler( existing.Name = request.Name; // watermark - bool hasWatermark = request.WatermarkMode is (DecoMode.Override or DecoMode.Merge); + bool hasWatermark = ConsumesWatermarkIds(request); existing.WatermarkMode = request.WatermarkMode; existing.UseWatermarkDuringFiller = hasWatermark && request.UseWatermarkDuringFiller; @@ -59,7 +59,7 @@ public class UpdateDecoHandler( } // graphics elements - bool hasGraphicsElements = request.GraphicsElementsMode is (DecoMode.Override or DecoMode.Merge); + bool hasGraphicsElements = ConsumesGraphicsElementIds(request); existing.GraphicsElementsMode = request.GraphicsElementsMode; existing.UseGraphicsElementsDuringFiller = hasGraphicsElements && request.UseGraphicsElementsDuringFiller; @@ -223,6 +223,19 @@ public class UpdateDecoHandler( await GraphicsElementIdsMustExist(dbContext, request, cancellationToken)) .Apply((deco, _, _, _, _) => deco); + // The mode decides whether an id list is data or dead weight: ApplyUpdateRequest reconciles the + // join table only under Override/Merge and Clear()s it otherwise, ignoring the ids entirely. The + // validators below read these same two predicates rather than restating the mode test, so a + // validator can never reject an id the apply path was going to discard (#568). The SPA sends both + // id lists regardless of the mode selector, so that shape arrives from the real editor: a draft + // holding an element that has since been deleted must still be able to save the deco back to + // Inherit. + private static bool ConsumesWatermarkIds(UpdateDeco request) => + request.WatermarkMode is (DecoMode.Override or DecoMode.Merge); + + private static bool ConsumesGraphicsElementIds(UpdateDeco request) => + request.GraphicsElementsMode is (DecoMode.Override or DecoMode.Merge); + // Mirrors UpdateChannelHandler.GraphicsElementIdsMustExist (#568): the reconcile in // ApplyUpdateRequest blindly Adds a DecoWatermark/DecoGraphicsElement for every incoming id, and // an id with no matching row hits the FK constraint at SaveChangesAsync and surfaces as an @@ -234,6 +247,11 @@ public class UpdateDecoHandler( UpdateDeco request, CancellationToken cancellationToken) { + if (!ConsumesWatermarkIds(request)) + { + return Unit.Default; + } + List requested = request.WatermarkIds?.Distinct().ToList() ?? []; if (requested.Count == 0) { @@ -256,6 +274,11 @@ public class UpdateDecoHandler( UpdateDeco request, CancellationToken cancellationToken) { + if (!ConsumesGraphicsElementIds(request)) + { + return Unit.Default; + } + List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; if (requested.Count == 0) { diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index 42830f234..5d4a03512 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -58,15 +58,17 @@ public class UpdateDecoGraphicsElementsTests private static UpdateDeco MakeUpdate( List graphicsElementIds = null, - List watermarkIds = null) => + List watermarkIds = null, + DecoMode? graphicsElementsMode = null, + DecoMode? watermarkMode = null) => new( 1, 1, "D", - DecoMode.Inherit, + watermarkMode ?? DecoMode.Inherit, watermarkIds ?? [], false, - graphicsElementIds is null ? DecoMode.Inherit : DecoMode.Override, + graphicsElementsMode ?? (graphicsElementIds is null ? DecoMode.Inherit : DecoMode.Override), graphicsElementIds ?? [], false, DecoMode.Inherit, @@ -85,6 +87,24 @@ public class UpdateDecoGraphicsElementsTests null, null); + private async Task SeedGraphicsElement() + { + await using TvContext context = _db.CreateContext(); + var element = new GraphicsElement { Path = "element-a.yml" }; + context.GraphicsElements.Add(element); + await context.SaveChangesAsync(); + return element.Id; + } + + private async Task AttachGraphicsElement(int elementId) + { + await using TvContext context = _db.CreateContext(); + Deco deco = await context.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1); + deco.GraphicsElementsMode = DecoMode.Override; + deco.DecoGraphicsElements.Add(new DecoGraphicsElement { DecoId = 1, GraphicsElementId = elementId }); + await context.SaveChangesAsync(); + } + // Reddens if UpdateDecoHandler.GraphicsElementIdsMustExist alone is removed from Validate. [Test] public async Task Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw() @@ -145,19 +165,57 @@ public class UpdateDecoGraphicsElementsTests error.Value.ShouldContain("999"); } + // The mode, not the id list, decides whether an id is data. ApplyUpdateRequest reconciles the + // join table only under Override/Merge and Clear()s it otherwise, so validating unconditionally + // would reject a save the apply path was going to discard. Reddens if the ConsumesGraphicsElementIds + // guard alone is removed from UpdateDecoHandler.GraphicsElementIdsMustExist. + [Test] + public async Task Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It() + { + await SeedDeco(); + int elementId = await SeedGraphicsElement(); + await AttachGraphicsElement(elementId); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementsMode: DecoMode.Inherit, graphicsElementIds: [999]), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + // the apply path discards the ids under Inherit, and the existing attachment with them + await using TvContext reload = _db.CreateContext(); + Deco reloaded = await reload.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1); + reloaded.GraphicsElementsMode.ShouldBe(DecoMode.Inherit); + reloaded.DecoGraphicsElements.ShouldBeEmpty(); + } + + // Twin of the above for the watermark half; reddens if the ConsumesWatermarkIds guard alone is + // removed from UpdateDecoHandler.WatermarkIdsMustExist. + [Test] + public async Task Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(watermarkMode: DecoMode.Disable, watermarkIds: [999]), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + await using TvContext reload = _db.CreateContext(); + Deco reloaded = await reload.Decos.Include(d => d.DecoWatermarks).SingleAsync(d => d.Id == 1); + reloaded.WatermarkMode.ShouldBe(DecoMode.Disable); + reloaded.DecoWatermarks.ShouldBeEmpty(); + } + [Test] public async Task Should_Accept_A_Known_GraphicsElementId() { await SeedDeco(); - int elementId; - await using (TvContext context = _db.CreateContext()) - { - var element = new GraphicsElement { Path = "element-a.yml" }; - context.GraphicsElements.Add(element); - await context.SaveChangesAsync(); - elementId = element.Id; - } + int elementId = await SeedGraphicsElement(); var handler = new UpdateDecoHandler(_db.Factory, _channel); Either result = await handler.Handle( diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 5835e5766..7fa9e19e4 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -942,7 +942,11 @@ via `Channel.ChannelGraphicsElements`); `UpdateChannelHandler.Validate` rejects every other field on this full-replace DTO. The identical shape existed on `PUT /api/v1/decos/{id}` (`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the deep-FK-in-a- nested-list carve-out below) and is hardened the same way by `UpdateDecoHandler.Validate` -(`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). `GraphicsElementResponseModel` exposes +(`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). Each deco validator is gated on the +same `Override`/`Merge` mode predicate that makes the apply path consume its id list — under +`Inherit`/`Disable` the reconcile clears the join and ignores the ids, so validating them there would +422 a request over ids it was about to discard. The SPA sends both id lists whatever the mode +selector says, so that shape arrives from the real editor. `GraphicsElementResponseModel` exposes a server-derived `builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as `GraphicsElementDefaults.IsOnNowNext(element.Path)` — ordinal equality against the full seeded path, not the bare filename, which was folder-agnostic (#568) — never client-settable. From cb11116d4f18dbc73ccaf8bf74f61ffc0f8646b6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 12:24:27 +0200 Subject: [PATCH 07/19] docs(568): stable row ids in the mutation table, and the case-sensitivity sign-off the issue's wording invites Two review findings, both about docs the branch already rewrote. The mutation-coverage table in docs/graphics-elements.md ended up with two rows numbered 24 -- the new IsOnNowNext row was inserted after 23 without checking what followed -- while 18 was vacated when the Kind-filter row moved to the "no red" list. The section's own prose cites rows by number ("the IsOnNowNext clause (row 10)"), so a duplicate id makes a citation ambiguous. The new row becomes 33, the next unused number, and the rule that made it 24 in the first place is now written down: a row number is an identity, not a position, so a new row takes the next unused number, nothing is renumbered, and a retired clause leaves its number vacant rather than having it reused under a new meaning. Both row claims were re-measured and are unchanged; only the id moves. #568's second half is titled "builtIn discriminator is filename-only, case-sensitive, folder-agnostic", and the branch removes the first and third while deliberately keeping case sensitivity -- which reads like two thirds of a done-when box. It is not: the remedy the same box prescribes, "full seeded relative path", is exactly as case-sensitive as the filename match it replaces, so the three adjectives describe one predicate rather than name three separable demands. Read the other way the box would be unsatisfiable by its own remedy. The reason case sensitivity is kept -- a case-INsensitive test hands the built-in identity to a user element differing from the seeded path only in case -- lived only in GraphicsElementDefaults.cs, where a reader arriving from the issue title would not find it. It is now in the active record that owns the discriminator. Refs #568 Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../records/graphics/channel-level-attachment.md | 6 +++++- docs/graphics-elements.md | 8 +++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index 6eb717e8c..40de2a664 100644 --- a/docs/decisions/records/graphics/channel-level-attachment.md +++ b/docs/decisions/records/graphics/channel-level-attachment.md @@ -44,7 +44,11 @@ four joins (composite key `{ChannelId, GraphicsElementId}`), added via a dual-pr `Name`. The test is `GraphicsElementDefaults.IsOnNowNext` — ordinal equality against the full seeded path. #568 sharpened it from the original bare-filename comparison, which was folder-agnostic: a user element named exactly `on-now-next.yml` in another template folder also - reported `builtIn:true`. See `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`. + reported `builtIn:true`. Case sensitivity is kept, deliberately: the remedy #568 prescribes — the + full seeded relative path — is exactly as case-sensitive as the filename match it replaces, and a + case-INsensitive test would hand the built-in identity to a user element differing from the seeded + path only in case. The built-in element is the exact file the seeder wrote, at the path it wrote it + to. See `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`. - The channel editor's Branding-tab "Show On Now / Next overlay" switch follows the exact pattern of the existing logo-bug toggle: on adds the built-in element's id to `graphicsElementIds`, off removes it; disabled (with an explanatory caption) when the channel is HLS-Direct. diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 7f7ad5858..79168dceb 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -203,6 +203,12 @@ a test that cannot fail: Rows are written per clause, not per block: a row naming a whole `if` or a whole style-merge block hides which individual fields inside it are actually reachable from a test. +A row number is an identity, not a position: this section's own prose cites rows by number ("the +`IsOnNowNext` clause (row 10)" above), so a new row takes the next unused number and the rest are +never renumbered. A clause that stops having a red keeps its number vacant instead — 18 is vacant +because the `Kind == Text` filter moved to the list above on 2026-09-05, and reusing it would silently +re-point every existing citation. + | # | Clause mutated | Reddens (measured) | |---|---|---| | 1 | DrawBackgroundBox returns immediately | `An_Oversized_Border_Is_Clamped_And_Does_Not_Flood_The_Element`
`Background_Color_Fills_The_Box`
`Background_Opacity_Percent_Is_Clamped_To_Its_Documented_Range`
`Background_Opacity_Percent_Scales_The_Alpha`
`Background_Padding_Actually_Insets_The_Text`
`Border_Color_Draws_A_Border_Distinct_From_The_Fill`
`Border_Color_Without_An_Explicit_Width_Draws_A_Hairline`
`Corner_Radius_Rounds_The_Corner_Away` | @@ -227,7 +233,6 @@ hides which individual fields inside it are actually reachable from a test. | 21 | duplicate guard removed from EnsureBuiltInElementRow | `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | | 22 | EnsureBuiltInElementRow removed from the ALREADY-SEEDED branch | `An_Already_Seeded_Install_Missing_Its_Element_Row_Gets_One`
`Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | | 23 | non-atomic in-place write restored | `A_Failed_Replace_After_A_Complete_Temp_Write_Leaves_The_Original_Intact`
`A_Failed_Write_Leaves_The_Original_Template_Intact` | -| 24 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | | 24 | marker written even with nothing resolved | `Stays_Armed_When_There_Is_No_Built_In_Element_To_Attach`
`While_Armed_A_Restored_Element_Is_Attached_To_Every_Eligible_Channel` | | 25 | the atomic replace becomes a non-atomic copy | `A_Failed_Replace_After_A_Complete_Temp_Write_Leaves_The_Original_Intact` | | 26 | the frame cap on the inset is disarmed | `Non_Finite_And_Absurd_Box_Values_Do_Not_Corrupt_The_Geometry`
`The_Inset_Is_Capped_Against_The_Frame_Even_With_No_Width_Percent` | @@ -237,3 +242,4 @@ hides which individual fields inside it are actually reachable from a test. | 30 | per-style halo_blur override dropped from the style merge | `A_Non_Base_Style_Uses_Its_Own_Halo_Blur` | | 31 | the 0-100 opacity clamp is removed | `Background_Opacity_Percent_Is_Clamped_To_Its_Documented_Range` | | 32 | the lineup create path stops attaching the built-in element | `Should_Attach_The_Built_In_On_Now_Next_Element` | +| 33 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | From 1149d96eff536ba0780cc913958f044263e33707 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 12:33:34 +0200 Subject: [PATCH 08/19] test(568): the watermark mode-gate test starts from an actual attachment Its graphics twin seeds an element, attaches it, then saves with Inherit and an unknown id, so "the join is empty afterwards" distinguishes a cleared attachment from one that was never there. The watermark half asserted the same emptiness on a deco that had no watermarks to begin with -- true of the fixture regardless of what the handler did, which is a fixture that omits the field it means to test. Seed a ChannelWatermark, attach it under Override, then save with Disable and watermarkId 777. Re-measured 2026-09-05 with the ConsumesWatermarkIds guard removed alone from the committed tree: 1 failed / 4 passed, the failure being Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It. Whole project green with the guard in place: 2123 passed, 6 skipped, 0 failed. Refs #568 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../UpdateDecoGraphicsElementsTests.cs | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index 5d4a03512..bf7fdcf84 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -96,6 +96,24 @@ public class UpdateDecoGraphicsElementsTests return element.Id; } + private async Task SeedWatermark() + { + await using TvContext context = _db.CreateContext(); + var watermark = new ChannelWatermark { Name = "W" }; + context.ChannelWatermarks.Add(watermark); + await context.SaveChangesAsync(); + return watermark.Id; + } + + private async Task AttachWatermark(int watermarkId) + { + await using TvContext context = _db.CreateContext(); + Deco deco = await context.Decos.Include(d => d.DecoWatermarks).SingleAsync(d => d.Id == 1); + deco.WatermarkMode = DecoMode.Override; + deco.DecoWatermarks.Add(new DecoWatermark { DecoId = 1, WatermarkId = watermarkId }); + await context.SaveChangesAsync(); + } + private async Task AttachGraphicsElement(int elementId) { await using TvContext context = _db.CreateContext(); @@ -196,6 +214,8 @@ public class UpdateDecoGraphicsElementsTests public async Task Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It() { await SeedDeco(); + int watermarkId = await SeedWatermark(); + await AttachWatermark(watermarkId); var handler = new UpdateDecoHandler(_db.Factory, _channel); Either result = await handler.Handle( From 74a6e017122a70bb264747b12752151ea48c9a83 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 13:16:59 +0200 Subject: [PATCH 09/19] fix(568): the seeder asks the built-in lookup, instead of re-deriving it in SQL EnsureBuiltInElementRow decided whether the built-in row already existed with its own `AnyAsync(e => e.Path == target)` -- the one discriminator site left comparing in SQL after 28827a7d3 moved the rest in memory. Two ways it could answer differently from GetBuiltInElementId, each leaving the built-in element undiscoverable for the life of the install: string equality in SQL is the provider's collation to decide, so on MySQL's normally case-insensitive default a case-variant row satisfied the check and the canonical row was never created; and it ignored Kind, so a row of another kind at the seeded path suppressed the Text row the lookup resolves. Ask GetBuiltInElementId instead, so the existence question and the resolution question are the same code. The wrong-kind half is observable under SQLite and is now pinned; the collation half is not (BINARY and an ordinal comparison agree on every input) and stays held by keeping the comparison out of SQL. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Graphics/GraphicsElementSeeder.cs | 20 ++++++-- .../GraphicsElementDefaultAttachTests.cs | 46 +++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index 338f0be84..f3ac98c74 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -173,8 +173,18 @@ public static class GraphicsElementSeeder return; } - bool exists = await context.GraphicsElements.AnyAsync(e => e.Path == target, cancellationToken); - if (exists) + // "Does the built-in row already exist?" is the same question every consumer asks later, so + // ask it with the same code instead of re-deriving it here. As its own SQL comparison + // (`AnyAsync(e => e.Path == target)`) it could answer differently in two ways, and either + // one leaves the built-in element undiscoverable after startup (#568): + // * string equality in SQL is the PROVIDER's collation to decide, so on MySQL's normally + // case-INsensitive default a case-variant row satisfied the check, the canonical row was + // never created, and the ordinal lookup below then matched nothing; + // * it ignored `Kind`, so a row of another kind sitting at the seeded path suppressed the + // Text row the lookup actually resolves. + // Creating the row stays idempotent because `target` IS the path the lookup matches -- held + // by `Repeated_Seeding_Does_Not_Accumulate_Element_Rows`, which reddens if the two drift. + if ((await GetBuiltInElementId(context, cancellationToken)).IsSome) { return; } @@ -250,8 +260,10 @@ public static class GraphicsElementSeeder /// Identity is the full seeded path, never the user-editable Name (the #67 lesson carried into /// #74) and never the bare filename (#568: filename-only matching is folder-agnostic, so a user /// element named exactly `on-now-next.yml` in a different template folder would also match). - /// The Kind filter stays as belt-and-braces since `OnNowNextSeededPath` is itself a Text-folder - /// path. + /// The Kind filter is load-bearing rather than decorative: EnsureBuiltInElementRow + /// asks this method whether the row it is about to create already exists, so a row of another + /// kind at the seeded path must NOT answer yes -- it would suppress the Text row every consumer + /// resolves. /// /// /// The path comparison is in memory rather diff --git a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index abf0e88f5..f22dffcee 100644 --- a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs +++ b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs @@ -293,6 +293,52 @@ public class GraphicsElementDefaultAttachTests (await GraphicsElementSeeder.GetBuiltInElementId(context, CancellationToken.None)).IsSome.ShouldBeTrue(); } + // #568: EnsureBuiltInElementRow's "does it exist already?" test and the lookup every consumer + // resolves through must be the same question. While that test was its own SQL comparison + // (`e.Path == target`), a row at the seeded path of ANY kind answered yes, so the Text row + // GetBuiltInElementId actually matches was never created and the built-in element stayed + // undiscoverable for the life of the install. + // + // This is the half of that divergence SQLite can observe. The other half -- a case-variant path, + // which MySQL's normally case-insensitive default collation matches in SQL while the ordinal + // lookup never does -- cannot be shown from a test here, because SQLite's BINARY collation and + // an ordinal comparison agree on every input; it is held by keeping the comparison out of SQL. + // + // RefreshGraphicsElements derives Kind from the template FOLDER, so it cannot itself produce the + // wrong-kind row seeded below. The point is not that this state is common but that the seeder + // must not answer a question differently from the code that consumes its answer. + [Test] + public async Task A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row() + { + var fs = new MockFileSystem(); + fs.Directory.CreateDirectory(FileSystemLayout.GraphicsElementsTextTemplatesFolder); + string target = Path.Combine( + FileSystemLayout.GraphicsElementsTextTemplatesFolder, + GraphicsElementDefaults.OnNowNextFileName); + await fs.File.WriteAllTextAsync(target, "name: On Now / Next\n"); + + await using TvContext context = _db.CreateContext(); + context.ConfigElements.Add( + new ConfigElement { Key = ConfigElementKey.GraphicsOnNowNextSeeded.Key, Value = "true" }); + await context.GraphicsElements.AddAsync( + new GraphicsElement { Path = target, Kind = GraphicsElementKind.Image }); + await context.SaveChangesAsync(); + + await GraphicsElementSeeder.SeedOnNowNext(context, fs, NullLogger.Instance, CancellationToken.None); + + List rows = await context.GraphicsElements.OrderBy(e => e.Id).ToListAsync(); + rows.Count.ShouldBe(2, "the built-in Text row must be created alongside the wrong-kind row"); + rows[1].Path.ShouldBe(target); + rows[1].Kind.ShouldBe(GraphicsElementKind.Text); + rows[1].Name.ShouldBe(GraphicsElementDefaults.OnNowNextName); + + // The lookup must land on the Text row, not on the lower-id wrong-kind row that shares its + // path -- so this also reddens if the Kind filter is dropped from GetBuiltInElementId. + int builtInId = (await GraphicsElementSeeder.GetBuiltInElementId(context, CancellationToken.None)) + .IfNone(-1); + builtInId.ShouldBe(rows[1].Id); + } + // The armed path is a real, reachable state: an operator deletes the template, refresh reaps the // row, and the backfill then has nothing to resolve. Pin what happens when the element comes // back -- a single global marker cannot both avoid stranding and avoid re-adding, and this is From 41fd1f64de68fd82c0ca527c4d6a905099dd9d39 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 13:51:28 +0200 Subject: [PATCH 10/19] docs(568): re-measure the rows the seeder's lookup call moved, and recount the table's own claims The seeder now resolves the built-in row through GetBuiltInElementId, which puts that lookup on a second call path, so every row whose clause the new call can reach was re-run against this tree: 10 and 33 unchanged, 18 reinstated (the Kind == Text filter has a red now that a wrong-kind row at the seeded path can suppress the row the lookup needs), 21 unchanged, 22 gains a third red, and 34 is new (the existence check re-derived as SQL instead of asking the lookup). Two stale measurements went with it. The "known clauses with no red" bullet for the Kind filter quoted 2121 passed against a tree that produces 2123, having been taken before the branch's last two tests existed -- the whole bullet is gone now that the clause has a red. And the per-fixture-filter trap counted thirteen multi-test rows with two spanning two fixture classes, true on origin/main and false here since the branch added rows: fifteen and three, both recounted from the table, with a note that they are. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../graphics/channel-level-attachment.md | 7 ++- docs/graphics-elements.md | 49 +++++++++++-------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/docs/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index 40de2a664..fdcae37dc 100644 --- a/docs/decisions/records/graphics/channel-level-attachment.md +++ b/docs/decisions/records/graphics/channel-level-attachment.md @@ -48,7 +48,12 @@ four joins (composite key `{ChannelId, GraphicsElementId}`), added via a dual-pr full seeded relative path — is exactly as case-sensitive as the filename match it replaces, and a case-INsensitive test would hand the built-in identity to a user element differing from the seeded path only in case. The built-in element is the exact file the seeder wrote, at the path it wrote it - to. See `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`. + to. Every site applies that predicate **in memory**, never as a `Where` clause — including the + seeder's own "does the row exist yet?" check, which calls `GetBuiltInElementId` rather than asking + the question a second way. `GraphicsElement.Path` carries no explicit collation, so a SQL `Path ==` + comparison answers case-sensitively under SQLite and normally case-INsensitively under MySQL: one + site in SQL and another in memory disagree on MySQL alone, which the SQLite suite cannot show. See + `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`. - The channel editor's Branding-tab "Show On Now / Next overlay" switch follows the exact pattern of the existing logo-bug toggle: on adds the built-in element's id to `graphicsElementIds`, off removes it; disabled (with an explanatory caption) when the channel is HLS-Direct. diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 79168dceb..9941eaf38 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -127,12 +127,22 @@ exactly `on-now-next.yml` in a different template folder also report `builtIn:tr `GraphicsElementResponseModel.BuiltIn` and `GraphicsElementSeeder.GetBuiltInElementId` resolve it through the one predicate, `GraphicsElementDefaults.IsOnNowNext`, so the SPA never name-matches. +The seeder's own "does this row exist yet?" test — `EnsureBuiltInElementRow`, which decides whether +to create the row at startup — does not re-derive that predicate either: it calls +`GetBuiltInElementId`, so the question asked at startup is the question every consumer asks +afterwards. As its own SQL `Path == target` comparison it could answer differently in two ways, each +leaving the built-in element undiscoverable for the life of the install (#568): the collation one +below, and `Kind`, which the SQL check ignored while the lookup requires it. That second one is what +makes the `Kind == Text` filter in the lookup load-bearing rather than belt-and-braces (row 18). +Creating the row stays idempotent because the path it writes is the path the lookup matches — held +by `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` (row 21), which reddens if the two drift. + `IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` pins one only on the listed name/title columns), so a SQL `Path ==` comparison is case-sensitive -under SQLite and normally case-INsensitive under MySQL. Evaluating one of the two discriminator -sites in SQL and the other in memory is what would let them disagree, on MySQL only. The SQLite test -suite cannot distinguish the two — BINARY collation and an ordinal comparison agree on every input — +under SQLite and normally case-INsensitive under MySQL. Evaluating one discriminator site in SQL and +another in memory is what would let them disagree, on MySQL only. The SQLite test suite cannot +distinguish the two — 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; what the case-variant tests in the table below pin is the ordinal rule itself, against a loosening to `OrdinalIgnoreCase`. @@ -158,8 +168,10 @@ Three traps this table is built to avoid: - **Measure against the whole project, never a per-fixture filter.** A filtered run structurally cannot observe a red in another fixture, so it under-reports the failure set while looking precise. - Thirteen rows below redden more than one test, and two of them span two fixture classes — those two - are exactly what a per-fixture filter cannot see. + Fifteen rows below redden more than one test, and three of them span two fixture classes — those + three are exactly what a per-fixture filter cannot see. Both figures are counted from the table + itself, so a row added without recounting them makes this bullet quietly false — recount both + whenever a row is added or its red set changes. - **A mutation that fails to COMPILE is not a result.** Warnings-as-error turn the obvious mutation shapes into build failures — `CS0162` for `if (true) { return; }`, `CS1717` for self-assignment, Sonar `S3981` for a constant-folded condition — and a build failure emits no test summary at all, @@ -189,25 +201,18 @@ a test that cannot fail: - the inner `catch` around that cleanup delete — it stops a failing delete from replacing the exception being unwound (which C# otherwise does, and which would downgrade a real cancellation to a swallowed warning). `MockFileSystem` does not propagate an interceptor throw raised during the - delete, so the clause is correct by language semantics but not reachable from a test here; -- the `Kind == GraphicsElementKind.Text` filter in `GetBuiltInElementId` (#568) — belt-and-braces - since `OnNowNextSeededPath` is itself a text-template-folder path, so the `IsOnNowNext` clause - (row 10) always excludes a wrong-`Kind` row first. Measured 2026-09-05: with the `Kind` filter - dropped, the WHOLE `ErsatzTV.Tests` suite stays green — 2121 passed, 6 skipped, 0 failed. The - whole project rather than a filtered subset on purpose: `ChannelGraphicsDefaults` reaches this - lookup from the channel-create handlers too, so a population scoped to the tests that name - `GetBuiltInElementId` would have missed them. `Ignores_A_Same_Named_Element_Of_A_Different_Kind` - stays green for the same reason as the rest — its element's path never equals the seeded path, - regardless of `Kind`. + delete, so the clause is correct by language semantics but not reachable from a test here. Rows are written per clause, not per block: a row naming a whole `if` or a whole style-merge block hides which individual fields inside it are actually reachable from a test. -A row number is an identity, not a position: this section's own prose cites rows by number ("the -`IsOnNowNext` clause (row 10)" above), so a new row takes the next unused number and the rest are -never renumbered. A clause that stops having a red keeps its number vacant instead — 18 is vacant -because the `Kind == Text` filter moved to the list above on 2026-09-05, and reusing it would silently -re-point every existing citation. +A row number is an identity, not a position: prose elsewhere cites rows by number (the built-in +element section above cites rows 18 and 21), so a new row takes the next unused number and the rest +are never renumbered. A clause that loses its red keeps its number vacant rather than freeing it for +some other clause — reusing it would silently re-point every existing citation. Reinstating the SAME +clause under its OWN number is the sole exception, because that is the same identity and not a new +one: row 18 is the `Kind == Text` filter, and stays that clause whether or not it currently has a +red. | # | Clause mutated | Reddens (measured) | |---|---|---| @@ -228,10 +233,11 @@ re-point every existing citation. | 15 | FitTextBlock drops HaloColor again | `The_Scale_Path_Preserves_Halo_Blur`
`The_Scale_Path_Preserves_The_Halo` | | 16 | unrounded inset subtracted from the budget | `Fractional_Padding_Still_Respects_Width_Percent` | | 17 | inset clamp removed | `An_Oversized_Padding_Is_Clamped_To_The_Largest_That_Fits` | +| 18 | `Kind == GraphicsElementKind.Text` filter dropped from the built-in lookup (#568) — the lookup is what `EnsureBuiltInElementRow` asks, so a wrong-kind row at the seeded path would answer for the Text row that never then gets created | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` | | 19 | Sanitize passes values through unchecked | `Non_Finite_And_Absurd_Box_Values_Do_Not_Corrupt_The_Geometry` | | 20 | the BOX itself is no longer clamped (only insetPixels) | `An_Oversized_Border_Is_Clamped_And_Does_Not_Flood_The_Element` | | 21 | duplicate guard removed from EnsureBuiltInElementRow | `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | -| 22 | EnsureBuiltInElementRow removed from the ALREADY-SEEDED branch | `An_Already_Seeded_Install_Missing_Its_Element_Row_Gets_One`
`Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | +| 22 | EnsureBuiltInElementRow removed from the ALREADY-SEEDED branch | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row`
`An_Already_Seeded_Install_Missing_Its_Element_Row_Gets_One`
`Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | | 23 | non-atomic in-place write restored | `A_Failed_Replace_After_A_Complete_Temp_Write_Leaves_The_Original_Intact`
`A_Failed_Write_Leaves_The_Original_Template_Intact` | | 24 | marker written even with nothing resolved | `Stays_Armed_When_There_Is_No_Built_In_Element_To_Attach`
`While_Armed_A_Restored_Element_Is_Attached_To_Every_Eligible_Channel` | | 25 | the atomic replace becomes a non-atomic copy | `A_Failed_Replace_After_A_Complete_Temp_Write_Leaves_The_Original_Intact` | @@ -243,3 +249,4 @@ re-point every existing citation. | 31 | the 0-100 opacity clamp is removed | `Background_Opacity_Percent_Is_Clamped_To_Its_Documented_Range` | | 32 | the lineup create path stops attaching the built-in element | `Should_Attach_The_Built_In_On_Now_Next_Element` | | 33 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | +| 34 | `EnsureBuiltInElementRow`'s existence check re-derived as a SQL `AnyAsync(e => e.Path == target)` instead of asking `GetBuiltInElementId` (#568) | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` | From c249ca41102bad3743a7438f1938652723529f61 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 14:17:57 +0200 Subject: [PATCH 11/19] test(568): bind the test's mutation claim to the measured rows, not to itself The comment asserted an outcome ("also reddens if the Kind filter is dropped") with nothing tying it to a measurement -- the shape testing.mutation-claims-are- executed exists to refuse. Both clauses it covers are rows of the mutation table in docs/graphics-elements.md, measured against this tree; cite them. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Infrastructure/GraphicsElementDefaultAttachTests.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index f22dffcee..3205c0eda 100644 --- a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs +++ b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs @@ -333,7 +333,9 @@ public class GraphicsElementDefaultAttachTests rows[1].Name.ShouldBe(GraphicsElementDefaults.OnNowNextName); // The lookup must land on the Text row, not on the lower-id wrong-kind row that shares its - // path -- so this also reddens if the Kind filter is dropped from GetBuiltInElementId. + // path. Both clauses this test covers are measured rows of the mutation table in + // docs/graphics-elements.md -- 18 (the Kind filter) and 34 (the existence check re-derived + // in SQL) -- rather than an outcome asserted only here. int builtInId = (await GraphicsElementSeeder.GetBuiltInElementId(context, CancellationToken.None)) .IfNone(-1); builtInId.ShouldBe(rows[1].Id); From 9f0f21ec52385d53f8350d12044834701b932812 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 14:32:30 +0200 Subject: [PATCH 12/19] docs(568): measure the five validator mutation claims the tests assert in prose Five test comments asserted "reddens if alone is removed" with nothing binding the sentence to a measurement -- the shape testing.mutation-claims-are-executed refuses, and the shape whose CLAIMS half of the manifest cannot reach a .NET proof. The repo's record for those is the mutation table, so each claim got a row: all five mutated in turn against this tree with the whole ErsatzTV.Tests project re-run (the tuple-arity fix included, since a mutation that does not compile is not a result). 35 GraphicsElementIdsMustExist out of UpdateChannelHandler.Validate -> 2 red; 36/37 the deco graphics/watermark validators out of UpdateDecoHandler.Validate -> 1 red each; 38/39 the two Consumes* mode gates -> 1 red each. Sixteen rows now redden more than one test; three still span two fixture classes. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../UpdateChannelGraphicsElementsTests.cs | 5 +++-- .../Scheduling/UpdateDecoGraphicsElementsTests.cs | 15 ++++++++++----- docs/graphics-elements.md | 7 ++++++- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs index bb981fde0..d10ffbcfe 100644 --- a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs @@ -80,8 +80,9 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase // #568: an unknown graphicsElementIds entry used to reach ApplyUpdateRequest unchecked, which // blindly Adds a ChannelGraphicsElement and lets SaveChangesAsync hit - // FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId -> unhandled 500. Reddens if the - // GraphicsElementIdsMustExist validator alone is removed from UpdateChannelHandler.Validate. + // FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId -> unhandled 500. Removing the + // GraphicsElementIdsMustExist validator alone from UpdateChannelHandler.Validate is row 35 of the + // mutation table in docs/graphics-elements.md, measured against the whole ErsatzTV.Tests project. [Test] public async Task Should_Reject_Unknown_GraphicsElementId_With_422_Not_500() { diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index bf7fdcf84..360ef2055 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -123,7 +123,8 @@ public class UpdateDecoGraphicsElementsTests await context.SaveChangesAsync(); } - // Reddens if UpdateDecoHandler.GraphicsElementIdsMustExist alone is removed from Validate. + // Removing UpdateDecoHandler.GraphicsElementIdsMustExist alone from Validate is row 36 of the + // mutation table in docs/graphics-elements.md, measured against the whole ErsatzTV.Tests project. [Test] public async Task Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw() { @@ -144,6 +145,8 @@ public class UpdateDecoGraphicsElementsTests reloaded.DecoGraphicsElements.ShouldBeEmpty(); } + // Removing UpdateDecoHandler.WatermarkIdsMustExist alone from Validate is row 37 of the + // mutation table in docs/graphics-elements.md. [Test] public async Task Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw() { @@ -185,8 +188,9 @@ public class UpdateDecoGraphicsElementsTests // The mode, not the id list, decides whether an id is data. ApplyUpdateRequest reconciles the // join table only under Override/Merge and Clear()s it otherwise, so validating unconditionally - // would reject a save the apply path was going to discard. Reddens if the ConsumesGraphicsElementIds - // guard alone is removed from UpdateDecoHandler.GraphicsElementIdsMustExist. + // would reject a save the apply path was going to discard. Removing the ConsumesGraphicsElementIds + // guard alone from UpdateDecoHandler.GraphicsElementIdsMustExist is row 38 of the mutation table + // in docs/graphics-elements.md. [Test] public async Task Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It() { @@ -208,8 +212,9 @@ public class UpdateDecoGraphicsElementsTests reloaded.DecoGraphicsElements.ShouldBeEmpty(); } - // Twin of the above for the watermark half; reddens if the ConsumesWatermarkIds guard alone is - // removed from UpdateDecoHandler.WatermarkIdsMustExist. + // Twin of the above for the watermark half; removing the ConsumesWatermarkIds guard alone from + // UpdateDecoHandler.WatermarkIdsMustExist is row 39 of the mutation table in + // docs/graphics-elements.md. [Test] public async Task Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It() { diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 9941eaf38..2ae4fe01d 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -168,7 +168,7 @@ Three traps this table is built to avoid: - **Measure against the whole project, never a per-fixture filter.** A filtered run structurally cannot observe a red in another fixture, so it under-reports the failure set while looking precise. - Fifteen rows below redden more than one test, and three of them span two fixture classes — those + Sixteen rows below redden more than one test, and three of them span two fixture classes — those three are exactly what a per-fixture filter cannot see. Both figures are counted from the table itself, so a row added without recounting them makes this bullet quietly false — recount both whenever a row is added or its red set changes. @@ -250,3 +250,8 @@ red. | 32 | the lineup create path stops attaching the built-in element | `Should_Attach_The_Built_In_On_Now_Next_Element` | | 33 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | | 34 | `EnsureBuiltInElementRow`'s existence check re-derived as a SQL `AnyAsync(e => e.Path == target)` instead of asking `GetBuiltInElementId` (#568) | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` | +| 35 | `GraphicsElementIdsMustExist` removed from `UpdateChannelHandler.Validate` (#568) — the unknown-id 422 the issue asks for | `Should_Reject_Unknown_GraphicsElementId_With_422_Not_500`
`Should_Reject_When_One_Of_Several_GraphicsElementIds_Is_Unknown` | +| 36 | `GraphicsElementIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw` | +| 37 | `WatermarkIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw` | +| 38 | the `ConsumesGraphicsElementIds` mode gate removed from `UpdateDecoHandler.GraphicsElementIdsMustExist` (#568) — a validator may only reject ids the apply path would consume | `Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It` | +| 39 | the `ConsumesWatermarkIds` mode gate removed from `UpdateDecoHandler.WatermarkIdsMustExist` (#568) | `Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It` | From 7fa223a5679ae40ee350ab810d1683b581fad30a Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 18:20:19 +0200 Subject: [PATCH 13/19] test(568): drop the different-kind case no single-clause mutation can redden, and measure the API-site clause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Ignores_A_Same_Named_Element_Of_A_Different_Kind` seeded an Image row at `/templates/image/on-now-next.yml` and asserted the backfill ignores it. Under the bare-filename lookup that row was rejected by the `Kind == Text` filter alone, which is what its comment described. Under the full-path predicate the path rejects it first, so neither clause is load-bearing for it: dropping the `Kind` filter reddens only `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` (row 18) and dropping `IsOnNowNext` reddens only the three tests of row 10. The test survived both and its comment claimed a mechanism it no longer exercised. Its scenario is the conjunction of two already-pinned negatives and is strictly weaker than `Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder`, so it is retired rather than reshaped, and graphics-elements.md now says why the combination is deliberately not shipped — otherwise the next reader re-adds it. Row 40 records the API-side half of the discriminator, which had a measured red and no row. Measured whole-project on this tree, `dotnet test ErsatzTV.Tests/ErsatzTV.Tests.csproj`: baseline `Failed: 0, Passed: 2123, Skipped: 6, Total: 2129`; with `BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path)` reverted to `Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName`, `Failed: 1, Passed: 2122`, the sole red being `GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn`. The table's two self-counts were recounted from the table after adding the row and both still hold: sixteen rows redden more than one test, three of those span two fixture classes (13, 22, 33). Refs #568 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../GraphicsElementDefaultAttachTests.cs | 21 ------------------- docs/graphics-elements.md | 7 +++++++ 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index 3205c0eda..f4b28096c 100644 --- a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs +++ b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs @@ -153,27 +153,6 @@ public class GraphicsElementDefaultAttachTests (await AttachedElementIds(context, channel.Id)).Count.ShouldBe(1); } - // Filename alone is ambiguous: the five template folders are separate namespaces, so an element - // of another kind may legitimately carry the same filename. - [Test] - public async Task Ignores_A_Same_Named_Element_Of_A_Different_Kind() - { - await using TvContext context = _db.CreateContext(); - await context.GraphicsElements.AddAsync( - new GraphicsElement - { - Path = $"/templates/image/{GraphicsElementDefaults.OnNowNextFileName}", - Kind = GraphicsElementKind.Image - }); - await context.SaveChangesAsync(); - - Channel channel = await SeedChannel(context, "1"); - - await GraphicsElementSeeder.AttachOnNowNextByDefault(context, CancellationToken.None); - - (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); - } - [Test] public async Task Ignores_A_Non_Built_In_Element_With_A_Different_Filename() { diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 2ae4fe01d..9745b0cef 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -137,6 +137,12 @@ makes the `Kind == Text` filter in the lookup load-bearing rather than belt-and- Creating the row stays idempotent because the path it writes is the path the lookup matches — held by `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` (row 21), which reddens if the two drift. +The `Kind` filter and the path predicate each reject a row on their own, so a row that is BOTH of +another kind AND outside the seeded folder is refused by either clause alone: no single-clause +mutation can let it through, and a test asserting such a row is ignored is a test that cannot fail. +That combination is deliberately not shipped; the two halves that ARE reachable are pinned +separately, by rows 10 and 18. + `IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` pins one only on the listed name/title columns), so a SQL `Path ==` comparison is case-sensitive @@ -255,3 +261,4 @@ red. | 37 | `WatermarkIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw` | | 38 | the `ConsumesGraphicsElementIds` mode gate removed from `UpdateDecoHandler.GraphicsElementIdsMustExist` (#568) — a validator may only reject ids the apply path would consume | `Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It` | | 39 | the `ConsumesWatermarkIds` mode gate removed from `UpdateDecoHandler.WatermarkIdsMustExist` (#568) | `Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It` | +| 40 | `GetAllGraphicsElementsForApiHandler`'s `BuiltIn` reverted from `GraphicsElementDefaults.IsOnNowNext(e.Path)` to `Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName` (#568) — the API-side half of the discriminator, the folder-agnostic shape the issue reports | `GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn` | From a8240aba72b7df1b7237a37f51f15b32b6489d1d Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 18:20:31 +0200 Subject: [PATCH 14/19] docs(568): point the deco-validator aside at the carve-out that is above it, not below MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §8 Channel-graphics aside cited the deep-FK-in-a-nested-list exception as "below"; that exception is §3b line 314 and the aside is line ~944, so the pointer sent the reader the wrong way. It now names the section (§3b above) rather than a direction alone, so a later reflow cannot invert it again. Re-wrapped the same passage so `deep-FK-in-a-nested-list` no longer straddles a soft line break — Markdown joins those with a space and the term rendered with a stray gap mid-word. Refs #568 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- docs/api-conventions.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 7fa9e19e4..c7f82a03e 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -940,14 +940,15 @@ carry `graphicsElementIds` (the channel's attached `GraphicsElement` ids, reconc via `Channel.ChannelGraphicsElements`); `UpdateChannelHandler.Validate` rejects any id not present in `GraphicsElements` with 422 (previously an unhandled 500 from the FK constraint — #568), matching every other field on this full-replace DTO. The identical shape existed on `PUT /api/v1/decos/{id}` -(`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the deep-FK-in-a- -nested-list carve-out below) and is hardened the same way by `UpdateDecoHandler.Validate` -(`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). Each deco validator is gated on the -same `Override`/`Merge` mode predicate that makes the apply path consume its id list — under -`Inherit`/`Disable` the reconcile clears the join and ignores the ids, so validating them there would -422 a request over ids it was about to discard. The SPA sends both id lists whatever the mode -selector says, so that shape arrives from the real editor. `GraphicsElementResponseModel` exposes -a server-derived `builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as +(`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the +`deep-FK-in-a-nested-list` carve-out of §3b above) and is hardened the same way by +`UpdateDecoHandler.Validate` (`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). Each +deco validator is gated on the same `Override`/`Merge` mode predicate that makes the apply path +consume its id list — under `Inherit`/`Disable` the reconcile clears the join and ignores the ids, +so validating them there would 422 a request over ids it was about to discard. The SPA sends both +id lists whatever the mode selector says, so that shape arrives from the real editor. +`GraphicsElementResponseModel` exposes a server-derived `builtIn`, computed by +`GetAllGraphicsElementsForApiHandler` as `GraphicsElementDefaults.IsOnNowNext(element.Path)` — ordinal equality against the full seeded path, not the bare filename, which was folder-agnostic (#568) — never client-settable. From 722f0057e8c91182df335525ae98c439c74c6650 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 18:38:42 +0200 Subject: [PATCH 15/19] docs(568): bound the "cannot fail" claim to the two rows that measure it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sentence explaining why the wrong-kind-and-wrong-folder case is not shipped said "no single-clause mutation can let it through" — an unbounded quantifier over a population nothing here measures. What is actually established is narrower and is established: rows 10 and 18 are the two clauses of `GetBuiltInElementId`, each measured, and dropping either leaves the other rejecting such a row. The claim now says that, and names those rows as its evidence. Refs #568 Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- docs/graphics-elements.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 9745b0cef..5847f4bd7 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -137,11 +137,11 @@ makes the `Kind == Text` filter in the lookup load-bearing rather than belt-and- Creating the row stays idempotent because the path it writes is the path the lookup matches — held by `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` (row 21), which reddens if the two drift. -The `Kind` filter and the path predicate each reject a row on their own, so a row that is BOTH of -another kind AND outside the seeded folder is refused by either clause alone: no single-clause -mutation can let it through, and a test asserting such a row is ignored is a test that cannot fail. -That combination is deliberately not shipped; the two halves that ARE reachable are pinned -separately, by rows 10 and 18. +The lookup's two clauses each reject a row on their own, so a row that is BOTH of another kind AND +outside the seeded folder is still refused when either one is dropped — the survivor rejects it. +A test asserting such a row is ignored therefore cannot appear in row 10's red set or row 18's; it +is a test that cannot fail, and the combination is deliberately not shipped. The two halves that +ARE reachable are pinned separately, by those two rows. `IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` From e7f794057d5108d03b69c7aa2c1086e5ebb34107 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 19:38:29 +0200 Subject: [PATCH 16/19] fix(568): bound the client-supplied id lists, name their field, and make Kind part of the built-in identity 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 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Channels/Commands/UpdateChannelHandler.cs | 87 +++++++--- .../GetAllGraphicsElementsForApiHandler.cs | 2 +- .../Scheduling/Commands/UpdateDecoHandler.cs | 122 ++++++++------ .../Validators/IdListValidation.cs | 73 +++++++++ .../Graphics/GraphicsElementDefaults.cs | 38 +++-- .../Graphics/GraphicsElementSeeder.cs | 29 ++-- .../UpdateChannelGraphicsElementsTests.cs | 151 ++++++++++++++++++ .../Graphics/GraphicsElementHandlerTests.cs | 24 +++ .../UpdateDecoGraphicsElementsTests.cs | 70 ++++++++ .../Support/ArmedSaveFailureInterceptor.cs | 43 +++++ ErsatzTV.Tests/Support/InMemoryTvContext.cs | 10 +- 11 files changed, 546 insertions(+), 103 deletions(-) create mode 100644 ErsatzTV.Application/Validators/IdListValidation.cs create mode 100644 ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs diff --git a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs index 5d5964514..ee57f751c 100644 --- a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs @@ -47,8 +47,13 @@ public class UpdateChannelHandler( { Either resolvedLogo = await ResolveLogoPath(request, cancellationToken); return await resolvedLogo.Match( - Right: async logoPath => Right( - await ApplyUpdateRequest(dbContext, c, request, logoPath, cancellationToken)), + Right: logoPath => + ApplyUpdateRequestTranslatingLostRace( + dbContext, + c, + request, + logoPath, + cancellationToken), Left: e => Task.FromResult(Left(e))); }, Fail: errors => Task.FromResult(Left(errors.Join()))); @@ -76,6 +81,47 @@ public class UpdateChannelHandler( return cached; } + // Validation and the write are two statements, not one atomic step: RefreshGraphicsElements + // deletes elements whose template file is gone, and a delete landing between the two turns the + // join insert back into the FK violation the validator exists to prevent -- the unhandled 500 + // again (#568). A transaction does not close that window either: neither provider locks the rows + // the validator merely READ, so the concurrent delete still commits. Ask the existence question + // again on the failure path instead, and return the same 422 the validator would have returned; + // a DbUpdateException from any other cause keeps its own exception rather than being reported as + // a client error. + private async Task> ApplyUpdateRequestTranslatingLostRace( + TvContext dbContext, + Channel channel, + UpdateChannel request, + string logoPath, + CancellationToken cancellationToken) + { + try + { + return Right( + await ApplyUpdateRequest(dbContext, channel, request, logoPath, cancellationToken)); + } + catch (DbUpdateException) + { + // a fresh context: the failed save left the original one tracking the changes that + // could not be written, so the same query there could be answered from those. + await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + Validation recheck = + await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken); + + Option maybeError = recheck.Match( + Succ: _ => Option.None, + Fail: errors => Some(errors.Join())); + + foreach (BaseError error in maybeError) + { + return Left(error); + } + + throw; + } + } + private async Task ApplyUpdateRequest( TvContext dbContext, Channel c, @@ -300,31 +346,22 @@ public class UpdateChannelHandler( // id; an id with no matching GraphicsElement row would otherwise hit // FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId at SaveChangesAsync and surface as // an unhandled 500 (there is no global exception filter). Reject it here instead, for parity - // with every other FK field on this full-replace DTO (#568). - private static async Task> GraphicsElementIdsMustExist( + // with every other FK field on this full-replace DTO (#568). The count cap, the request field + // named in the message and the cap on echoed ids all live in Validators.IdsMustExist, shared + // with the two UpdateDecoHandler twins so the three cannot drift apart. + private static Task> GraphicsElementIdsMustExist( TvContext dbContext, UpdateChannel request, - CancellationToken cancellationToken) - { - List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; - if (requested.Count == 0) - { - return Unit.Default; - } - - List existingIds = await dbContext.GraphicsElements - .Where(e => requested.Contains(e.Id)) - .Select(e => e.Id) - .ToListAsync(cancellationToken); - - List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); - if (missingIds.Count == 0) - { - return Unit.Default; - } - - return BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}"); - } + CancellationToken cancellationToken) => + Validators.IdsMustExist( + request, + r => r.GraphicsElementIds, + "Graphics element", + (ids, token) => dbContext.GraphicsElements + .Where(e => ids.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(token), + cancellationToken); private static async Task> MirrorSourceMustBeValid( TvContext dbContext, diff --git a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs index 78e1e710a..1d7a3847f 100644 --- a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs +++ b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs @@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory db .Select(e => new { Vm = ProjectToViewModel(e), - BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path) + BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path, e.Kind) }) .OrderBy(x => x.Vm.Name == x.Vm.FileName) .ThenBy(x => x.Vm.Name) diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index db8e558b7..51ef9aee2 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -19,7 +19,49 @@ public class UpdateDecoHandler( { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); Validation validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(ps => ApplyUpdateRequest(dbContext, ps, request, cancellationToken)); + return await validation.Match( + Succ: deco => ApplyUpdateRequestTranslatingLostRace(dbContext, deco, request, cancellationToken), + Fail: errors => Task.FromResult(Left(errors.Join()))); + } + + // Mirrors UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace (#568): validation and the + // write are two statements, so a concurrent delete of a validated watermark or graphics element + // -- RefreshGraphicsElements deletes elements whose template file is gone -- lands the join + // insert on the FK violation the validators exist to prevent, as an unhandled 500. A transaction + // does not close that window either (neither provider locks the rows the validator merely READ), + // so ask the existence questions again on the failure path and return the same 422; a + // DbUpdateException from any other cause keeps its own exception. + private async Task> ApplyUpdateRequestTranslatingLostRace( + TvContext dbContext, + Deco existing, + UpdateDeco request, + CancellationToken cancellationToken) + { + try + { + return await ApplyUpdateRequest(dbContext, existing, request, cancellationToken); + } + catch (DbUpdateException) + { + // a fresh context: the failed save left the original one tracking the changes that + // could not be written, so the same query there could be answered out of those. + await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + Validation recheck = ( + await WatermarkIdsMustExist(recheckContext, request, cancellationToken), + await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken)) + .Apply((_, _) => Unit.Default); + + Option maybeError = recheck.Match( + Succ: _ => Option.None, + Fail: errors => Some(errors.Join())); + + foreach (BaseError error in maybeError) + { + return Left(error); + } + + throw; + } } private async Task ApplyUpdateRequest( @@ -241,60 +283,40 @@ public class UpdateDecoHandler( // an id with no matching row hits the FK constraint at SaveChangesAsync and surfaces as an // unhandled 500 (there is no global exception filter). These are top-level fields on // ReplaceDecoRequest, the same position as graphicsElementIds on UpdateChannelRequest -- not the - // "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md. - private static async Task> WatermarkIdsMustExist( + // "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md. Both go + // through Validators.IdsMustExist, the one place the count cap, the request field named in the + // message and the cap on echoed ids are written. + private static Task> WatermarkIdsMustExist( TvContext dbContext, UpdateDeco request, - CancellationToken cancellationToken) - { - if (!ConsumesWatermarkIds(request)) - { - return Unit.Default; - } + CancellationToken cancellationToken) => + !ConsumesWatermarkIds(request) + ? Task.FromResult>(Unit.Default) + : Validators.IdsMustExist( + request, + r => r.WatermarkIds, + "Watermark", + (ids, token) => dbContext.ChannelWatermarks + .Where(w => ids.Contains(w.Id)) + .Select(w => w.Id) + .ToListAsync(token), + cancellationToken); - List requested = request.WatermarkIds?.Distinct().ToList() ?? []; - if (requested.Count == 0) - { - return Unit.Default; - } - - List existingIds = await dbContext.ChannelWatermarks - .Where(w => requested.Contains(w.Id)) - .Select(w => w.Id) - .ToListAsync(cancellationToken); - - List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); - return missingIds.Count == 0 - ? Unit.Default - : BaseError.New($"Watermark(s) do not exist: {string.Join(", ", missingIds)}"); - } - - private static async Task> GraphicsElementIdsMustExist( + private static Task> GraphicsElementIdsMustExist( TvContext dbContext, UpdateDeco request, - CancellationToken cancellationToken) - { - if (!ConsumesGraphicsElementIds(request)) - { - return Unit.Default; - } - - List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; - if (requested.Count == 0) - { - return Unit.Default; - } - - List existingIds = await dbContext.GraphicsElements - .Where(e => requested.Contains(e.Id)) - .Select(e => e.Id) - .ToListAsync(cancellationToken); - - List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); - return missingIds.Count == 0 - ? Unit.Default - : BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}"); - } + CancellationToken cancellationToken) => + !ConsumesGraphicsElementIds(request) + ? Task.FromResult>(Unit.Default) + : Validators.IdsMustExist( + request, + r => r.GraphicsElementIds, + "Graphics element", + (ids, token) => dbContext.GraphicsElements + .Where(e => ids.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(token), + cancellationToken); private static Task> DecoMustExist( TvContext dbContext, diff --git a/ErsatzTV.Application/Validators/IdListValidation.cs b/ErsatzTV.Application/Validators/IdListValidation.cs new file mode 100644 index 000000000..70226c979 --- /dev/null +++ b/ErsatzTV.Application/Validators/IdListValidation.cs @@ -0,0 +1,73 @@ +using System.Linq.Expressions; +using ErsatzTV.Core; + +namespace ErsatzTV.Application; + +public static partial class Validators +{ + /// + /// The largest id list a full-replace write path accepts in one of its top-level id fields. + /// Deliberately far above any real payload -- the lists it bounds select from tables an + /// operator curates by hand (graphics elements, watermarks), where a few dozen rows is a + /// large install -- so the bound is a ceiling on abuse, not a product limit anyone can reach + /// by using the editor (#568). + /// + public const int MaximumIdListCount = 512; + + // A 422 that echoes every rejected id turns an oversized request into an oversized response. + // Enough ids to fix the payload by hand, then a count. + private const int MaximumReportedMissingIds = 10; + + /// + /// The shared existence check for a top-level list of FK ids on a full-replace request: + /// bound the list, resolve which of its ids exist through , + /// and reject the rest with a 422 that names the request field it came from. + /// + /// + /// The count is taken from the RAW list, before Distinct and before any database + /// work: deduplication is not what the request costs. A million-entry list of one repeated + /// id parses, allocates and materializes in full whatever the distinct count turns out to + /// be, so a cap applied after Distinct would bound the query and leave the request + /// itself unbounded. + /// + public static async Task> IdsMustExist( + T input, + Expression>> expression, + string noun, + Func, CancellationToken, Task>> findExisting, + CancellationToken cancellationToken) + { + string field = GetMemberName(expression); + List submitted = expression.Compile()(input) ?? []; + + if (submitted.Count > MaximumIdListCount) + { + return BaseError.New( + $"[{field}] contains {submitted.Count} ids; at most {MaximumIdListCount} are accepted. " + + "The whole list is materialized into one existence query and then reconciled against every " + + "row already attached, so a longer list turns a single request into unbounded work."); + } + + List requested = submitted.Distinct().ToList(); + if (requested.Count == 0) + { + return Unit.Default; + } + + List existingIds = await findExisting(requested, cancellationToken); + + List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); + if (missingIds.Count == 0) + { + return Unit.Default; + } + + return BaseError.New($"[{field}] {noun}(s) do not exist: {DescribeIds(missingIds)}"); + } + + private static string DescribeIds(IReadOnlyList ids) => + ids.Count <= MaximumReportedMissingIds + ? string.Join(", ", ids) + : $"{string.Join(", ", ids.Take(MaximumReportedMissingIds))} (and " + + $"{ids.Count - MaximumReportedMissingIds} more)"; +} diff --git a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs index 7b32e85c1..f946d1cea 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -1,4 +1,5 @@ using System.IO; +using ErsatzTV.Core.Domain; namespace ErsatzTV.Core.Graphics; @@ -21,20 +22,31 @@ public static class GraphicsElementDefaults Path.Combine(FileSystemLayout.GraphicsElementsTextTemplatesFolder, OnNowNextFileName); /// - /// The one identity test for the built-in On Now / Next element. Ordinal on purpose, and so - /// case-sensitive on purpose: the built-in element is the exact file the seeder wrote, at - /// the exact path it wrote it to. + /// 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. /// /// - /// Callers compare in memory rather than in a Where clause, because in SQL the answer - /// would be the PROVIDER's to give: GraphicsElement.Path takes no explicit collation - /// (TvContext.OnModelCreating 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. + /// + /// Kind 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). + /// + /// + /// Callers compare in memory rather than in a Where clause, because in SQL the + /// answer would be the PROVIDER's to give: GraphicsElement.Path takes no explicit + /// collation (TvContext.OnModelCreating 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. + /// /// - public static bool IsOnNowNext(string path) => - string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal); + public static bool IsOnNowNext(string path, GraphicsElementKind kind) => + kind == GraphicsElementKind.Text && string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal); } diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index f3ac98c74..4ee808601 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -260,29 +260,32 @@ public static class GraphicsElementSeeder /// Identity is the full seeded path, never the user-editable Name (the #67 lesson carried into /// #74) and never the bare filename (#568: filename-only matching is folder-agnostic, so a user /// element named exactly `on-now-next.yml` in a different template folder would also match). - /// The Kind filter is load-bearing rather than decorative: EnsureBuiltInElementRow - /// asks this method whether the row it is about to create already exists, so a row of another - /// kind at the seeded path must NOT answer yes -- it would suppress the Text row every consumer - /// resolves. + /// The Kind half of that identity is load-bearing rather than decorative: + /// EnsureBuiltInElementRow asks this method whether the row it is about to create already + /// exists, so a row of another kind at the seeded path must NOT answer yes -- it would suppress + /// the Text row every consumer resolves. /// /// - /// The path comparison is in memory rather - /// than a Where clause: in SQL the match would be the provider's collation to decide, and - /// this site would then disagree with the API's `builtIn` (which compares in memory) on MySQL. - /// The Kind filter stays in SQL because it is an enum, not a string. + /// Both halves are + /// in memory rather than a Where clause. The path half must be, or the match would be the + /// provider's collation to decide and this site would disagree with the API's `builtIn` (which + /// compares in memory) on MySQL. The Kind half could be a SQL filter -- it is an enum, + /// not a string -- but then this site would hold half the identity and the predicate the other + /// half, and the API site could apply the predicate alone and quietly answer for rows this one + /// rejects. That is exactly the disagreement #568 found, so identity is one predicate applied + /// whole, at every site. /// public static async Task> GetBuiltInElementId( TvContext context, CancellationToken cancellationToken) { - List<(int Id, string Path)> candidates = await context.GraphicsElements - .Where(e => e.Kind == GraphicsElementKind.Text) - .Select(e => new { e.Id, e.Path }) + List<(int Id, string Path, GraphicsElementKind Kind)> candidates = await context.GraphicsElements + .Select(e => new { e.Id, e.Path, e.Kind }) .ToListAsync(cancellationToken) - .Map(rows => rows.Select(r => (r.Id, r.Path)).ToList()); + .Map(rows => rows.Select(r => (r.Id, r.Path, r.Kind)).ToList()); List matches = candidates - .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path)) + .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path, c.Kind)) .Select(c => c.Id) .OrderBy(id => id) .ToList(); diff --git a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs index d10ffbcfe..97e7a901c 100644 --- a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs @@ -1,3 +1,5 @@ +using System.Globalization; +using ErsatzTV.Application; using ErsatzTV.Application.Channels; using ErsatzTV.Core; using ErsatzTV.Core.Domain; @@ -29,6 +31,28 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase return (elementA.Id, elementB.Id); } + private async Task> SeedGraphicsElements(int count) + { + await using TvContext context = Db.CreateContext(); + List elements = Enumerable.Range(0, count) + .Select(i => new GraphicsElement { Path = $"element-{i}.yml" }) + .ToList(); + context.GraphicsElements.AddRange(elements); + await context.SaveChangesAsync(); + return elements.Select(e => e.Id).ToList(); + } + + // Replaces the harness the base class built with one whose SaveChangesAsync can be made to fail + // on demand. Foreign keys are off in InMemoryTvContext, so the FK violation a concurrent delete + // really produces cannot be raised by seeding alone. + private async Task UseFailingSaveHarness() + { + await Db.DisposeAsync(); + var interceptor = new ArmedSaveFailureInterceptor(); + Db = await InMemoryTvContext.CreateAsync(interceptor); + return interceptor; + } + [Test] public async Task Should_Reconcile_GraphicsElement_Join_Add_Then_Remove() { @@ -95,6 +119,7 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase BaseError error = LeftOf(result); error.ShouldNotBeOfType(); + error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain("999"); // no partial write: the channel keeps no graphics element association @@ -117,6 +142,132 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase BaseError error = LeftOf(result); error.ShouldNotBeOfType(); + error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain("12345"); } + + // #568: the id list is client-supplied and was bounded only by the Kestrel body cap, which is a + // transport limit and not a collection limit. The cap is Validators.MaximumIdListCount, shared + // by all three id-list validators; these three tests pin its two edges and the shape that makes + // its placement matter. Removing the cap from Validators.IdsMustExist is row 41 of the mutation + // table in docs/graphics-elements.md. + [Test] + public async Task Should_Accept_Exactly_The_Maximum_Number_Of_GraphicsElementIds() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + List ids = await SeedGraphicsElements(Validators.MaximumIdListCount); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + await using TvContext context = Db.CreateContext(); + Channel reloaded = await context.Channels.Include(c => c.ChannelGraphicsElements) + .SingleAsync(c => c.Id == channel.Id); + reloaded.ChannelGraphicsElements.Count.ShouldBe(Validators.MaximumIdListCount); + } + + [Test] + public async Task Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + List ids = Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain((Validators.MaximumIdListCount + 1).ToString(CultureInfo.InvariantCulture)); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + // The cap counts the RAW list, before Distinct: every one of these ids exists and they collapse + // to a single distinct id, so a cap applied after deduplication would accept this request and + // leave the parse and materialization it costs unbounded. + [Test] + public async Task Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + List ids = Enumerable.Repeat(elementAId, Validators.MaximumIdListCount + 1).ToList(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain((Validators.MaximumIdListCount + 1).ToString(CultureInfo.InvariantCulture)); + error.Value.ShouldNotContain("do not exist"); + } + + // A 422 that echoes every rejected id turns an oversized request into an oversized response. + // Removing the truncation from Validators.DescribeIds is row 42 of the mutation table. + [Test] + public async Task Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + List ids = Enumerable.Range(1001, 30).ToList(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("1001"); + error.Value.ShouldContain("(and 20 more)"); + error.Value.ShouldNotContain("1030"); + } + + // #568: validation and the write are two statements, so RefreshGraphicsElements can delete a + // validated element in between and hand the join insert the FK violation the validator exists to + // prevent -- the unhandled 500 again. Removing the DbUpdateException catch from + // ApplyUpdateRequestTranslatingLostRace is row 43 of the mutation table. + [Test] + public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422() + { + ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness(); + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + + interceptor.SqlBeforeFailing = + $"DELETE FROM GraphicsElement WHERE Id = {elementAId.ToString(CultureInfo.InvariantCulture)}"; + interceptor.Armed = true; + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId]), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(elementAId.ToString(CultureInfo.InvariantCulture)); + } + + // The other half of that catch: a DbUpdateException whose cause is NOT a missing graphics + // element is a real fault and must keep its own exception rather than be reported to the client + // as a validation error about ids that are all still present. + [Test] + public async Task Should_Not_Report_An_Unrelated_DbUpdateException_As_A_Graphics_Element_422() + { + ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness(); + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + + interceptor.Armed = true; + + await Should.ThrowAsync( + async () => await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId]), + CancellationToken.None)); + } } diff --git a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs index ff0a8c540..839a2bd77 100644 --- a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs @@ -120,6 +120,30 @@ public class GraphicsElementHandlerTests result[0].BuiltIn.ShouldBeFalse(); } + // #568: Kind is part of the built-in element's identity, not a second test the seeder applies + // and the API skips. GetBuiltInElementId refuses a wrong-kind row at the seeded path -- it has + // to, since EnsureBuiltInElementRow asks it whether the Text row it is about to create already + // exists -- so an API that reported the same row as builtIn:true would have the two sites + // disagreeing about one row. Reddens if the Kind conjunct is dropped from + // GraphicsElementDefaults.IsOnNowNext (row 18 of the mutation table in docs/graphics-elements.md). + [Test] + public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_A_Wrong_Kind_Row_At_The_Seeded_Path_As_BuiltIn() + { + await SeedElement( + 1, + GraphicsElementDefaults.OnNowNextSeededPath, + GraphicsElementKind.Image, + string.Empty); + + var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); + + List result = + await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None); + + result.Count.ShouldBe(1); + result[0].BuiltIn.ShouldBeFalse(); + } + // The seeded path with only the FILENAME's case changed -- same folder, same spelling. private static string CaseVariantOfSeededPath() => System.IO.Path.Combine( diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index 360ef2055..136c77b5f 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using System.Threading.Channels; using ErsatzTV.Application; using ErsatzTV.Application.Scheduling; @@ -137,6 +138,7 @@ public class UpdateDecoGraphicsElementsTests IsLeft(result).ShouldBeTrue(); BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain("999"); // no partial write: the deco keeps no graphics element association @@ -183,6 +185,7 @@ public class UpdateDecoGraphicsElementsTests IsLeft(result).ShouldBeTrue(); BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[WatermarkIds]"); error.Value.ShouldContain("999"); } @@ -253,4 +256,71 @@ public class UpdateDecoGraphicsElementsTests Deco reloaded = await reload.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1); reloaded.DecoGraphicsElements.Select(x => x.GraphicsElementId).ShouldBe(new[] { elementId }); } + + // Both deco id lists go through the same Validators.IdsMustExist as the channel's, so both + // inherit the same raw-count cap; the channel fixture pins its edges, these two pin that each + // deco field is actually behind it and names itself when it rejects. + [Test] + public async Task Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementIds: Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList()), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + [Test] + public async Task Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate( + watermarkMode: DecoMode.Override, + watermarkIds: Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList()), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[WatermarkIds]"); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + // The deco twin of the channel handler's lost-race translation: an element deleted between + // Validate and SaveChangesAsync must come back as the validator's own 422, not the FK + // exception. Removing the DbUpdateException catch from + // UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace is row 44 of the mutation table in + // docs/graphics-elements.md. + [Test] + public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422() + { + var interceptor = new ArmedSaveFailureInterceptor(); + await _db.DisposeAsync(); + _db = await InMemoryTvContext.CreateAsync(interceptor); + + await SeedDeco(); + int elementId = await SeedGraphicsElement(); + + interceptor.SqlBeforeFailing = + $"DELETE FROM GraphicsElement WHERE Id = {elementId.ToString(CultureInfo.InvariantCulture)}"; + interceptor.Armed = true; + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementIds: [elementId]), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(elementId.ToString(CultureInfo.InvariantCulture)); + } } diff --git a/ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs b/ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs new file mode 100644 index 000000000..f1d9e532e --- /dev/null +++ b/ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs @@ -0,0 +1,43 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; + +namespace ErsatzTV.Tests.Support; + +/// +/// Makes the next SaveChangesAsync fail the way a lost race against a concurrent delete +/// does: run first (the delete), then throw the +/// the foreign-key violation would have produced. +/// +/// +/// What is simulated is the TRIGGER, not the decision: runs +/// with foreign keys OFF, so a real constraint cannot fire here. The handler's recovery — re-ask +/// the existence question against the post-delete state, translate a now-missing id into the +/// validator's own 422, re-throw anything else — is real code running against real data. +/// Disarms itself on the first fire so the recovery path gets a working context. +/// +public sealed class ArmedSaveFailureInterceptor : SaveChangesInterceptor +{ + public bool Armed { get; set; } + + public string SqlBeforeFailing { get; set; } = string.Empty; + + public override async ValueTask> SavingChangesAsync( + DbContextEventData eventData, + InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (!Armed) + { + return await base.SavingChangesAsync(eventData, result, cancellationToken); + } + + Armed = false; + + if (!string.IsNullOrEmpty(SqlBeforeFailing)) + { + await eventData.Context!.Database.ExecuteSqlRawAsync(SqlBeforeFailing, cancellationToken); + } + + throw new DbUpdateException("simulated: a validated principal row was deleted concurrently"); + } +} diff --git a/ErsatzTV.Tests/Support/InMemoryTvContext.cs b/ErsatzTV.Tests/Support/InMemoryTvContext.cs index b8911980c..e74bd5761 100644 --- a/ErsatzTV.Tests/Support/InMemoryTvContext.cs +++ b/ErsatzTV.Tests/Support/InMemoryTvContext.cs @@ -3,6 +3,7 @@ using ErsatzTV.Infrastructure.Data; using ErsatzTV.Infrastructure.Sqlite.Data; using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.Logging.Abstractions; namespace ErsatzTV.Tests.Support; @@ -27,7 +28,13 @@ public sealed class InMemoryTvContext : IAsyncDisposable public IDbContextFactory Factory => new TestDbContextFactory(_options); - public static async Task CreateAsync() + /// + /// Extra EF interceptors registered on every context this harness hands out. The only + /// current use is making SaveChangesAsync fail on demand: foreign keys are off here + /// (see the type summary), so a write-path failure a real FK constraint would raise cannot + /// be produced by seeding alone. + /// + public static async Task CreateAsync(params IInterceptor[] interceptors) { TvContext.IsSqlite = true; TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation; @@ -38,6 +45,7 @@ public sealed class InMemoryTvContext : IAsyncDisposable DbContextOptions options = new DbContextOptionsBuilder() .UseSqlite(connection) + .AddInterceptors(interceptors) .Options; await using (TvContext context = Create(options)) From 3541683c99a44a65d7d48705bb8d2eb1f474a726 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 20:08:29 +0200 Subject: [PATCH 17/19] docs(568): re-measure every mutation row the merged identity and the shared validator moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nine rows of the mutation table in docs/graphics-elements.md name a clause that this branch's last commit moved, merged or gave new callers, and a red set is a measurement of the tree it ships in. All were re-taken against the whole ErsatzTV.Tests project (2139 tests, 6 skipped) on the code as it now stands, and four new rows added for the clauses the fix introduced. What moved and why the numbers changed: - Row 10 was the seeded-path filter alone. `Kind` now lives inside `IsOnNowNext`, so dropping the lookup's `Where` drops both halves at once and reddens four tests, not three. - Row 18 was the seeder's SQL `Kind == Text` filter and is now the `kind` conjunct of the shared predicate, so it reddens the API site too — a second cross-fixture row. - Rows 35-37 pick up the count-cap tests, since the cap rides in the validator they disarm. Rows 33, 34, 38, 39 re-measured unchanged. - Row 40's mutation text follows the API call's new two-argument shape; it reddens the new wrong-kind test as well. - Rows 41-44 are the new clauses: the raw-count cap (one clause, three call sites, which is what its red set shows), the diagnostic-id truncation, and the two lost-race catches. The two self-counted figures above the table were recounted from the table itself rather than adjusted: twenty-one multi-test rows and five cross-fixture ones (13, 18, 22, 33, 41). The deco lost-race test is renamed so no two rows cite the same test name. api-conventions.md gains the three rules the fix establishes for any write path with a top-level FK id list — bound the raw list, name the field, translate a lost check-then-write race — in the handler-hardening checklist where they belong rather than as a #568 anecdote. Refs #568 Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../UpdateDecoGraphicsElementsTests.cs | 2 +- docs/api-conventions.md | 28 +++++++- .../graphics/channel-level-attachment.md | 8 ++- .../graphics/on-now-next-on-by-default.md | 4 +- docs/domain-model.md | 2 +- docs/graphics-elements.md | 67 +++++++++++-------- 6 files changed, 73 insertions(+), 38 deletions(-) diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index 136c77b5f..f2c8c196e 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -300,7 +300,7 @@ public class UpdateDecoGraphicsElementsTests // UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace is row 44 of the mutation table in // docs/graphics-elements.md. [Test] - public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422() + public async Task Should_Translate_A_Deco_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422() { var interceptor = new ArmedSaveFailureInterceptor(); await _db.DisposeAsync(); diff --git a/docs/api-conventions.md b/docs/api-conventions.md index c7f82a03e..1118036fb 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -302,6 +302,24 @@ handler's validation when a lookup fails, so the controller-side mapping falls o caller) → surface as 422 instead of silently returning a shorter list. - Unbounded `int`/`TimeSpan` inputs from the request → clamp or validate, per the Logs pagination pattern above. +- **A top-level LIST of FK ids is an unbounded input too** — the body-size cap is a transport limit, + not a collection limit, and millions of compact integers fit under it. Bound the list with + `Validators.IdsMustExist` (#568), which counts the **raw** list before `Distinct` and before any + database work — deduplication is not what the request costs to parse and materialize — and caps how + many rejected ids the 422 echoes back, so an oversized request does not earn an oversized response. + The rollout to the other FK-id validators on these DTOs is #917. +- **Name the request field in the validation message**, `[GraphicsElementIds] …`, using the same + `[{GetMemberName(expression)}]` prefix `Validators.NotEmpty`/`NotLongerThan`/`AtLeast` already + produce. A full-replace DTO carries several id lists, and `Graphics element(s) do not exist: 42` + does not say which one to fix. +- **Validation and the write are two statements, so an FK the validator confirmed can be gone by + `SaveChangesAsync`** — a background job deleting the principal row (e.g. `RefreshGraphicsElements` + removing elements whose template file is gone) restores the very 500 the validator was added to + prevent. A transaction does not close that window: neither provider locks the rows the validator + merely READ. Catch `DbUpdateException` around the save, re-ask the existence question on a **fresh** + context (the failed one still tracks the changes it could not write), and return the validator's own + 422 if an id has since gone — re-throwing anything else, so a real fault is not reported as a client + error (#568). - **Dereferencing a request `string` (e.g. `request.Name.Length`) is a latent 500** — request DTOs carry no `#nullable` context (§2), so a `string Name` binds `null` from `name: null`/an omitted field and there is no implicit `[Required]`; a raw `.Length`/`.Trim()` throws `NullReferenceException` → an @@ -947,10 +965,16 @@ deco validator is gated on the same `Override`/`Merge` mode predicate that makes consume its id list — under `Inherit`/`Disable` the reconcile clears the join and ignores the ids, so validating them there would 422 a request over ids it was about to discard. The SPA sends both id lists whatever the mode selector says, so that shape arrives from the real editor. +All three id lists go through one shared validator, `Validators.IdsMustExist`, which is where the +count cap (`MaximumIdListCount`), the request field named in the message and the cap on echoed ids +are written once rather than three times. `GraphicsElementResponseModel` exposes a server-derived `builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as -`GraphicsElementDefaults.IsOnNowNext(element.Path)` — ordinal equality against the full seeded path, -not the bare filename, which was folder-agnostic (#568) — never client-settable. +`GraphicsElementDefaults.IsOnNowNext(element.Path, element.Kind)` — ordinal equality against the full +seeded path AND `Kind == Text`, not the bare filename, which was folder-agnostic (#568) — never +client-settable. `Kind` is inside that predicate rather than a filter each caller adds: the seeder +requires it, and an API that did not would report a wrong-kind row at the seeded path as +`builtIn:true` while the seeder refused to treat it as the built-in element (#568). ## 9. Authentication — session-or-key posture (fail-closed) diff --git a/docs/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index fdcae37dc..28dd04d1b 100644 --- a/docs/decisions/records/graphics/channel-level-attachment.md +++ b/docs/decisions/records/graphics/channel-level-attachment.md @@ -42,9 +42,11 @@ four joins (composite key `{ChannelId, GraphicsElementId}`), added via a dual-pr direct #67 lesson (`WatermarkResponseModel.imageSource`). `GraphicsElementResponseModel` gained a server-derived `BuiltIn` bool, computed from the row's `Path` rather than the element's editable `Name`. The test is `GraphicsElementDefaults.IsOnNowNext` — ordinal equality against the full - seeded path. #568 sharpened it from the original bare-filename comparison, which was - folder-agnostic: a user element named exactly `on-now-next.yml` in another template folder also - reported `builtIn:true`. Case sensitivity is kept, deliberately: the remedy #568 prescribes — the + seeded path, plus `Kind == Text`. #568 sharpened it from the original bare-filename comparison, + which was folder-agnostic: a user element named exactly `on-now-next.yml` in another template + folder also reported `builtIn:true`. `Kind` is inside the predicate rather than a filter each + caller adds, because the seeder's lookup requires it: split across the two sites, an Image row at + the seeded path was `builtIn:true` on the wire while the seeder refused to resolve it. Case sensitivity is kept, deliberately: the remedy #568 prescribes — the full seeded relative path — is exactly as case-sensitive as the filename match it replaces, and a case-INsensitive test would hand the built-in identity to a user element differing from the seeded path only in case. The built-in element is the exact file the seeder wrote, at the path it wrote it diff --git a/docs/decisions/records/graphics/on-now-next-on-by-default.md b/docs/decisions/records/graphics/on-now-next-on-by-default.md index 5279b03b5..21789e94b 100644 --- a/docs/decisions/records/graphics/on-now-next-on-by-default.md +++ b/docs/decisions/records/graphics/on-now-next-on-by-default.md @@ -60,8 +60,8 @@ is simply absent, indistinguishable from never having enabled it — so the one- re-attaches it. That is inherent to "enable it on all channels by default" rather than a defect. The never-re-attach guarantee therefore holds *from the marker onwards*, not across the upgrade boundary. -Identity is the element's **full seeded path** — `GraphicsElementDefaults.IsOnNowNext`, ordinal -equality against `OnNowNextSeededPath` — never the user-editable `Name`: the #67 lesson carried +Identity is the element's **full seeded path and `Kind == Text`** — `GraphicsElementDefaults.IsOnNowNext`, +ordinal equality against `OnNowNextSeededPath` — never the user-editable `Name`: the #67 lesson carried through #74, sharpened from a bare filename to the full path by #568 because a filename-only match was folder-agnostic (see `graphics.channel-level-attachment` and `docs/graphics-elements.md`). diff --git a/docs/domain-model.md b/docs/domain-model.md index 16eb8295f..dc04b1cc7 100644 --- a/docs/domain-model.md +++ b/docs/domain-model.md @@ -68,7 +68,7 @@ Channel (1) ──< Playout (0..N per channel; ChannelPlayoutSource distinguishe | **Seasonal / date-conditional scheduling** (#73) | Holiday/seasonal channels are **not a separate feature** — they are the existing date predicate on `IAlternateScheduleItem`, implemented by `ProgramScheduleAlternate` (Classic) and `PlayoutTemplate` (Block), evaluated by `AlternateScheduleSelector.GetScheduleForDate` (first match by `Index`, catch-all last). **Leaving `StartYear`/`EndYear` empty makes the range repeat every year** — the "set once, works every December" switch; explicit years (required in pairs) mean a one-off window and disable wrap-around detection. Wrap-around (Nov→Feb) and invalid/leap dates (Feb 31) are handled. No *soft* prioritization primitive exists (binary first-match-wins); that ask belongs to #70's weighting work. See `channels.md` → "Recipe: seasonal / holiday programming" and `decisions.md` 2026-07-17. | `IAlternateScheduleItem`, `ProgramScheduleAlternate`, `PlayoutTemplate` | `/app/playouts/{id}/alternate-schedules`, `/app/playouts/{id}/templates` | | **Playback order** | How a schedule item's source(s) are sequenced (`PlaybackOrder`). Note three that are easily confused: **`Shuffle`** is Fisher–Yates over the flattened items, so airtime is implicitly proportional to collection size (a 200-episode show swamps a 20-episode one). **`ShuffleInOrder`** is a balanced shuffle (keyj) that pads sources to equal length with non-emitting spacers — it plays every item exactly once per cycle, so it prevents *clumping* but leaves airtime proportional to size; it is **not** fair-share. **`WeightedShuffle`** (#70) picks a *source* by smooth weighted round-robin then takes its next item, so each source's `Weight` is its share of airtime — equal weights (the default) mean equal airtime regardless of library size, with small sources looping. Classic engine only; rejected at the write path for playlist/block items. See `decisions.md` 2026-07-17. | `PlaybackOrder`, `MultiCollectionItem.Weight`, `MultiCollectionSmartItem.Weight` | `WeightedShuffle` is offered as a Playback Order **only** on classic schedule items whose source is a MultiCollection (`web/src/schedules/itemRules.ts`, #404); the per-source weights themselves are edited at `/app/multi-collections` | | **Watermark** | `ChannelWatermark` image overlay; attached at channel, schedule-item, block-item, deco, or playout-item level with position/size/opacity. | `ChannelWatermark`, `DecoWatermark`, `BlockItemWatermark`, `ProgramScheduleItemWatermark` | `/app/watermarks` | -| **Graphics element** | YAML-authored (`Text`/`Image`/`Subtitle`/`Motion`/`Script`) render-engine overlay, distinct from the image-only `ChannelWatermark` system. Attaches via 5 parallel join tables: `PlayoutItemGraphicsElement`, `ProgramScheduleItemGraphicsElement`, `BlockItemGraphicsElement`, `DecoGraphicsElement`, and (#74) **`ChannelGraphicsElement`** — a direct `Channel`-level attachment that did not exist before #74. `GraphicsElementSelector.SelectGraphicsElements` treats channel-level elements as the final fall-through **base layer**: they merge with `Merge`-mode deco elements and per-playout-item elements, but a deco in `Override`/`Disable` mode returns before that fall-through and so **suppresses** the channel overlay (and on a **filler** item, a deco whose graphics-elements section is not set to run during filler — `UseGraphicsElementsDuringFiller` false — clears it too, for `Merge` and `Override` alike); `HttpLiveStreamingDirect` always returns empty (ErsatzTV isn't transcoding, so nothing can be burned in). A built-in seeded text element, `on-now-next.yml` (`GraphicsElementDefaults.OnNowNextFileName`), is written once (`GraphicsElementSeeder.SeedOnNowNext`, guarded by the `graphics.on_now_next_seeded` ConfigElement marker, adopt-not-clobber like the #67 watermark seed) and identified to API clients via a server-derived `GraphicsElementResponseModel.builtIn` flag (path-name comparison, not name matching). Edited per-channel at Channel editor → Branding → "Show On Now / Next overlay". See `decisions.md` → `graphics.channel-level-attachment` (#74). | `GraphicsElement`, `ChannelGraphicsElement` | `/app/edit-channel/{id}` (Branding tab); YAML files under `GraphicsElementsTextTemplatesFolder` etc. are not directly SPA-edited | +| **Graphics element** | YAML-authored (`Text`/`Image`/`Subtitle`/`Motion`/`Script`) render-engine overlay, distinct from the image-only `ChannelWatermark` system. Attaches via 5 parallel join tables: `PlayoutItemGraphicsElement`, `ProgramScheduleItemGraphicsElement`, `BlockItemGraphicsElement`, `DecoGraphicsElement`, and (#74) **`ChannelGraphicsElement`** — a direct `Channel`-level attachment that did not exist before #74. `GraphicsElementSelector.SelectGraphicsElements` treats channel-level elements as the final fall-through **base layer**: they merge with `Merge`-mode deco elements and per-playout-item elements, but a deco in `Override`/`Disable` mode returns before that fall-through and so **suppresses** the channel overlay (and on a **filler** item, a deco whose graphics-elements section is not set to run during filler — `UseGraphicsElementsDuringFiller` false — clears it too, for `Merge` and `Override` alike); `HttpLiveStreamingDirect` always returns empty (ErsatzTV isn't transcoding, so nothing can be burned in). A built-in seeded text element, `on-now-next.yml` (`GraphicsElementDefaults.OnNowNextFileName`), is written once (`GraphicsElementSeeder.SeedOnNowNext`, guarded by the `graphics.on_now_next_seeded` ConfigElement marker, adopt-not-clobber like the #67 watermark seed) and identified to API clients via a server-derived `GraphicsElementResponseModel.builtIn` flag (`GraphicsElementDefaults.IsOnNowNext` — ordinal full-seeded-path equality plus `Kind == Text`, not name matching and not a bare filename, #568). Edited per-channel at Channel editor → Branding → "Show On Now / Next overlay". See `decisions.md` → `graphics.channel-level-attachment` (#74). | `GraphicsElement`, `ChannelGraphicsElement` | `/app/edit-channel/{id}` (Branding tab); YAML files under `GraphicsElementsTextTemplatesFolder` etc. are not directly SPA-edited | | **Collection** | Manual list of media items (`CollectionItem`). | `Collection` | `/app/collections` | | **SmartCollection** | Saved search — a `Query` string, no static item list. | `SmartCollection` | `/app/collections` | | **MultiCollection** | Combines multiple `Collection`s and/or `SmartCollection`s (with grouping via `MultiCollectionItem`/`MultiCollectionSmartItem`). Both join entities carry a per-source `Weight` (default 1) used by `PlaybackOrder.WeightedShuffle` (#70) and ignored by every other order — the two are mirrors, so a change to one belongs on the other. The editor exposes a per-source weight input (1..1000, mirroring the API validator) with a computed % share and a "Reset to fair share" action; it round-trips `weight` from the GET because the PUT replaces the item list (#404). | `MultiCollection` | `/app/multi-collections` (#151, weight UI #404) | diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 5847f4bd7..a2ec2303b 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -121,27 +121,32 @@ never overwrites an operator's file. Two rules govern it after that: - Updating the shipped default → `graphics.seeded-template-upgrade-by-fingerprint`. - It is attached to channels by default → `graphics.on-now-next-on-by-default`. -Identity is the **full seeded path**, never the editable `Name` and never the bare filename -(`OnNowNextFileName`) alone — a filename-only, folder-agnostic comparison let a user element named -exactly `on-now-next.yml` in a different template folder also report `builtIn:true` (#568). Both -`GraphicsElementResponseModel.BuiltIn` and `GraphicsElementSeeder.GetBuiltInElementId` resolve it -through the one predicate, `GraphicsElementDefaults.IsOnNowNext`, so the SPA never name-matches. +Identity is the **full seeded path plus `Kind == Text`**, never the editable `Name` and never the +bare filename (`OnNowNextFileName`) alone — a filename-only, folder-agnostic comparison let a user +element named exactly `on-now-next.yml` in a different template folder also report `builtIn:true` +(#568). Both halves live in one predicate, `GraphicsElementDefaults.IsOnNowNext(path, kind)`, and +both `GraphicsElementResponseModel.BuiltIn` and `GraphicsElementSeeder.GetBuiltInElementId` resolve +through it whole, so the SPA never name-matches and the two sites cannot answer differently about +the same row. `Kind` was a `Where` clause on the seeder's lookup alone until #568, and that split is +exactly what let the API report an Image row at the seeded path as `builtIn:true` while the lookup +refused to treat it as the built-in element. -The seeder's own "does this row exist yet?" test — `EnsureBuiltInElementRow`, which decides whether -to create the row at startup — does not re-derive that predicate either: it calls -`GetBuiltInElementId`, so the question asked at startup is the question every consumer asks -afterwards. As its own SQL `Path == target` comparison it could answer differently in two ways, each -leaving the built-in element undiscoverable for the life of the install (#568): the collation one -below, and `Kind`, which the SQL check ignored while the lookup requires it. That second one is what -makes the `Kind == Text` filter in the lookup load-bearing rather than belt-and-braces (row 18). -Creating the row stays idempotent because the path it writes is the path the lookup matches — held -by `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` (row 21), which reddens if the two drift. +`Kind` is load-bearing rather than decorative, which is why it is inside the identity rather than +a filter any one caller may add: the seeder's own "does this row exist yet?" test — +`EnsureBuiltInElementRow`, which decides whether to create the row at startup — calls +`GetBuiltInElementId`, so a wrong-kind row at the seeded path answering yes would suppress the Text +row every consumer resolves. Asking that question a second way, as its own SQL `Path == target` +comparison, could answer differently in two ways, each leaving the built-in element undiscoverable +for the life of the install (#568): the collation one below, and `Kind`, which the SQL check ignored +(rows 18 and 34). Creating the row stays idempotent because the path it writes is the path the +lookup matches — held by `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` (row 21), which reddens +if the two drift. -The lookup's two clauses each reject a row on their own, so a row that is BOTH of another kind AND -outside the seeded folder is still refused when either one is dropped — the survivor rejects it. -A test asserting such a row is ignored therefore cannot appear in row 10's red set or row 18's; it -is a test that cannot fail, and the combination is deliberately not shipped. The two halves that -ARE reachable are pinned separately, by those two rows. +Each half of the identity rejects a row on its own, so a row that is BOTH of another kind AND +outside the seeded folder is still refused when either half is dropped — the survivor rejects it. A +test asserting such a row is ignored therefore cannot appear in row 18's red set; it is a test that +cannot fail, and the combination is deliberately not shipped. Row 10 drops the identity call +entirely, which is a different mutation from dropping either half. `IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` @@ -174,10 +179,10 @@ Three traps this table is built to avoid: - **Measure against the whole project, never a per-fixture filter.** A filtered run structurally cannot observe a red in another fixture, so it under-reports the failure set while looking precise. - Sixteen rows below redden more than one test, and three of them span two fixture classes — those - three are exactly what a per-fixture filter cannot see. Both figures are counted from the table - itself, so a row added without recounting them makes this bullet quietly false — recount both - whenever a row is added or its red set changes. + Twenty-one rows below redden more than one test, and five of them (13, 18, 22, 33, 41) span two + fixture classes — those five are exactly what a per-fixture filter cannot see. Both figures are + counted from the table itself, so a row added without recounting them makes this bullet quietly + false — recount both whenever a row is added or its red set changes. - **A mutation that fails to COMPILE is not a result.** Warnings-as-error turn the obvious mutation shapes into build failures — `CS0162` for `if (true) { return; }`, `CS1717` for self-assignment, Sonar `S3981` for a constant-folded condition — and a build failure emits no test summary at all, @@ -231,7 +236,7 @@ red. | 7 | line-ending normalisation dropped from Normalize | `Upgrades_An_Untouched_Previous_Default_With_Windows_Line_Endings` | | 8 | HLS-Direct exclusion removed from the backfill | `Skips_Hls_Direct_Channels_Where_The_Overlay_Cannot_Render` | | 9 | already-attached filter removed | `Does_Not_Duplicate_An_Existing_Attachment`
`While_Armed_A_Restored_Element_Is_Attached_To_Every_Eligible_Channel` | -| 10 | seeded-path check removed from the built-in lookup (#568: the `IsOnNowNext` filter, the full path, not a bare-filename match) | `Ignores_A_Case_Variant_Of_The_Seeded_Path`
`Ignores_A_Non_Built_In_Element_With_A_Different_Filename`
`Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder` | +| 10 | the whole `IsOnNowNext(c.Path, c.Kind)` filter removed from the built-in lookup (#568) — both halves of the identity at once, so every candidate row matches | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row`
`Ignores_A_Case_Variant_Of_The_Seeded_Path`
`Ignores_A_Non_Built_In_Element_With_A_Different_Filename`
`Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder` | | 11 | graphics.on_now_next_default_attached guard never fires | `Does_Not_Re_Attach_After_An_Operator_Clears_It` | | 12 | create-time `ChannelGraphicsDefaults.Attach` call removed from `CreateChannelHandler` | `Attaches_The_Built_In_Element_To_A_New_Channel` | | 13 | HLS-Direct skip removed from the create path | `Leaves_An_Hls_Direct_Channel_Alone_Because_Nothing_Can_Render_There`
`Should_Not_Attach_The_Overlay_To_An_Hls_Direct_Channel` | @@ -239,7 +244,7 @@ red. | 15 | FitTextBlock drops HaloColor again | `The_Scale_Path_Preserves_Halo_Blur`
`The_Scale_Path_Preserves_The_Halo` | | 16 | unrounded inset subtracted from the budget | `Fractional_Padding_Still_Respects_Width_Percent` | | 17 | inset clamp removed | `An_Oversized_Padding_Is_Clamped_To_The_Largest_That_Fits` | -| 18 | `Kind == GraphicsElementKind.Text` filter dropped from the built-in lookup (#568) — the lookup is what `EnsureBuiltInElementRow` asks, so a wrong-kind row at the seeded path would answer for the Text row that never then gets created | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` | +| 18 | `kind == GraphicsElementKind.Text` conjunct dropped from `GraphicsElementDefaults.IsOnNowNext` (#568) — one row for both discriminator sites since they share the predicate; the lookup is what `EnsureBuiltInElementRow` asks, so a wrong-kind row at the seeded path would answer for the Text row that never then gets created, and the API would report it as `builtIn:true` | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row`
`GetAllGraphicsElementsForApi_Should_Not_Mark_A_Wrong_Kind_Row_At_The_Seeded_Path_As_BuiltIn` | | 19 | Sanitize passes values through unchecked | `Non_Finite_And_Absurd_Box_Values_Do_Not_Corrupt_The_Geometry` | | 20 | the BOX itself is no longer clamped (only insetPixels) | `An_Oversized_Border_Is_Clamped_And_Does_Not_Flood_The_Element` | | 21 | duplicate guard removed from EnsureBuiltInElementRow | `Repeated_Seeding_Does_Not_Accumulate_Element_Rows` | @@ -256,9 +261,13 @@ red. | 32 | the lineup create path stops attaching the built-in element | `Should_Attach_The_Built_In_On_Now_Next_Element` | | 33 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | | 34 | `EnsureBuiltInElementRow`'s existence check re-derived as a SQL `AnyAsync(e => e.Path == target)` instead of asking `GetBuiltInElementId` (#568) | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` | -| 35 | `GraphicsElementIdsMustExist` removed from `UpdateChannelHandler.Validate` (#568) — the unknown-id 422 the issue asks for | `Should_Reject_Unknown_GraphicsElementId_With_422_Not_500`
`Should_Reject_When_One_Of_Several_GraphicsElementIds_Is_Unknown` | -| 36 | `GraphicsElementIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw` | -| 37 | `WatermarkIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw` | +| 35 | `GraphicsElementIdsMustExist` removed from `UpdateChannelHandler.Validate` (#568) — the unknown-id 422 the issue asks for, and with it the count cap and the diagnostic cap that validator carries | `Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422`
`Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count`
`Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Unknown_GraphicsElementId_With_422_Not_500`
`Should_Reject_When_One_Of_Several_GraphicsElementIds_Is_Unknown` | +| 36 | `GraphicsElementIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw` | +| 37 | `WatermarkIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds`
`Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw` | | 38 | the `ConsumesGraphicsElementIds` mode gate removed from `UpdateDecoHandler.GraphicsElementIdsMustExist` (#568) — a validator may only reject ids the apply path would consume | `Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It` | | 39 | the `ConsumesWatermarkIds` mode gate removed from `UpdateDecoHandler.WatermarkIdsMustExist` (#568) | `Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It` | -| 40 | `GetAllGraphicsElementsForApiHandler`'s `BuiltIn` reverted from `GraphicsElementDefaults.IsOnNowNext(e.Path)` to `Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName` (#568) — the API-side half of the discriminator, the folder-agnostic shape the issue reports | `GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn` | +| 40 | `GetAllGraphicsElementsForApiHandler`'s `BuiltIn` reverted from `GraphicsElementDefaults.IsOnNowNext(e.Path, e.Kind)` to `Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName` (#568) — the API-side call, the folder-agnostic and kind-agnostic shape the issue reports | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Wrong_Kind_Row_At_The_Seeded_Path_As_BuiltIn`
`GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn` | +| 41 | the raw-count cap removed from `Validators.IdsMustExist` (#568) — one clause, three call sites, so its red set is what shows the cap is shared rather than restated | `Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count`
`Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds`
`Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds` | +| 42 | `Validators.DescribeIds` stops truncating and joins every missing id (#568) | `Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422` | +| 43 | the `DbUpdateException` catch disarmed in `UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace` (#568) — retyped to an exception the save cannot raise, so the FK failure propagates as it did before the fix | `Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 44 | the same catch disarmed in `UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace` (#568) | `Should_Translate_A_Deco_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | From 9fc54fed870d23bee344e2150101d04d94962278 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 21:13:08 +0200 Subject: [PATCH 18/19] fix(568): the raw-count cap is the request's bound, and a recovery path re-asks the whole validator set Two holes the review round found in the previous fix, both of the same shape: a guard that names its own fields instead of deriving them. The deco validators short-circuited the entire Validators.IdsMustExist call when the DecoMode does not consume the ids, which took the 512-item raw-count cap with it -- an arbitrarily large array under Inherit/Disable parsed and materialized with nothing bounding it. Only the EXISTENCE half is the apply path's business, so the mode predicate is now a required argument of the shared validator and gates that half alone; the cap runs under every mode. The channel recovery path rechecked GraphicsElementIdsMustExist alone, so a watermark deleted between validation and SaveChangesAsync still surfaced as the unhandled 500 the fix exists to remove -- WatermarkId, FFmpegProfileId, FallbackFillerId and MirrorSourceChannelId are all written by the same save and lose the same race. Both handlers now re-ask the whole of Validate on DbUpdateException, so a validator added later is covered without editing the recovery path. The API-site outside-folder discriminator test seeded an Image row, so the Kind conjunct rejected it whatever the path comparison did: a composite revert to Path.GetFileName(path) == filename && kind == Text passed every API test. It now carries the seeded Kind, mirroring the seeder-site twin, so only the path half can reject it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- .../Channels/Commands/UpdateChannelHandler.cs | 16 +++- .../Scheduling/Commands/UpdateDecoHandler.cs | 56 ++++++------ .../Validators/IdListValidation.cs | 14 +++ .../UpdateChannelGraphicsElementsTests.cs | 35 ++++++++ .../Graphics/GraphicsElementHandlerTests.cs | 13 ++- .../UpdateDecoGraphicsElementsTests.cs | 90 +++++++++++++++++++ .../Support/ChannelHandlerTestBase.cs | 5 +- 7 files changed, 193 insertions(+), 36 deletions(-) diff --git a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs index ee57f751c..d71ebc11b 100644 --- a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs @@ -89,6 +89,13 @@ public class UpdateChannelHandler( // again on the failure path instead, and return the same 422 the validator would have returned; // a DbUpdateException from any other cause keeps its own exception rather than being reported as // a client error. + // + // What is re-asked is the WHOLE of Validate, not the graphics-element half: every FK on this + // full-replace DTO -- FFmpegProfileId, WatermarkId, FallbackFillerId, MirrorSourceChannelId and + // the graphics element ids -- is written by ApplyUpdateRequest and can lose the same race, and a + // recovery path that names its fields one by one silently omits the next FK the DTO gains. + // Re-running the validator set is what keeps the two paths from drifting: a check added to + // Validate is covered here by construction. private async Task> ApplyUpdateRequestTranslatingLostRace( TvContext dbContext, Channel channel, @@ -104,10 +111,12 @@ public class UpdateChannelHandler( catch (DbUpdateException) { // a fresh context: the failed save left the original one tracking the changes that - // could not be written, so the same query there could be answered from those. + // could not be written, so the same query there could be answered from those. The + // channel entity is still the tracked one from the failed context, which Validate reads + // only in memory (MirrorSourceMustBeValid's own-playout count) and never re-queries. await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); - Validation recheck = - await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken); + Validation recheck = + await Validate(recheckContext, request, channel, cancellationToken); Option maybeError = recheck.Match( Succ: _ => Option.None, @@ -357,6 +366,7 @@ public class UpdateChannelHandler( request, r => r.GraphicsElementIds, "Graphics element", + idsAreConsumed: true, (ids, token) => dbContext.GraphicsElements .Where(e => ids.Contains(e.Id)) .Select(e => e.Id) diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index 51ef9aee2..2207e60b6 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -31,6 +31,10 @@ public class UpdateDecoHandler( // does not close that window either (neither provider locks the rows the validator merely READ), // so ask the existence questions again on the failure path and return the same 422; a // DbUpdateException from any other cause keeps its own exception. + // + // The whole of Validate is re-asked rather than a named pair of fields, for the same reason as + // the channel twin: a recovery path that enumerates its own fields omits the next one the DTO + // gains, while re-running the validator set covers a check added to Validate by construction. private async Task> ApplyUpdateRequestTranslatingLostRace( TvContext dbContext, Deco existing, @@ -46,10 +50,7 @@ public class UpdateDecoHandler( // a fresh context: the failed save left the original one tracking the changes that // could not be written, so the same query there could be answered out of those. await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); - Validation recheck = ( - await WatermarkIdsMustExist(recheckContext, request, cancellationToken), - await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken)) - .Apply((_, _) => Unit.Default); + Validation recheck = await Validate(recheckContext, request, cancellationToken); Option maybeError = recheck.Match( Succ: _ => Option.None, @@ -271,7 +272,10 @@ public class UpdateDecoHandler( // validator can never reject an id the apply path was going to discard (#568). The SPA sends both // id lists regardless of the mode selector, so that shape arrives from the real editor: a draft // holding an element that has since been deleted must still be able to save the deco back to - // Inherit. + // Inherit. The predicate is handed to Validators.IdsMustExist rather than short-circuiting the + // call, because only the EXISTENCE half belongs to the apply path: a discarded list was still + // parsed and materialized out of the request body, so the raw-count cap has to apply under + // every mode. private static bool ConsumesWatermarkIds(UpdateDeco request) => request.WatermarkMode is (DecoMode.Override or DecoMode.Merge); @@ -290,33 +294,31 @@ public class UpdateDecoHandler( TvContext dbContext, UpdateDeco request, CancellationToken cancellationToken) => - !ConsumesWatermarkIds(request) - ? Task.FromResult>(Unit.Default) - : Validators.IdsMustExist( - request, - r => r.WatermarkIds, - "Watermark", - (ids, token) => dbContext.ChannelWatermarks - .Where(w => ids.Contains(w.Id)) - .Select(w => w.Id) - .ToListAsync(token), - cancellationToken); + Validators.IdsMustExist( + request, + r => r.WatermarkIds, + "Watermark", + idsAreConsumed: ConsumesWatermarkIds(request), + (ids, token) => dbContext.ChannelWatermarks + .Where(w => ids.Contains(w.Id)) + .Select(w => w.Id) + .ToListAsync(token), + cancellationToken); private static Task> GraphicsElementIdsMustExist( TvContext dbContext, UpdateDeco request, CancellationToken cancellationToken) => - !ConsumesGraphicsElementIds(request) - ? Task.FromResult>(Unit.Default) - : Validators.IdsMustExist( - request, - r => r.GraphicsElementIds, - "Graphics element", - (ids, token) => dbContext.GraphicsElements - .Where(e => ids.Contains(e.Id)) - .Select(e => e.Id) - .ToListAsync(token), - cancellationToken); + Validators.IdsMustExist( + request, + r => r.GraphicsElementIds, + "Graphics element", + idsAreConsumed: ConsumesGraphicsElementIds(request), + (ids, token) => dbContext.GraphicsElements + .Where(e => ids.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(token), + cancellationToken); private static Task> DecoMustExist( TvContext dbContext, diff --git a/ErsatzTV.Application/Validators/IdListValidation.cs b/ErsatzTV.Application/Validators/IdListValidation.cs index 70226c979..59beeeed1 100644 --- a/ErsatzTV.Application/Validators/IdListValidation.cs +++ b/ErsatzTV.Application/Validators/IdListValidation.cs @@ -23,6 +23,14 @@ public static partial class Validators /// bound the list, resolve which of its ids exist through , /// and reject the rest with a 422 that names the request field it came from. /// + /// + /// Whether the apply path will actually read this list — false where another field of the + /// same request (a deco's DecoMode) makes the reconcile discard it. It gates the + /// EXISTENCE half only, never the count: a validator may not reject an id the apply path + /// was going to throw away, but the raw list was still parsed and materialized out of the + /// request body whatever is done with it afterwards, so the cap is the request's bound and + /// not the apply path's (#568). + /// /// /// The count is taken from the RAW list, before Distinct and before any database /// work: deduplication is not what the request costs. A million-entry list of one repeated @@ -34,6 +42,7 @@ public static partial class Validators T input, Expression>> expression, string noun, + bool idsAreConsumed, Func, CancellationToken, Task>> findExisting, CancellationToken cancellationToken) { @@ -48,6 +57,11 @@ public static partial class Validators "row already attached, so a longer list turns a single request into unbounded work."); } + if (!idsAreConsumed) + { + return Unit.Default; + } + List requested = submitted.Distinct().ToList(); if (requested.Count == 0) { diff --git a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs index 97e7a901c..b832f84d0 100644 --- a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs @@ -31,6 +31,15 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase return (elementA.Id, elementB.Id); } + private async Task SeedWatermark() + { + await using TvContext context = Db.CreateContext(); + var watermark = new ChannelWatermark { Name = "W" }; + context.ChannelWatermarks.Add(watermark); + await context.SaveChangesAsync(); + return watermark.Id; + } + private async Task> SeedGraphicsElements(int count) { await using TvContext context = Db.CreateContext(); @@ -252,6 +261,32 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase error.Value.ShouldContain(elementAId.ToString(CultureInfo.InvariantCulture)); } + // graphicsElementIds is not the only FK this DTO writes, and the recovery path re-asks the whole + // of Validate rather than the graphics-element half precisely so the other FKs are covered: + // WatermarkId is written by the same SaveChangesAsync and loses the same race. Reddens if the + // recheck is narrowed back to GraphicsElementIdsMustExist -- row 45 of the mutation table in + // docs/graphics-elements.md. + [Test] + public async Task Should_Translate_A_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422() + { + ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness(); + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + int watermarkId = await SeedWatermark(); + + interceptor.SqlBeforeFailing = + $"DELETE FROM ChannelWatermark WHERE Id = {watermarkId.ToString(CultureInfo.InvariantCulture)}"; + interceptor.Armed = true; + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", watermarkId: watermarkId), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("Watermark"); + error.Value.ShouldContain(watermarkId.ToString(CultureInfo.InvariantCulture)); + } + // The other half of that catch: a DbUpdateException whose cause is NOT a missing graphics // element is a real fault and must keep its own exception rather than be reported to the client // as a validation error about ids that are all still present. diff --git a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs index 839a2bd77..fa29f7d37 100644 --- a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs @@ -66,13 +66,18 @@ public class GraphicsElementHandlerTests // #568: the discriminator used to be Path.GetFileName(e.Path) == OnNowNextFileName, which is // folder-agnostic -- a user element named exactly "on-now-next.yml" outside the seeded text - // template folder would also report builtIn:true. Reddens if the fix (compare the full seeded - // path) is reverted to a filename-only comparison. + // template folder would also report builtIn:true. The row carries the SAME Kind as the real + // seeded element, deliberately: a wrong-kind row here would be rejected by the Kind conjunct + // whatever the path comparison is, so the composite revert (filename AND Kind == Text) would + // pass. Only the PATH half can reject a Text row in another folder, which is what this pins -- + // it mirrors the seeder-site Ignores_A_Same_Named_Same_Kind_Element_Outside_The_Seeded_Folder. [Test] public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn() { - string userElementPath = System.IO.Path.Combine("/config/graphics-elements/image", GraphicsElementDefaults.OnNowNextFileName); - await SeedElement(1, userElementPath, GraphicsElementKind.Image, string.Empty); + string userElementPath = System.IO.Path.Combine( + "/config/graphics-elements/text/some-subfolder", + GraphicsElementDefaults.OnNowNextFileName); + await SeedElement(1, userElementPath, GraphicsElementKind.Text, string.Empty); var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index f2c8c196e..09cbd0df2 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -97,6 +97,28 @@ public class UpdateDecoGraphicsElementsTests return element.Id; } + private async Task> SeedGraphicsElements(int count) + { + await using TvContext context = _db.CreateContext(); + List elements = Enumerable.Range(0, count) + .Select(i => new GraphicsElement { Path = $"element-{i}.yml" }) + .ToList(); + context.GraphicsElements.AddRange(elements); + await context.SaveChangesAsync(); + return elements.Select(e => e.Id).ToList(); + } + + private async Task> SeedWatermarks(int count) + { + await using TvContext context = _db.CreateContext(); + List watermarks = Enumerable.Range(0, count) + .Select(i => new ChannelWatermark { Name = $"W{i}" }) + .ToList(); + context.ChannelWatermarks.AddRange(watermarks); + await context.SaveChangesAsync(); + return watermarks.Select(w => w.Id).ToList(); + } + private async Task SeedWatermark() { await using TvContext context = _db.CreateContext(); @@ -294,6 +316,46 @@ public class UpdateDecoGraphicsElementsTests error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); } + // The mode gate is handed to Validators.IdsMustExist rather than short-circuiting the call, + // because only the EXISTENCE half is the apply path's business: a list the reconcile discards + // was still parsed and materialized out of the request body. These two pin that the cap holds + // under a mode that consumes nothing -- row 47 of the mutation table in + // docs/graphics-elements.md. Note the ids all EXIST here, so nothing but the cap can reject + // them: a rejection is the cap's, not a smuggled existence check. + [Test] + public async Task Should_Reject_Too_Many_GraphicsElementIds_Even_Under_A_Mode_That_Does_Not_Consume_Them() + { + await SeedDeco(); + List ids = await SeedGraphicsElements(Validators.MaximumIdListCount + 1); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementsMode: DecoMode.Inherit, graphicsElementIds: ids), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + [Test] + public async Task Should_Reject_Too_Many_WatermarkIds_Even_Under_A_Mode_That_Does_Not_Consume_Them() + { + await SeedDeco(); + List ids = await SeedWatermarks(Validators.MaximumIdListCount + 1); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(watermarkMode: DecoMode.Disable, watermarkIds: ids), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[WatermarkIds]"); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + // The deco twin of the channel handler's lost-race translation: an element deleted between // Validate and SaveChangesAsync must come back as the validator's own 422, not the FK // exception. Removing the DbUpdateException catch from @@ -323,4 +385,32 @@ public class UpdateDecoGraphicsElementsTests error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain(elementId.ToString(CultureInfo.InvariantCulture)); } + + // The watermark half of the same recovery. Without it, removing the watermark question from the + // recheck would redden nothing -- and the recheck re-asks the whole of Validate exactly so that + // neither id list is the only one covered. Row 46 of the mutation table. + [Test] + public async Task Should_Translate_A_Deco_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422() + { + var interceptor = new ArmedSaveFailureInterceptor(); + await _db.DisposeAsync(); + _db = await InMemoryTvContext.CreateAsync(interceptor); + + await SeedDeco(); + int watermarkId = await SeedWatermark(); + + interceptor.SqlBeforeFailing = + $"DELETE FROM ChannelWatermark WHERE Id = {watermarkId.ToString(CultureInfo.InvariantCulture)}"; + interceptor.Armed = true; + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(watermarkMode: DecoMode.Override, watermarkIds: [watermarkId]), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[WatermarkIds]"); + error.Value.ShouldContain(watermarkId.ToString(CultureInfo.InvariantCulture)); + } } diff --git a/ErsatzTV.Tests/Support/ChannelHandlerTestBase.cs b/ErsatzTV.Tests/Support/ChannelHandlerTestBase.cs index dc2a9ca1d..bdb6246d5 100644 --- a/ErsatzTV.Tests/Support/ChannelHandlerTestBase.cs +++ b/ErsatzTV.Tests/Support/ChannelHandlerTestBase.cs @@ -128,7 +128,8 @@ public abstract class ChannelHandlerTestBase string logoPath = "", string name = "Test", string group = "ErsatzTV", - List graphicsElementIds = null) => + List graphicsElementIds = null, + int? watermarkId = null) => new( channelId, name, @@ -147,7 +148,7 @@ public abstract class ChannelHandlerTestBase null, null, StreamingMode.TransportStreamHybrid, - null, + watermarkId, null, string.Empty, ChannelSubtitleMode.None, From fcdc381246b81a1b1f30981793d40a95dc46b83b Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 5 Sep 2026 21:34:21 +0200 Subject: [PATCH 19/19] docs(568): the rule the branch establishes gets a record, and every row the fix moved is re-measured api-conventions.md now says which half of an id-list validator a sibling field may gate (existence, never the raw-count cap) and that a lost-race recovery re-asks the whole validator set rather than the fields whoever wrote the catch remembered. Those, with the bound and the field-named 422, are one convention with residuals, so they get a record -- api.top-level-id-list-validation -- and a task-signal row. The record states what #568 does NOT settle: three validators on two DTOs is a per-field constant, not the repo-wide rule #917 owns, and it says to expect #917 to replace the mechanism. graphics-elements.md: rows 35-44 re-measured against the whole ErsatzTV.Tests project on this tree, because the fix moved five of their red sets -- Validate is now also what the recovery path re-runs, so removing a validator from it reddens that handler's race test too. Rows 45-47 are new and measured the same way. The "redden more than one test" figure is recounted from the table (21 -> 24); the cross-fixture set is unchanged at five. The negative discriminator rows now carry a stated seeding rule: vary one half of the identity and hold the other at the seeded value. Varying both leaves the row rejected by the pre-#568 predicate as well, so a composite revert to it would pass every test at that site. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV --- docs/README.md | 1 + docs/api-conventions.md | 32 ++++++++----- docs/decisions/README.md | 1 + .../api/top-level-id-list-validation.md | 45 +++++++++++++++++++ docs/graphics-elements.md | 28 ++++++++---- 5 files changed, 88 insertions(+), 19 deletions(-) create mode 100644 docs/decisions/records/api/top-level-id-list-validation.md diff --git a/docs/README.md b/docs/README.md index ab8ee63f5..e6b15be11 100644 --- a/docs/README.md +++ b/docs/README.md @@ -30,6 +30,7 @@ doc below, or that changes which sections a task signal points to.** | CI / release pipeline work | `docs/ci-cd.md` + `docs/decisions/release-ci-governance.md` | | Proposing a new guard / CI check / regression test convention | `docs/defect-shapes-773.md` §4 (detector menu + the classes where no detector is plausible), then the rules every guard must satisfy: `docs/decisions/records/testing/guard-derives-population-from-source.md`, `…/guard-ships-with-mutation-proof.md` and `…/mutation-claims-are-executed.md` (a `MUTATION` grade carries a DECLARED clause mutation that is re-run every suite — and so does a PROSE claim that some mutation reddens, or does not redden, a named test, wherever it is written: it is a `CLAIMS` entry in the same manifest, bound to its site and verbatim quote, or it is not written) — plus `…/verification-code-needs-its-own-proof.md`, which extends the same obligation BEYOND guards to the harness, wrapper or checker doing the checking, and says where its proof lives when the checker holds no row. Then `…/guard-pins-the-artifact-not-a-shape.md` for the SHAPE of the predicate itself: over an artifact whose GRAMMAR the predicate does not implement, pin the artifact WHOLE by default — matching a shape inside it is the exception and must carry that record's four-part argument; that record's `rule:` is the one place the qualifying grammars are enumerated | | Adding or bounding a consequential numeric config field (an FFmpeg profile tunable, a pipeline knob) | `docs/api-conventions.md` §3d — reject out of range with a 422 naming the bound and its consequence, never accept-then-rewrite; validate against the constants the renderer reads, keep the render-time clamp for pre-existing rows, and let an UNCHANGED legacy value through on update. Then `api.ffmpeg-profile-numeric-bounds` | +| Adding or changing a write path that takes a top-level LIST of FK ids (`graphicsElementIds`, `watermarkIds`) | `docs/api-conventions.md` §3b — bound the RAW list through `Validators.IdsMustExist` before `Distinct`, reject an unknown id with a 422 that names the request field, gate only the EXISTENCE half on whatever makes the apply path discard the list, and translate a lost race by re-running the handler's whole `Validate` on a fresh context. Then `api.top-level-id-list-validation`; the repo-wide rollout is #917 | | Testing a surface gated by config / an env var / a credential | `docs/decisions/records/testing/deny-path-at-production-config-value.md` — cover the setting absent, at its production value, and each opt-out, and assert the DENY branch | | Touching a full-replace write path or a hand-built request object | `docs/decisions/records/testing/full-replace-asserts-field-list.md` — derive the field list from the DTO and assert set equality; reconcile by id where child state exists. In the SPA the same rule is enforced by the type system: `docs/spa-conventions.md` §4b — build the body as `Complete`, annotating BOTH the wrapper parameter and every construction site | | Writing or editing any doc, or answering a review finding in prose | `docs/decisions/records/docs/no-session-narrative.md` — the doc records the END STATE; the path to it goes in the commit message. Apply the who-benefits test, and read the carve-out before you cut (dated measurements, stated snapshot boundaries and tested-and-rejected results stay) | diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 1118036fb..e49d98959 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -307,7 +307,12 @@ handler's validation when a lookup fails, so the controller-side mapping falls o `Validators.IdsMustExist` (#568), which counts the **raw** list before `Distinct` and before any database work — deduplication is not what the request costs to parse and materialize — and caps how many rejected ids the 422 echoes back, so an oversized request does not earn an oversized response. - The rollout to the other FK-id validators on these DTOs is #917. + The count is the REQUEST's bound, not the apply path's: where another field of the same body (a + deco's `DecoMode`) makes the reconcile discard the list, the mode gates the EXISTENCE half only — + a discarded list was still parsed and materialized out of the body. Pass that predicate to + `IdsMustExist` as `idsAreConsumed` rather than short-circuiting the call, which takes the cap with + it. The rollout to the other FK-id validators on these DTOs is #917; the rule and its residuals are + `api.top-level-id-list-validation`. - **Name the request field in the validation message**, `[GraphicsElementIds] …`, using the same `[{GetMemberName(expression)}]` prefix `Validators.NotEmpty`/`NotLongerThan`/`AtLeast` already produce. A full-replace DTO carries several id lists, and `Graphics element(s) do not exist: 42` @@ -316,10 +321,13 @@ handler's validation when a lookup fails, so the controller-side mapping falls o `SaveChangesAsync`** — a background job deleting the principal row (e.g. `RefreshGraphicsElements` removing elements whose template file is gone) restores the very 500 the validator was added to prevent. A transaction does not close that window: neither provider locks the rows the validator - merely READ. Catch `DbUpdateException` around the save, re-ask the existence question on a **fresh** - context (the failed one still tracks the changes it could not write), and return the validator's own - 422 if an id has since gone — re-throwing anything else, so a real fault is not reported as a client - error (#568). + merely READ. Catch `DbUpdateException` around the save, re-ask the question on a **fresh** context + (the failed one still tracks the changes it could not write), and return the validator's own 422 if + an id has since gone — re-throwing anything else, so a real fault is not reported as a client error + (#568). **Re-run the handler's whole `Validate`, never a named subset of its fields**: every FK the + save writes loses the same race, and a recovery path that enumerates the fields it knows about + silently omits the next one the DTO gains. Re-running the validator set covers a check added later + by construction. - **Dereferencing a request `string` (e.g. `request.Name.Length`) is a latent 500** — request DTOs carry no `#nullable` context (§2), so a `string Name` binds `null` from `name: null`/an omitted field and there is no implicit `[Required]`; a raw `.Length`/`.Trim()` throws `NullReferenceException` → an @@ -961,13 +969,17 @@ every other field on this full-replace DTO. The identical shape existed on `PUT (`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the `deep-FK-in-a-nested-list` carve-out of §3b above) and is hardened the same way by `UpdateDecoHandler.Validate` (`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). Each -deco validator is gated on the same `Override`/`Merge` mode predicate that makes the apply path -consume its id list — under `Inherit`/`Disable` the reconcile clears the join and ignores the ids, -so validating them there would 422 a request over ids it was about to discard. The SPA sends both -id lists whatever the mode selector says, so that shape arrives from the real editor. +deco validator passes the same `Override`/`Merge` mode predicate that makes the apply path consume +its id list — under `Inherit`/`Disable` the reconcile clears the join and ignores the ids, so +validating them there would 422 a request over ids it was about to discard. The SPA sends both id +lists whatever the mode selector says, so that shape arrives from the real editor. The predicate is +an argument (`idsAreConsumed`) rather than a short-circuit around the call, because it gates the +existence half alone and the raw-count cap applies under every mode. All three id lists go through one shared validator, `Validators.IdsMustExist`, which is where the count cap (`MaximumIdListCount`), the request field named in the message and the cap on echoed ids -are written once rather than three times. +are written once rather than three times. Both handlers translate a lost race the same way, by +re-running their own `Validate` on a fresh context after a `DbUpdateException` — so every FK on the +DTO is covered, not just the id lists this issue was about. `GraphicsElementResponseModel` exposes a server-derived `builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as `GraphicsElementDefaults.IsOnNowNext(element.Path, element.Kind)` — ordinal equality against the full diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 6c52cd5b2..8d8fab068 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -35,6 +35,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `api.search-field-values-unicode-fold` | The EF-sourced facet fields (`genre`, `show_genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`'s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite's `LOWER()` folds ASCII only (`lower('Édith')` is `'Édith'` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware, so `LOWER('Édith')` really is `'édith'` and the existing predicate reaches the row unaided. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT AS Value FROM WHERE [ AND] etv_upper() LIKE @Pattern ESCAPE '\' ORDER BY LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today's EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record's "Known limitation inherited, not introduced" clause; everything else it settles still holds. | 2026-07-27 | [link](records/api/search-field-values-unicode-fold.md) | | `api.search-paging-cap` | Search stays capped at 100 items per media kind; an overflowing kind's "See all" reuses library-browse paging instead of adding new API surface. | 2026-07-11 | [link](records/api/search-paging-cap.md) | | `api.selection-projection-include-chain` | Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET. | 2026-07-28 | [link](records/api/selection-projection-include-chain.md) | +| `api.top-level-id-list-validation` | A list of foreign-key ids that is a TOP-LEVEL field of a full-replace write DTO (`UpdateChannelRequest.graphicsElementIds`, `ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`) is validated like every other FK field on that DTO, through the one shared primitive `Validators.IdsMustExist`. Four properties, in this order: (1) the RAW list length is capped at `Validators.MaximumIdListCount` before `Distinct` and before any database work, because deduplication is not what the request cost to parse and materialize; (2) an id with no matching row is a 422, never the FK constraint reaching `SaveChangesAsync` as an unhandled 500; (3) the message carries the request field name in brackets, `[GraphicsElementIds] ...`, the prefix `Validators.NotEmpty`/`NotLongerThan`/`AtLeast` already produce, and echoes at most ten missing ids plus a count; (4) where a SIBLING field of the same request makes the apply path discard the list — a deco `DecoMode` outside `Override`/`Merge` — that predicate is passed in as `idsAreConsumed` and gates the EXISTENCE half ONLY. It never gates the cap: a discarded list was parsed and materialized out of the request body all the same. Because validation and the write are two statements, a handler that writes any FK also catches `DbUpdateException` around the save and re-runs ITS OWN `Validate` on a FRESH context, returning the validator's own 422 when a principal row has since gone and re-throwing otherwise. The whole validator set is re-asked, never a named subset of fields. This is NOT the deep-FK carve-out of api-conventions.md 3b, which stays: ids nested inside item-list request bodies are still not existence-checked at that depth. | 2026-09-05 | [link](records/api/top-level-id-list-validation.md) | | `api.versioning-v1` | The entire `/api` surface is versioned to `/api/v1` uniformly (no unversioned corner); legacy unversioned callers are rewritten in-pipeline (not redirected) with Deprecation/Link/Sunset headers, and post-freeze `/api/v1` is additive-only — a breaking change requires `/api/v2`. | 2026-07-13 | [link](records/api/versioning-v1.md) | | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | | `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) | diff --git a/docs/decisions/records/api/top-level-id-list-validation.md b/docs/decisions/records/api/top-level-id-list-validation.md new file mode 100644 index 000000000..ccb66ab21 --- /dev/null +++ b/docs/decisions/records/api/top-level-id-list-validation.md @@ -0,0 +1,45 @@ +--- +key: api.top-level-id-list-validation +title: '2026-09-05 — A top-level list of FK ids on a full-replace write DTO is bounded, existence-checked, named in its own 422, and re-asked after a lost race (#568)' +status: active +since: '2026-09-05' +supersedes: none +superseded-by: none +rule: 'A list of foreign-key ids that is a TOP-LEVEL field of a full-replace write DTO (`UpdateChannelRequest.graphicsElementIds`, `ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`) is validated like every other FK field on that DTO, through the one shared primitive `Validators.IdsMustExist`. Four properties, in this order: (1) the RAW list length is capped at `Validators.MaximumIdListCount` before `Distinct` and before any database work, because deduplication is not what the request cost to parse and materialize; (2) an id with no matching row is a 422, never the FK constraint reaching `SaveChangesAsync` as an unhandled 500; (3) the message carries the request field name in brackets, `[GraphicsElementIds] ...`, the prefix `Validators.NotEmpty`/`NotLongerThan`/`AtLeast` already produce, and echoes at most ten missing ids plus a count; (4) where a SIBLING field of the same request makes the apply path discard the list — a deco `DecoMode` outside `Override`/`Merge` — that predicate is passed in as `idsAreConsumed` and gates the EXISTENCE half ONLY. It never gates the cap: a discarded list was parsed and materialized out of the request body all the same. Because validation and the write are two statements, a handler that writes any FK also catches `DbUpdateException` around the save and re-runs ITS OWN `Validate` on a FRESH context, returning the validator''s own 422 when a principal row has since gone and re-throwing otherwise. The whole validator set is re-asked, never a named subset of fields. This is NOT the deep-FK carve-out of api-conventions.md 3b, which stays: ids nested inside item-list request bodies are still not existence-checked at that depth.' +signals: 'unknown graphicsElementIds returns 500 · unbounded client-supplied id array · which id list does this 422 mean · FK deleted between validation and SaveChangesAsync · deco mode gate skips the count cap · IdsMustExist · MaximumIdListCount · idsAreConsumed · ApplyUpdateRequestTranslatingLostRace · paths: `ErsatzTV.Application/Validators/IdListValidation.cs`, `ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs`, `ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs`, `docs/api-conventions.md` 3b · issues: #568, #917, #74' +mechanics: '`Validators.IdsMustExist(input, expression, noun, idsAreConsumed, findExisting, ct)` in `ErsatzTV.Application/Validators/IdListValidation.cs`; the field name comes from `GetMemberName(expression)`, so it cannot drift from the DTO. Pinned by `UpdateChannelGraphicsElementsTests` and `UpdateDecoGraphicsElementsTests`; the clause mutations and their measured red sets are rows 35-47 of the table in `docs/graphics-elements.md`.' +--- + +- **The cap belongs to the request, the existence check belongs to the apply path.** These are two + different questions and the first attempt at the mode gate conflated them: the deco validators + short-circuited the whole `IdsMustExist` call when `DecoMode` was `Inherit`/`Disable`, which was + right about existence and wrong about the count. A validator may not reject an id the reconcile + was going to throw away — the SPA sends both id lists whatever the mode selector says, so a draft + holding a since-deleted element must still be able to save the deco back to `Inherit` — but the + array was still bound, allocated and materialized before any of that was known. Hence + `idsAreConsumed` as a parameter of the shared primitive rather than an `if` around the call site: + the primitive decides what the flag may switch off, and no call site can widen it. + +- **A recovery path that names its own fields is the defect it is recovering from.** Re-asking only + the graphics-element question after a `DbUpdateException` left `WatermarkId`, `FFmpegProfileId`, + `FallbackFillerId` and `MirrorSourceChannelId` — every one of them written by the same + `SaveChangesAsync`, every one of them a real FK — surfacing the same unhandled 500 the catch exists + to remove. Re-running the handler's own `Validate` is what makes the coverage derive from the + validator set instead of from whoever last edited the catch. A transaction is not an alternative: + neither provider locks rows the validator merely READ, so the concurrent delete commits either way. + The context must be a fresh one — the failed context still tracks the changes it could not write, + and would answer the existence query out of them. + +- **Bounded, but only here — and deliberately.** Three validators on two DTOs carry this cap; the + other FK validators on the same DTOs, and the `O(existing x desired)` reconcile loops the ids then + feed, do not. That is not an oversight and it is not a finished rule: capping three sites and not + the rest is a per-field constant, and #917 owns the repo-wide question (where the bound lives, how + the limit is derived, and bounding the apply paths as well as the validators). Cite this record for + the SHAPE a bounded id list takes; expect #917 to replace the mechanism. + +- **`MaximumIdListCount` is a ceiling on abuse, not a product limit.** 512, against tables an + operator curates by hand where a few dozen rows is a large install. Nothing reachable from the + editor can approach it, which is what lets it be a hard rejection rather than a clamp. + +- **A 422 that echoes every rejected id turns an oversized request into an oversized response.** Ten + ids and a count: enough to fix the payload by hand, bounded by construction. diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index a2ec2303b..ff9c07d72 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -148,6 +148,13 @@ test asserting such a row is ignored therefore cannot appear in row 18's red set cannot fail, and the combination is deliberately not shipped. Row 10 drops the identity call entirely, which is a different mutation from dropping either half. +The same arithmetic constrains how each negative case is SEEDED, at both discriminator sites: a +row varies exactly one half of the identity and holds the other at the seeded value. The +outside-folder rows carry `Kind == Text`, the wrong-kind rows sit at the seeded path. A negative row +that varies both is refused by the surviving half of the *pre-#568* predicate too — filename plus +`Kind == Text` — so a composite revert to it would pass every discriminator test at that site while +restoring the folder-agnostic bug the issue reports (#568). + `IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` pins one only on the listed name/title columns), so a SQL `Path ==` comparison is case-sensitive @@ -179,7 +186,7 @@ Three traps this table is built to avoid: - **Measure against the whole project, never a per-fixture filter.** A filtered run structurally cannot observe a red in another fixture, so it under-reports the failure set while looking precise. - Twenty-one rows below redden more than one test, and five of them (13, 18, 22, 33, 41) span two + Twenty-four rows below redden more than one test, and five of them (13, 18, 22, 33, 41) span two fixture classes — those five are exactly what a per-fixture filter cannot see. Both figures are counted from the table itself, so a row added without recounting them makes this bullet quietly false — recount both whenever a row is added or its red set changes. @@ -261,13 +268,16 @@ red. | 32 | the lineup create path stops attaching the built-in element | `Should_Attach_The_Built_In_On_Now_Next_Element` | | 33 | `IsOnNowNext` loosened from `Ordinal` to `OrdinalIgnoreCase` (#568) — one row for both discriminator sites, since they share the predicate | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn`
`Ignores_A_Case_Variant_Of_The_Seeded_Path` | | 34 | `EnsureBuiltInElementRow`'s existence check re-derived as a SQL `AnyAsync(e => e.Path == target)` instead of asking `GetBuiltInElementId` (#568) | `A_Row_Of_Another_Kind_At_The_Seeded_Path_Does_Not_Suppress_The_Built_In_Row` | -| 35 | `GraphicsElementIdsMustExist` removed from `UpdateChannelHandler.Validate` (#568) — the unknown-id 422 the issue asks for, and with it the count cap and the diagnostic cap that validator carries | `Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422`
`Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count`
`Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Unknown_GraphicsElementId_With_422_Not_500`
`Should_Reject_When_One_Of_Several_GraphicsElementIds_Is_Unknown` | -| 36 | `GraphicsElementIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw` | -| 37 | `WatermarkIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) | `Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds`
`Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw` | -| 38 | the `ConsumesGraphicsElementIds` mode gate removed from `UpdateDecoHandler.GraphicsElementIdsMustExist` (#568) — a validator may only reject ids the apply path would consume | `Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It` | -| 39 | the `ConsumesWatermarkIds` mode gate removed from `UpdateDecoHandler.WatermarkIdsMustExist` (#568) | `Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It` | +| 35 | `GraphicsElementIdsMustExist` removed from `UpdateChannelHandler.Validate` (#568) — the unknown-id 422 the issue asks for, and with it the count cap and the diagnostic cap that validator carries, and the lost-race recheck, which re-runs `Validate` whole | `Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422`
`Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count`
`Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Unknown_GraphicsElementId_With_422_Not_500`
`Should_Reject_When_One_Of_Several_GraphicsElementIds_Is_Unknown`
`Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 36 | `GraphicsElementIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) — and so from the lost-race recheck too, which re-runs `Validate` whole | `Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Too_Many_GraphicsElementIds_Even_Under_A_Mode_That_Does_Not_Consume_Them`
`Should_Reject_Unknown_GraphicsElementId_With_A_Validation_Error_Not_A_Throw`
`Should_Translate_A_Deco_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 37 | `WatermarkIdsMustExist` removed from `UpdateDecoHandler.Validate` (#568) — and so from the lost-race recheck too | `Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds`
`Should_Reject_Too_Many_WatermarkIds_Even_Under_A_Mode_That_Does_Not_Consume_Them`
`Should_Reject_Unknown_WatermarkId_With_A_Validation_Error_Not_A_Throw`
`Should_Translate_A_Deco_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 38 | `idsAreConsumed: ConsumesGraphicsElementIds(request)` replaced by `idsAreConsumed: true` at `UpdateDecoHandler.GraphicsElementIdsMustExist`'s call site (#568) — a validator may only reject ids the apply path would consume | `Should_Ignore_An_Unknown_GraphicsElementId_When_The_Mode_Does_Not_Consume_It` | +| 39 | the same replacement at `UpdateDecoHandler.WatermarkIdsMustExist`'s call site (#568) | `Should_Ignore_An_Unknown_WatermarkId_When_The_Mode_Does_Not_Consume_It` | | 40 | `GetAllGraphicsElementsForApiHandler`'s `BuiltIn` reverted from `GraphicsElementDefaults.IsOnNowNext(e.Path, e.Kind)` to `Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName` (#568) — the API-side call, the folder-agnostic and kind-agnostic shape the issue reports | `GetAllGraphicsElementsForApi_Should_Not_Mark_A_Wrong_Kind_Row_At_The_Seeded_Path_As_BuiltIn`
`GetAllGraphicsElementsForApi_Should_Not_Mark_Same_Filename_Outside_Seeded_Folder_As_BuiltIn` | -| 41 | the raw-count cap removed from `Validators.IdsMustExist` (#568) — one clause, three call sites, so its red set is what shows the cap is shared rather than restated | `Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count`
`Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds`
`Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds` | +| 41 | the raw-count cap removed from `Validators.IdsMustExist` (#568) — one clause, three call sites, so its red set is what shows the cap is shared rather than restated | `Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count`
`Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds`
`Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds`
`Should_Reject_Too_Many_GraphicsElementIds_Even_Under_A_Mode_That_Does_Not_Consume_Them`
`Should_Reject_Too_Many_WatermarkIds_Even_Under_A_Mode_That_Does_Not_Consume_Them` | | 42 | `Validators.DescribeIds` stops truncating and joins every missing id (#568) | `Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422` | -| 43 | the `DbUpdateException` catch disarmed in `UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace` (#568) — retyped to an exception the save cannot raise, so the FK failure propagates as it did before the fix | `Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | -| 44 | the same catch disarmed in `UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace` (#568) | `Should_Translate_A_Deco_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 43 | the `DbUpdateException` catch disarmed in `UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace` (#568) — retyped to an exception the save cannot raise, so the FK failure propagates as it did before the fix | `Should_Translate_A_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422`
`Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 44 | the same catch disarmed in `UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace` (#568) | `Should_Translate_A_Deco_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422`
`Should_Translate_A_Deco_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 45 | the lost-race recheck in `UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace` narrowed from `Validate` back to `GraphicsElementIdsMustExist` alone (#568) — the recovery enumerating its own fields instead of re-asking the validator set, which omits every other FK the same save writes | `Should_Translate_A_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 46 | the same narrowing in `UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace` (#568) | `Should_Translate_A_Deco_Watermark_Deleted_Between_Validation_And_Save_Into_The_Same_422` | +| 47 | the `idsAreConsumed` gate hoisted ABOVE the raw-count cap in `Validators.IdsMustExist` (#568) — the shape the deco mode gate had while it short-circuited the whole call, which took the cap with it | `Should_Reject_Too_Many_GraphicsElementIds_Even_Under_A_Mode_That_Does_Not_Consume_Them`
`Should_Reject_Too_Many_WatermarkIds_Even_Under_A_Mode_That_Does_Not_Consume_Them` |