From 162b334e5da0da083bfb73754c98aee333c8b193 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 21:53:50 +0200 Subject: [PATCH] =?UTF-8?q?test(#259):=20id-based=20reconcile=20matrix=20+?= =?UTF-8?q?=20docs=20(api-conventions=20=C2=A77c,=20decisions)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add the id-based reconcile tests to ReplaceProgramScheduleItemsReconcileTests: reorder moves state with the logical item (the non-vacuous core — proven to fail under forced-positional), insert-in-middle, delete-unreferenced, unknown-id→422, duplicate-id→422, and stale-version+unknown-id→412 (412 precedes 422, §7c). The GET→map→PUT lossless round-trip now round-trips r.Id so it exercises id-mode. Threads the new int? Id through all command/wire construction sites in tests. Docs: api-conventions §7c (stable child identity + the deliberate #2-#5 positional asymmetry) and a decisions.md entry. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...ramScheduleItemsHandlerConcurrencyTests.cs | 1 + ...placeProgramScheduleItemsReconcileTests.cs | 264 +++++++++++++++++- .../ScheduleItemResponseRoundTripTests.cs | 2 + .../ScheduleItemWriteProjectionTests.cs | 2 +- .../Controllers/ScheduleControllerTests.cs | 1 + docs/api-conventions.md | 32 +++ docs/decisions.md | 24 ++ 7 files changed, 324 insertions(+), 2 deletions(-) diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs index d665cdf3b..bd6625a85 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs @@ -190,6 +190,7 @@ public class ReplaceProgramScheduleItemsHandlerConcurrencyTests private static ReplaceProgramScheduleItem MakeItem(int index, PlayoutMode mode, string searchQuery) => new( + null, index, StartType.Dynamic, StartTime: null, diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs index d73eb0698..1a0ba42f1 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsReconcileTests.cs @@ -3,6 +3,7 @@ using ErsatzTV.Application; using ErsatzTV.Application.ProgramSchedules; using ErsatzTV.Core; using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Errors; using ErsatzTV.Core.Scheduling; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; @@ -10,6 +11,7 @@ using LanguageExt; using Microsoft.EntityFrameworkCore; using NUnit.Framework; using Shouldly; +using static LanguageExt.Prelude; namespace ErsatzTV.Tests.Application.ProgramSchedules; @@ -234,6 +236,265 @@ public class ReplaceProgramScheduleItemsReconcileTests } } + [Test] + public async Task Reorder_ById_Should_Move_State_With_The_Logical_Item_Not_The_Slot() + { + // The non-vacuous core of #259: two SAME-typed items are swapped while round-tripping their ids. + // In id-mode each item is matched by id, so idA MOVES to index 1 and keeps its own fill-group state. + // In the old positional reconcile idA would have been reused in place at index 0 (its slot), silently + // pairing item "a"'s enumerator state with item "b"'s content — so `idA.Index == 1` is the assertion + // that fails under positional and passes under id-based reconcile. + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var seedItems = new List + { + MakeItem(0, PlayoutMode.Multiple, "a") with { FillWithGroupMode = FillWithGroupMode.FillWithShuffledGroups }, + MakeItem(1, PlayoutMode.Multiple, "b") with { FillWithGroupMode = FillWithGroupMode.FillWithShuffledGroups } + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, seedItems), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + int idA, idB; + await using (TvContext ctx = _db.CreateContext()) + { + List persisted = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).OrderBy(i => i.Index).ToListAsync(); + idA = persisted[0].Id; + idB = persisted[1].Id; + AddFillGroup(ctx, idA, seed: 100); + AddFillGroup(ctx, idB, seed: 200); + await ctx.SaveChangesAsync(); + } + + // Act: swap the two items, round-tripping their ids. + var swapped = new List + { + MakeItem(0, PlayoutMode.Multiple, "b", id: idB) with { FillWithGroupMode = FillWithGroupMode.FillWithShuffledGroups }, + MakeItem(1, PlayoutMode.Multiple, "a", id: idA) with { FillWithGroupMode = FillWithGroupMode.FillWithShuffledGroups } + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, swapped), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + List after = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).ToListAsync(); + after.Count.ShouldBe(2); + + // identity mapping: each id moved to its new position (positional would have kept idA at 0) + after.Single(i => i.Id == idA).Index.ShouldBe(1); + after.Single(i => i.Id == idB).Index.ShouldBe(0); + + // each item's fill-group state followed its own id — no misattribution, no cascade + List groups = await ctx.Set() + .Include(x => x.EnumeratorState).ToListAsync(); + groups.Single(g => g.ProgramScheduleItemId == idA).EnumeratorState.Seed.ShouldBe(100); + groups.Single(g => g.ProgramScheduleItemId == idB).EnumeratorState.Seed.ShouldBe(200); + } + } + + [Test] + public async Task Insert_ById_In_Middle_Should_Keep_Existing_Ids_And_State() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var seed = new List + { + MakeItem(0, PlayoutMode.One, "a"), + MakeItem(1, PlayoutMode.One, "b") + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, seed), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + int idA, idB; + await using (TvContext ctx = _db.CreateContext()) + { + List p = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).OrderBy(i => i.Index).ToListAsync(); + idA = p[0].Id; + idB = p[1].Id; + AddFillGroup(ctx, idA, seed: 111); + AddFillGroup(ctx, idB, seed: 222); + await ctx.SaveChangesAsync(); + } + + // insert a brand-new (id-less) item between the two existing ones + var withInsert = new List + { + MakeItem(0, PlayoutMode.One, "a", id: idA), + MakeItem(1, PlayoutMode.One, "new"), + MakeItem(2, PlayoutMode.One, "b", id: idB) + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, withInsert), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + List after = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).ToListAsync(); + after.Count.ShouldBe(3); + after.Single(i => i.Id == idA).Index.ShouldBe(0); + after.Single(i => i.Id == idB).Index.ShouldBe(2); // pushed down by the inserted middle item + + List groups = await ctx.Set() + .Include(x => x.EnumeratorState).ToListAsync(); + groups.Single(g => g.ProgramScheduleItemId == idA).EnumeratorState.Seed.ShouldBe(111); + groups.Single(g => g.ProgramScheduleItemId == idB).EnumeratorState.Seed.ShouldBe(222); + } + } + + [Test] + public async Task Delete_ById_Should_Remove_Unreferenced_And_Keep_Survivor_State() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + var seed = new List + { + MakeItem(0, PlayoutMode.One, "a"), + MakeItem(1, PlayoutMode.One, "b") + }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, seed), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + int idA; + await using (TvContext ctx = _db.CreateContext()) + { + List p = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).OrderBy(i => i.Index).ToListAsync(); + idA = p[0].Id; + AddFillGroup(ctx, idA, seed: 333); + await ctx.SaveChangesAsync(); + } + + // PUT only item A (by id) — B is no longer referenced and must be deleted + var onlyA = new List { MakeItem(0, PlayoutMode.One, "a", id: idA) }; + (await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, onlyA), CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + await using (TvContext ctx = _db.CreateContext()) + { + List after = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).ToListAsync(); + after.Count.ShouldBe(1); + after.Single().Id.ShouldBe(idA); + + // survivor kept its id and its fill-group state (no cascade on the item that stayed) + PlayoutScheduleItemFillGroupIndex survivor = await ctx.Set() + .Include(x => x.EnumeratorState).SingleAsync(g => g.ProgramScheduleItemId == idA); + survivor.EnumeratorState.Seed.ShouldBe(333); + } + } + + [Test] + public async Task Unknown_Id_Should_Return_422_And_Persist_Nothing() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + (await handler.Handle( + new ReplaceProgramScheduleItems(scheduleId, [MakeItem(0, PlayoutMode.One, "a")]), + CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + (int idA, int versionBefore) = await ReadSingle(scheduleId); + + // a request id that doesn't belong to this schedule is rejected (stale/foreign identity) + var bad = new List { MakeItem(0, PlayoutMode.One, "x", id: 999999) }; + Either> result = + await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, bad), CancellationToken.None); + + BaseError error = LeftOrThrow(result); + error.GetType().ShouldBe(typeof(BaseError)); // exactly BaseError (→422), not PreconditionFailedError (→412) + error.ToString().ShouldContain("999999"); + await AssertUnchanged(scheduleId, idA, versionBefore); + } + + [Test] + public async Task Duplicate_Id_Should_Return_422_And_Persist_Nothing() + { + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + (await handler.Handle( + new ReplaceProgramScheduleItems(scheduleId, [MakeItem(0, PlayoutMode.One, "a")]), + CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + (int idA, int versionBefore) = await ReadSingle(scheduleId); + + // the same id twice in one payload would map two request items onto one row — reject + var dup = new List + { + MakeItem(0, PlayoutMode.One, "a", id: idA), + MakeItem(1, PlayoutMode.One, "a-again", id: idA) + }; + Either> result = + await handler.Handle(new ReplaceProgramScheduleItems(scheduleId, dup), CancellationToken.None); + + BaseError error = LeftOrThrow(result); + error.GetType().ShouldBe(typeof(BaseError)); + error.ToString().ShouldContain(idA.ToString()); + await AssertUnchanged(scheduleId, idA, versionBefore); + } + + [Test] + public async Task Stale_Version_And_Unknown_Id_Should_Return_412_Not_422() + { + // Ordering ruling (§7c): the concurrency pre-check runs before the id guards, so a client that is + // BOTH version-stale and id-stale gets 412 (reload signal), never 422 (which reads as a payload bug). + int scheduleId = await SeedSchedule(); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + (await handler.Handle( + new ReplaceProgramScheduleItems(scheduleId, [MakeItem(0, PlayoutMode.One, "a")]), + CancellationToken.None)) + .IsRight.ShouldBeTrue(); + + (int idA, int versionBefore) = await ReadSingle(scheduleId); + + var bad = new List { MakeItem(0, PlayoutMode.One, "x", id: 999999) }; + Either> result = await handler.Handle( + new ReplaceProgramScheduleItems(scheduleId, bad, ExpectedVersion: Some(versionBefore + 1)), + CancellationToken.None); + + LeftOrThrow(result).GetType().ShouldBe(typeof(PreconditionFailedError)); + await AssertUnchanged(scheduleId, idA, versionBefore); + } + + private static void AddFillGroup(TvContext ctx, int itemId, int seed) => + ctx.Add(new PlayoutScheduleItemFillGroupIndex + { + PlayoutId = 1, + ProgramScheduleItemId = itemId, + EnumeratorState = new CollectionEnumeratorState { Seed = seed, Index = 0 } + }); + + private static BaseError LeftOrThrow(Either> result) => + result.Match(Left: e => e, Right: _ => throw new ShouldAssertException("expected a Left error")); + + // read the single seeded item's id and the schedule's current version + private async Task<(int Id, int Version)> ReadSingle(int scheduleId) + { + await using TvContext ctx = _db.CreateContext(); + int id = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).Select(i => i.Id).SingleAsync(); + int version = await ctx.ProgramSchedules + .Where(s => s.Id == scheduleId).Select(s => s.Version).SingleAsync(); + return (id, version); + } + + // a rejected replace must leave the schedule exactly as it was (no partial write, no version bump) + private async Task AssertUnchanged(int scheduleId, int expectedItemId, int expectedVersion) + { + await using TvContext ctx = _db.CreateContext(); + List ids = await ctx.ProgramScheduleItems + .Where(i => i.ProgramScheduleId == scheduleId).Select(i => i.Id).ToListAsync(); + ids.ShouldBe([expectedItemId]); + int version = await ctx.ProgramSchedules + .Where(s => s.Id == scheduleId).Select(s => s.Version).SingleAsync(); + version.ShouldBe(expectedVersion); + } + private async Task SeedSchedule() { await using TvContext ctx = _db.CreateContext(); @@ -249,8 +510,9 @@ public class ReplaceProgramScheduleItemsReconcileTests return schedule.Id; } - private static ReplaceProgramScheduleItem MakeItem(int index, PlayoutMode mode, string searchQuery) => + private static ReplaceProgramScheduleItem MakeItem(int index, PlayoutMode mode, string searchQuery, int? id = null) => new( + id, index, StartType.Dynamic, StartTime: null, diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs index edb5c9aad..0d34bd88d 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs @@ -256,6 +256,7 @@ public class ScheduleItemResponseRoundTripTests private static ReplaceProgramScheduleItem MakeReplace(int index, PlayoutMode playoutMode, CollectionType collectionType) => new( + null, index, StartType.Dynamic, StartTime: null, @@ -299,6 +300,7 @@ public class ScheduleItemResponseRoundTripTests // response DTO (the SPA does this same field-for-field copy in TypeScript before a PUT). private static ReplaceProgramScheduleItem ToReplaceCommand(ScheduleItemResponseModel r, int index) => new( + r.Id, index, r.StartType, r.StartTime, diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs index 6602fc198..bf8fbb2d4 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs @@ -139,7 +139,7 @@ public class ScheduleItemWriteProjectionTests private static ReplaceProgramScheduleItem ItemWithGraphics(int index) => new( - index, StartType.Dynamic, StartTime: null, FixedStartTimeBehavior: null, PlayoutMode.One, + null, index, StartType.Dynamic, StartTime: null, FixedStartTimeBehavior: null, PlayoutMode.One, CollectionType.Collection, CollectionId: 1, MultiCollectionId: null, SmartCollectionId: null, RerunCollectionId: null, MediaItemId: null, PlaylistId: null, SearchTitle: null, SearchQuery: null, PlaybackOrder: PlaybackOrder.Shuffle, MarathonGroupBy: MarathonGroupBy.None, diff --git a/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs b/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs index a0494db99..d4362da7c 100644 --- a/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs @@ -403,6 +403,7 @@ public class ScheduleControllerTests private static ScheduleItemRequest MakeItemRequest(PlayoutMode playoutMode) => new( + Id: null, StartType.Dynamic, StartTime: null, FixedStartTimeBehavior: null, diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 3ed6b2514..fb0543a37 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -424,6 +424,38 @@ making it explicit is optional cleanup, not required. Config handlers that commi `IConfigElementRepository.Upsert` calls are a distinct partial-commit-under-cancellation case not covered by this rule (tracked separately). +## 7c. Stable child identity in replace lists (schedule items) + +§7 indexes replace-list children by **array order**, which is the reconcile *key* for most replace PUTs. +That is correct only when a child row is pure config: reordering merely re-numbers otherwise-interchangeable +rows. **Schedule items are the exception** (issue #259): a schedule item anchors persisted runtime state — +`PlayoutScheduleItemFillGroupIndex` (fill-group / shuffle enumerator progression) FKs the item row with +`OnDelete(Cascade)`. Reconciling those by position makes a **moved** item inherit the state of whatever item +previously occupied its new slot. So `PUT /api/schedules/{id}/items` carries a stable child identity: + +- `ScheduleItemRequest.Id` (`int?`) round-trips each existing item's server id (as returned by the items GET). + **null / absent / 0 ⇒ a new item** (the controller normalizes `0`→null so the handler contract is + two-state). Never fabricate an id. +- When **any** request item carries an id, `ReplaceProgramScheduleItemsHandler` reconciles **by id**: matched + same-subtype rows are updated in place (keeping the id, so the fill-group index never cascades and follows + the logical item across reorders/inserts); a matched row whose TPT subtype changed is delete+insert (state + resets, a **new id is returned** — clients must re-sync from the PUT response); unreferenced existing rows are + deleted; id-less request items are inserted. `Index` is still array-position (ordering is a separate axis + from identity). +- **A fully id-less payload falls back to the verbatim positional reconcile** (legacy clients). This preserves + today's misattribution-on-reorder for such payloads — it is temporary and retires together with the §7a + Phase-2 `If-Match`→428 flip. +- **Guards run inside the handler, after the §7a `CheckVersion`** (so a client that is both version-stale and + id-stale gets **412**, the reload signal, not 422): a duplicate id in one payload → **422**; an id not + belonging to this schedule → **422** (under Phase-1 force-write a stale id is a live lost-update signal, not + a new item — reject rather than silently duplicate). Both persist nothing. + +**Deliberate asymmetry**: the other positional replace handlers (blocks #2, templates #3, deco-templates #4, +playlists #5) do **not** carry a child id — their children are stateless config rows where positional churn is +unobservable (#3/#4 don't even emit a child id on GET). Child ids are added only where a child row anchors +server-side state; positional replace stays the default. A per-endpoint child-id contract can be retrofitted +later without breaking anything (the field stays optional). + ## 8. Known API warts (don't "fix" without discussion — they're deliberate synthesized rows) `GET /api/blocks` and `GET /api/templates` (via `GetAllBlocksHandler` / `GetAllTemplatesHandler` in diff --git a/docs/decisions.md b/docs/decisions.md index d9f37517d..9c4e4017c 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -890,3 +890,27 @@ Also removed the now-dead ersatztv#25 razor-Sonar `S6966;S3267;…