From 02a493e95edb679a0fe4cff7c38a458adcd60ace Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 21:43:52 +0200 Subject: [PATCH] feat(#259): id-based reconcile for schedule-items replace (backend + DTO) Add optional `int? Id` to ScheduleItemRequest/ReplaceProgramScheduleItem so a client can round-trip each existing item's server id. When ids are present, ReplaceProgramScheduleItemsHandler reconciles by id (not 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 occupied its new slot (#259, split from #252/#253). A fully id-less payload keeps the verbatim positional fallback. Guards (inside PersistItems, after CheckVersion so 412 precedes 422): duplicate id -> 422; id not in this schedule -> 422 (a stale id under Phase-1 force-write is a live lost-update signal, not a new item). Index stays array-position derived. Regenerated v1.json + TS client. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/ReplaceProgramScheduleItems.cs | 1 + .../ReplaceProgramScheduleItemsHandler.cs | 143 ++++++++++++++---- .../ProgramScheduleHandlerTests.cs | 1 + .../ScheduleItemTptIntegrationTests.cs | 1 + .../Api/Requests/ScheduleItemRequest.cs | 10 ++ ErsatzTV/wwwroot/openapi/v1.json | 8 + web/src/api/generated/v1.d.ts | 1 + 7 files changed, 133 insertions(+), 32 deletions(-) diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs index 9a9d4568c..3d75257f2 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs @@ -5,6 +5,7 @@ using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Application.ProgramSchedules; public record ReplaceProgramScheduleItem( + int? Id, int Index, StartType StartType, TimeSpan? StartTime, diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs index 43a626177..02e3fba8f 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs @@ -48,16 +48,22 @@ public class ReplaceProgramScheduleItemsHandler( ProgramSchedule programSchedule, CancellationToken cancellationToken) { - // Positional in-place reconcile (rather than delete-and-reinsert): a schedule item owns the - // persisted fill-group/shuffle enumerator state via PlayoutScheduleItemFillGroupIndex, whose + // In-place reconcile (rather than delete-and-reinsert): a schedule item owns persisted + // fill-group/shuffle enumerator state via PlayoutScheduleItemFillGroupIndex, whose // ProgramScheduleItemId FK is OnDelete(Cascade). Removing and re-inserting every item — as the // original handler did on every save, including a no-op PUT-back — cascade-deleted that state for - // all playouts using the schedule (#252). Reusing the existing item row for the same-typed slot - // keeps its id, so the cascade never fires and progression survives. The request DTO carries no - // stable item id, so position is the only key available here; true content-aware stable identity - // is deferred to the shared concurrency/round-trip contract in #253. + // all playouts using the schedule (#252). Reusing the existing item row keeps its id, so the cascade + // never fires and progression survives. + // + // Two reconcile modes (#259): + // * id-based (preferred) — when the client round-trips each existing item's server id, map request + // items to existing rows BY id, so per-child state follows the LOGICAL item across reorders and + // mid-list inserts rather than staying with whatever previously occupied a slot. + // * positional (legacy fallback) — a fully id-less payload keeps the original by-array-position + // reconcile. This preserves today's misattribution on legacy reorders; it is temporary and + // retires with the §7a Phase-2 If-Match flip. + // Index (ordering) is always derived from array position; identity (id) is a separate axis. var orderedItems = request.Items.OrderBy(i => i.Index).ToList(); - List existingItems = programSchedule.Items.OrderBy(i => i.Index).ToList(); // load the watermark/graphics join rows for the existing items so they can be rebuilt in place await dbContext.Entry(programSchedule) @@ -67,39 +73,112 @@ public class ReplaceProgramScheduleItemsHandler( .Include(i => i.ProgramScheduleItemGraphicsElements) .LoadAsync(cancellationToken); - int commonCount = Math.Min(existingItems.Count, orderedItems.Count); - for (var i = 0; i < commonCount; i++) + List existingItems = programSchedule.Items.ToList(); + List requestIds = orderedItems.Where(i => i.Id.HasValue).Select(i => i.Id.Value).ToList(); + + if (requestIds.Count == 0) { - ProgramScheduleItem existing = existingItems[i]; - ProgramScheduleItem built = BuildItem(programSchedule, i, orderedItems[i]); - if (existing.GetType() == built.GetType()) + // ---- positional fallback (verbatim pre-#259 behavior) ---- + List orderedExisting = existingItems.OrderBy(i => i.Index).ToList(); + int commonCount = Math.Min(orderedExisting.Count, orderedItems.Count); + for (var i = 0; i < commonCount; i++) { - // same TPT subtype: copy all scalar values in place (BuildItem is the single source of - // item construction, so no field is silently dropped) and rebuild the join rows, keeping - // the item's id — and with it the fill-group index that would otherwise cascade away. - built.Id = existing.Id; - dbContext.Entry(existing).CurrentValues.SetValues(built); - RebuildChildren(existing, orderedItems[i]); + ProgramScheduleItem existing = orderedExisting[i]; + ProgramScheduleItem built = BuildItem(programSchedule, i, orderedItems[i]); + if (existing.GetType() == built.GetType()) + { + built.Id = existing.Id; + dbContext.Entry(existing).CurrentValues.SetValues(built); + RebuildChildren(existing, orderedItems[i]); + } + else + { + dbContext.Remove(existing); + programSchedule.Items.Add(built); + } } - else + + for (int i = commonCount; i < orderedExisting.Count; i++) { - // EF can't change a TPT row's type in place; this slot must be replaced (its fill-group - // index resets, which is acceptable — the item fundamentally changed). - dbContext.Remove(existing); - programSchedule.Items.Add(built); + dbContext.Remove(orderedExisting[i]); + } + + for (int i = commonCount; i < orderedItems.Count; i++) + { + programSchedule.Items.Add(BuildItem(programSchedule, i, orderedItems[i])); } } - - // remove surplus existing items - for (int i = commonCount; i < existingItems.Count; i++) + else { - dbContext.Remove(existingItems[i]); - } + // ---- id-based reconcile ---- + // Guards run HERE (inside PersistItems, after CheckVersion in Handle) so a client that is BOTH + // version-stale and id-stale gets 412 (reload signal) — not 422, which reads as a payload bug + // (§7c). Both guards persist nothing (plain BaseError → 422). - // add surplus incoming items - for (int i = commonCount; i < orderedItems.Count; i++) - { - programSchedule.Items.Add(BuildItem(programSchedule, i, orderedItems[i])); + // 7b: a duplicated id would map two request items onto one row (SetValues twice, last-writer-wins, + // one item's config silently lost). Always a client bug. + int? duplicateId = requestIds.GroupBy(id => id).Where(g => g.Count() > 1).Select(g => (int?)g.Key) + .FirstOrDefault(); + if (duplicateId.HasValue) + { + return new BaseError($"Schedule item id {duplicateId.Value} appears more than once in the request."); + } + + // 7a: an id not belonging to THIS schedule is a stale/foreign identity. Under Phase-1 (a missing + // If-Match force-writes) it is a live lost-update signal; inserting-as-new would silently duplicate + // the item and return a different id than the client sent. Reject loudly rather than mask it. + var existingIds = existingItems.Select(e => e.Id).ToHashSet(); + foreach (int id in requestIds) + { + if (!existingIds.Contains(id)) + { + return new BaseError($"Schedule item id {id} does not belong to this schedule."); + } + } + + // delete pass: existing rows the request no longer references (their fill-group state cascades — + // correct, the logical item is gone) + var referencedIds = requestIds.ToHashSet(); + foreach (ProgramScheduleItem existing in existingItems) + { + if (!referencedIds.Contains(existing.Id)) + { + dbContext.Remove(existing); + } + } + + // match/insert pass in array order (Index = i) + var existingById = existingItems.ToDictionary(e => e.Id); + for (var i = 0; i < orderedItems.Count; i++) + { + ReplaceProgramScheduleItem requestItem = orderedItems[i]; + ProgramScheduleItem built = BuildItem(programSchedule, i, requestItem); + if (requestItem.Id.HasValue && existingById.TryGetValue(requestItem.Id.Value, out ProgramScheduleItem existing)) + { + if (existing.GetType() == built.GetType()) + { + // same TPT subtype: copy all scalars in place (BuildItem is the single source of item + // construction, so no field is dropped) and rebuild join rows, keeping the id — and + // with it the fill-group index that would otherwise cascade away. State follows the + // logical item regardless of its new position. + built.Id = existing.Id; + dbContext.Entry(existing).CurrentValues.SetValues(built); + RebuildChildren(existing, requestItem); + } + else + { + // EF can't retype a TPT row in place; replace it. Fill-group state resets and the + // response returns a NEW id (the item fundamentally changed — clients re-sync from it). + dbContext.Remove(existing); + programSchedule.Items.Add(built); + } + } + else + { + // id-less request item → new (unknown ids were already rejected above) + programSchedule.Items.Add(built); + } + } } // Unconditional bump: this handler frequently saves with only CHILD changes and no root-scalar diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ProgramScheduleHandlerTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ProgramScheduleHandlerTests.cs index f9224b8f6..277151938 100644 --- a/ErsatzTV.Tests/Application/ProgramSchedules/ProgramScheduleHandlerTests.cs +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ProgramScheduleHandlerTests.cs @@ -172,6 +172,7 @@ public class ProgramScheduleHandlerTests { AddProgramScheduleItem add = MakeAdd(1, CollectionType.SearchQuery); return new ReplaceProgramScheduleItem( + null, index, add.StartType, add.StartTime, diff --git a/ErsatzTV.Tests/Integration/ScheduleItemTptIntegrationTests.cs b/ErsatzTV.Tests/Integration/ScheduleItemTptIntegrationTests.cs index afd7213c2..eea4e1e3b 100644 --- a/ErsatzTV.Tests/Integration/ScheduleItemTptIntegrationTests.cs +++ b/ErsatzTV.Tests/Integration/ScheduleItemTptIntegrationTests.cs @@ -143,6 +143,7 @@ public class ScheduleItemTptIntegrationTests { AddProgramScheduleItem add = MakeAdd(1, playoutMode); return new ReplaceProgramScheduleItem( + null, index, add.StartType, add.StartTime, diff --git a/ErsatzTV/Controllers/Api/Requests/ScheduleItemRequest.cs b/ErsatzTV/Controllers/Api/Requests/ScheduleItemRequest.cs index 9a3794106..c1e22c54f 100644 --- a/ErsatzTV/Controllers/Api/Requests/ScheduleItemRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ScheduleItemRequest.cs @@ -4,7 +4,14 @@ using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Controllers.Api.Requests; +/// +/// Server-assigned identity of an existing schedule item, as returned by GET /api/schedules/{id}/items. +/// Omit (or send null / 0) for a new item. On a replace, the item carrying this id keeps its persisted +/// fill-group/shuffle progression even when moved to a different position. Unknown or duplicated ids are +/// rejected with 422 — never fabricate an id. +/// public record ScheduleItemRequest( + int? Id, StartType StartType, TimeSpan? StartTime, FixedStartTimeBehavior? FixedStartTimeBehavior, @@ -86,6 +93,9 @@ public record ScheduleItemRequest( public ReplaceProgramScheduleItem ToReplaceCommand(int index) => new( + // normalize the two-state client value (null / 0 / positive) to the handler's null-or-real + // contract so 0-defaulting clients read as "new item" rather than "existing item 0" + Id is > 0 ? Id : null, index, StartType, StartTime, diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 89b9f5d21..3b4d11c46 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -24363,6 +24363,7 @@ }, "ScheduleItemRequest": { "required": [ + "id", "startType", "startTime", "fixedStartTimeBehavior", @@ -24403,6 +24404,13 @@ ], "type": "object", "properties": { + "id": { + "type": [ + "null", + "integer" + ], + "format": "int32" + }, "startType": { "$ref": "#/components/schemas/StartType" }, diff --git a/web/src/api/generated/v1.d.ts b/web/src/api/generated/v1.d.ts index 2a1ed580b..d51908279 100644 --- a/web/src/api/generated/v1.d.ts +++ b/web/src/api/generated/v1.d.ts @@ -1279,6 +1279,7 @@ export interface components { "deepScan"?: boolean; }; "ScheduleItemRequest": { + "id": null | number; "startType": components["schemas"]["StartType"]; "startTime": null | string; "fixedStartTimeBehavior": null | components["schemas"]["FixedStartTimeBehavior"];