From 5c9f04fdec53a49ef276864fb41b21d7fba0a136 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:36:25 +0200 Subject: [PATCH] feat(#253 PR2): optimistic-concurrency on schedule-items aggregate Wire the frozen #253 ETag/If-Match/412 recipe onto ProgramSchedule / schedule-items, keeping the PR#258 positional in-place reconcile intact. Backend: - ReplaceProgramScheduleItems command gains Option ExpectedVersion; ReplaceScheduleItemsRequest.ToCommand threads it. - Handler: standalone CheckVersion Either AFTER validation (so 412 isn't flattened to 422), unconditional Version++ before save, guarded save via SaveChangesWithConcurrencyGuard, and 412 propagated without running the post-save reload/enqueue. - ProgramScheduleViewModel + Mapper carry Version. - ScheduleController: GET /items emits ETag; PUT /items parses If-Match (malformed -> 400), threads ExpectedVersion, re-queries for the new ETag, and advertises 400/412. - Sibling config-writers (Add/Delete item, Update schedule) bump Version. Frontend: - schedules.ts: getScheduleItemsWithMeta + replaceScheduleItems(ifMatch) returning ResponseWithMeta. - SchedulesScreen: etagRef threaded through the #242 dirty-guard (set from load + every successful save); 412 opens a conflict ConfirmDialog whose Reload discards the draft and re-runs loadItems. Tests: handler concurrency suite (stale->412 no mutation + fill-group state untouched, match/absent success+bump, no-op still bumps, racing save->412); controller ETag/If-Match/412 cases; SchedulesScreen 412-conflict-dialog test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/AddProgramScheduleItemHandler.cs | 3 + .../DeleteProgramScheduleItemHandler.cs | 4 + .../Commands/ReplaceProgramScheduleItems.cs | 5 +- .../ReplaceProgramScheduleItemsHandler.cs | 29 ++- .../Commands/UpdateProgramScheduleHandler.cs | 3 + .../ProgramSchedules/Mapper.cs | 3 +- .../ProgramScheduleViewModel.cs | 3 +- .../Queries/GetAllProgramSchedulesHandler.cs | 3 +- ...ramScheduleItemsHandlerConcurrencyTests.cs | 231 ++++++++++++++++++ .../Controllers/PlayoutControllerTests.cs | 2 +- .../Controllers/ScheduleControllerTests.cs | 96 +++++++- .../Requests/ReplaceScheduleItemsRequest.cs | 5 +- .../Controllers/Api/ScheduleController.cs | 43 +++- web/src/api/schedules.ts | 26 +- web/src/screens/SchedulesScreen.test.tsx | 30 +++ web/src/screens/SchedulesScreen.tsx | 50 +++- 16 files changed, 506 insertions(+), 30 deletions(-) create mode 100644 ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs index 1ec7ceb71..1dc9e6172 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs @@ -52,6 +52,9 @@ public class AddProgramScheduleItemHandler : ProgramScheduleItemCommandBase, ProgramScheduleItem item = BuildItem(programSchedule, nextIndex, request); programSchedule.Items.Add(item); + // bump the optimistic-concurrency token so this config edit rotates other clients' ETags (#253) + programSchedule.Version++; + await dbContext.SaveChangesAsync(cancellationToken); // refresh any playouts that use this schedule diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs index f54ccdd5f..278d1c6dc 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/DeleteProgramScheduleItemHandler.cs @@ -34,6 +34,10 @@ public class DeleteProgramScheduleItemHandler( List playouts = item.ProgramSchedule.Playouts; dbContext.ProgramScheduleItems.Remove(item); + + // bump the optimistic-concurrency token so this config edit rotates other clients' ETags (#253) + item.ProgramSchedule.Version++; + await dbContext.SaveChangesAsync(cancellationToken); foreach (Playout playout in playouts) diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs index 0c3c0b8ec..9a9d4568c 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItems.cs @@ -44,5 +44,8 @@ public record ReplaceProgramScheduleItem( string PreferredSubtitleLanguageCode, ChannelSubtitleMode? SubtitleMode) : IProgramScheduleItemRequest; -public record ReplaceProgramScheduleItems(int ProgramScheduleId, List Items) : IRequest< +public record ReplaceProgramScheduleItems( + int ProgramScheduleId, + List Items, + Option ExpectedVersion = default) : IRequest< Either>>; diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs index 0ffc63e05..c477367c0 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs @@ -26,13 +26,23 @@ public class ReplaceProgramScheduleItemsHandler( Some: async programSchedule => { Validation validation = await Validate(dbContext, request, programSchedule); - return await validation.Apply(ps => PersistItems(dbContext, request, ps, cancellationToken)); + + // Introduce the optimistic-concurrency check as a standalone Either AFTER the validation + // pipeline (never via Apply) so a PreconditionFailedError survives to a 412 and is not + // flattened to a generic 422 by Join() (issue #253 / api-conventions §7a). + Either validated = LanguageExtensions.ToEither(validation) + .Bind(ps => ps.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: ps => PersistItems(dbContext, request, ps, cancellationToken), + Left: error => + Task.FromResult>>(error)); }, None: () => Task.FromResult>>( new NotFoundError("[ProgramScheduleId] does not exist."))); } - private async Task> PersistItems( + private async Task>> PersistItems( TvContext dbContext, ReplaceProgramScheduleItems request, ProgramSchedule programSchedule, @@ -92,7 +102,20 @@ public class ReplaceProgramScheduleItemsHandler( programSchedule.Items.Add(BuildItem(programSchedule, i, orderedItems[i])); } - await dbContext.SaveChangesAsync(cancellationToken); + // Unconditional bump: this handler frequently saves with only CHILD changes and no root-scalar + // change, so without an explicit bump EF would emit no root UPDATE and the concurrency token + // would never fire (nor rotate other clients' ETags). Bumping guarantees both on every save, + // including a no-op same-items PUT-back (issue #253 / api-conventions §7a). + programSchedule.Version++; + + // Save through the guard so an EF concurrency failure (a racing writer won between our load and + // save) maps to 412 rather than surfacing as a 500. On failure, propagate the error WITHOUT + // running the post-save reload/enqueue below. + Either saved = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + if (saved.IsLeft) + { + return saved.Map(_ => (IEnumerable)[]); + } // refresh any playouts that use this schedule foreach (Playout playout in programSchedule.Playouts) diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/UpdateProgramScheduleHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/UpdateProgramScheduleHandler.cs index 9198db1a3..6500e9b5a 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/UpdateProgramScheduleHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/UpdateProgramScheduleHandler.cs @@ -54,6 +54,9 @@ public class UpdateProgramScheduleHandler( programSchedule.RandomStartPoint = request.RandomStartPoint; programSchedule.FixedStartTimeBehavior = request.FixedStartTimeBehavior; + // bump the optimistic-concurrency token so this config edit rotates other clients' ETags (#253) + programSchedule.Version++; + await dbContext.SaveChangesAsync(); if (needToRefreshPlayout) diff --git a/ErsatzTV.Application/ProgramSchedules/Mapper.cs b/ErsatzTV.Application/ProgramSchedules/Mapper.cs index 43460b45b..21f1821d0 100644 --- a/ErsatzTV.Application/ProgramSchedules/Mapper.cs +++ b/ErsatzTV.Application/ProgramSchedules/Mapper.cs @@ -12,7 +12,8 @@ internal static class Mapper programSchedule.TreatCollectionsAsShows, programSchedule.ShuffleScheduleItems, programSchedule.RandomStartPoint, - programSchedule.FixedStartTimeBehavior); + programSchedule.FixedStartTimeBehavior, + programSchedule.Version); internal static ProgramScheduleItemViewModel ProjectToViewModel(ProgramScheduleItem programScheduleItem) => programScheduleItem switch diff --git a/ErsatzTV.Application/ProgramSchedules/ProgramScheduleViewModel.cs b/ErsatzTV.Application/ProgramSchedules/ProgramScheduleViewModel.cs index c8750ad4e..1eee27acf 100644 --- a/ErsatzTV.Application/ProgramSchedules/ProgramScheduleViewModel.cs +++ b/ErsatzTV.Application/ProgramSchedules/ProgramScheduleViewModel.cs @@ -9,4 +9,5 @@ public record ProgramScheduleViewModel( bool TreatCollectionsAsShows, bool ShuffleScheduleItems, bool RandomStartPoint, - FixedStartTimeBehavior FixedStartTimeBehavior); + FixedStartTimeBehavior FixedStartTimeBehavior, + int Version); diff --git a/ErsatzTV.Application/ProgramSchedules/Queries/GetAllProgramSchedulesHandler.cs b/ErsatzTV.Application/ProgramSchedules/Queries/GetAllProgramSchedulesHandler.cs index c13f160eb..1abe7626f 100644 --- a/ErsatzTV.Application/ProgramSchedules/Queries/GetAllProgramSchedulesHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Queries/GetAllProgramSchedulesHandler.cs @@ -19,7 +19,8 @@ public class GetAllProgramSchedulesHandler(IDbContextFactory dbContex ps.TreatCollectionsAsShows, ps.ShuffleScheduleItems, ps.RandomStartPoint, - ps.FixedStartTimeBehavior)) + ps.FixedStartTimeBehavior, + ps.Version)) .ToListAsync(cancellationToken); } } diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs new file mode 100644 index 000000000..d665cdf3b --- /dev/null +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ReplaceProgramScheduleItemsHandlerConcurrencyTests.cs @@ -0,0 +1,231 @@ +using System.Threading.Channels; +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; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NUnit.Framework; +using Shouldly; +using static LanguageExt.Prelude; + +namespace ErsatzTV.Tests.Application.ProgramSchedules; + +/// +/// Contract tests for the #253 optimistic-concurrency mechanic on the ProgramSchedule / schedule-items +/// aggregate: the handler pre-check (stale If-Match → 412, before the positional reconcile runs — so the +/// item rows AND persisted fill-group/shuffle state are left untouched), the force-write path (no +/// If-Match), the unconditional Version bump on every save (including a no-op same-items PUT-back where +/// only child rows change), and the EF concurrency-token backstop that catches a writer that lost the +/// load→save race. The backstop test is non-vacuous by construction — remove the +/// IsConcurrencyToken() config on ProgramSchedule and the losing save silently succeeds instead +/// of mapping to a . +/// +[TestFixture] +public class ReplaceProgramScheduleItemsHandlerConcurrencyTests +{ + private InMemoryTvContext _db = null!; + private ChannelWriter _worker = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _worker = System.Threading.Channels.Channel.CreateUnbounded().Writer; + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + // Seeds a schedule (Id=1) with a single One/SearchQuery item (Id=1) and a persisted fill-group + // enumerator state pointing at that item, so the stale-If-Match test can prove the reconcile never ran. + private async Task SeedScheduleAsync(int version) + { + await using TvContext ctx = _db.CreateContext(); + ctx.ProgramSchedules.Add( + new ProgramSchedule + { + Id = 1, + Name = "Concurrency", + Version = version, + Items = new List + { + new ProgramScheduleItemOne + { + Id = 1, + Index = 0, + CollectionType = CollectionType.SearchQuery, + SearchTitle = "a", + SearchQuery = "a", + PlaybackOrder = PlaybackOrder.Shuffle, + GuideMode = GuideMode.Normal + } + }, + Playouts = [], + ProgramScheduleAlternates = [] + }); + await ctx.SaveChangesAsync(); + + ctx.Add(new PlayoutScheduleItemFillGroupIndex + { + PlayoutId = 1, + ProgramScheduleItemId = 1, + EnumeratorState = new CollectionEnumeratorState { Seed = 12345, Index = 7 } + }); + await ctx.SaveChangesAsync(); + } + + private static ReplaceProgramScheduleItems Command( + Option expectedVersion, + List? items = null) => + new(1, items ?? [MakeItem(0, PlayoutMode.One, "a")], expectedVersion); + + private async Task ReadVersionAsync() + { + await using TvContext ctx = _db.CreateContext(); + return await ctx.ProgramSchedules.Where(s => s.Id == 1).Select(s => s.Version).SingleAsync(); + } + + private static BaseError? LeftOrNull(Either> result) => + result.Match(Right: _ => null, Left: e => e); + + [Test] + public async Task Stale_If_Match_Should_Fail_Precondition_And_Not_Mutate() + { + await SeedScheduleAsync(version: 2); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + // An empty item list WOULD delete the existing item (and cascade its fill-group state) if the + // reconcile ran. A stale If-Match must reject before that, leaving everything untouched. + Either> result = + await handler.Handle(Command(Some(1), []), CancellationToken.None); + + LeftOrNull(result).ShouldBeOfType(); + + (await ReadVersionAsync()).ShouldBe(2); + await using TvContext ctx = _db.CreateContext(); + (await ctx.ProgramScheduleItems.CountAsync(i => i.ProgramScheduleId == 1)).ShouldBe(1); + + // The reconcile never ran: the fill-group enumerator state is exactly as seeded. + PlayoutScheduleItemFillGroupIndex fillGroup = await ctx.Set() + .Include(x => x.EnumeratorState) + .SingleAsync(); + fillGroup.ProgramScheduleItemId.ShouldBe(1); + fillGroup.EnumeratorState.Seed.ShouldBe(12345); + fillGroup.EnumeratorState.Index.ShouldBe(7); + } + + [Test] + public async Task Matching_If_Match_Should_Succeed_And_Bump_Version() + { + await SeedScheduleAsync(version: 2); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + Either> result = + await handler.Handle(Command(Some(2)), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersionAsync()).ShouldBe(3); + } + + [Test] + public async Task Absent_If_Match_Should_Force_Write_And_Bump_Version() + { + await SeedScheduleAsync(version: 2); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + // None expected version = Phase-1 force-write regardless of the stored version. + Either> result = + await handler.Handle(Command(None), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersionAsync()).ShouldBe(3); + } + + [Test] + public async Task NoOp_Save_Should_Bump_Version_Even_With_Only_Child_Changes() + { + await SeedScheduleAsync(version: 5); + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + + // Same content twice: this handler saves with only CHILD changes and no root-scalar change, so the + // unconditional bump (M1) must still rotate the version each time, otherwise a no-op PUT-back would + // neither fire the token nor rotate other clients' ETags. + (await handler.Handle(Command(Some(5)), CancellationToken.None)).IsRight.ShouldBeTrue(); + (await ReadVersionAsync()).ShouldBe(6); + (await handler.Handle(Command(Some(6)), CancellationToken.None)).IsRight.ShouldBeTrue(); + (await ReadVersionAsync()).ShouldBe(7); + } + + [Test] + public async Task Racing_Save_Should_Map_Concurrency_Failure_To_412() + { + await SeedScheduleAsync(version: 1); + + // Two writers load the same version, then both bump-and-save. The IsConcurrencyToken guard on + // ProgramSchedule makes the second UPDATE key on the original version; it matches zero rows and + // throws DbUpdateConcurrencyException, which the shared save helper maps to a PreconditionFailedError. + await using TvContext ctxWinner = _db.CreateContext(); + await using TvContext ctxLoser = _db.CreateContext(); + + ProgramSchedule winner = await ctxWinner.ProgramSchedules.SingleAsync(s => s.Id == 1); + ProgramSchedule loser = await ctxLoser.ProgramSchedules.SingleAsync(s => s.Id == 1); + + winner.Version++; + Either winnerResult = await ctxWinner.SaveChangesWithConcurrencyGuard(CancellationToken.None); + winnerResult.IsRight.ShouldBeTrue(); + + loser.Version++; + Either loserResult = await ctxLoser.SaveChangesWithConcurrencyGuard(CancellationToken.None); + + loserResult.Match(Right: _ => null, Left: e => e).ShouldBeOfType(); + + // Non-vacuous: the winner's write stuck at exactly its +1; the loser did not overwrite it. + (await ReadVersionAsync()).ShouldBe(2); + } + + private static ReplaceProgramScheduleItem MakeItem(int index, PlayoutMode mode, string searchQuery) => + new( + index, + StartType.Dynamic, + StartTime: null, + FixedStartTimeBehavior: null, + mode, + CollectionType.SearchQuery, + CollectionId: null, + MultiCollectionId: null, + SmartCollectionId: null, + RerunCollectionId: null, + MediaItemId: null, + PlaylistId: null, + SearchTitle: searchQuery, + SearchQuery: searchQuery, + PlaybackOrder.Shuffle, + MarathonGroupBy.None, + MarathonShuffleGroups: false, + MarathonShuffleItems: false, + MarathonBatchSize: null, + FillWithGroupMode.None, + MultipleMode.Count, + MultipleCount: "1", + PlayoutDuration: null, + TailMode.None, + DiscardToFillAttempts: null, + CustomTitle: null, + GuideMode.Normal, + PreRollFillerId: null, + MidRollFillerId: null, + PostRollFillerId: null, + TailFillerId: null, + FallbackFillerId: null, + WatermarkIds: [], + GraphicsElementIds: [], + PreferredAudioLanguageCode: null, + PreferredAudioTitle: null, + PreferredSubtitleLanguageCode: null, + SubtitleMode: null); +} diff --git a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs index d0af315c4..9153cf8c6 100644 --- a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs @@ -1247,7 +1247,7 @@ public class PlayoutControllerTests new(0, programScheduleId, [], [], [], false, 1, 1, null, 12, 31, null); private static ProgramScheduleViewModel MakeScheduleVm(int id) => - new(id, $"Schedule {id}", false, false, false, false, FixedStartTimeBehavior.Strict); + new(id, $"Schedule {id}", false, false, false, false, FixedStartTimeBehavior.Strict, 0); private static TemplateViewModel MakeTemplateViewModel(int id) => new(id, 1, "Group", $"Template {id}"); diff --git a/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs b/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs index e09792cc3..506d62cdd 100644 --- a/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/ScheduleControllerTests.cs @@ -10,6 +10,7 @@ using ErsatzTV.Core.Errors; using ErsatzTV.Core.Scheduling; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -30,7 +31,12 @@ public class ScheduleControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new ScheduleController(_mediator); + _controller = new ScheduleController(_mediator) + { + // Provide a real HttpContext so the ETag/If-Match concurrency headers (#253) can be + // read from Request and written to Response. + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() } + }; } [Test] @@ -271,6 +277,90 @@ public class ScheduleControllerTests Arg.Any()); } + [Test] + public async Task GetItems_Should_Set_ETag_From_Schedule_Version() + { + var response = new ProgramScheduleItemsWithDurationViewModel([], null); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeSchedule(4, "Daily", version: 9))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(response); + + await _controller.GetItems(4, CancellationToken.None); + + _controller.Response.Headers.ETag.ToString().ShouldBe("\"9\""); + } + + [Test] + public async Task ReplaceItems_Should_Return_400_On_Malformed_If_Match() + { + _controller.Request.Headers.IfMatch = "not-an-etag"; + + IActionResult result = await _controller.ReplaceItems( + 4, + new ReplaceScheduleItemsRequest([MakeItemRequest(PlayoutMode.One)]), + CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } + + [Test] + public async Task ReplaceItems_Should_Thread_If_Match_Version_Into_Command_And_Set_New_ETag() + { + _controller.Request.Headers.IfMatch = "\"3\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>([MakeOneItem(21)])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeSchedule(4, "Daily", version: 4))); + + IActionResult result = await _controller.ReplaceItems( + 4, + new ReplaceScheduleItemsRequest([MakeItemRequest(PlayoutMode.One)]), + CancellationToken.None); + + result.ShouldBeOfType(); + // On success the response carries the refreshed schedule's ETag. + _controller.Response.Headers.ETag.ToString().ShouldBe("\"4\""); + await _mediator.Received(1).Send( + Arg.Is(c => c.ExpectedVersion == Option.Some(3)), + Arg.Any()); + } + + [Test] + public async Task ReplaceItems_Without_If_Match_Should_Force_Write() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>([MakeOneItem(21)])); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeSchedule(4, "Daily", version: 1))); + + await _controller.ReplaceItems( + 4, + new ReplaceScheduleItemsRequest([MakeItemRequest(PlayoutMode.One)]), + CancellationToken.None); + + await _mediator.Received(1).Send( + Arg.Is(c => c.ExpectedVersion == Option.None), + Arg.Any()); + } + + [Test] + public async Task ReplaceItems_Should_Return_412_On_Precondition_Failed() + { + _controller.Request.Headers.IfMatch = "\"2\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Left>(new PreconditionFailedError("stale"))); + + IActionResult result = await _controller.ReplaceItems( + 4, + new ReplaceScheduleItemsRequest([MakeItemRequest(PlayoutMode.One)]), + CancellationToken.None); + + var objectResult = result.ShouldBeOfType(); + objectResult.StatusCode.ShouldBe(StatusCodes.Status412PreconditionFailed); + } + [Test] public async Task DeleteItem_Should_Return_204_And_Map_Route_Ids() { @@ -343,8 +433,8 @@ public class ScheduleControllerTests PreferredSubtitleLanguageCode: null, SubtitleMode: null); - private static ProgramScheduleViewModel MakeSchedule(int id, string name) => - new(id, name, true, true, false, false, FixedStartTimeBehavior.Flexible); + private static ProgramScheduleViewModel MakeSchedule(int id, string name, int version = 0) => + new(id, name, true, true, false, false, FixedStartTimeBehavior.Flexible, version); private static ProgramScheduleItemOneViewModel MakeOneItem(int id) => new( diff --git a/ErsatzTV/Controllers/Api/Requests/ReplaceScheduleItemsRequest.cs b/ErsatzTV/Controllers/Api/Requests/ReplaceScheduleItemsRequest.cs index 40a8f6864..fd17ec5df 100644 --- a/ErsatzTV/Controllers/Api/Requests/ReplaceScheduleItemsRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ReplaceScheduleItemsRequest.cs @@ -4,8 +4,9 @@ namespace ErsatzTV.Controllers.Api.Requests; public record ReplaceScheduleItemsRequest(List Items) { - public ReplaceProgramScheduleItems ToCommand(int scheduleId) => + public ReplaceProgramScheduleItems ToCommand(int scheduleId, Option expectedVersion = default) => new( scheduleId, - (Items ?? []).Select((item, index) => item.ToReplaceCommand(index)).ToList()); + (Items ?? []).Select((item, index) => item.ToReplaceCommand(index)).ToList(), + expectedVersion); } diff --git a/ErsatzTV/Controllers/Api/ScheduleController.cs b/ErsatzTV/Controllers/Api/ScheduleController.cs index 0a46e4e16..86b006b1b 100644 --- a/ErsatzTV/Controllers/Api/ScheduleController.cs +++ b/ErsatzTV/Controllers/Api/ScheduleController.cs @@ -104,7 +104,9 @@ public class ScheduleController(IMediator mediator) : ControllerBase [EndpointDescription( "Returns the schedule's items plus a computed, best-effort runtime estimate: each item carries a " + "nullable durationEstimate and the envelope carries a nullable totalDurationEstimate. Estimates are " + - "derived from referenced collection/media runtimes and are null when unbounded or unknown.")] + "derived from referenced collection/media runtimes and are null when unbounded or unknown. The " + + "response also carries a strong ETag of the schedule's version; pass that ETag back as If-Match on " + + "the replace (PUT) to detect a concurrent edit (issue #253).")] [EndpointGroupName("general")] [ProducesResponseType(typeof(ScheduleItemsResponseModel), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] @@ -116,6 +118,9 @@ public class ScheduleController(IMediator mediator) : ControllerBase return ApiResults.NotFoundProblem(); } + // The items GET returns children, not the root, so read the schedule's version for the ETag. + ConcurrencyHeaders.SetETag(Response, schedule.Map(s => s.Version).IfNone(0)); + ProgramScheduleItemsWithDurationViewModel items = await mediator.Send(new GetProgramScheduleItemsWithDurations(id), cancellationToken); return new OkObjectResult(ScheduleItemResponseMapper.ProjectToResponseModel(items)); @@ -143,20 +148,48 @@ public class ScheduleController(IMediator mediator) : ControllerBase [HttpPut("/api/schedules/{id:int}/items")] [Tags("Schedules")] [EndpointSummary("Replace schedule items")] + [EndpointDescription( + "Replaces the schedule's full item list; item indexes are assigned from the array order. Send the " + + "ETag from the items GET as If-Match to reject a stale overwrite with 412 (issue #253); a successful " + + "response carries the new ETag.")] [EndpointGroupName("general")] [ProducesResponseType(typeof(List), StatusCodes.Status200OK)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status412PreconditionFailed)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task ReplaceItems( int id, [Required] [FromBody] ReplaceScheduleItemsRequest request, CancellationToken cancellationToken) { + IfMatchCondition ifMatch = ConcurrencyHeaders.ParseIfMatch(Request); + if (ifMatch.Kind is IfMatchKind.Malformed) + { + return new BadRequestObjectResult( + new ProblemDetails + { + Status = StatusCodes.Status400BadRequest, + Title = "Invalid If-Match header", + Detail = "If-Match must be a strong ETag of the resource version (e.g. \"3\") or \"*\"." + }); + } + Either> result = - await mediator.Send(request.ToCommand(id), cancellationToken); - return result - .Map(items => items.Select(ScheduleItemResponseMapper.ProjectToResponseModel).ToList()) - .ToUpdatedResult(); + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); + + return await result.Match( + Left: error => Task.FromResult(error.ToErrorResult()), + Right: async items => + { + // Return the new ETag so a same-tab second save doesn't 412 against its own write. + Option refreshed = + await mediator.Send(new GetProgramScheduleById(id), cancellationToken); + refreshed.IfSome(vm => ConcurrencyHeaders.SetETag(Response, vm.Version)); + + return (IActionResult)new OkObjectResult( + items.Select(ScheduleItemResponseMapper.ProjectToResponseModel).ToList()); + }); } [HttpDelete("/api/schedules/{id:int}/items/{itemId:int}")] diff --git a/web/src/api/schedules.ts b/web/src/api/schedules.ts index 4aac218a7..b87ba5b28 100644 --- a/web/src/api/schedules.ts +++ b/web/src/api/schedules.ts @@ -1,4 +1,4 @@ -import { ApiError, request } from './client'; +import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client'; import type { components } from './generated/v1'; // FillerPreset is exported by ./pickers — import (don't re-export) to avoid a duplicate `export *` // name in api/index.ts. LanguageCode/getLanguages moved to ./languages (also re-exported via @@ -46,17 +46,31 @@ export function getScheduleItems(scheduleId: number): Promise(`/api/schedules/${scheduleId}/items`); } +/** Load schedule items together with the schedule's concurrency ETag (issue #253). */ +export function getScheduleItemsWithMeta( + scheduleId: number +): Promise> { + return requestWithMeta(`/api/schedules/${scheduleId}/items`); +} + export function addScheduleItem(scheduleId: number, body: ScheduleItemRequest): Promise { return request(`/api/schedules/${scheduleId}/items`, { body, method: 'POST' }); } -// Destructive replace: the server deletes+recreates every item row (new ids) and triggers playout -// rebuilds. The editor batches all local draft edits into this single call. See docs/decisions.md. +// Positional in-place reconcile: the server reuses same-typed item rows (keeping fill-group state) and +// triggers playout rebuilds. The editor batches all local draft edits into this single call. Pass the +// last-seen ETag as `If-Match` to reject a stale overwrite with 412; the resolved value carries the new +// ETag for a subsequent save (issue #253). See docs/decisions.md. export function replaceScheduleItems( scheduleId: number, - body: ReplaceScheduleItemsRequest -): Promise { - return request(`/api/schedules/${scheduleId}/items`, { body, method: 'PUT' }); + body: ReplaceScheduleItemsRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/schedules/${scheduleId}/items`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } export function deleteScheduleItem(scheduleId: number, itemId: number): Promise { diff --git a/web/src/screens/SchedulesScreen.test.tsx b/web/src/screens/SchedulesScreen.test.tsx index f536bc51f..30aeff3b0 100644 --- a/web/src/screens/SchedulesScreen.test.tsx +++ b/web/src/screens/SchedulesScreen.test.tsx @@ -267,6 +267,36 @@ describe('SchedulesScreen — error + dirty handling', () => { expect((screen.getByRole('button', { name: /^Save$/ }) as HTMLButtonElement).disabled).toBe(false); }); + it('412 on Save opens the conflict dialog; Reload discards the draft and reloads', async () => { + let putCount = 0; + const handle = await renderReady({ + onRequest: (url, method) => { + if (url === '/api/schedules/1/items' && method === 'PUT') { + putCount += 1; + return jsonResponse({ title: 'Precondition Failed', detail: 'stale' }, 412); + } + return null; + } + }); + // Make dirty, then Save → 412. + fireEvent.click(screen.getByRole('button', { name: /Add item/ })); + const getsBefore = handle.requests.filter((r) => r.url === '/api/schedules/1/items' && r.method === 'GET').length; + fireEvent.click(screen.getByRole('button', { name: /^Save$/ })); + + // The conflict dialog appears (distinct from the generic save-error path). + await screen.findByText('Schedule changed elsewhere'); + expect(putCount).toBe(1); + + // Reload discards the draft and re-fetches the active schedule's items. + fireEvent.click(screen.getByRole('button', { name: /^Reload$/ })); + await waitFor(() => + expect(handle.requests.filter((r) => r.url === '/api/schedules/1/items' && r.method === 'GET').length) + .toBeGreaterThan(getsBefore)); + // Dialog closed; draft reset to the (single-item) server baseline. + await waitFor(() => expect(screen.queryByText('Schedule changed elsewhere')).toBeNull()); + expect(within(screen.getByLabelText('Schedule lineup')).getAllByRole('listitem')).toHaveLength(1); + }); + it('guards a schedule switch when dirty (confirm=false aborts)', async () => { vi.spyOn(window, 'confirm').mockReturnValue(false); const handle = await renderReady({ schedules: [schedule, { ...schedule, id: 2, name: 'Late' }] }); diff --git a/web/src/screens/SchedulesScreen.tsx b/web/src/screens/SchedulesScreen.tsx index 5a291e692..d269b560e 100644 --- a/web/src/screens/SchedulesScreen.tsx +++ b/web/src/screens/SchedulesScreen.tsx @@ -10,12 +10,13 @@ import { Spinner } from '../components'; import { + ApiError, deleteSchedule, getFillerPresetsByKind, getLanguages, getPlaylistGroups, getRerunCollections, - getScheduleItems, + getScheduleItemsWithMeta, getSchedules, getGraphicsElements, getWatermarks, @@ -87,11 +88,16 @@ export function SchedulesScreen() { const [mutationError, setMutationError] = useState(null); const [form, setForm] = useState(null); const [confirmDelete, setConfirmDelete] = useState(false); + // Opened when a save 412s because the schedule was changed elsewhere since it was loaded (#253). + const [conflictOpen, setConflictOpen] = useState(false); const activeRef = useRef(true); const dirtyRef = useRef(false); const baselineRef = useRef([]); const itemSeq = useRef(0); + // Last-seen concurrency ETag: set from the items GET, replaced by every successful save's response + // ETag (a same-tab second save must use the new tag or it would 412 against its own write) (#253). + const etagRef = useRef(null); const setDirtyState = useCallback((value: boolean) => { dirtyRef.current = value; @@ -131,11 +137,13 @@ export function SchedulesScreen() { // ---- Load items for the active schedule -------------------------------- const loadItems = useCallback((scheduleId: number) => { const seq = ++itemSeq.current; - getScheduleItems(scheduleId) - .then((response) => { + getScheduleItemsWithMeta(scheduleId) + .then(({ data: response, etag }) => { if (!activeRef.current || itemSeq.current !== seq) { return; } + // Only the current load owns the ETag (same stale-guard as the items below). + etagRef.current = etag; // Defensive: array position becomes the persisted index on the next PUT, so ingest strictly by the // server-provided `index` rather than trusting response row order (see #229 — the API now orders, but // the SPA must not silently reshuffle the lineup if that guarantee ever regresses). @@ -318,11 +326,14 @@ export function SchedulesScreen() { } setSaving(true); setMutationError(null); - replaceScheduleItems(activeId, { items: items.map(normalizeForSave) }) - .then((response) => { + replaceScheduleItems(activeId, { items: items.map(normalizeForSave) }, etagRef.current) + .then(({ data: response, etag }) => { if (!activeRef.current) { return; } + // Load-bearing: a same-tab second save must use the ETag this write produced, or it would 412 + // against its own change (#253). + etagRef.current = etag; const drafts = [...response].sort((a, b) => a.index - b.index).map(fromResponse); baselineRef.current = drafts; setItems(drafts); @@ -337,11 +348,28 @@ export function SchedulesScreen() { if (!activeRef.current) { return; } - setMutationError(messageFromScheduleError(error, 'Unable to save schedule items')); + if (error instanceof ApiError && error.status === 412) { + // The schedule was changed elsewhere since we loaded it — force a reload rather than + // overwriting the fresher edit (#253). Distinct from all other errors. + setConflictOpen(true); + } else { + setMutationError(messageFromScheduleError(error, 'Unable to save schedule items')); + } setSaving(false); }); }; + // Conflict "Reload": discard the dirty draft and re-run the load for the active schedule. loadItems + // already bumps itemSeq (stale-guard), re-seeds baselineRef, clears dirty, and captures the new ETag — + // so there's no separate reloadKey to invent (#253). + const reloadAfterConflict = () => { + setConflictOpen(false); + setMutationError(null); + if (activeId != null) { + loadItems(activeId); + } + }; + // Opening the properties editor is blocked while the item draft is dirty (confirm-to-discard, same // semantics as guardedSwitch). This is the smaller, fully-consistent fix for the shuffle-toggle // normalization gap (#230 finding 2): if editing shuffleScheduleItems could change under a dirty @@ -562,6 +590,16 @@ export function SchedulesScreen() { onConfirm={onDeleteSchedule} onCancel={() => setConfirmDelete(false)} /> + + setConflictOpen(false)} + /> ); }