diff --git a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs index 078e77c0d..5b9b73925 100644 --- a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs @@ -1,4 +1,5 @@ using System.Threading.Channels; +using ErsatzTV.Application.Scheduling; using ErsatzTV.Core; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Scheduling; @@ -86,6 +87,29 @@ public class ReplacePlayoutAlternateScheduleItemsHandler( var incoming = request.Items.Except([highest]).ToList(); + // Reject an EXPLICITLY empty recurrence set before any mutation (#880). The checked set is + // `incoming` -- the exact list whose DaysOfWeek/DaysOfMonth/MonthsOfYear the loops below + // write -- so the check and its subject cannot drift apart. That EXCLUDES the highest-Index + // catch-all by construction: its recurrence is discarded along with its date range (only its + // ProgramScheduleId is read, further down), so an empty set there cannot make anything "never + // apply" and rejecting it would state a reason that is false for that item. + foreach (ReplacePlayoutAlternateSchedule item in incoming) + { + ProgramScheduleAlternate stored = existing.FirstOrDefault(e => e.Id == item.Id); + Option recurrenceError = RecurrenceSetBounds.Validate( + item.DaysOfWeek, + item.DaysOfMonth, + item.MonthsOfYear, + stored?.DaysOfWeek, + stored?.DaysOfMonth, + stored?.MonthsOfYear); + + foreach (BaseError error in recurrenceError) + { + return error; + } + } + var toAdd = incoming.Filter(x => existing.All(e => e.Id != x.Id)).ToList(); var toRemove = existing.Filter(e => incoming.All(m => m.Id != e.Id)).ToList(); var toUpdate = incoming.Except(toAdd).ToList(); diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs index 998bd04c7..c8ee54ffb 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs @@ -44,6 +44,25 @@ public class ReplacePlayoutTemplateItemsHandler( List incoming = request.Items; + // Same rule as the alternate-schedule path (#880), over ALL items: unlike that one, every + // template item's recurrence IS stored, so there is no catch-all to exclude here. + foreach (ReplacePlayoutTemplate item in incoming) + { + PlayoutTemplate stored = existing.FirstOrDefault(e => e.Id == item.Id); + Option recurrenceError = RecurrenceSetBounds.Validate( + item.DaysOfWeek, + item.DaysOfMonth, + item.MonthsOfYear, + stored?.DaysOfWeek, + stored?.DaysOfMonth, + stored?.MonthsOfYear); + + if (recurrenceError.IsSome) + { + return recurrenceError; + } + } + var toAdd = incoming.Filter(x => existing.All(e => e.Id != x.Id)).ToList(); var toRemove = existing.Filter(e => incoming.All(m => m.Id != e.Id)).ToList(); var toUpdate = incoming.Except(toAdd).ToList(); diff --git a/ErsatzTV.Application/Scheduling/RecurrenceSetBounds.cs b/ErsatzTV.Application/Scheduling/RecurrenceSetBounds.cs new file mode 100644 index 000000000..49e5cc2d2 --- /dev/null +++ b/ErsatzTV.Application/Scheduling/RecurrenceSetBounds.cs @@ -0,0 +1,74 @@ +using ErsatzTV.Core; + +namespace ErsatzTV.Application.Scheduling; + +/// +/// Validates the three recurrence sets shared by ProgramScheduleAlternate and +/// PlayoutTemplate (ersatztv#880). One validator called from BOTH replace handlers, mirroring +/// FFmpegProfileBounds — the exemplar for `api.ffmpeg-profile-numeric-bounds`, whose shape this +/// follows deliberately. +/// +/// +/// +/// An EMPTY set is rejected because the three are read CONJUNCTIVELY by +/// AlternateScheduleSelector.GetScheduleForDate — a miss on any one continues to the next +/// item — so an empty one matches NO date and stores an item that can never apply. Rejecting +/// rather than substituting is the point: accept-then-rewrite would make an explicit `[]` +/// indistinguishable from an omitted field, which is the very collapse this issue removed. +/// +/// +/// An UNCHANGED empty set that the row ALREADY holds is let through. Both PUT paths are +/// whole-list replaces, so a hard rejection would make every OTHER item in the playout +/// uneditable over a row the operator never touched — the same reason +/// `api.ffmpeg-profile-numeric-bounds` rejects only a NEWLY submitted out-of-range value. A row +/// whose stored set is NULL is NOT exempt: null means unrestricted, so submitting `[]` for it is +/// a new emptying, not an unchanged legacy value. +/// +/// +/// This runs on the COMMAND, after the request records have normalized an ABSENT array to the +/// All*() sets, so an empty set reaching here is one a caller sent EXPLICITLY. That also means a +/// direct (non-HTTP) caller is held to the same rule rather than being able to write a dead row. +/// +/// +public static class RecurrenceSetBounds +{ + public static Option Validate( + ICollection daysOfWeek, + ICollection daysOfMonth, + ICollection monthsOfYear, + ICollection storedDaysOfWeek, + ICollection storedDaysOfMonth, + ICollection storedMonthsOfYear) + { + if (IsNewlyEmpty(daysOfWeek, storedDaysOfWeek)) + { + return Some(BaseError.New(Message("DaysOfWeek", "no day of the week"))); + } + + if (IsNewlyEmpty(daysOfMonth, storedDaysOfMonth)) + { + return Some(BaseError.New(Message("DaysOfMonth", "no day of the month"))); + } + + if (IsNewlyEmpty(monthsOfYear, storedMonthsOfYear)) + { + return Some(BaseError.New(Message("MonthsOfYear", "no month"))); + } + + return Option.None; + } + + // "send null" rather than "omit the property": all three are listed in the schema's `required` array + // in v1.json (they are nullable, not optional), so a client generated from the published contract + // cannot omit them. Omitting also works at runtime -- Newtonsoft maps a missing property and an + // explicit null to the same thing -- but naming only that would tell a conforming client to send + // something its own schema forbids. + private static string Message(string field, string consequence) => + $"[{field}] must not be empty; an empty set matches {consequence}, so the item would never apply. " + + "Send null to leave it unrestricted"; + + // A new item (no stored row) has `stored` null, so an empty set is newly empty and is rejected. + // Only a stored set that is ITSELF already empty exempts an empty submission. + private static bool IsNewlyEmpty(ICollection submitted, ICollection stored) => + submitted is { Count: 0 } && stored is not { Count: 0 }; +} diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs index a3efd7175..0c96a9656 100644 --- a/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs @@ -4,6 +4,7 @@ using ErsatzTV.Application.Playouts; using ErsatzTV.Application.Scheduling; using ErsatzTV.Core; using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; using ErsatzTV.Core.Errors; using ErsatzTV.Core.Scheduling; using ErsatzTV.Infrastructure.Data; @@ -121,13 +122,200 @@ public class PlayoutHandlerTests LeftOf(result).Value.ShouldContain("must not be empty"); } + // ---- #880 empty recurrence sets ---- + + [Test] + public async Task ReplaceAlternateSchedules_Should_Reject_A_Newly_Empty_Recurrence_On_A_Stored_Item() + { + await SeedPlayout(1, version: 1); + var handler = new ReplacePlayoutAlternateScheduleItemsHandler( + _db.Factory, + _worker, + NullLogger.Instance); + + // TWO items: index 1 is the catch-all (highest index), so index 0 is a real alternate whose + // recurrence IS stored. The empty set goes on THAT one. + ReplacePlayoutAlternateSchedule empty = AltItem(index: 0) with { DaysOfWeek = [] }; + + Either result = await handler.Handle( + new ReplacePlayoutAlternateScheduleItems(1, [empty, AltItem(index: 1)]), + CancellationToken.None); + + LeftOf(result).Value.ShouldContain("[DaysOfWeek]"); + LeftOf(result).Value.ShouldContain("no day of the week"); + + // rejected BEFORE any mutation -- the version bump is the observable proof nothing was written + (await ReadPlayoutVersion(1)).ShouldBe(1); + } + + // The catch-all's recurrence is discarded by the handler (only its ProgramScheduleId is used), so an + // empty set there cannot make anything "never apply". Rejecting it would state a reason that is FALSE + // for that item, which is why the check walks `incoming` rather than every submitted item. + [Test] + public async Task ReplaceAlternateSchedules_Should_Allow_An_Empty_Recurrence_On_The_CatchAll_Item() + { + await SeedPlayout(1, version: 1); + var handler = new ReplacePlayoutAlternateScheduleItemsHandler( + _db.Factory, + _worker, + NullLogger.Instance); + + // a single item IS the catch-all + ReplacePlayoutAlternateSchedule catchAll = AltItem(index: 0) with { DaysOfWeek = [], MonthsOfYear = [] }; + + Either result = await handler.Handle( + new ReplacePlayoutAlternateScheduleItems(1, [catchAll]), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadPlayoutVersion(1)).ShouldBe(2); + } + + [Test] + public async Task ReplaceTemplates_Should_Reject_A_Newly_Empty_Recurrence() + { + await SeedPlayout(1, version: 1); + var handler = new ReplacePlayoutTemplateItemsHandler( + _db.Factory, + NullLogger.Instance); + + ReplacePlayoutTemplate empty = TemplateItem() with { DaysOfMonth = [] }; + + Option result = await handler.Handle( + new ReplacePlayoutTemplateItems(1, [empty]), + CancellationToken.None); + + result.IfNone(() => throw new AssertionException("Expected a Some(error)")) + .Value.ShouldContain("[DaysOfMonth]"); + (await ReadPlayoutVersion(1)).ShouldBe(1); + } + + // `api.ffmpeg-profile-numeric-bounds`: reject a NEWLY submitted bad value, not an UNCHANGED one the row + // already holds. Both PUT paths are whole-list replaces, so without this a single pre-existing empty row + // would make every OTHER item in the playout uneditable. + [Test] + public async Task ReplaceTemplates_Should_Allow_An_UNCHANGED_Empty_Recurrence_That_Is_Already_Stored() + { + int templateItemId = await SeedPlayoutWithEmptyTemplateRecurrence(); + var handler = new ReplacePlayoutTemplateItemsHandler( + _db.Factory, + NullLogger.Instance); + + // same row, same empty DaysOfWeek -- an edit to some OTHER field on the same list + ReplacePlayoutTemplate unchanged = TemplateItem() with { Id = templateItemId, DaysOfWeek = [] }; + + Option result = await handler.Handle( + new ReplacePlayoutTemplateItems(1, [unchanged]), + CancellationToken.None); + + result.IsNone.ShouldBeTrue(); + (await ReadPlayoutVersion(1)).ShouldBe(2); + } + + // ... and the complement: the SAME stored row rejects a DIFFERENT field being newly emptied, so the + // exemption is per-field rather than "this row is grandfathered". + [Test] + public async Task ReplaceTemplates_Should_Still_Reject_A_Different_Field_Newly_Emptied_On_A_Stored_Row() + { + int templateItemId = await SeedPlayoutWithEmptyTemplateRecurrence(); + var handler = new ReplacePlayoutTemplateItemsHandler( + _db.Factory, + NullLogger.Instance); + + // DaysOfWeek is the stored-empty one; MonthsOfYear is stored FULL, so emptying it is new + ReplacePlayoutTemplate item = TemplateItem() with + { + Id = templateItemId, + DaysOfWeek = [], + MonthsOfYear = [] + }; + + Option result = await handler.Handle( + new ReplacePlayoutTemplateItems(1, [item]), + CancellationToken.None); + + result.IfNone(() => throw new AssertionException("Expected a Some(error)")) + .Value.ShouldContain("[MonthsOfYear]"); + (await ReadPlayoutVersion(1)).ShouldBe(1); + } + + private async Task SeedPlayoutWithEmptyTemplateRecurrence() + { + await using TvContext context = _db.CreateContext(); + + // The handler loads templates with `.Include(p => p.Templates).ThenInclude(t => t.Template)`, and + // that navigation is required -- so a PlayoutTemplate whose Template row does not exist is joined + // OUT and never reaches `existing`. Without seeding this, the stored row is invisible, the + // exemption cannot match, and the test fails for a reason that has nothing to do with the rule. + context.TemplateGroups.Add(new TemplateGroup { Id = 5, Name = "Group", Templates = [] }); + context.Templates.Add(new Template { Id = 20, TemplateGroupId = 5, Name = "Template 20", Items = [] }); + await context.SaveChangesAsync(); + + var template = new PlayoutTemplate + { + Index = 0, + TemplateId = 20, + DaysOfWeek = [], + DaysOfMonth = AlternateScheduleSelector.AllDaysOfMonth(), + MonthsOfYear = AlternateScheduleSelector.AllMonthsOfYear(), + LimitToDateRange = false, + StartMonth = 1, + StartDay = 1, + EndMonth = 12, + EndDay = 31 + }; + context.Playouts.Add( + new Playout + { + Id = 1, + ChannelId = 1, + ProgramScheduleId = 10, + Version = 1, + Items = [], + ProgramScheduleAlternates = [], + Templates = [template] + }); + await context.SaveChangesAsync(); + return template.Id; + } + // ---- #253 optimistic concurrency (alternate schedules #7 + templates #8, shared Playout.Version) ---- - private static ReplacePlayoutAlternateSchedule AltItem(int programScheduleId = 10) => - new(0, 0, programScheduleId, [], [], [], false, 1, 1, null, 12, 31, null); + // Recurrence sets are UNRESTRICTED here, not empty (#880): an empty set now means "matches no date" + // and is rejected on any item whose recurrence is stored, so an empty fixture would make these + // concurrency tests measure the recurrence guard instead of the version check. + private static ReplacePlayoutAlternateSchedule AltItem(int programScheduleId = 10, int index = 0) => + new( + 0, + index, + programScheduleId, + AlternateScheduleSelector.AllDaysOfWeek(), + AlternateScheduleSelector.AllDaysOfMonth(), + AlternateScheduleSelector.AllMonthsOfYear(), + false, + 1, + 1, + null, + 12, + 31, + null); private static ReplacePlayoutTemplate TemplateItem(int templateId = 20) => - new(0, 0, templateId, null, [], [], [], false, 1, 1, null, 12, 31, null); + new( + 0, + 0, + templateId, + null, + AlternateScheduleSelector.AllDaysOfWeek(), + AlternateScheduleSelector.AllDaysOfMonth(), + AlternateScheduleSelector.AllMonthsOfYear(), + false, + 1, + 1, + null, + 12, + 31, + null); private async Task SeedPlayout(int id, int version, int? programScheduleId = 10) { diff --git a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs index ab59c4893..4e353bf26 100644 --- a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs @@ -978,7 +978,7 @@ public class PlayoutControllerTests .Returns(Option.Some(MakePlayout(9))); var request = new ReplacePlayoutAlternateSchedulesRequest( - [new PlayoutAlternateScheduleItemRequest(0, 7, [], [], [], true, startMonth, startDay, null, endMonth, endDay, null)]); + [new PlayoutAlternateScheduleItemRequest(0, 7, null, null, null, true, startMonth, startDay, null, endMonth, endDay, null)]); IActionResult result = await _controller.ReplaceAlternateSchedules(9, request, CancellationToken.None); @@ -1002,7 +1002,7 @@ public class PlayoutControllerTests .Returns(Option.Some(MakePlayout(9))); var request = new ReplacePlayoutAlternateSchedulesRequest( - [new PlayoutAlternateScheduleItemRequest(0, 7, [], [], [], true, startMonth, startDay, null, endMonth, endDay, null)]); + [new PlayoutAlternateScheduleItemRequest(0, 7, null, null, null, true, startMonth, startDay, null, endMonth, endDay, null)]); IActionResult result = await _controller.ReplaceAlternateSchedules(9, request, CancellationToken.None); @@ -1025,7 +1025,7 @@ public class PlayoutControllerTests // LimitToDateRange is false, so the out-of-range month/day here must not block the save. var request = new ReplacePlayoutAlternateSchedulesRequest( - [new PlayoutAlternateScheduleItemRequest(0, 7, [], [], [], false, 0, 0, null, 13, 32, null)]); + [new PlayoutAlternateScheduleItemRequest(0, 7, null, null, null, false, 0, 0, null, 13, 32, null)]); IActionResult result = await _controller.ReplaceAlternateSchedules(9, request, CancellationToken.None); @@ -1079,6 +1079,110 @@ public class PlayoutControllerTests Arg.Any()); } + // ----- #880: absent recurrence means UNRESTRICTED, an explicit [] is rejected ----- + + // Reddens if ToReplaceItem's `?? All*()` is reverted to `?? []`: the counts drop to 0. That is the + // point of the test -- the normalization is the fix, so it is what must be pinned. + [Test] + public async Task ReplaceAlternateSchedules_Should_Normalize_Absent_Recurrence_To_Unrestricted() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakePlayout(9))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeScheduleVm(7)]); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right(Unit.Default)); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeAltVm(1, 0, 7)]); + + // All three recurrence arrays omitted -- the shape an API client sends and the SPA never does. + var request = new ReplacePlayoutAlternateSchedulesRequest( + [new PlayoutAlternateScheduleItemRequest(0, 7, null, null, null, false, 1, 1, null, 12, 31, null)]); + + IActionResult result = await _controller.ReplaceAlternateSchedules(9, request, CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send( + Arg.Is(c => + c.Items.Count == 1 && + c.Items[0].DaysOfWeek.Count == 7 && + c.Items[0].DaysOfMonth.Count == 31 && + c.Items[0].MonthsOfYear.Count == 12), + Arg.Any()); + } + + // The complement of the test above: normalization must fill in ONLY what was absent. Without this a + // fix that substituted All*() unconditionally would still pass the normalization test. + [Test] + public async Task ReplaceAlternateSchedules_Should_Preserve_An_Explicit_Recurrence_Selection() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakePlayout(9))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeScheduleVm(7)]); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right(Unit.Default)); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeAltVm(1, 0, 7)]); + + var request = new ReplacePlayoutAlternateSchedulesRequest( + [ + new PlayoutAlternateScheduleItemRequest( + 0, + 7, + [DayOfWeek.Monday, DayOfWeek.Tuesday], + null, + [6], + false, + 1, + 1, + null, + 12, + 31, + null) + ]); + + IActionResult result = await _controller.ReplaceAlternateSchedules(9, request, CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send( + Arg.Is(c => + c.Items[0].DaysOfWeek.Count == 2 && + c.Items[0].DaysOfWeek.Contains(DayOfWeek.Monday) && + c.Items[0].DaysOfMonth.Count == 31 && + c.Items[0].MonthsOfYear.Count == 1 && + c.Items[0].MonthsOfYear.Contains(6)), + Arg.Any()); + } + + [Test] + public async Task ReplaceTemplates_Should_Normalize_Absent_Recurrence_To_Unrestricted() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns( + Option.Some(MakePlayout(9) with { ScheduleKind = PlayoutScheduleKind.Block })); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeTemplateViewModel(7)]); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.None); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns([MakeTemplateVm(1, 0, 7, null)]); + + var request = new ReplacePlayoutTemplatesRequest( + [new PlayoutTemplateItemRequest(0, 7, null, null, null, null, false, 1, 1, null, 12, 31, null)]); + + IActionResult result = await _controller.ReplaceTemplates(9, request, CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send( + Arg.Is(c => + c.Items.Count == 1 && + c.Items[0].DaysOfWeek.Count == 7 && + c.Items[0].DaysOfMonth.Count == 31 && + c.Items[0].MonthsOfYear.Count == 12), + Arg.Any()); + } + // ----- Playout templates ----- [Test] @@ -1187,7 +1291,7 @@ public class PlayoutControllerTests .Returns(Option.Some(MakePlayout(9) with { ScheduleKind = PlayoutScheduleKind.Block })); var request = new ReplacePlayoutTemplatesRequest( - [new PlayoutTemplateItemRequest(0, 7, null, [], [], [], true, startMonth, startDay, null, endMonth, endDay, null)]); + [new PlayoutTemplateItemRequest(0, 7, null, null, null, null, true, startMonth, startDay, null, endMonth, endDay, null)]); IActionResult result = await _controller.ReplaceTemplates(9, request, CancellationToken.None); @@ -1212,7 +1316,7 @@ public class PlayoutControllerTests // LimitToDateRange is false, so the out-of-range month/day here must not block the save. var request = new ReplacePlayoutTemplatesRequest( - [new PlayoutTemplateItemRequest(0, 7, null, [], [], [], false, 0, 0, null, 13, 32, null)]); + [new PlayoutTemplateItemRequest(0, 7, null, null, null, null, false, 0, 0, null, 13, 32, null)]); IActionResult result = await _controller.ReplaceTemplates(9, request, CancellationToken.None); @@ -1400,7 +1504,7 @@ public class PlayoutControllerTests new(id, index, programScheduleId, [], [], [], false, 1, 1, null, 12, 31, null); private static PlayoutAlternateScheduleItemRequest MakeAltRequest(int programScheduleId) => - new(0, programScheduleId, [], [], [], false, 1, 1, null, 12, 31, null); + new(0, programScheduleId, null, null, null, false, 1, 1, null, 12, 31, null); private static ProgramScheduleViewModel MakeScheduleVm(int id) => new(id, $"Schedule {id}", false, false, false, false, FixedStartTimeBehavior.Strict, null, 0); @@ -1426,7 +1530,7 @@ public class PlayoutControllerTests null); private static PlayoutTemplateItemRequest MakeTemplateRequest(int templateId, int? decoTemplateId) => - new(0, templateId, decoTemplateId, [], [], [], false, 1, 1, null, 12, 31, null); + new(0, templateId, decoTemplateId, null, null, null, false, 1, 1, null, 12, 31, null); private static PlayoutNameViewModel MakePlayout(int id) => new( diff --git a/ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs b/ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs index e165eaafc..5b002dea3 100644 --- a/ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs @@ -1,13 +1,15 @@ +#nullable enable using ErsatzTV.Application.Playouts; +using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Controllers.Api.Requests; public record PlayoutAlternateScheduleItemRequest( int Id, int ProgramScheduleId, - List DaysOfWeek, - List DaysOfMonth, - List MonthsOfYear, + List? DaysOfWeek, + List? DaysOfMonth, + List? MonthsOfYear, bool LimitToDateRange, int StartMonth, int StartDay, @@ -16,14 +18,26 @@ public record PlayoutAlternateScheduleItemRequest( int EndDay, int? EndYear) { + // An ABSENT recurrence array means UNRESTRICTED, not "matches nothing" (#880). The three sets are + // conjunctive in AlternateScheduleSelector.GetScheduleForDate -- a miss on any one `continue`s -- so + // the previous `?? []` stored an item that could never apply on any date, and returned 200 while + // doing it. Absence now normalizes to the same All*() sets the READ side substitutes for a NULL + // column (AlternateScheduleSelector's read guard and both Mapper.ProjectToViewModel overloads), so + // the two halves of "this field is absent" finally agree -- residual (3) of + // `media.nullable-primitive-collection-mutation`. + // + // These are nullable so that ABSENT is distinguishable from an explicitly-sent `[]`, which is a + // different request and is REJECTED with a 422 in PlayoutController rather than normalized here. + // Newtonsoft maps both a missing property and an explicit `null` to null, so both read as absent; + // only a literal `[]` survives as empty. public ReplacePlayoutAlternateSchedule ToReplaceItem(int index) => new( Id, index, ProgramScheduleId, - DaysOfWeek ?? [], - DaysOfMonth ?? [], - MonthsOfYear ?? [], + DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(), + DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(), + MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(), LimitToDateRange, StartMonth, StartDay, diff --git a/ErsatzTV/Controllers/Api/Requests/PlayoutTemplateItemRequest.cs b/ErsatzTV/Controllers/Api/Requests/PlayoutTemplateItemRequest.cs index 540b4f99a..5e3689ae7 100644 --- a/ErsatzTV/Controllers/Api/Requests/PlayoutTemplateItemRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/PlayoutTemplateItemRequest.cs @@ -1,4 +1,6 @@ +#nullable enable using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Controllers.Api.Requests; @@ -6,9 +8,9 @@ public record PlayoutTemplateItemRequest( int Id, int TemplateId, int? DecoTemplateId, - List DaysOfWeek, - List DaysOfMonth, - List MonthsOfYear, + List? DaysOfWeek, + List? DaysOfMonth, + List? MonthsOfYear, bool LimitToDateRange, int StartMonth, int StartDay, @@ -17,15 +19,18 @@ public record PlayoutTemplateItemRequest( int EndDay, int? EndYear) { + // Same contract as PlayoutAlternateScheduleItemRequest: ABSENT means UNRESTRICTED (the All*() sets + // the read side substitutes for a NULL column), an explicit `[]` is rejected with a 422 in + // PlayoutController. See that record for the full rationale (#880). public ReplacePlayoutTemplate ToReplaceItem(int index) => new( Id, index, TemplateId, DecoTemplateId, - DaysOfWeek ?? [], - DaysOfMonth ?? [], - MonthsOfYear ?? [], + DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(), + DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(), + MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(), LimitToDateRange, StartMonth, StartDay, diff --git a/docs/api-conventions.md b/docs/api-conventions.md index d87a95014..03ef5a896 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -383,6 +383,46 @@ configurable — never materialized into a stored number on save, so an untouche identically. Document the range in the schema with `[property: Description("…")]` on the request record's positional parameter (`System.ComponentModel`); it renders into `v1.json`. +### 3e. An ABSENT collection means unrestricted; an explicitly EMPTY one is rejected + +A nullable collection on a write path carries two different requests that are easy to collapse into +one, and collapsing them is how ersatztv#880 shipped a 200 that stored a row which could never apply. +Decide both, separately: + +- **Absent** (the property is missing, or explicitly `null` — Newtonsoft maps both to `null`) means + *the client is not expressing a restriction*. Normalize it to the **permissive** value, which is + whatever the READ side already substitutes for the same absence. It must not become the empty set: + for a filter, empty is the maximally *restrictive* value, so `?? []` silently inverts the request. +- **Explicitly empty** (`[]`) is a different, well-formed request. If the empty set has no meaningful + outcome — a conjunctive filter that then matches nothing — **reject it with a 422 naming the + consequence**, per §3d. Do not accept-then-rewrite it into the permissive value: that would make + `[]` and absence indistinguishable again, in the other direction. + +Distinguishing them requires the request record's property to be **nullable** (`List?`, with a +per-file `#nullable enable` where the project has annotations off), because `?? …` on a +non-nullable-annotated `List` cannot tell absence from empty. Normalization belongs in the request +record's `ToReplaceItem`; **rejection does not belong beside it.** Three rules that come with it: + +- **Say "send `null`", not "omit the property".** Making a C# property nullable does NOT make it + optional in the generated schema: all three recurrence properties are still listed in `required` in + `v1.json`, with type `["null","array"]`. A client generated from the published contract therefore + *cannot* omit them, and an error message telling it to would be instructing a schema violation. + Omission still works at runtime; `null` is the form that is also contract-legal. +- **Validate where the STORED value is in hand — the handler, not the controller.** §3d's rule that an + *unchanged* bad value must still be accepted applies here in its sharpest form: these are whole-list + replace PUTs, so rejecting a pre-existing empty set would make every *other* item in the list + uneditable over a row the operator never touched. That comparison needs the stored row, which the + controller does not have and the handler already loaded. Exemplar: `RecurrenceSetBounds` + (`ErsatzTV.Application/Scheduling`), one validator called from both replace handlers — the same shape + as `FFmpegProfileBounds`. The exemption is **per field**, not per row: a row grandfathered on + `DaysOfWeek` still cannot newly empty `MonthsOfYear`. +- **Derive the validated set from the list the handler actually writes.** In the alternate-schedule + path that is `incoming`, which *excludes* the highest-`Index` catch-all — the handler discards that + item's recurrence along with its date range, so an empty set there cannot make anything "never + apply" and rejecting it would state a reason that is false for that item. Walking the same list the + writes iterate is what keeps the check and its subject from drifting; do not re-derive "which item is + the catch-all" in a second place (`api.put-replace-index-order`). + ## 4. Artwork contract API response DTOs return **rooted, directly-usable artwork URLs** — e.g. `/artwork/posters/...`, diff --git a/docs/decisions/README.md b/docs/decisions/README.md index f2b4eb3c3..cb8b53bb3 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -8,6 +8,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | Key | Current rule | Since | Record | | --- | --- | --- | --- | +| `api.absent-collection-means-unrestricted` | A nullable collection on a write path carries TWO distinct requests and they get DIFFERENT answers. ABSENT (property missing, or explicit `null` — Newtonsoft maps both to null, so they are indistinguishable and are treated as ONE case) means the client expresses NO restriction and NORMALIZES to the PERMISSIVE value; EXPLICITLY EMPTY (`[]`) is a well-formed request whose outcome is meaningless for a conjunctive filter, and is REJECTED with a 422 naming the consequence rather than rewritten. Collapsing the two is what ersatztv#880 records: `?? []` on the three recurrence arrays of `PlayoutAlternateScheduleItemRequest` / `PlayoutTemplateItemRequest` returned HTTP 200 and stored a row that could NEVER apply on any date, because `AlternateScheduleSelector.GetScheduleForDate` treats DaysOfWeek/DaysOfMonth/MonthsOfYear CONJUNCTIVELY (a miss on any one `continue`s), so empty is the maximally RESTRICTIVE value, not a neutral one. The permissive value is not chosen freshly: it must be the SAME SYMBOL the read side substitutes for the same absence — here `AlternateScheduleSelector.All*()`, already used by the selector's null guard and both `Mapper.ProjectToViewModel` overloads (`media.nullable-primitive-collection-mutation`), whose residual (3) this closes. Distinguishing absence from empty REQUIRES the property to be nullable (`List?`), which in the `ErsatzTV` web project needs a per-file `#nullable enable` — the project has annotations off, and sibling request records already use that form. THE TWO HALVES LIVE IN DIFFERENT LAYERS AND THAT SPLIT IS THE DECISION: normalization sits in the request record's `ToReplaceItem`, but REJECTION SITS IN THE HANDLER, in `RecurrenceSetBounds` (`ErsatzTV.Application/Scheduling`) called from BOTH replace handlers — the `FFmpegProfileBounds` shape. It CANNOT sit beside the normalization in the controller, because `api.ffmpeg-profile-numeric-bounds`' rule that an UNCHANGED bad value must still be accepted binds here in its sharpest form: both PUT paths are WHOLE-LIST replaces, so rejecting a pre-existing empty set would make every OTHER item in the list uneditable over a row the operator never touched. That comparison needs the STORED row, which the controller does not have and the handler already loaded. The exemption is PER FIELD, not per row — a row grandfathered on `DaysOfWeek` still cannot newly empty `MonthsOfYear` — and a STORED NULL is NOT exempt, because null means unrestricted, so submitting `[]` against it is a new emptying. The validated set is DERIVED from the list the handler actually writes (`incoming`), which EXCLUDES the highest-`Index` catch-all: the handler discards that item's recurrence along with its date range, so an empty set there cannot make anything "never apply" and rejecting it would state a reason that is FALSE for that item — do not re-derive "which item is the catch-all" in a second place (`api.put-replace-index-order`). Finally, the error message says SEND NULL, not "omit the property": making a C# property nullable does NOT make it optional in the generated schema — all three are still listed in `required` in `v1.json` with type `["null","array"]` — so a client generated from the published contract cannot omit them, and telling it to would instruct a schema violation. Omission still works at runtime; `null` is the form that is also contract-legal. | 2026-08-30 | [link](records/api/absent-collection-means-unrestricted.md) | | `api.artwork-rooted-urls` | API response DTOs return artwork as rooted, directly-usable URLs (plus passthrough for absolute/proxy URLs), never relative Blazor-convention paths. | 2026-07-07 | [link](records/api/artwork-rooted-urls.md) | | `api.async-op-contract` | Queue-triggering `/api/*` endpoints normalize onto one contract — 202 Accepted (queued), 404 (missing entity), 409 (lock held), 422 (domain precondition) — with Trakt as the reference implementation; playout list/detail GETs also carry an `isLocked` observability flag as the HTTP-observable substitute for a live push channel. | 2026-07-11 | [link](records/api/async-op-contract.md) | | `api.channel-health-object` | `ChannelResponseModel`/`ChannelDetailResponseModel` carry a server-derived `health` object (`ChannelHealthResponseModel { Status, Faults[], PlayoutCount, BrokenSourceItemCount }`) computed **read-time** from the built timeline (`Playout.BuildStatus` + upcoming `PlayoutItem → MediaItem.State`, `Finish >= now`), kind-agnostic across all 5 `PlayoutScheduleKind` values; `Status`/`Faults` are const-string classes (`ChannelHealthStatus`, `ChannelFault`), not C# enums, so the SPA hand-maintains the union (mirrors `ChannelPreviewAvailability`). This supersedes #72's "raw fact only, no derived enum, empty-schedule/broken-source deliberately not computed" stance now that the auto-tune taxonomy churn (#383/#384) it was waiting on has landed (see `channel.origin-marker` sibling record, #414). | 2026-07-23 | [link](records/api/channel-health-object.md) | @@ -110,7 +111,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) | | `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, and EVERY tool (read and write) declares exactly its endpoint's query parameters, both asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields` and `Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. An omitted query parameter is UNREACHABLE, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | | `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) | -| `media.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record's `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` 'allocates for nothing', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today's callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun. | 2026-08-22 | [link](records/media/nullable-primitive-collection-mutation.md) | +| `media.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predated this record and was deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. It is now CLOSED by `api.absent-collection-means-unrestricted` (#880), which splits the write side in two rather than copying this record's answer across: an ABSENT array normalizes to the SAME `All*()` symbols the read side substitutes, so the two halves of 'this field is absent' finally agree, while an EXPLICITLY EMPTY `[]` — a request this record never had a view on — is rejected with a 422 instead of being stored as a row that can never apply. For these six the guard form is `?? All*()` into a local, NOT this record's `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` 'allocates for nothing', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today's callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun. | 2026-08-22 | [link](records/media/nullable-primitive-collection-mutation.md) | | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | | `media.source-mgmt-write-api` | Media-source management (local/Plex/Jellyfin/Emby) is a REST write API + SPA under `/app/libraries/*`, wrapping existing MediatR commands 1:1 with no new commands or DB migration; connection GETs never leak a stored `apiKey`, and each PUT-replace family's identity contract is documented per-family (not assumed uniform). | 2026-07-11 | [link](records/media/source-mgmt-write-api.md) | diff --git a/docs/decisions/records/api/absent-collection-means-unrestricted.md b/docs/decisions/records/api/absent-collection-means-unrestricted.md new file mode 100644 index 000000000..44965994f --- /dev/null +++ b/docs/decisions/records/api/absent-collection-means-unrestricted.md @@ -0,0 +1,47 @@ +--- +key: api.absent-collection-means-unrestricted +title: '2026-08-30 — An ABSENT request collection means unrestricted; an explicitly EMPTY one is rejected with a 422 (#880)' +status: active +since: '2026-08-30' +supersedes: none +superseded-by: none +rule: 'A nullable collection on a write path carries TWO distinct requests and they get DIFFERENT answers. ABSENT (property missing, or explicit `null` — Newtonsoft maps both to null, so they are indistinguishable and are treated as ONE case) means the client expresses NO restriction and NORMALIZES to the PERMISSIVE value; EXPLICITLY EMPTY (`[]`) is a well-formed request whose outcome is meaningless for a conjunctive filter, and is REJECTED with a 422 naming the consequence rather than rewritten. Collapsing the two is what ersatztv#880 records: `?? []` on the three recurrence arrays of `PlayoutAlternateScheduleItemRequest` / `PlayoutTemplateItemRequest` returned HTTP 200 and stored a row that could NEVER apply on any date, because `AlternateScheduleSelector.GetScheduleForDate` treats DaysOfWeek/DaysOfMonth/MonthsOfYear CONJUNCTIVELY (a miss on any one `continue`s), so empty is the maximally RESTRICTIVE value, not a neutral one. The permissive value is not chosen freshly: it must be the SAME SYMBOL the read side substitutes for the same absence — here `AlternateScheduleSelector.All*()`, already used by the selector''s null guard and both `Mapper.ProjectToViewModel` overloads (`media.nullable-primitive-collection-mutation`), whose residual (3) this closes. Distinguishing absence from empty REQUIRES the property to be nullable (`List?`), which in the `ErsatzTV` web project needs a per-file `#nullable enable` — the project has annotations off, and sibling request records already use that form. THE TWO HALVES LIVE IN DIFFERENT LAYERS AND THAT SPLIT IS THE DECISION: normalization sits in the request record''s `ToReplaceItem`, but REJECTION SITS IN THE HANDLER, in `RecurrenceSetBounds` (`ErsatzTV.Application/Scheduling`) called from BOTH replace handlers — the `FFmpegProfileBounds` shape. It CANNOT sit beside the normalization in the controller, because `api.ffmpeg-profile-numeric-bounds`'' rule that an UNCHANGED bad value must still be accepted binds here in its sharpest form: both PUT paths are WHOLE-LIST replaces, so rejecting a pre-existing empty set would make every OTHER item in the list uneditable over a row the operator never touched. That comparison needs the STORED row, which the controller does not have and the handler already loaded. The exemption is PER FIELD, not per row — a row grandfathered on `DaysOfWeek` still cannot newly empty `MonthsOfYear` — and a STORED NULL is NOT exempt, because null means unrestricted, so submitting `[]` against it is a new emptying. The validated set is DERIVED from the list the handler actually writes (`incoming`), which EXCLUDES the highest-`Index` catch-all: the handler discards that item''s recurrence along with its date range, so an empty set there cannot make anything "never apply" and rejecting it would state a reason that is FALSE for that item — do not re-derive "which item is the catch-all" in a second place (`api.put-replace-index-order`). Finally, the error message says SEND NULL, not "omit the property": making a C# property nullable does NOT make it optional in the generated schema — all three are still listed in `required` in `v1.json` with type `["null","array"]` — so a client generated from the published contract cannot omit them, and telling it to would instruct a schema violation. Omission still works at runtime; `null` is the form that is also contract-legal.' +signals: 'omitted array means never applies · absent versus explicitly empty on a write path · `?? []` inverts a conjunctive filter · empty set is the most restrictive value not the neutral one · normalize absence to the same symbol the read side substitutes · `List?` plus per-file `#nullable enable` in a project with annotations off · reject an empty recurrence with 422 naming the consequence · rejection needs the STORED row so it lives in the handler not the controller · unchanged stored empty is exempt per FIELD not per row · a stored NULL is not exempt because null means unrestricted · validate the list the handler writes so the catch-all is excluded by construction · nullable does not mean optional in the generated schema so say send null not omit · paths: `ErsatzTV.Application/Scheduling/RecurrenceSetBounds.cs`, `ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs`, `ErsatzTV/Controllers/Api/Requests/PlayoutTemplateItemRequest.cs`, `ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs`, `ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs` · issues: #880, #823, #701' +mechanics: 'Two halves, pinned in two files and mutation-proved SEPARATELY by EXECUTION against the real predecessor clause. NORMALIZATION is pinned in `ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs` by `Replace{AlternateSchedules,Templates}_Should_Normalize_Absent_Recurrence_To_Unrestricted` plus `ReplaceAlternateSchedules_Should_Preserve_An_Explicit_Recurrence_Selection` — the last is the complement, without which a fix that substituted `All*()` UNCONDITIONALLY would still pass. Proof: reverting the three `?? All*()` clauses to `?? []` in both request records reddens EXACTLY those three. REJECTION is pinned in `ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs` against a real `InMemoryTvContext`, and the four cases are chosen to be mutually distinguishing: a newly-empty field on a NON-catch-all item is rejected; the same empty on the CATCH-ALL is ACCEPTED; an UNCHANGED stored empty is accepted; and the SAME stored row still REJECTS a different field newly emptied. Each rejection case also asserts the version did NOT bump, which is the anti-vacuity control — a 422 that had already mutated would be a different bug. One fixture trap is load-bearing and was measured, not reasoned about: the template handler loads `.Include(p => p.Templates).ThenInclude(t => t.Template)` and that navigation is REQUIRED, so a `PlayoutTemplate` seeded WITHOUT its `Template` row is joined OUT and never reaches `existing` — the stored row is then invisible, the exemption cannot match, and the test fails claiming the rule is broken when the fixture is. Seed the `TemplateGroup` and `Template` too.' +--- + +**The empty set is not the neutral value, and that is the whole defect.** For a conjunctive filter, +absence and emptiness sit at OPPOSITE ends: absence restricts nothing, empty admits nothing. `?? []` +reads as a harmless "default to no filter" and is in fact "default to matching nothing" — the row is +stored, the write returns 200, and the item silently never applies. Nothing logs it, because from the +scheduler's point of view a non-matching item is ordinary. The SPA never triggered it (it sends full +arrays and its generated type marks the field `null | Array`, so the key is always present) and the +MCP server exposes no such write tool, which is why this survived: it was reachable only by a direct +API client, and only by OMITTING a field. + +**Why the two cases must not be merged in either direction.** Normalizing `[]` to the permissive value +too would make it impossible to say "no days" at all, and would re-collapse the distinction the +nullable annotation was added to express. Rejecting absence instead would break every client that +legitimately omits an optional field. The pair — normalize absence, reject empty — is the only +assignment that leaves both requests expressible and neither one silent. + +**The first implementation put the rejection in the controller, and an independent review was right to +block it.** Validating the request there is cheaper and matches the sibling `ValidateDateRanges`, but +it has no access to the stored row, so it rejected an unchanged legacy empty and — because these are +whole-list replaces — would have made every sibling item unsaveable. It also validated EVERY item +uniformly, including the catch-all whose recurrence is discarded, producing a 422 whose stated reason +("the item would never apply") is false for exactly that item. Both disappear once the check moves to +the handler and derives its population from `incoming`. The uniformity of `ValidateDateRanges` is +precedent for a STYLE, not evidence that the behaviour is correct; that was the reasoning error. + +**Prod was measured before choosing to reject rather than grandfather** (2026-08-30, jazz +`/config/ersatztv.sqlite3`): `ProgramScheduleAlternate` and `PlayoutTemplate` both hold ZERO rows. That +measurement is why the exemption could not be justified by pointing at live data — it had to be +justified by the shape of a whole-list replace instead, which holds for any deployment. Do not cite +the zero-row count as a reason to drop the exemption. + +**The SPA can still produce an empty set** (`PlayoutScheduleEditors.tsx` toggles days individually, +with no floor at one), so a user who unchecks every day now receives the 422 instead of silently +saving a dead row. That is the intended outcome and it is a visible behaviour change; a client-side +floor mirroring the server message would be the §3d "mirror the exemption in the client" follow-up if +the error proves awkward in the form. diff --git a/docs/decisions/records/media/nullable-primitive-collection-mutation.md b/docs/decisions/records/media/nullable-primitive-collection-mutation.md index 07b3fad23..d9b2f5140 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,7 +5,7 @@ status: active since: '2026-08-22' supersedes: none superseded-by: none -rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record''s `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` ''allocates for nothing'', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today''s callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun.' +rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predated this record and was deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. It is now CLOSED by `api.absent-collection-means-unrestricted` (#880), which splits the write side in two rather than copying this record''s answer across: an ABSENT array normalizes to the SAME `All*()` symbols the read side substitutes, so the two halves of ''this field is absent'' finally agree, while an EXPLICITLY EMPTY `[]` — a request this record never had a view on — is rejected with a 422 instead of being stored as a row that can never apply. For these six the guard form is `?? All*()` into a local, NOT this record''s `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` ''allocates for nothing'', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today''s callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun.' signals: 'Artists ??= [] on a tracked entity · nullable primitive collection not a navigation · JSON array in one column · value converter is the same hazard as a primitive collection · IntCollectionValueConverter EnumCollectionJsonValueConverter · derive the collection-column population from the model configuration · SaveChanges writes empty array over NULL · entity flips to Modified on a read guard · AsNoTracking today is not a safety argument · untagged song loses its NULL artists · foreach over null throws NRE while string.Join throws ArgumentNullException · sweep by FIELD not by file · Optional Flatten hoisted local for the SongMetadata pair · but the six SCHEDULING columns substitute the All*() sets, not empty · a legacy NULL DaysOfMonth means NO day-of-month restriction, not ''matches no day'' · reading it as empty INVERTS a pre-2024 PlayoutTemplate row · EF skips the value converter entirely for a NULL column, on both providers · AlternateScheduleSelector .Contains throws NRE on a null · guard the two Mapper.ProjectToViewModel overloads too or the SPA spreads a null and throws TypeError · appliesToDate is an exact port and must preview what is scheduled · the full-replace PUT persists the substituted set over the NULL on the next save · write side normalizes an omitted array to [] while the read side reads NULL as unrestricted (#880) · paths: `ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs`, `ErsatzTV.Application/Playouts/Mapper.cs`, `ErsatzTV.Application/Scheduling/Mapper.cs`, `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs`, `ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs`, `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs`, `ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs`, `ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs`, `ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs`, `ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs` · issues: #701, #691, #671, #823, #824' mechanics: 'Pinned by `SongIndexerMetadataMutationTests.UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity`, which drives the real `LuceneSearchIndex` against a real `TvContext` on SQLite with a deliberately TRACKED song. That fixture covers LUCENE ONLY; `ElasticSearchIndex` holds an independent copy and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that does not DECLARE a runnable test of its own. Test-ness is decided by NUnit''s `ITestBuilder`/`ISimpleTestBuilder` interfaces rather than a list of attribute types, because two successive hand-written lists each falsely reddened whatever they omitted (`[TestCase]`, then `[Theory]`); `DeclaredOnly` is what stops a fixture SUBCLASSING another from inheriting its `[Test]` and passing while driving the wrong indexer, which `Values.Distinct()` cannot see since the two types differ; and `abstract`, `[Explicit]` and `[Ignore]` are each rejected as the fixture-level form of ''wired is not running''. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. No repo-wide detector: the `??=` idiom is correct on navigations and appears 92 times across the app projects (`grep -rn ''??= '' --include=''*.cs'' ErsatzTV ErsatzTV.Core ErsatzTV.Application ErsatzTV.Infrastructure ErsatzTV.Scanner | grep -v ''/obj/\|/bin/'' | grep -c ''''`, 2026-08-22), so a grep for it would be noise — the eight-column population is small enough to sweep by field instead.' ---