test(#259): id-based reconcile matrix + docs (api-conventions §7c, decisions)

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) <noreply@anthropic.com>
2026-07-11 21:53:50 +02:00
parent 02a493e95e
commit 162b334e5d
7 changed files with 324 additions and 2 deletions
@@ -190,6 +190,7 @@ public class ReplaceProgramScheduleItemsHandlerConcurrencyTests

    private static ReplaceProgramScheduleItem MakeItem(int index, PlayoutMode mode, string searchQuery) =>
        new(
+            null,
            index,
            StartType.Dynamic,
            StartTime: null,
@@ -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<ReplaceProgramScheduleItem>
+        {
+            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<ProgramScheduleItem> 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<ReplaceProgramScheduleItem>
+        {
+            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<ProgramScheduleItem> 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<PlayoutScheduleItemFillGroupIndex> groups = await ctx.Set<PlayoutScheduleItemFillGroupIndex>()
+                .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<ReplaceProgramScheduleItem>
+        {
+            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<ProgramScheduleItem> 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<ReplaceProgramScheduleItem>
+        {
+            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<ProgramScheduleItem> 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<PlayoutScheduleItemFillGroupIndex> groups = await ctx.Set<PlayoutScheduleItemFillGroupIndex>()
+                .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<ReplaceProgramScheduleItem>
+        {
+            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<ProgramScheduleItem> 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<ReplaceProgramScheduleItem> { 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<ProgramScheduleItem> 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<PlayoutScheduleItemFillGroupIndex>()
+                .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<ReplaceProgramScheduleItem> { MakeItem(0, PlayoutMode.One, "x", id: 999999) };
+        Either<BaseError, IEnumerable<ProgramScheduleItemViewModel>> 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<ReplaceProgramScheduleItem>
+        {
+            MakeItem(0, PlayoutMode.One, "a", id: idA),
+            MakeItem(1, PlayoutMode.One, "a-again", id: idA)
+        };
+        Either<BaseError, IEnumerable<ProgramScheduleItemViewModel>> 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<ReplaceProgramScheduleItem> { MakeItem(0, PlayoutMode.One, "x", id: 999999) };
+        Either<BaseError, IEnumerable<ProgramScheduleItemViewModel>> 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<BaseError, IEnumerable<ProgramScheduleItemViewModel>> 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<int> 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<int> 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,
@@ -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,
@@ -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,
@@ -403,6 +403,7 @@ public class ScheduleControllerTests

    private static ScheduleItemRequest MakeItemRequest(PlayoutMode playoutMode) =>
        new(
+            Id: null,
            StartType.Dynamic,
            StartTime: null,
            FixedStartTimeBehavior: null,
@@ -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
@@ -890,3 +890,27 @@ Also removed the now-dead ersatztv#25 razor-Sonar `<NoWarn>S6966;S3267;…</NoWa
 **Rollback.** The tag `blazor-final` was cut on the pre-removal `main` commit as the first step (see the
 2026-07-11 "Pre-removal Blazor rollback tag `blazor-final` (#205)" entry above for the exact command +
 restore path). Not a `v*` tag → no prod release build.
+
+## 2026-07-11 — Stable child identity for schedule-item replace (#259, split from #252/#253)
+
+`PUT /api/schedules/{id}/items` now reconciles by an optional round-tripped child id, not by array
+position, so an item's persisted fill-group/shuffle state (`PlayoutScheduleItemFillGroupIndex`, FK
+`OnDelete(Cascade)`) follows the logical item across reorders/inserts instead of being inherited by
+whatever previously held its new slot. Contract + rules in **api-conventions §7c**. Key decisions:
+
+- **`ScheduleItemRequest.Id` (`int?`)**: null/absent/`0` ⇒ new item (controller normalizes `0`→null so the
+  handler is two-state). Any id present ⇒ id-based reconcile; a fully id-less payload keeps the verbatim
+  positional fallback (legacy; retires with the §7a Phase-2 `If-Match`→428 flip).
+- **Unknown or duplicate id ⇒ 422, nothing persisted**; the guards live in the handler **after** §7a
+  `CheckVersion`, so **412 precedes 422** — a client that is both version-stale and id-stale gets the reload
+  signal, not a payload-bug signal. Rationale for reject-not-insert on an unknown id: under Phase-1
+  force-write a stale id is a live lost-update signal, so silently inserting-as-new would duplicate the item
+  and return a different id than the client sent (the exact class §7a exists to surface). This is also the
+  correct #197 posture — never honor an unrecognized identifier.
+- **Scope = schedule items only.** Blocks/templates/deco-templates/playlists stay positional: their children
+  are stateless config rows (no FK'd state to misattribute; #3/#4 have no GET child id). Child ids are added
+  only where a child row anchors server-side state; the contract can be retrofitted per-endpoint later
+  (field stays optional) — so this is not #197 ossification pressure.
+- **TPT subtype change at a matched id** stays delete+insert (EF can't retype in place); state resets and a
+  new id is returned, so the SPA must re-seed item state from the PUT response (a stale id on a second save
+  now 422s).