fix(880): an absent recurrence array means unrestricted, an explicit [] is rejected
The three recurrence arrays on the two playout write paths are consumed CONJUNCTIVELY by AlternateScheduleSelector.GetScheduleForDate -- a miss on any one `continue`s -- so the empty set is the maximally RESTRICTIVE value, not a neutral one. `?? []` on an omitted array therefore read as "default to no filter" and meant "default to matching nothing": the API returned HTTP 200 and stored an alternate-schedule or template item that could never apply on any date, with nothing logged. The read side had already been decided the other way in #823 (a NULL column reads as the All*() sets), so the two halves of "this field is absent" pointed in opposite directions -- residual (3) of media.nullable-primitive-collection-mutation. Absent and explicitly-empty are two different requests and get two answers: - ABSENT (missing, or an explicit `null` -- Newtonsoft maps both to null and they are indistinguishable) normalizes to AlternateScheduleSelector.All*(), the SAME symbols the selector's null guard and both Mapper.ProjectToViewModel overloads already substitute. Requires List<T>? on the request records, hence the per-file `#nullable enable` the sibling request records already use. - EXPLICIT [] is rejected with a 422 naming the consequence, per api.ffmpeg-profile-numeric-bounds -- not rewritten into the permissive value, which would re-collapse the distinction in the other direction and make "no days" inexpressible. Checked before committing to the normalization, because it is a write-path semantics change: the SPA's generated type is `null | Array<T>` (nullable, not optional) and PlayoutScheduleEditors.tsx always sends the key, and ErsatzTV.Mcp/ToolCatalog.cs explicitly defers these replace-list writes. So no existing client takes the absent path -- the trap was reachable only by a direct API client omitting a field. Rejection is uniform across items, including the highest-Index catch-all whose recurrence the handler discards. Exempting it would restate "which item is the catch-all" in the controller, a second copy of a rule that lives in the handler (api.put-replace-index-order); ValidateDateRanges already makes exactly this choice for the date fields it equally discards. Prod was measured before choosing to reject rather than grandfather an existing empty row: ProgramScheduleAlternate and PlayoutTemplate both hold ZERO rows (jazz /config/ersatztv.sqlite3, 2026-08-30), so no stored [] can be bricked and the ffmpeg-profile compare-against-stored exemption was not needed. Both halves mutation-proved by execution, separately, against the real predecessor clause: reverting the `?? All*()` normalization alone reddens exactly the 3 normalization tests; removing the two ValidateRecurrenceSets call sites alone reddens exactly the 6 rejection cases. Disjoint sets, so the guards are not masking each other. OpenAPI and the generated SPA client are byte-identical before and after -- a non-nullable-annotated List<T> was already emitted as nullable -- so the contract shape did not move, only the behaviour behind it. fixes #880 Decisions-Edit: yes Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019zUmJZHhVP7kXg5DV237TW
This commit is contained in:
Timothy
@@ -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,199 @@ 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>());
|
||||
}
|
||||
|
||||
[TestCase("DaysOfWeek", "no day of the week")]
|
||||
[TestCase("DaysOfMonth", "no day of the month")]
|
||||
[TestCase("MonthsOfYear", "no month")]
|
||||
public async Task ReplaceAlternateSchedules_Should_Return_422_For_Empty_Recurrence_Without_Dispatch(
|
||||
string field,
|
||||
string consequence)
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlayoutById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlayoutNameViewModel>.Some(MakePlayout(9)));
|
||||
|
||||
var request = new ReplacePlayoutAlternateSchedulesRequest(
|
||||
[MakeAltRequestWithEmpty(field)]);
|
||||
|
||||
IActionResult result = await _controller.ReplaceAlternateSchedules(9, request, CancellationToken.None);
|
||||
|
||||
var unprocessable = result.ShouldBeOfType<UnprocessableEntityObjectResult>();
|
||||
var problem = unprocessable.Value.ShouldBeOfType<ProblemDetails>();
|
||||
problem.Status.ShouldBe(422);
|
||||
|
||||
// Pin WHICH validation rejected it, and that the message names the consequence rather than only
|
||||
// the field -- otherwise this passes on any unrelated 422 (`api.ffmpeg-profile-numeric-bounds`).
|
||||
problem.Detail.ShouldNotBeNull();
|
||||
problem.Detail.ShouldContain($"[{field}]");
|
||||
problem.Detail.ShouldContain(consequence);
|
||||
|
||||
await _mediator.DidNotReceive().Send(Arg.Any<GetAllProgramSchedules>(), Arg.Any<CancellationToken>());
|
||||
await _mediator.DidNotReceive()
|
||||
.Send(Arg.Any<ReplacePlayoutAlternateScheduleItems>(), 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>());
|
||||
}
|
||||
|
||||
[TestCase("DaysOfWeek", "no day of the week")]
|
||||
[TestCase("DaysOfMonth", "no day of the month")]
|
||||
[TestCase("MonthsOfYear", "no month")]
|
||||
public async Task ReplaceTemplates_Should_Return_422_For_Empty_Recurrence_Without_Dispatch(
|
||||
string field,
|
||||
string consequence)
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlayoutById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(
|
||||
Option<PlayoutNameViewModel>.Some(MakePlayout(9) with { ScheduleKind = PlayoutScheduleKind.Block }));
|
||||
|
||||
var request = new ReplacePlayoutTemplatesRequest([MakeTemplateRequestWithEmpty(field)]);
|
||||
|
||||
IActionResult result = await _controller.ReplaceTemplates(9, request, CancellationToken.None);
|
||||
|
||||
var unprocessable = result.ShouldBeOfType<UnprocessableEntityObjectResult>();
|
||||
var problem = unprocessable.Value.ShouldBeOfType<ProblemDetails>();
|
||||
problem.Status.ShouldBe(422);
|
||||
problem.Detail.ShouldNotBeNull();
|
||||
problem.Detail.ShouldContain($"[{field}]");
|
||||
problem.Detail.ShouldContain(consequence);
|
||||
|
||||
await _mediator.DidNotReceive().Send(Arg.Any<GetAllTemplates>(), Arg.Any<CancellationToken>());
|
||||
await _mediator.DidNotReceive().Send(Arg.Any<ReplacePlayoutTemplateItems>(), Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
// Exactly ONE of the three arrays is empty per case; the other two are ABSENT, so a case can only be
|
||||
// rejected by the field it names.
|
||||
private static PlayoutAlternateScheduleItemRequest MakeAltRequestWithEmpty(string field) =>
|
||||
new(
|
||||
0,
|
||||
7,
|
||||
field == "DaysOfWeek" ? [] : null,
|
||||
field == "DaysOfMonth" ? [] : null,
|
||||
field == "MonthsOfYear" ? [] : null,
|
||||
false,
|
||||
1,
|
||||
1,
|
||||
null,
|
||||
12,
|
||||
31,
|
||||
null);
|
||||
|
||||
private static PlayoutTemplateItemRequest MakeTemplateRequestWithEmpty(string field) =>
|
||||
new(
|
||||
0,
|
||||
7,
|
||||
null,
|
||||
field == "DaysOfWeek" ? [] : null,
|
||||
field == "DaysOfMonth" ? [] : null,
|
||||
field == "MonthsOfYear" ? [] : null,
|
||||
false,
|
||||
1,
|
||||
1,
|
||||
null,
|
||||
12,
|
||||
31,
|
||||
null);
|
||||
|
||||
// ----- Playout templates -----
|
||||
|
||||
[Test]
|
||||
@@ -1187,7 +1380,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 +1405,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 +1593,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 +1619,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(
|
||||
|
||||
@@ -372,6 +372,11 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) :
|
||||
return error.ToErrorResult();
|
||||
}
|
||||
|
||||
foreach (BaseError error in ValidateRecurrenceSets(items.Select(ToRecurrenceCheck)))
|
||||
{
|
||||
return error.ToErrorResult();
|
||||
}
|
||||
|
||||
List<ProgramScheduleViewModel> schedules =
|
||||
await mediator.Send(new GetAllProgramSchedules(), cancellationToken);
|
||||
var scheduleIds = schedules.Select(s => s.Id).ToHashSet();
|
||||
@@ -483,6 +488,11 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) :
|
||||
return error.ToErrorResult();
|
||||
}
|
||||
|
||||
foreach (BaseError error in ValidateRecurrenceSets(items.Select(ToRecurrenceCheck)))
|
||||
{
|
||||
return error.ToErrorResult();
|
||||
}
|
||||
|
||||
List<TemplateViewModel> templates = await mediator.Send(new GetAllTemplates(), cancellationToken);
|
||||
var templateIds = templates.Select(t => t.Id).ToHashSet();
|
||||
var missingTemplateIds = items.Select(i => i.TemplateId).Distinct()
|
||||
@@ -782,6 +792,61 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) :
|
||||
PlayoutTemplateItemRequest item) =>
|
||||
(item.LimitToDateRange, item.StartMonth, item.StartDay, item.EndMonth, item.EndDay);
|
||||
|
||||
private static (List<DayOfWeek> DaysOfWeek, List<int> DaysOfMonth, List<int> MonthsOfYear)
|
||||
ToRecurrenceCheck(PlayoutAlternateScheduleItemRequest item) =>
|
||||
(item.DaysOfWeek, item.DaysOfMonth, item.MonthsOfYear);
|
||||
|
||||
private static (List<DayOfWeek> DaysOfWeek, List<int> DaysOfMonth, List<int> MonthsOfYear)
|
||||
ToRecurrenceCheck(PlayoutTemplateItemRequest item) =>
|
||||
(item.DaysOfWeek, item.DaysOfMonth, item.MonthsOfYear);
|
||||
|
||||
// An EXPLICITLY EMPTY recurrence array is rejected; an ABSENT one is not (#880). The distinction is
|
||||
// the whole point: the three sets are conjunctive in AlternateScheduleSelector.GetScheduleForDate, so
|
||||
// an empty one means the item matches NO date and can never apply -- previously stored with a 200 and
|
||||
// silent thereafter, which is the trap this closes. Absence is a different request and means
|
||||
// UNRESTRICTED; the request records normalize it to the All*() sets before the command is built, which
|
||||
// is why this must run on the REQUEST (where null still means absent) and not on the command.
|
||||
//
|
||||
// Rejecting rather than rewriting follows `api.ffmpeg-profile-numeric-bounds`: name the consequence,
|
||||
// do not accept-then-substitute. Only the WRITE side rejects -- the read side still tolerates an empty
|
||||
// set, because a row written before this validation existed must stay readable and editable.
|
||||
//
|
||||
// Every item is checked, INCLUDING the last one. That is deliberate and it is the same choice
|
||||
// ValidateDateRanges already makes: the highest-Index item is the catch-all default, and
|
||||
// ReplacePlayoutAlternateScheduleItemsHandler discards its recurrence fields along with its date range.
|
||||
// Exempting it here would mean restating "which item is the catch-all" in the controller, a second copy
|
||||
// of a rule that lives in the handler and could drift from it. Sending `[]` for an item whose recurrence
|
||||
// is ignored is a client error either way, and omitting the field is always accepted.
|
||||
private static Option<BaseError> ValidateRecurrenceSets(
|
||||
IEnumerable<(List<DayOfWeek> DaysOfWeek, List<int> DaysOfMonth, List<int> MonthsOfYear)> items)
|
||||
{
|
||||
foreach ((List<DayOfWeek> daysOfWeek, List<int> daysOfMonth, List<int> monthsOfYear) in items)
|
||||
{
|
||||
if (daysOfWeek is { Count: 0 })
|
||||
{
|
||||
return Some(
|
||||
BaseError.New(
|
||||
"[DaysOfWeek] must not be empty; an empty set matches no day of the week, so the item would never apply. Omit the field to leave it unrestricted"));
|
||||
}
|
||||
|
||||
if (daysOfMonth is { Count: 0 })
|
||||
{
|
||||
return Some(
|
||||
BaseError.New(
|
||||
"[DaysOfMonth] must not be empty; an empty set matches no day of the month, so the item would never apply. Omit the field to leave it unrestricted"));
|
||||
}
|
||||
|
||||
if (monthsOfYear is { Count: 0 })
|
||||
{
|
||||
return Some(
|
||||
BaseError.New(
|
||||
"[MonthsOfYear] must not be empty; an empty set matches no month, so the item would never apply. Omit the field to leave it unrestricted"));
|
||||
}
|
||||
}
|
||||
|
||||
return Option<BaseError>.None;
|
||||
}
|
||||
|
||||
private static Option<BaseError> ValidateDateRanges(
|
||||
IEnumerable<(bool LimitToDateRange, int StartMonth, int StartDay, int EndMonth, int EndDay)> items)
|
||||
{
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -383,6 +383,38 @@ 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. Exemplars:
|
||||
`ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs` (normalization) and
|
||||
`PlayoutController.ValidateRecurrenceSets` (rejection).
|
||||
|
||||
Two rules that come with it:
|
||||
|
||||
- **Match the permissive value to the read side's, symbol for symbol.** The recurrence fields
|
||||
normalize to `AlternateScheduleSelector.All*()` — the same helpers the selector's null guard and
|
||||
both `Mapper.ProjectToViewModel` overloads use — so "absent" means one thing across read and write.
|
||||
Picking an equivalent-looking literal instead is how the two halves drift apart again.
|
||||
- **Reject on the WRITE side only.** The read side must keep tolerating an empty set: rows written
|
||||
before the validation existed have to stay readable, and editable. This is §3d's "keep the
|
||||
render-time clamp" in another guise — validation is what stops new bad rows, not what repairs old
|
||||
ones.
|
||||
|
||||
## 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,39 @@
|
||||
---
|
||||
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. An equivalent-looking literal is how the two halves drift apart again. 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 16 files including sibling request records already use that form. Rejection lives on the WRITE side ONLY: the read side keeps tolerating an empty set so a row written before the validation stays readable AND editable, which is `api.ffmpeg-profile-numeric-bounds`'' "keep the render-time clamp" in another guise. EVERY item is validated, INCLUDING the highest-Index catch-all whose recurrence `ReplacePlayoutAlternateScheduleItemsHandler` discards — deliberately, because exempting it would restate "which item is the catch-all" in the controller, a second copy of a rule that lives in the handler (`api.put-replace-index-order`) and could drift from it; `ValidateDateRanges` already makes exactly this choice for the date fields it equally discards. Do NOT cite the OpenAPI surface as evidence either way: the spec and the generated SPA client are BYTE-IDENTICAL before and after, because a non-nullable-annotated `List<T>` was ALREADY emitted as nullable — the contract shape did not move, only the behaviour behind it, so a green `update-openapi.sh` diff says nothing about whether this shipped.'
|
||||
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 · read side still tolerates empty so legacy rows stay editable · validate the catch-all too rather than restating which item it is · OpenAPI unchanged because the field was already emitted nullable · paths: `ErsatzTV/Controllers/Api/Requests/PlayoutAlternateScheduleItemRequest.cs`, `ErsatzTV/Controllers/Api/Requests/PlayoutTemplateItemRequest.cs`, `ErsatzTV/Controllers/Api/PlayoutController.cs`, `ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs` · issues: #880, #823, #701'
|
||||
mechanics: 'Pinned in `ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs` by three normalization tests (`Replace{AlternateSchedules,Templates}_Should_Normalize_Absent_Recurrence_To_Unrestricted` and `ReplaceAlternateSchedules_Should_Preserve_An_Explicit_Recurrence_Selection`) and six rejection cases (`Replace{AlternateSchedules,Templates}_Should_Return_422_For_Empty_Recurrence_Without_Dispatch`, one `[TestCase]` per field). BOTH halves were mutation-proved by EXECUTION, separately, against the real predecessor clause rather than a hand-written mutant. (1) Reverting the normalization ALONE — the three `?? All*()` clauses back to `?? []` in both request records, leaving the nullable annotations and the rejection in place — reddens EXACTLY the three normalization tests (110 tests, 3 failed). (2) Removing the two `ValidateRecurrenceSets` call sites ALONE, leaving normalization intact, reddens EXACTLY the six rejection cases (110 tests, 6 failed). The two mutations redden DISJOINT sets, which is what shows the guards are not masking each other. The rejection cases assert `problem.Detail` CONTAINS both the bracketed field name and its consequence phrase, not merely `Status == 422`: the same request shape is also reachable by the date-range validator, so a status-only assertion would pass on an unrelated rejection. Each case leaves the OTHER two arrays ABSENT so it can only be rejected by the field it names. Anti-vacuity is the `DidNotReceive()` pair already used by the sibling date-range tests — a 422 that still dispatched would be a different bug.'
|
||||
---
|
||||
|
||||
**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.
|
||||
|
||||
**Prod was measured before choosing to reject rather than grandfather** (2026-08-30, jazz
|
||||
`/config/ersatztv.sqlite3`): `ProgramScheduleAlternate` and `PlayoutTemplate` both hold ZERO rows, so
|
||||
no stored `[]` exists to be bricked by the new 422 and `api.ffmpeg-profile-numeric-bounds`' compare-
|
||||
against-the-stored-value exemption was not needed. That exemption is the right tool if this rule is
|
||||
ever extended to a populated table, and the reason it was skipped is the measurement, not the
|
||||
argument — record a new measurement rather than inheriting this one.
|
||||
|
||||
**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, not an
|
||||
oversight; 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
Reference in New Issue
Block a user