diff --git a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs index 5d5964514..ee57f751c 100644 --- a/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs +++ b/ErsatzTV.Application/Channels/Commands/UpdateChannelHandler.cs @@ -47,8 +47,13 @@ public class UpdateChannelHandler( { Either resolvedLogo = await ResolveLogoPath(request, cancellationToken); return await resolvedLogo.Match( - Right: async logoPath => Right( - await ApplyUpdateRequest(dbContext, c, request, logoPath, cancellationToken)), + Right: logoPath => + ApplyUpdateRequestTranslatingLostRace( + dbContext, + c, + request, + logoPath, + cancellationToken), Left: e => Task.FromResult(Left(e))); }, Fail: errors => Task.FromResult(Left(errors.Join()))); @@ -76,6 +81,47 @@ public class UpdateChannelHandler( return cached; } + // Validation and the write are two statements, not one atomic step: RefreshGraphicsElements + // deletes elements whose template file is gone, and a delete landing between the two turns the + // join insert back into the FK violation the validator exists to prevent -- the unhandled 500 + // again (#568). A transaction does not close that window either: neither provider locks the rows + // the validator merely READ, so the concurrent delete still commits. Ask the existence question + // again on the failure path instead, and return the same 422 the validator would have returned; + // a DbUpdateException from any other cause keeps its own exception rather than being reported as + // a client error. + private async Task> ApplyUpdateRequestTranslatingLostRace( + TvContext dbContext, + Channel channel, + UpdateChannel request, + string logoPath, + CancellationToken cancellationToken) + { + try + { + return Right( + await ApplyUpdateRequest(dbContext, channel, request, logoPath, cancellationToken)); + } + catch (DbUpdateException) + { + // a fresh context: the failed save left the original one tracking the changes that + // could not be written, so the same query there could be answered from those. + await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + Validation recheck = + await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken); + + Option maybeError = recheck.Match( + Succ: _ => Option.None, + Fail: errors => Some(errors.Join())); + + foreach (BaseError error in maybeError) + { + return Left(error); + } + + throw; + } + } + private async Task ApplyUpdateRequest( TvContext dbContext, Channel c, @@ -300,31 +346,22 @@ public class UpdateChannelHandler( // id; an id with no matching GraphicsElement row would otherwise hit // FK_ChannelGraphicsElement_GraphicsElement_GraphicsElementId at SaveChangesAsync and surface as // an unhandled 500 (there is no global exception filter). Reject it here instead, for parity - // with every other FK field on this full-replace DTO (#568). - private static async Task> GraphicsElementIdsMustExist( + // with every other FK field on this full-replace DTO (#568). The count cap, the request field + // named in the message and the cap on echoed ids all live in Validators.IdsMustExist, shared + // with the two UpdateDecoHandler twins so the three cannot drift apart. + private static Task> GraphicsElementIdsMustExist( TvContext dbContext, UpdateChannel request, - CancellationToken cancellationToken) - { - List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; - if (requested.Count == 0) - { - return Unit.Default; - } - - List existingIds = await dbContext.GraphicsElements - .Where(e => requested.Contains(e.Id)) - .Select(e => e.Id) - .ToListAsync(cancellationToken); - - List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); - if (missingIds.Count == 0) - { - return Unit.Default; - } - - return BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}"); - } + CancellationToken cancellationToken) => + Validators.IdsMustExist( + request, + r => r.GraphicsElementIds, + "Graphics element", + (ids, token) => dbContext.GraphicsElements + .Where(e => ids.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(token), + cancellationToken); private static async Task> MirrorSourceMustBeValid( TvContext dbContext, diff --git a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs index 78e1e710a..1d7a3847f 100644 --- a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs +++ b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs @@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory db .Select(e => new { Vm = ProjectToViewModel(e), - BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path) + BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path, e.Kind) }) .OrderBy(x => x.Vm.Name == x.Vm.FileName) .ThenBy(x => x.Vm.Name) diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs index db8e558b7..51ef9aee2 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDecoHandler.cs @@ -19,7 +19,49 @@ public class UpdateDecoHandler( { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); Validation validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(ps => ApplyUpdateRequest(dbContext, ps, request, cancellationToken)); + return await validation.Match( + Succ: deco => ApplyUpdateRequestTranslatingLostRace(dbContext, deco, request, cancellationToken), + Fail: errors => Task.FromResult(Left(errors.Join()))); + } + + // Mirrors UpdateChannelHandler.ApplyUpdateRequestTranslatingLostRace (#568): validation and the + // write are two statements, so a concurrent delete of a validated watermark or graphics element + // -- RefreshGraphicsElements deletes elements whose template file is gone -- lands the join + // insert on the FK violation the validators exist to prevent, as an unhandled 500. A transaction + // does not close that window either (neither provider locks the rows the validator merely READ), + // so ask the existence questions again on the failure path and return the same 422; a + // DbUpdateException from any other cause keeps its own exception. + private async Task> ApplyUpdateRequestTranslatingLostRace( + TvContext dbContext, + Deco existing, + UpdateDeco request, + CancellationToken cancellationToken) + { + try + { + return await ApplyUpdateRequest(dbContext, existing, request, cancellationToken); + } + catch (DbUpdateException) + { + // a fresh context: the failed save left the original one tracking the changes that + // could not be written, so the same query there could be answered out of those. + await using TvContext recheckContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + Validation recheck = ( + await WatermarkIdsMustExist(recheckContext, request, cancellationToken), + await GraphicsElementIdsMustExist(recheckContext, request, cancellationToken)) + .Apply((_, _) => Unit.Default); + + Option maybeError = recheck.Match( + Succ: _ => Option.None, + Fail: errors => Some(errors.Join())); + + foreach (BaseError error in maybeError) + { + return Left(error); + } + + throw; + } } private async Task ApplyUpdateRequest( @@ -241,60 +283,40 @@ public class UpdateDecoHandler( // an id with no matching row hits the FK constraint at SaveChangesAsync and surfaces as an // unhandled 500 (there is no global exception filter). These are top-level fields on // ReplaceDecoRequest, the same position as graphicsElementIds on UpdateChannelRequest -- not the - // "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md. - private static async Task> WatermarkIdsMustExist( + // "deep FK ids nested inside item-list request bodies" carve-out in api-conventions.md. Both go + // through Validators.IdsMustExist, the one place the count cap, the request field named in the + // message and the cap on echoed ids are written. + private static Task> WatermarkIdsMustExist( TvContext dbContext, UpdateDeco request, - CancellationToken cancellationToken) - { - if (!ConsumesWatermarkIds(request)) - { - return Unit.Default; - } + CancellationToken cancellationToken) => + !ConsumesWatermarkIds(request) + ? Task.FromResult>(Unit.Default) + : Validators.IdsMustExist( + request, + r => r.WatermarkIds, + "Watermark", + (ids, token) => dbContext.ChannelWatermarks + .Where(w => ids.Contains(w.Id)) + .Select(w => w.Id) + .ToListAsync(token), + cancellationToken); - List requested = request.WatermarkIds?.Distinct().ToList() ?? []; - if (requested.Count == 0) - { - return Unit.Default; - } - - List existingIds = await dbContext.ChannelWatermarks - .Where(w => requested.Contains(w.Id)) - .Select(w => w.Id) - .ToListAsync(cancellationToken); - - List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); - return missingIds.Count == 0 - ? Unit.Default - : BaseError.New($"Watermark(s) do not exist: {string.Join(", ", missingIds)}"); - } - - private static async Task> GraphicsElementIdsMustExist( + private static Task> GraphicsElementIdsMustExist( TvContext dbContext, UpdateDeco request, - CancellationToken cancellationToken) - { - if (!ConsumesGraphicsElementIds(request)) - { - return Unit.Default; - } - - List requested = request.GraphicsElementIds?.Distinct().ToList() ?? []; - if (requested.Count == 0) - { - return Unit.Default; - } - - List existingIds = await dbContext.GraphicsElements - .Where(e => requested.Contains(e.Id)) - .Select(e => e.Id) - .ToListAsync(cancellationToken); - - List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); - return missingIds.Count == 0 - ? Unit.Default - : BaseError.New($"Graphics element(s) do not exist: {string.Join(", ", missingIds)}"); - } + CancellationToken cancellationToken) => + !ConsumesGraphicsElementIds(request) + ? Task.FromResult>(Unit.Default) + : Validators.IdsMustExist( + request, + r => r.GraphicsElementIds, + "Graphics element", + (ids, token) => dbContext.GraphicsElements + .Where(e => ids.Contains(e.Id)) + .Select(e => e.Id) + .ToListAsync(token), + cancellationToken); private static Task> DecoMustExist( TvContext dbContext, diff --git a/ErsatzTV.Application/Validators/IdListValidation.cs b/ErsatzTV.Application/Validators/IdListValidation.cs new file mode 100644 index 000000000..70226c979 --- /dev/null +++ b/ErsatzTV.Application/Validators/IdListValidation.cs @@ -0,0 +1,73 @@ +using System.Linq.Expressions; +using ErsatzTV.Core; + +namespace ErsatzTV.Application; + +public static partial class Validators +{ + /// + /// The largest id list a full-replace write path accepts in one of its top-level id fields. + /// Deliberately far above any real payload -- the lists it bounds select from tables an + /// operator curates by hand (graphics elements, watermarks), where a few dozen rows is a + /// large install -- so the bound is a ceiling on abuse, not a product limit anyone can reach + /// by using the editor (#568). + /// + public const int MaximumIdListCount = 512; + + // A 422 that echoes every rejected id turns an oversized request into an oversized response. + // Enough ids to fix the payload by hand, then a count. + private const int MaximumReportedMissingIds = 10; + + /// + /// The shared existence check for a top-level list of FK ids on a full-replace request: + /// bound the list, resolve which of its ids exist through , + /// and reject the rest with a 422 that names the request field it came from. + /// + /// + /// The count is taken from the RAW list, before Distinct and before any database + /// work: deduplication is not what the request costs. A million-entry list of one repeated + /// id parses, allocates and materializes in full whatever the distinct count turns out to + /// be, so a cap applied after Distinct would bound the query and leave the request + /// itself unbounded. + /// + public static async Task> IdsMustExist( + T input, + Expression>> expression, + string noun, + Func, CancellationToken, Task>> findExisting, + CancellationToken cancellationToken) + { + string field = GetMemberName(expression); + List submitted = expression.Compile()(input) ?? []; + + if (submitted.Count > MaximumIdListCount) + { + return BaseError.New( + $"[{field}] contains {submitted.Count} ids; at most {MaximumIdListCount} are accepted. " + + "The whole list is materialized into one existence query and then reconciled against every " + + "row already attached, so a longer list turns a single request into unbounded work."); + } + + List requested = submitted.Distinct().ToList(); + if (requested.Count == 0) + { + return Unit.Default; + } + + List existingIds = await findExisting(requested, cancellationToken); + + List missingIds = requested.Except(existingIds).OrderBy(id => id).ToList(); + if (missingIds.Count == 0) + { + return Unit.Default; + } + + return BaseError.New($"[{field}] {noun}(s) do not exist: {DescribeIds(missingIds)}"); + } + + private static string DescribeIds(IReadOnlyList ids) => + ids.Count <= MaximumReportedMissingIds + ? string.Join(", ", ids) + : $"{string.Join(", ", ids.Take(MaximumReportedMissingIds))} (and " + + $"{ids.Count - MaximumReportedMissingIds} more)"; +} diff --git a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs index 7b32e85c1..f946d1cea 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -1,4 +1,5 @@ using System.IO; +using ErsatzTV.Core.Domain; namespace ErsatzTV.Core.Graphics; @@ -21,20 +22,31 @@ public static class GraphicsElementDefaults Path.Combine(FileSystemLayout.GraphicsElementsTextTemplatesFolder, OnNowNextFileName); /// - /// The one identity test for the built-in On Now / Next element. Ordinal on purpose, and so - /// case-sensitive on purpose: the built-in element is the exact file the seeder wrote, at - /// the exact path it wrote it to. + /// The one identity test for the built-in On Now / Next element: the exact file the seeder + /// wrote, at the exact path it wrote it to, of the kind it wrote it as. Ordinal on purpose, + /// and so case-sensitive on purpose. /// /// - /// Callers compare in memory rather than in a Where clause, because in SQL the answer - /// would be the PROVIDER's to give: GraphicsElement.Path takes no explicit collation - /// (TvContext.OnModelCreating pins one only on the listed name/title columns), so - /// SQLite compares it case-sensitively while MySQL uses the server default, which is - /// normally case-INsensitive. Evaluating one discriminator site in SQL and the other in - /// memory would let the two disagree on MySQL alone. The SQLite test suite cannot tell the - /// two apart -- BINARY collation and an ordinal comparison agree on every input -- so this - /// is held by keeping the comparison out of SQL, not by a test. + /// + /// Kind is part of the identity rather than a second test any caller may add or + /// skip: the seeder's own "does the built-in row exist yet?" check resolves through this + /// predicate, so a row of another kind at the seeded path answering yes would suppress + /// the Text row every consumer resolves. A caller applying only the path half would + /// report that wrong-kind row as the built-in element while the seeder refused to treat + /// it as one -- the two sites disagreeing about the same row, which is the defect this + /// predicate exists to make impossible (#568). + /// + /// + /// Callers compare in memory rather than in a Where clause, because in SQL the + /// answer would be the PROVIDER's to give: GraphicsElement.Path takes no explicit + /// collation (TvContext.OnModelCreating pins one only on the listed name/title + /// columns), so SQLite compares it case-sensitively while MySQL uses the server default, + /// which is normally case-INsensitive. Evaluating one discriminator site in SQL and the + /// other in memory would let the two disagree on MySQL alone. The SQLite test suite + /// cannot tell the two apart -- BINARY collation and an ordinal comparison agree on + /// every input -- so this is held by keeping the comparison out of SQL, not by a test. + /// /// - public static bool IsOnNowNext(string path) => - string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal); + public static bool IsOnNowNext(string path, GraphicsElementKind kind) => + kind == GraphicsElementKind.Text && string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal); } diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index f3ac98c74..4ee808601 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -260,29 +260,32 @@ public static class GraphicsElementSeeder /// Identity is the full seeded path, never the user-editable Name (the #67 lesson carried into /// #74) and never the bare filename (#568: filename-only matching is folder-agnostic, so a user /// element named exactly `on-now-next.yml` in a different template folder would also match). - /// The Kind filter is load-bearing rather than decorative: EnsureBuiltInElementRow - /// asks this method whether the row it is about to create already exists, so a row of another - /// kind at the seeded path must NOT answer yes -- it would suppress the Text row every consumer - /// resolves. + /// The Kind half of that identity is load-bearing rather than decorative: + /// EnsureBuiltInElementRow asks this method whether the row it is about to create already + /// exists, so a row of another kind at the seeded path must NOT answer yes -- it would suppress + /// the Text row every consumer resolves. /// /// - /// The path comparison is in memory rather - /// than a Where clause: in SQL the match would be the provider's collation to decide, and - /// this site would then disagree with the API's `builtIn` (which compares in memory) on MySQL. - /// The Kind filter stays in SQL because it is an enum, not a string. + /// Both halves are + /// in memory rather than a Where clause. The path half must be, or the match would be the + /// provider's collation to decide and this site would disagree with the API's `builtIn` (which + /// compares in memory) on MySQL. The Kind half could be a SQL filter -- it is an enum, + /// not a string -- but then this site would hold half the identity and the predicate the other + /// half, and the API site could apply the predicate alone and quietly answer for rows this one + /// rejects. That is exactly the disagreement #568 found, so identity is one predicate applied + /// whole, at every site. /// public static async Task> GetBuiltInElementId( TvContext context, CancellationToken cancellationToken) { - List<(int Id, string Path)> candidates = await context.GraphicsElements - .Where(e => e.Kind == GraphicsElementKind.Text) - .Select(e => new { e.Id, e.Path }) + List<(int Id, string Path, GraphicsElementKind Kind)> candidates = await context.GraphicsElements + .Select(e => new { e.Id, e.Path, e.Kind }) .ToListAsync(cancellationToken) - .Map(rows => rows.Select(r => (r.Id, r.Path)).ToList()); + .Map(rows => rows.Select(r => (r.Id, r.Path, r.Kind)).ToList()); List matches = candidates - .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path)) + .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path, c.Kind)) .Select(c => c.Id) .OrderBy(id => id) .ToList(); diff --git a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs index d10ffbcfe..97e7a901c 100644 --- a/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Channels/UpdateChannelGraphicsElementsTests.cs @@ -1,3 +1,5 @@ +using System.Globalization; +using ErsatzTV.Application; using ErsatzTV.Application.Channels; using ErsatzTV.Core; using ErsatzTV.Core.Domain; @@ -29,6 +31,28 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase return (elementA.Id, elementB.Id); } + private async Task> SeedGraphicsElements(int count) + { + await using TvContext context = Db.CreateContext(); + List elements = Enumerable.Range(0, count) + .Select(i => new GraphicsElement { Path = $"element-{i}.yml" }) + .ToList(); + context.GraphicsElements.AddRange(elements); + await context.SaveChangesAsync(); + return elements.Select(e => e.Id).ToList(); + } + + // Replaces the harness the base class built with one whose SaveChangesAsync can be made to fail + // on demand. Foreign keys are off in InMemoryTvContext, so the FK violation a concurrent delete + // really produces cannot be raised by seeding alone. + private async Task UseFailingSaveHarness() + { + await Db.DisposeAsync(); + var interceptor = new ArmedSaveFailureInterceptor(); + Db = await InMemoryTvContext.CreateAsync(interceptor); + return interceptor; + } + [Test] public async Task Should_Reconcile_GraphicsElement_Join_Add_Then_Remove() { @@ -95,6 +119,7 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase BaseError error = LeftOf(result); error.ShouldNotBeOfType(); + error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain("999"); // no partial write: the channel keeps no graphics element association @@ -117,6 +142,132 @@ public class UpdateChannelGraphicsElementsTests : ChannelHandlerTestBase BaseError error = LeftOf(result); error.ShouldNotBeOfType(); + error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain("12345"); } + + // #568: the id list is client-supplied and was bounded only by the Kestrel body cap, which is a + // transport limit and not a collection limit. The cap is Validators.MaximumIdListCount, shared + // by all three id-list validators; these three tests pin its two edges and the shape that makes + // its placement matter. Removing the cap from Validators.IdsMustExist is row 41 of the mutation + // table in docs/graphics-elements.md. + [Test] + public async Task Should_Accept_Exactly_The_Maximum_Number_Of_GraphicsElementIds() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + List ids = await SeedGraphicsElements(Validators.MaximumIdListCount); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + + await using TvContext context = Db.CreateContext(); + Channel reloaded = await context.Channels.Include(c => c.ChannelGraphicsElements) + .SingleAsync(c => c.Id == channel.Id); + reloaded.ChannelGraphicsElements.Count.ShouldBe(Validators.MaximumIdListCount); + } + + [Test] + public async Task Should_Reject_One_More_Than_The_Maximum_Number_Of_GraphicsElementIds() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + List ids = Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain((Validators.MaximumIdListCount + 1).ToString(CultureInfo.InvariantCulture)); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + // The cap counts the RAW list, before Distinct: every one of these ids exists and they collapse + // to a single distinct id, so a cap applied after deduplication would accept this request and + // leave the parse and materialization it costs unbounded. + [Test] + public async Task Should_Reject_A_Duplicate_Heavy_List_On_Its_Raw_Count() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + List ids = Enumerable.Repeat(elementAId, Validators.MaximumIdListCount + 1).ToList(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain((Validators.MaximumIdListCount + 1).ToString(CultureInfo.InvariantCulture)); + error.Value.ShouldNotContain("do not exist"); + } + + // A 422 that echoes every rejected id turns an oversized request into an oversized response. + // Removing the truncation from Validators.DescribeIds is row 42 of the mutation table. + [Test] + public async Task Should_Cap_The_Ids_Echoed_Back_In_The_Unknown_Id_422() + { + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + List ids = Enumerable.Range(1001, 30).ToList(); + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: ids), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("1001"); + error.Value.ShouldContain("(and 20 more)"); + error.Value.ShouldNotContain("1030"); + } + + // #568: validation and the write are two statements, so RefreshGraphicsElements can delete a + // validated element in between and hand the join insert the FK violation the validator exists to + // prevent -- the unhandled 500 again. Removing the DbUpdateException catch from + // ApplyUpdateRequestTranslatingLostRace is row 43 of the mutation table. + [Test] + public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422() + { + ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness(); + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + + interceptor.SqlBeforeFailing = + $"DELETE FROM GraphicsElement WHERE Id = {elementAId.ToString(CultureInfo.InvariantCulture)}"; + interceptor.Armed = true; + + Either result = await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId]), + CancellationToken.None); + + BaseError error = LeftOf(result); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(elementAId.ToString(CultureInfo.InvariantCulture)); + } + + // The other half of that catch: a DbUpdateException whose cause is NOT a missing graphics + // element is a real fault and must keep its own exception rather than be reported to the client + // as a validation error about ids that are all still present. + [Test] + public async Task Should_Not_Report_An_Unrelated_DbUpdateException_As_A_Graphics_Element_422() + { + ArmedSaveFailureInterceptor interceptor = await UseFailingSaveHarness(); + await SeedFFmpegProfile(); + Channel channel = await SeedChannel(1, "5"); + (int elementAId, _) = await SeedGraphicsElements(); + + interceptor.Armed = true; + + await Should.ThrowAsync( + async () => await MakeHandler().Handle( + MakeUpdate(channel.Id, number: "5", graphicsElementIds: [elementAId]), + CancellationToken.None)); + } } diff --git a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs index ff0a8c540..839a2bd77 100644 --- a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs @@ -120,6 +120,30 @@ public class GraphicsElementHandlerTests result[0].BuiltIn.ShouldBeFalse(); } + // #568: Kind is part of the built-in element's identity, not a second test the seeder applies + // and the API skips. GetBuiltInElementId refuses a wrong-kind row at the seeded path -- it has + // to, since EnsureBuiltInElementRow asks it whether the Text row it is about to create already + // exists -- so an API that reported the same row as builtIn:true would have the two sites + // disagreeing about one row. Reddens if the Kind conjunct is dropped from + // GraphicsElementDefaults.IsOnNowNext (row 18 of the mutation table in docs/graphics-elements.md). + [Test] + public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_A_Wrong_Kind_Row_At_The_Seeded_Path_As_BuiltIn() + { + await SeedElement( + 1, + GraphicsElementDefaults.OnNowNextSeededPath, + GraphicsElementKind.Image, + string.Empty); + + var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); + + List result = + await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None); + + result.Count.ShouldBe(1); + result[0].BuiltIn.ShouldBeFalse(); + } + // The seeded path with only the FILENAME's case changed -- same folder, same spelling. private static string CaseVariantOfSeededPath() => System.IO.Path.Combine( diff --git a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs index 360ef2055..136c77b5f 100644 --- a/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs +++ b/ErsatzTV.Tests/Application/Scheduling/UpdateDecoGraphicsElementsTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using System.Threading.Channels; using ErsatzTV.Application; using ErsatzTV.Application.Scheduling; @@ -137,6 +138,7 @@ public class UpdateDecoGraphicsElementsTests IsLeft(result).ShouldBeTrue(); BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); error.Value.ShouldContain("999"); // no partial write: the deco keeps no graphics element association @@ -183,6 +185,7 @@ public class UpdateDecoGraphicsElementsTests IsLeft(result).ShouldBeTrue(); BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[WatermarkIds]"); error.Value.ShouldContain("999"); } @@ -253,4 +256,71 @@ public class UpdateDecoGraphicsElementsTests Deco reloaded = await reload.Decos.Include(d => d.DecoGraphicsElements).SingleAsync(d => d.Id == 1); reloaded.DecoGraphicsElements.Select(x => x.GraphicsElementId).ShouldBe(new[] { elementId }); } + + // Both deco id lists go through the same Validators.IdsMustExist as the channel's, so both + // inherit the same raw-count cap; the channel fixture pins its edges, these two pin that each + // deco field is actually behind it and names itself when it rejects. + [Test] + public async Task Should_Reject_More_Than_The_Maximum_Number_Of_GraphicsElementIds() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementIds: Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList()), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + [Test] + public async Task Should_Reject_More_Than_The_Maximum_Number_Of_WatermarkIds() + { + await SeedDeco(); + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate( + watermarkMode: DecoMode.Override, + watermarkIds: Enumerable.Range(1, Validators.MaximumIdListCount + 1).ToList()), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[WatermarkIds]"); + error.Value.ShouldContain(Validators.MaximumIdListCount.ToString(CultureInfo.InvariantCulture)); + } + + // The deco twin of the channel handler's lost-race translation: an element deleted between + // Validate and SaveChangesAsync must come back as the validator's own 422, not the FK + // exception. Removing the DbUpdateException catch from + // UpdateDecoHandler.ApplyUpdateRequestTranslatingLostRace is row 44 of the mutation table in + // docs/graphics-elements.md. + [Test] + public async Task Should_Translate_An_Element_Deleted_Between_Validation_And_Save_Into_The_Same_422() + { + var interceptor = new ArmedSaveFailureInterceptor(); + await _db.DisposeAsync(); + _db = await InMemoryTvContext.CreateAsync(interceptor); + + await SeedDeco(); + int elementId = await SeedGraphicsElement(); + + interceptor.SqlBeforeFailing = + $"DELETE FROM GraphicsElement WHERE Id = {elementId.ToString(CultureInfo.InvariantCulture)}"; + interceptor.Armed = true; + + var handler = new UpdateDecoHandler(_db.Factory, _channel); + Either result = await handler.Handle( + MakeUpdate(graphicsElementIds: [elementId]), + CancellationToken.None); + + IsLeft(result).ShouldBeTrue(); + BaseError error = result.Match(Left: e => e, Right: _ => throw new AssertionException("expected Left")); + error.Value.ShouldContain("[GraphicsElementIds]"); + error.Value.ShouldContain(elementId.ToString(CultureInfo.InvariantCulture)); + } } diff --git a/ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs b/ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs new file mode 100644 index 000000000..f1d9e532e --- /dev/null +++ b/ErsatzTV.Tests/Support/ArmedSaveFailureInterceptor.cs @@ -0,0 +1,43 @@ +using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; + +namespace ErsatzTV.Tests.Support; + +/// +/// Makes the next SaveChangesAsync fail the way a lost race against a concurrent delete +/// does: run first (the delete), then throw the +/// the foreign-key violation would have produced. +/// +/// +/// What is simulated is the TRIGGER, not the decision: runs +/// with foreign keys OFF, so a real constraint cannot fire here. The handler's recovery — re-ask +/// the existence question against the post-delete state, translate a now-missing id into the +/// validator's own 422, re-throw anything else — is real code running against real data. +/// Disarms itself on the first fire so the recovery path gets a working context. +/// +public sealed class ArmedSaveFailureInterceptor : SaveChangesInterceptor +{ + public bool Armed { get; set; } + + public string SqlBeforeFailing { get; set; } = string.Empty; + + public override async ValueTask> SavingChangesAsync( + DbContextEventData eventData, + InterceptionResult result, + CancellationToken cancellationToken = default) + { + if (!Armed) + { + return await base.SavingChangesAsync(eventData, result, cancellationToken); + } + + Armed = false; + + if (!string.IsNullOrEmpty(SqlBeforeFailing)) + { + await eventData.Context!.Database.ExecuteSqlRawAsync(SqlBeforeFailing, cancellationToken); + } + + throw new DbUpdateException("simulated: a validated principal row was deleted concurrently"); + } +} diff --git a/ErsatzTV.Tests/Support/InMemoryTvContext.cs b/ErsatzTV.Tests/Support/InMemoryTvContext.cs index b8911980c..e74bd5761 100644 --- a/ErsatzTV.Tests/Support/InMemoryTvContext.cs +++ b/ErsatzTV.Tests/Support/InMemoryTvContext.cs @@ -3,6 +3,7 @@ using ErsatzTV.Infrastructure.Data; using ErsatzTV.Infrastructure.Sqlite.Data; using Microsoft.Data.Sqlite; using Microsoft.EntityFrameworkCore; +using Microsoft.EntityFrameworkCore.Diagnostics; using Microsoft.Extensions.Logging.Abstractions; namespace ErsatzTV.Tests.Support; @@ -27,7 +28,13 @@ public sealed class InMemoryTvContext : IAsyncDisposable public IDbContextFactory Factory => new TestDbContextFactory(_options); - public static async Task CreateAsync() + /// + /// Extra EF interceptors registered on every context this harness hands out. The only + /// current use is making SaveChangesAsync fail on demand: foreign keys are off here + /// (see the type summary), so a write-path failure a real FK constraint would raise cannot + /// be produced by seeding alone. + /// + public static async Task CreateAsync(params IInterceptor[] interceptors) { TvContext.IsSqlite = true; TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation; @@ -38,6 +45,7 @@ public sealed class InMemoryTvContext : IAsyncDisposable DbContextOptions options = new DbContextOptionsBuilder() .UseSqlite(connection) + .AddInterceptors(interceptors) .Options; await using (TvContext context = Create(options))