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/Channels/Commands/UpdateChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs index 294a3736b..d71ebc11b 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,56 @@ 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. + // + // 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, + 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. 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 Validate(recheckContext, request, channel, 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, @@ -229,14 +284,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 +351,28 @@ 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). 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) => + Validators.IdsMustExist( + request, + r => r.GraphicsElementIds, + "Graphics element", + idsAreConsumed: true, + (ids, token) => dbContext.GraphicsElements + .Where(e => ids.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(token), + cancellationToken); + 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..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 = Path.GetFileName(e.Path) == GraphicsElementDefaults.OnNowNextFileName + 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 dd88739a3..2207e60b6 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -19,7 +19,50 @@ 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. + // + // 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, + 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 Validate(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( @@ -31,7 +74,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 +102,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; @@ -218,8 +261,64 @@ 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); + + // 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. 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); + + 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 + // 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. 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) => + 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) => + 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 new file mode 100644 index 000000000..59beeeed1 --- /dev/null +++ b/ErsatzTV.Application/Validators/IdListValidation.cs @@ -0,0 +1,87 @@ +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. + /// + /// + /// 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 + /// 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, + bool idsAreConsumed, + 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."); + } + + if (!idsAreConsumed) + { + return Unit.Default; + } + + 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 6b5c6f778..f946d1cea 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -1,10 +1,52 @@ +using System.IO; +using ErsatzTV.Core.Domain; + 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 (#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 the filename above (#67 / #74). + // Display name only. Never use it for identity -- that is IsOnNowNext below (#67 / #74 / #568). public const string OnNowNextName = "On Now / Next"; + + // The full path the seeder writes the built-in template to (GraphicsElementSeeder.SeedOnNowNext + // builds `target` the same way). A `builtIn` discriminator must match THIS, not + // `Path.GetFileName(...) == OnNowNextFileName` -- a filename-only comparison is folder-agnostic: + // a user element named exactly `on-now-next.yml` in any of the other four template folders + // (image/motion/subtitle/script) would also report `builtIn:true` (#568). `Kind == Text` alone + // does not close this either, since a second text template could share the filename in principle. + public static string OnNowNextSeededPath => + Path.Combine(FileSystemLayout.GraphicsElementsTextTemplatesFolder, OnNowNextFileName); + + /// + /// 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. + /// + /// + /// + /// 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, 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 98067ebb7..4ee808601 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -173,14 +173,25 @@ 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; } - // 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 { @@ -246,29 +257,42 @@ 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 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. /// + /// + /// 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()); - var matches = candidates - .Where(c => System.IO.Path.GetFileName(c.Path) == GraphicsElementDefaults.OnNowNextFileName) - .OrderBy(c => c.Id) + List matches = candidates + .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path, c.Kind)) + .Select(c => c.Id) + .OrderBy(id => 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..b832f84d0 100644 --- a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs @@ -1,6 +1,9 @@ +using System.Globalization; +using ErsatzTV.Application; 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 +18,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(); @@ -25,6 +31,37 @@ 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(); + 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() { @@ -73,4 +110,199 @@ 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. 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() + { + 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("[GraphicsElementIds]"); + 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("[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)); + } + + // 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. + [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 d68fa1ca0..fa29f7d37 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,97 @@ 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. 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/text/some-subfolder", + GraphicsElementDefaults.OnNowNextFileName); + await SeedElement(1, userElementPath, 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(); + } + + [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(); + } + + // #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(); + } + + // #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( + 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/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs new file mode 100644 index 000000000..09cbd0df2 --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -0,0 +1,416 @@ +using System.Globalization; +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, + DecoMode? graphicsElementsMode = null, + DecoMode? watermarkMode = null) => + new( + 1, + 1, + "D", + watermarkMode ?? DecoMode.Inherit, + watermarkIds ?? [], + false, + graphicsElementsMode ?? (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); + + 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> 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(); + 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(); + 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(); + } + + // 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() + { + 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("[GraphicsElementIds]"); + 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(); + } + + // 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() + { + 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("[WatermarkIds]"); + 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. 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() + { + 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; 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() + { + await SeedDeco(); + int watermarkId = await SeedWatermark(); + await AttachWatermark(watermarkId); + + 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 SeedGraphicsElement(); + + 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 }); + } + + // 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 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 + // UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace is row 44 of the mutation table in + // docs/graphics-elements.md. + [Test] + public async Task Should_Translate_A_Deco_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)); + } + + // 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/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index f06c129bf..f4b28096c 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 }; @@ -153,17 +153,36 @@ 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() + public async Task Ignores_A_Non_Built_In_Element_With_A_Different_Filename() + { + await using TvContext context = _db.CreateContext(); + await context.GraphicsElements.AddAsync( + new GraphicsElement { Path = "/templates/text/something-else.yml", Kind = GraphicsElementKind.Text }); + await context.SaveChangesAsync(); + + Channel channel = await SeedChannel(context, "1"); + + await GraphicsElementSeeder.AttachOnNowNextByDefault(context, CancellationToken.None); + + (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 = $"/templates/image/{GraphicsElementDefaults.OnNowNextFileName}", - Kind = GraphicsElementKind.Image + Path = System.IO.Path.Combine( + "/config/graphics-elements/text/some-subfolder", + GraphicsElementDefaults.OnNowNextFileName), + Kind = GraphicsElementKind.Text }); await context.SaveChangesAsync(); @@ -174,12 +193,23 @@ 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_Non_Built_In_Element_With_A_Different_Filename() + public async Task Ignores_A_Case_Variant_Of_The_Seeded_Path() { await using TvContext context = _db.CreateContext(); await context.GraphicsElements.AddAsync( - new GraphicsElement { Path = "/templates/text/something-else.yml", Kind = GraphicsElementKind.Text }); + 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"); @@ -242,6 +272,54 @@ 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. 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); + } + // 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 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/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, 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)) 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 8c2c61100..e49d98959 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -302,6 +302,32 @@ 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 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` + 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 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 @@ -935,11 +961,32 @@ 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 of §3b above) and is hardened the same way by +`UpdateDecoHandler.Validate` (`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). Each +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. 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 +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/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/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index f25a2b216..28dd04d1b 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,22 @@ 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 - `GraphicsElementDefaults.OnNowNextFileName` rather than trusting the element's editable `Name`. + 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, 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 + 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/decisions/records/graphics/on-now-next-on-by-default.md b/docs/decisions/records/graphics/on-now-next-on-by-default.md index f925cd269..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,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 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`). **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/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 2e1bd9d06..ff9c07d72 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -121,8 +121,48 @@ 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 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. + +`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. + +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. + +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 +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`. ## Tests @@ -146,8 +186,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. + 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. - **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, @@ -182,6 +224,14 @@ 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: 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) | |---|---|---| | 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` | @@ -193,7 +243,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 | 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` | @@ -201,11 +251,11 @@ 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` | +| 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` | -| 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` | @@ -216,3 +266,18 @@ 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` | +| 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, 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`
`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_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` |