fix(880): an absent recurrence array means unrestricted, an explicit [] is rejected (#892)
Build ErsatzTV Image / CI toolchain image resolves (push) Successful in 9s
Build ErsatzTV Image / Delimiter ban (release path) (push) Successful in 21s
Build ErsatzTV Image / Build & test (.NET) (push) Successful in 10m54s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (push) Successful in 7m34s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (push) Successful in 6m59s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (push) Skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (push) Skipped
Build ErsatzTV Image / Build & push image (amd64) (push) Failing after 1m27s

The three recurrence arrays are read CONJUNCTIVELY by
AlternateScheduleSelector.GetScheduleForDate, so an empty set matches no date.
`?? []` on an omitted array therefore returned HTTP 200 while storing an
alternate-schedule or template item that could never apply, silently -- while the
read side (#823) already read a NULL column as the All*() sets.

Absent and explicitly-empty are two different requests and get two answers:
ABSENT (missing, or explicit null) normalizes to AlternateScheduleSelector.All*(),
the same symbols the read side substitutes; EXPLICIT [] is rejected with a 422
naming the consequence, via RecurrenceSetBounds called from both replace handlers.

The rejection lives in the handlers, not the controller, because
api.ffmpeg-profile-numeric-bounds' "accept an UNCHANGED bad value" rule binds
hardest here: both PUT paths are whole-list replaces, so rejecting a pre-existing
empty set would make every OTHER item in the list uneditable. That comparison
needs the stored row. The validated set is derived from `incoming`, so the
highest-Index catch-all -- whose recurrence the handler discards -- is excluded by
construction.

Verified: full ErsatzTV.Tests suite green; three mutation proofs with disjoint
reddened sets; live-E2E against a real instance confirmed an OMITTED property
round-trips as unrestricted (the Newtonsoft missing-property chain unit tests
cannot reach), an explicit [] returns the 422, and [] on the catch-all is accepted.
Cross-family cold review BLOCKED the first implementation with 3 findings, all real
and all fixed; re-review returned MERGEABLE.

Follow-up #894 filed: the SPA can still build the empty state the server rejects.

fixes #880

Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit was merged in pull request #892.
This commit is contained in:
2026-08-30 09:29:02 +00:00
co-authored by Claude Opus 5
parent 58681b3a79
commit 528383cf3a
11 changed files with 540 additions and 24 deletions
@@ -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<BaseError> 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();
@@ -44,6 +44,25 @@ public class ReplacePlayoutTemplateItemsHandler(
List<ReplacePlayoutTemplate> 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<BaseError> 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();
@@ -0,0 +1,74 @@
using ErsatzTV.Core;
namespace ErsatzTV.Application.Scheduling;
/// <summary>
/// Validates the three recurrence sets shared by <c>ProgramScheduleAlternate</c> and
/// <c>PlayoutTemplate</c> (ersatztv#880). One validator called from BOTH replace handlers, mirroring
/// <c>FFmpegProfileBounds</c> — the exemplar for `api.ffmpeg-profile-numeric-bounds`, whose shape this
/// follows deliberately.
/// </summary>
/// <remarks>
/// <para>
/// An EMPTY set is rejected because the three are read CONJUNCTIVELY by
/// <c>AlternateScheduleSelector.GetScheduleForDate</c> — 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.
/// </para>
/// <para>
/// 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.
/// </para>
/// <para>
/// 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.
/// </para>
/// </remarks>
public static class RecurrenceSetBounds
{
public static Option<BaseError> Validate(
ICollection<DayOfWeek> daysOfWeek,
ICollection<int> daysOfMonth,
ICollection<int> monthsOfYear,
ICollection<DayOfWeek> storedDaysOfWeek,
ICollection<int> storedDaysOfMonth,
ICollection<int> 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<BaseError>.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<T>(ICollection<T> submitted, ICollection<T> stored) =>
submitted is { Count: 0 } && stored is not { Count: 0 };
}
@@ -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<ReplacePlayoutAlternateScheduleItemsHandler>.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<BaseError, Unit> 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<ReplacePlayoutAlternateScheduleItemsHandler>.Instance);
// a single item IS the catch-all
ReplacePlayoutAlternateSchedule catchAll = AltItem(index: 0) with { DaysOfWeek = [], MonthsOfYear = [] };
Either<BaseError, Unit> 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<ReplacePlayoutTemplateItemsHandler>.Instance);
ReplacePlayoutTemplate empty = TemplateItem() with { DaysOfMonth = [] };
Option<BaseError> 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<ReplacePlayoutTemplateItemsHandler>.Instance);
// same row, same empty DaysOfWeek -- an edit to some OTHER field on the same list
ReplacePlayoutTemplate unchanged = TemplateItem() with { Id = templateItemId, DaysOfWeek = [] };
Option<BaseError> 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<ReplacePlayoutTemplateItemsHandler>.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<BaseError> 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<int> 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)
{
@@ -978,7 +978,7 @@ public class PlayoutControllerTests
.Returns(Option<PlayoutNameViewModel>.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<PlayoutNameViewModel>.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<CancellationToken>());
}
// ----- #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<GetPlayoutById>(), Arg.Any<CancellationToken>())
.Returns(Option<PlayoutNameViewModel>.Some(MakePlayout(9)));
_mediator.Send(Arg.Any<GetAllProgramSchedules>(), Arg.Any<CancellationToken>())
.Returns([MakeScheduleVm(7)]);
_mediator.Send(Arg.Any<ReplacePlayoutAlternateScheduleItems>(), Arg.Any<CancellationToken>())
.Returns(Right<BaseError, Unit>(Unit.Default));
_mediator.Send(Arg.Any<GetPlayoutAlternateSchedules>(), Arg.Any<CancellationToken>())
.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<OkObjectResult>();
await _mediator.Received(1).Send(
Arg.Is<ReplacePlayoutAlternateScheduleItems>(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<CancellationToken>());
}
// 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<GetPlayoutById>(), Arg.Any<CancellationToken>())
.Returns(Option<PlayoutNameViewModel>.Some(MakePlayout(9)));
_mediator.Send(Arg.Any<GetAllProgramSchedules>(), Arg.Any<CancellationToken>())
.Returns([MakeScheduleVm(7)]);
_mediator.Send(Arg.Any<ReplacePlayoutAlternateScheduleItems>(), Arg.Any<CancellationToken>())
.Returns(Right<BaseError, Unit>(Unit.Default));
_mediator.Send(Arg.Any<GetPlayoutAlternateSchedules>(), Arg.Any<CancellationToken>())
.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<OkObjectResult>();
await _mediator.Received(1).Send(
Arg.Is<ReplacePlayoutAlternateScheduleItems>(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<CancellationToken>());
}
[Test]
public async Task ReplaceTemplates_Should_Normalize_Absent_Recurrence_To_Unrestricted()
{
_mediator.Send(Arg.Any<GetPlayoutById>(), Arg.Any<CancellationToken>())
.Returns(
Option<PlayoutNameViewModel>.Some(MakePlayout(9) with { ScheduleKind = PlayoutScheduleKind.Block }));
_mediator.Send(Arg.Any<GetAllTemplates>(), Arg.Any<CancellationToken>())
.Returns([MakeTemplateViewModel(7)]);
_mediator.Send(Arg.Any<ReplacePlayoutTemplateItems>(), Arg.Any<CancellationToken>())
.Returns(Option<BaseError>.None);
_mediator.Send(Arg.Any<GetPlayoutTemplates>(), Arg.Any<CancellationToken>())
.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<OkObjectResult>();
await _mediator.Received(1).Send(
Arg.Is<ReplacePlayoutTemplateItems>(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<CancellationToken>());
}
// ----- Playout templates -----
[Test]
@@ -1187,7 +1291,7 @@ public class PlayoutControllerTests
.Returns(Option<PlayoutNameViewModel>.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(
@@ -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<DayOfWeek> DaysOfWeek,
List<int> DaysOfMonth,
List<int> MonthsOfYear,
List<DayOfWeek>? DaysOfWeek,
List<int>? DaysOfMonth,
List<int>? 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,
@@ -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<DayOfWeek> DaysOfWeek,
List<int> DaysOfMonth,
List<int> MonthsOfYear,
List<DayOfWeek>? DaysOfWeek,
List<int>? DaysOfMonth,
List<int>? 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,
+40
View File
@@ -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<T>?`, with a
per-file `#nullable enable` where the project has annotations off), because `?? …` on a
non-nullable-annotated `List<T>` 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/...`,
File diff suppressed because one or more lines are too long
@@ -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<T>?`), 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<T>?` 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<T>`, 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.
File diff suppressed because one or more lines are too long