From 1a8c0f60defa184e2eb2f6328ede326836c0fc41 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:33:40 +0200 Subject: [PATCH 01/12] feat(playlists): wire optimistic-concurrency contract onto Playlist (#253 PR2) Fans the frozen ETag/If-Match/412 recipe (Block reference, #253) onto the Playlist aggregate: - ReplacePlaylistItems command carries ExpectedVersion; the handler runs CheckVersion as a standalone Either after validation (so a stale write survives as 412, not flattened to 422 by Apply/Join), bumps Version unconditionally before saving, and persists via SaveChangesWithConcurrencyGuard (EF concurrency-token backstop). - PlaylistViewModel carries Version; the items GET sets a strong ETag and the PUT parses If-Match, threads it into the command, and returns the refreshed ETag on success (400 on a malformed If-Match). - Sibling item-adding handlers (AddItemsToPlaylist, AddMovie/Episode/ Season/ShowToPlaylist) bump Version too, since they mutate the same editor-visible item list. - SPA: playlists.ts exposes getPlaylistItemsWithMeta and an If-Match-aware updatePlaylist; PlaylistEditor holds the ETag in a ref, round-trips it on save, and opens a "changed elsewhere" ConfirmDialog on 412 (mirrors BlockEditor). Tests: new ReplacePlaylistItemsHandlerConcurrencyTests (stale/match/ force-write/no-op-bump/racing-save), new PlaylistController tests (ETag on GET items, 400/412/thread-version/force-write on PUT), and a vitest 412-conflict-dialog test for PlaylistsScreen. dotnet test: 1304/1304 green. web: npm run typecheck clean, npm run build clean, vitest 664/664 green. Ref #253 PR2. --- .../Commands/AddEpisodeToPlaylistHandler.cs | 4 + .../Commands/AddItemsToPlaylistHandler.cs | 3 + .../Commands/AddMovieToPlaylistHandler.cs | 4 + .../Commands/AddSeasonToPlaylistHandler.cs | 4 + .../Commands/AddShowToPlaylistHandler.cs | 4 + .../Commands/ReplacePlaylistItems.cs | 6 +- .../Commands/ReplacePlaylistItemsHandler.cs | 25 ++- .../MediaCollections/Mapper.cs | 2 +- .../MediaCollections/PlaylistViewModel.cs | 2 +- ...acePlaylistItemsHandlerConcurrencyTests.cs | 155 ++++++++++++++++++ .../Controllers/DecoControllerTests.cs | 2 +- .../Controllers/PlaylistControllerTests.cs | 121 ++++++++++++-- .../Controllers/Api/PlaylistController.cs | 38 ++++- .../Api/Requests/ReplacePlaylistRequest.cs | 5 +- web/src/api/playlists.ts | 23 ++- web/src/screens/PlaylistsScreen.test.tsx | 38 +++++ web/src/screens/PlaylistsScreen.tsx | 48 +++++- 17 files changed, 447 insertions(+), 37 deletions(-) create mode 100644 ErsatzTV.Tests/Application/MediaCollections/ReplacePlaylistItemsHandlerConcurrencyTests.cs diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToPlaylistHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToPlaylistHandler.cs index 43c0cac87..05cad00de 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToPlaylistHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddEpisodeToPlaylistHandler.cs @@ -32,6 +32,10 @@ public class AddEpisodeToPlaylistHandler(IDbContextFactory dbContextF }; parameters.Playlist.Items.Add(playlistItem); + + // Mutates the playlist's editor-visible item list, so bump the concurrency token (issue #253). + parameters.Playlist.Version++; + await dbContext.SaveChangesAsync(); return Unit.Default; } diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddItemsToPlaylistHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddItemsToPlaylistHandler.cs index fd8586c21..f6ab29b8b 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddItemsToPlaylistHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddItemsToPlaylistHandler.cs @@ -69,6 +69,9 @@ public class AddItemsToPlaylistHandler : IRequestHandler dbContextFac }; parameters.Playlist.Items.Add(playlistItem); + + // Mutates the playlist's editor-visible item list, so bump the concurrency token (issue #253). + parameters.Playlist.Version++; + await dbContext.SaveChangesAsync(); return Unit.Default; } diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToPlaylistHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToPlaylistHandler.cs index 40519bf6c..e9873bc46 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToPlaylistHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddSeasonToPlaylistHandler.cs @@ -32,6 +32,10 @@ public class AddSeasonToPlaylistHandler(IDbContextFactory dbContextFa }; parameters.Playlist.Items.Add(playlistItem); + + // Mutates the playlist's editor-visible item list, so bump the concurrency token (issue #253). + parameters.Playlist.Version++; + await dbContext.SaveChangesAsync(); return Unit.Default; } diff --git a/ErsatzTV.Application/MediaCollections/Commands/AddShowToPlaylistHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/AddShowToPlaylistHandler.cs index 2795fd711..3159988ff 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/AddShowToPlaylistHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/AddShowToPlaylistHandler.cs @@ -32,6 +32,10 @@ public class AddShowToPlaylistHandler(IDbContextFactory dbContextFact }; parameters.Playlist.Items.Add(playlistItem); + + // Mutates the playlist's editor-visible item list, so bump the concurrency token (issue #253). + parameters.Playlist.Version++; + await dbContext.SaveChangesAsync(); return Unit.Default; } diff --git a/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItems.cs b/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItems.cs index 74efb5729..caffd1928 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItems.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItems.cs @@ -2,5 +2,9 @@ using ErsatzTV.Core; namespace ErsatzTV.Application.MediaCollections; -public record ReplacePlaylistItems(int PlaylistId, string Name, List Items) +public record ReplacePlaylistItems( + int PlaylistId, + string Name, + List Items, + Option ExpectedVersion = default) : IRequest>>; diff --git a/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItemsHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItemsHandler.cs index b55436918..85d26a50b 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItemsHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/ReplacePlaylistItemsHandler.cs @@ -15,10 +15,21 @@ public class ReplacePlaylistItemsHandler(IDbContextFactory dbContextF { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); Validation validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(ps => Persist(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). + // LanguageExtensions.ToEither joins the Seq to a single BaseError (the native + // Validation.ToEither() would keep Seq and is called explicitly here to avoid that shadow). + Either validated = LanguageExtensions.ToEither(validation) + .Bind(playlist => playlist.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: playlist => Persist(dbContext, request, playlist, cancellationToken), + Left: error => Task.FromResult>>(error)); } - private static async Task> Persist( + private static async Task>> Persist( TvContext dbContext, ReplacePlaylistItems request, Playlist playlist, @@ -30,9 +41,15 @@ public class ReplacePlaylistItemsHandler(IDbContextFactory dbContextF dbContext.RemoveRange(playlist.Items); playlist.Items = request.Items.Map(i => BuildItem(playlist, i.Index, i)).ToList(); - await dbContext.SaveChangesAsync(cancellationToken); + // Unconditional bump: EF emits the root UPDATE only when a scalar actually differs, so a + // same-value/no-op save would otherwise write no root row and neither fire the concurrency + // token nor rotate other clients' ETags. Bumping guarantees both on every save (issue #253). + playlist.Version++; - return playlist.Items.Map(Mapper.ProjectToViewModel).ToList(); + // 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. + Either saved = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + return saved.Map(_ => playlist.Items.Map(Mapper.ProjectToViewModel).ToList()); } private static PlaylistItem BuildItem(Playlist playlist, int index, ReplacePlaylistItem item) => diff --git a/ErsatzTV.Application/MediaCollections/Mapper.cs b/ErsatzTV.Application/MediaCollections/Mapper.cs index 05271e474..de15ca1f2 100644 --- a/ErsatzTV.Application/MediaCollections/Mapper.cs +++ b/ErsatzTV.Application/MediaCollections/Mapper.cs @@ -89,7 +89,7 @@ internal static class Mapper new(playlistGroup.Id, playlistGroup.Name, playlistGroup.Playlists.Count, playlistGroup.IsSystem); internal static PlaylistViewModel ProjectToViewModel(Playlist playlist) => - new(playlist.Id, playlist.PlaylistGroupId, playlist.Name, playlist.IsSystem); + new(playlist.Id, playlist.PlaylistGroupId, playlist.Name, playlist.IsSystem, playlist.Version); internal static PlaylistItemViewModel ProjectToViewModel(PlaylistItem playlistItem) => new( diff --git a/ErsatzTV.Application/MediaCollections/PlaylistViewModel.cs b/ErsatzTV.Application/MediaCollections/PlaylistViewModel.cs index 0b57c33ae..febb7e254 100644 --- a/ErsatzTV.Application/MediaCollections/PlaylistViewModel.cs +++ b/ErsatzTV.Application/MediaCollections/PlaylistViewModel.cs @@ -1,3 +1,3 @@ namespace ErsatzTV.Application.MediaCollections; -public record PlaylistViewModel(int Id, int PlaylistGroupId, string Name, bool IsSystem); +public record PlaylistViewModel(int Id, int PlaylistGroupId, string Name, bool IsSystem, int Version); diff --git a/ErsatzTV.Tests/Application/MediaCollections/ReplacePlaylistItemsHandlerConcurrencyTests.cs b/ErsatzTV.Tests/Application/MediaCollections/ReplacePlaylistItemsHandlerConcurrencyTests.cs new file mode 100644 index 000000000..d1ae0037e --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/ReplacePlaylistItemsHandlerConcurrencyTests.cs @@ -0,0 +1,155 @@ +using ErsatzTV.Application; +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Errors; +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.MediaCollections; + +/// +/// Contract tests for the #253 optimistic-concurrency mechanic on the Playlist aggregate (mirrors +/// ReplaceBlockItemsHandlerConcurrencyTests, the Block reference implementation): the handler +/// pre-check (stale If-Match → 412), the force-write path (no If-Match), the unconditional Version +/// bump on every save, 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 Playlist and the losing save silently succeeds instead of +/// mapping to a . +/// +[TestFixture] +public class ReplacePlaylistItemsHandlerConcurrencyTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private async Task SeedPlaylistAsync(int version) + { + await using TvContext ctx = _db.CreateContext(); + ctx.Playlists.Add( + new Playlist + { + Id = 1, + PlaylistGroupId = 1, + Name = "Kids", + IsSystem = false, + Version = version, + Items = new List() + }); + await ctx.SaveChangesAsync(); + } + + private static ReplacePlaylistItems Command(Option expectedVersion) => + new( + 1, + "Kids", + new List + { + new(0, CollectionType.Movie, null, null, null, 55, PlaybackOrder.Shuffle, null, false, true) + }, + expectedVersion); + + private async Task ReadVersionAsync() + { + await using TvContext ctx = _db.CreateContext(); + return await ctx.Playlists.Where(p => p.Id == 1).Select(p => p.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 SeedPlaylistAsync(version: 2); + var handler = new ReplacePlaylistItemsHandler(_db.Factory); + + Either> result = + await handler.Handle(Command(Some(1)), CancellationToken.None); + + LeftOrNull(result).ShouldBeOfType(); + + // The pre-check runs before any mutation: version unchanged, no items written. + (await ReadVersionAsync()).ShouldBe(2); + await using TvContext ctx = _db.CreateContext(); + (await ctx.PlaylistItems.CountAsync(i => i.PlaylistId == 1)).ShouldBe(0); + } + + [Test] + public async Task Matching_If_Match_Should_Succeed_And_Bump_Version() + { + await SeedPlaylistAsync(version: 2); + var handler = new ReplacePlaylistItemsHandler(_db.Factory); + + 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 SeedPlaylistAsync(version: 2); + var handler = new ReplacePlaylistItemsHandler(_db.Factory); + + // 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 Save_Should_Bump_Version_Even_When_Content_Is_Unchanged() + { + await SeedPlaylistAsync(version: 5); + var handler = new ReplacePlaylistItemsHandler(_db.Factory); + + // Same content twice: the unconditional bump (M1) must still rotate the version each time, + // otherwise a no-op PUT-back would not fire the token or 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 SeedPlaylistAsync(version: 1); + + // Two writers load the same version, then both bump-and-save. The IsConcurrencyToken guard on + // Playlist 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(); + + Playlist winner = await ctxWinner.Playlists.SingleAsync(p => p.Id == 1); + Playlist loser = await ctxLoser.Playlists.SingleAsync(p => p.Id == 1); + + winner.Version++; + Either winnerResult = await ctxWinner.SaveChangesWithConcurrencyGuard(CancellationToken.None); + winnerResult.IsRight.ShouldBeTrue(); + + loser.Version++; + Either loserResult = await ctxLoser.SaveChangesWithConcurrencyGuard(CancellationToken.None); + + LeftOrNull(loserResult).ShouldBeOfType(); + + // Non-vacuous: the winner's write stuck at exactly its +1; the loser did not overwrite it. + (await ReadVersionAsync()).ShouldBe(2); + } +} diff --git a/ErsatzTV.Tests/Controllers/DecoControllerTests.cs b/ErsatzTV.Tests/Controllers/DecoControllerTests.cs index 38068d4ac..e96eae117 100644 --- a/ErsatzTV.Tests/Controllers/DecoControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/DecoControllerTests.cs @@ -152,7 +152,7 @@ public class DecoControllerTests null, null, null, - new ErsatzTV.Application.MediaCollections.PlaylistViewModel(9, 3, "Idents", false), + new ErsatzTV.Application.MediaCollections.PlaylistViewModel(9, 3, "Idents", false, 1), DecoBreakPlacement.BlockStart) ]); _mediator.Send(Arg.Any(), Arg.Any()) diff --git a/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs b/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs index 07dc7a086..8ca6377e9 100644 --- a/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlaylistControllerTests.cs @@ -7,8 +7,10 @@ using ErsatzTV.Controllers.Api.Requests; using ErsatzTV.Core; using ErsatzTV.Core.Api.MediaCollections; using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Errors; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -26,7 +28,12 @@ public class PlaylistControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new PlaylistController(_mediator); + _controller = new PlaylistController(_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() } + }; } private PlaylistController _controller = null!; @@ -204,7 +211,7 @@ public class PlaylistControllerTests public async Task GetById_Should_Return_200_For_Some() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); IActionResult result = await _controller.GetById(4, CancellationToken.None); @@ -226,7 +233,7 @@ public class PlaylistControllerTests public async Task GetItems_Should_Return_200_And_Flatten_Names() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(new List { @@ -280,11 +287,24 @@ public class PlaylistControllerTests await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); } + [Test] + public async Task GetItems_Should_Set_ETag_From_Playlist_Version() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 9))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); + + await _controller.GetItems(4, CancellationToken.None); + + _controller.Response.Headers.ETag.ToString().ShouldBe("\"9\""); + } + [Test] public async Task Create_Should_Return_201_And_Map_Request() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Right(new PlaylistViewModel(9, 1, "Kids", false))); + .Returns(Right(new PlaylistViewModel(9, 1, "Kids", false, 1))); IActionResult result = await _controller.Create( new CreatePlaylistRequest(1, "Kids"), @@ -313,8 +333,12 @@ public class PlaylistControllerTests [Test] public async Task Update_Should_Return_200_With_Items_And_Map_Request_By_Array_Order() { + // Existence pre-check reads version 1; the post-save re-query reads the bumped version 2 — + // the response ETag must carry the refreshed value (issue #253). _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns( + Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)), + Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 2))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right>(new List { @@ -346,6 +370,8 @@ public class PlaylistControllerTests result.ShouldBeOfType().Value.ShouldBeOfType>().Count .ShouldBe(1); + // On success the response carries the refreshed playlist's ETag. + _controller.Response.Headers.ETag.ToString().ShouldBe("\"2\""); await _mediator.Received(1).Send( Arg.Is(c => c.PlaylistId == 4 && @@ -358,6 +384,77 @@ public class PlaylistControllerTests Arg.Any()); } + [Test] + public async Task Update_Should_Return_400_On_Malformed_If_Match() + { + _controller.Request.Headers.IfMatch = "not-an-etag"; + + IActionResult result = await _controller.Update( + 4, + new ReplacePlaylistRequest("Kids", new List()), + CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } + + [Test] + public async Task Update_Should_Thread_If_Match_Version_Into_Command() + { + _controller.Request.Headers.IfMatch = "\"3\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 3))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>(new List())); + + IActionResult result = await _controller.Update( + 4, + new ReplacePlaylistRequest("Kids", new List()), + CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send( + Arg.Is(c => c.ExpectedVersion == Option.Some(3)), + Arg.Any()); + } + + [Test] + public async Task Update_Without_If_Match_Should_Force_Write() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>(new List())); + + await _controller.Update( + 4, + new ReplacePlaylistRequest("Kids", new List()), + CancellationToken.None); + + await _mediator.Received(1).Send( + Arg.Is(c => c.ExpectedVersion == Option.None), + Arg.Any()); + } + + [Test] + public async Task Update_Should_Return_412_On_Precondition_Failed() + { + _controller.Request.Headers.IfMatch = "\"2\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 5))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Left>(new PreconditionFailedError("stale"))); + + IActionResult result = await _controller.Update( + 4, + new ReplacePlaylistRequest("Kids", new List()), + CancellationToken.None); + + var objectResult = result.ShouldBeOfType(); + objectResult.StatusCode.ShouldBe(StatusCodes.Status412PreconditionFailed); + } + [Test] public async Task Update_Should_Return_404_When_Playlist_Missing() { @@ -377,7 +474,7 @@ public class PlaylistControllerTests public async Task Update_Should_Return_422_On_Validation_Error() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Left>(BaseError.New("bad item"))); @@ -393,7 +490,7 @@ public class PlaylistControllerTests public async Task Update_Should_Return_422_On_System_Playlist_And_Not_Replace_Items() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "System", true))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "System", true, 1))); IActionResult result = await _controller.Update( 4, @@ -410,7 +507,7 @@ public class PlaylistControllerTests public async Task Delete_Should_Return_204_On_Success() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.None); @@ -438,7 +535,7 @@ public class PlaylistControllerTests public async Task Delete_Should_Return_422_On_System_Playlist() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "System", true))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "System", true, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.Some(BaseError.New("Cannot delete system (generated) playlist"))); @@ -451,7 +548,7 @@ public class PlaylistControllerTests public async Task AddItems_Should_Return_204_And_Map_Request() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Right(Unit.Default)); @@ -488,7 +585,7 @@ public class PlaylistControllerTests public async Task AddItems_Should_Return_422_On_System_Playlist() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "System", true))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "System", true, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Left(BaseError.New("Cannot add items to system (generated) playlist"))); @@ -504,7 +601,7 @@ public class PlaylistControllerTests public async Task AddItems_Should_Return_422_On_Validation_Error() { _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false))); + .Returns(Option.Some(new PlaylistViewModel(4, 1, "Kids", false, 1))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Left(BaseError.New("Movie does not exist"))); diff --git a/ErsatzTV/Controllers/Api/PlaylistController.cs b/ErsatzTV/Controllers/Api/PlaylistController.cs index 85055617d..c2762af42 100644 --- a/ErsatzTV/Controllers/Api/PlaylistController.cs +++ b/ErsatzTV/Controllers/Api/PlaylistController.cs @@ -134,6 +134,9 @@ public class PlaylistController(IMediator mediator) : ControllerBase [HttpGet("/api/playlists/{id:int}/items", Name = "GetPlaylistItems")] [Tags("Playlists")] [EndpointSummary("Get the items in a playlist")] + [EndpointDescription( + "Returns the playlist's items and a strong ETag of the playlist's version. Pass that ETag back " + + "as If-Match on the replace (PUT) to detect a concurrent edit (issue #253).")] [EndpointGroupName("general")] [ProducesResponseType(typeof(List), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] @@ -145,6 +148,9 @@ public class PlaylistController(IMediator mediator) : ControllerBase return ApiResults.NotFoundProblem(); } + // The items GET returns children, not the root, so read the playlist's version for the ETag. + ConcurrencyHeaders.SetETag(Response, maybePlaylist.Map(p => p.Version).IfNone(0)); + List items = await mediator.Send(new GetPlaylistItems(id), cancellationToken); return new OkObjectResult(items.Map(ProjectToItemResponse).ToList()); } @@ -168,15 +174,33 @@ public class PlaylistController(IMediator mediator) : ControllerBase [HttpPut("/api/playlists/{id:int}", Name = "UpdatePlaylist")] [Tags("Playlists")] [EndpointSummary("Update a playlist (rename and replace its items)")] + [EndpointDescription( + "Replaces the playlist's name and its 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 Update( int id, [Required] [FromBody] ReplacePlaylistRequest 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 \"*\"." + }); + } + Option maybePlaylist = await mediator.Send(new GetPlaylistById(id), cancellationToken); if (maybePlaylist.IsNone) { @@ -195,10 +219,16 @@ public class PlaylistController(IMediator mediator) : ControllerBase } Either> result = - await mediator.Send(request.ToCommand(id), cancellationToken); - return result.Match( - Left: error => error.ToErrorResult(), - Right: items => (IActionResult)new OkObjectResult(items.Map(ProjectToItemResponse).ToList())); + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); + return await result.Match( + Left: error => Task.FromResult(error.ToErrorResult()), + Right: async items => + { + Option refreshed = + await mediator.Send(new GetPlaylistById(id), cancellationToken); + refreshed.Do(vm => ConcurrencyHeaders.SetETag(Response, vm.Version)); + return (IActionResult)new OkObjectResult(items.Map(ProjectToItemResponse).ToList()); + }); } [HttpDelete("/api/playlists/{id:int}", Name = "DeletePlaylist")] diff --git a/ErsatzTV/Controllers/Api/Requests/ReplacePlaylistRequest.cs b/ErsatzTV/Controllers/Api/Requests/ReplacePlaylistRequest.cs index 2e1ad4ef4..6f5bdc5f6 100644 --- a/ErsatzTV/Controllers/Api/Requests/ReplacePlaylistRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ReplacePlaylistRequest.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Linq; using ErsatzTV.Application.MediaCollections; using ErsatzTV.Core.Domain; +using LanguageExt; namespace ErsatzTV.Controllers.Api.Requests; @@ -56,8 +57,8 @@ public record PlaylistItemRequest( public record ReplacePlaylistRequest(string? Name, List? Items) { - public ReplacePlaylistItems ToCommand(int id) => - new(id, Name ?? string.Empty, BuildItems()); + public ReplacePlaylistItems ToCommand(int id, Option expectedVersion = default) => + new(id, Name ?? string.Empty, BuildItems(), expectedVersion); // Preview operates on the posted draft, so there is no persisted playlist id (0). public ReplacePlaylistItems ToReplaceCommand() => diff --git a/web/src/api/playlists.ts b/web/src/api/playlists.ts index bc6b4af49..c6535ab70 100644 --- a/web/src/api/playlists.ts +++ b/web/src/api/playlists.ts @@ -1,4 +1,4 @@ -import { ApiError, request } from './client'; +import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client'; import type { components } from './generated/v1'; export type AddItemsToPlaylistRequest = components['schemas']['AddItemsToPlaylistRequest']; @@ -41,13 +41,28 @@ export function getPlaylistItems(id: number): Promise { return request(`/api/playlists/${id}/items`); } +/** Load playlist items together with the playlist's concurrency ETag (issue #253). */ +export function getPlaylistItemsWithMeta(id: number): Promise> { + return requestWithMeta(`/api/playlists/${id}/items`); +} + export function createPlaylist(body: CreatePlaylistRequest): Promise { return request('/api/playlists', { body, method: 'POST' }); } -// PUT = rename + replace the full item list; returns the persisted (re-indexed) items. -export function updatePlaylist(id: number, body: ReplacePlaylistRequest): Promise { - return request(`/api/playlists/${id}`, { body, method: 'PUT' }); +// PUT = rename + replace the full item list; returns the persisted (re-indexed) items. 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). +export function updatePlaylist( + id: number, + body: ReplacePlaylistRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/playlists/${id}`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } export function deletePlaylist(id: number): Promise { diff --git a/web/src/screens/PlaylistsScreen.test.tsx b/web/src/screens/PlaylistsScreen.test.tsx index cea52a5a5..bc3890daf 100644 --- a/web/src/screens/PlaylistsScreen.test.tsx +++ b/web/src/screens/PlaylistsScreen.test.tsx @@ -206,6 +206,44 @@ describe('PlaylistsScreen', () => { }); }); + it('shows a conflict dialog and reloads when the playlist changed elsewhere (412)', async () => { + let putCount = 0; + const fetchMock = mockApi({ + onRequest: (url, method) => { + if (url === '/api/playlists/10' && method === 'PUT') { + putCount += 1; + if (putCount === 1) { + return new Response( + JSON.stringify({ status: 412, title: 'Precondition Failed', detail: 'stale' }), + { headers: { 'Content-Type': 'application/json' }, status: 412 } + ); + } + return jsonResponse([], 200); + } + return null; + } + }); + + render(); + + fireEvent.click(await screen.findByText('Bumps')); + expect(await screen.findByText('Favorites')).toBeInTheDocument(); + + const itemsGetCount = () => + fetchMock.mock.calls.filter(([u, init]) => u === '/api/playlists/10/items' && (init?.method ?? 'GET') === 'GET') + .length; + const before = itemsGetCount(); + + fireEvent.click(screen.getByRole('button', { name: 'Save playlist' })); + + // A 412 opens the "changed elsewhere" dialog rather than showing a generic save error. + expect(await screen.findByText(/Reload to get the latest version/i)).toBeInTheDocument(); + + // Reloading re-fetches the playlist items. + fireEvent.click(screen.getByRole('button', { name: /^Reload$/ })); + await waitFor(() => expect(itemsGetCount()).toBeGreaterThan(before)); + }); + it('disables the playback-order select for single media-item types', async () => { mockApi(); render(); diff --git a/web/src/screens/PlaylistsScreen.tsx b/web/src/screens/PlaylistsScreen.tsx index a67844e25..f6714dc5b 100644 --- a/web/src/screens/PlaylistsScreen.tsx +++ b/web/src/screens/PlaylistsScreen.tsx @@ -18,6 +18,7 @@ import { import { Badge, Button, Card, ConfirmDialog, Dialog, IconButton, Input, Select, Spinner, Switch } from '../components'; import type { SelectOption } from '../components'; import { + ApiError, createPlaylist, createPlaylistGroup, deletePlaylist, @@ -27,7 +28,7 @@ import { getMultiCollections, getPlaylistById, getPlaylistGroups, - getPlaylistItems, + getPlaylistItemsWithMeta, getPlaylists, getSmartCollections, messageFromPlaylistError, @@ -379,17 +380,26 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o const [previewItems, setPreviewItems] = useState(null); const [previewMessage, setPreviewMessage] = useState(null); const [previewing, setPreviewing] = useState(false); + const [conflictOpen, setConflictOpen] = useState(false); + const [reloadKey, setReloadKey] = useState(0); const activeRef = useRef(true); + // Concurrency ETag (issue #253): captured from the items GET, sent as If-Match on save, and + // replaced from the PUT response on every successful save. + const etagRef = useRef(null); useEffect(() => { activeRef.current = true; - Promise.all([getPlaylistById(playlistId), getPlaylistItems(playlistId)]) - .then(([playlist, loaded]) => { + // Read items + ETag FIRST, then the root metadata, so the ETag is never newer than the data + // the draft is built from (issue #253) — any resulting inconsistency fails safe via a 412 on + // save rather than a silent overwrite. + Promise.all([getPlaylistItemsWithMeta(playlistId), getPlaylistById(playlistId)]) + .then(([itemsMeta, playlist]) => { if (!activeRef.current) { return; } - const drafts = loaded.map(draftFromItem); + etagRef.current = itemsMeta.etag; + const drafts = itemsMeta.data.map(draftFromItem); setName(playlist.name); setItems(drafts); setSelectedKey(drafts.length === 1 ? drafts[0].key : null); @@ -404,7 +414,7 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o return () => { activeRef.current = false; }; - }, [playlistId]); + }, [playlistId, reloadKey]); const selectedItem = items.find((item) => item.key === selectedKey) ?? null; const selectedType = selectedItem?.collectionType; @@ -506,15 +516,28 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o setSaveError(null); try { - await updatePlaylist(playlistId, buildRequest()); + const { etag } = await updatePlaylist(playlistId, buildRequest(), etagRef.current); + etagRef.current = etag; onSaved(); } catch (error) { - setSaveError(messageFromPlaylistError(error, 'Unable to save playlist')); + if (error instanceof ApiError && error.status === 412) { + // Another edit landed since we loaded — force a reload rather than overwriting it (#253). + setConflictOpen(true); + } else { + setSaveError(messageFromPlaylistError(error, 'Unable to save playlist')); + } } finally { setSaving(false); } }; + const reloadAfterConflict = () => { + setConflictOpen(false); + setSaveError(null); + setState({ status: 'loading' }); + setReloadKey((key) => key + 1); + }; + const runPreview = async () => { setPreviewing(true); setPreviewMessage(null); @@ -774,6 +797,17 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o )} )} + + setConflictOpen(false)} + onConfirm={reloadAfterConflict} + open={conflictOpen} + title="Playlist changed elsewhere" + tone="danger" + /> ); } From 5c9f04fdec53a49ef276864fb41b21d7fba0a136 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:36:25 +0200 Subject: [PATCH 02/12] 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)} + /> ); } From 611924c0ee961cf2aaedc431dc53063706f62da3 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:36:35 +0200 Subject: [PATCH 03/12] feat(#253 PR2): optimistic-concurrency contract for Template and DecoTemplate Wire the frozen ETag/If-Match/412 recipe (Block reference implementation) onto the Template and DecoTemplate aggregates: - ReplaceTemplateItems / ReplaceDecoTemplateItems commands gain Option ExpectedVersion; ToCommand() on the request DTOs threads it through from If-Match. - Handlers introduce the version check as a standalone Either after validation (never via Apply), bump Version unconditionally before saving, and persist through SaveChangesWithConcurrencyGuard so a losing writer maps to 412 instead of 500. DecoTemplate's post-commit playout Reset enqueue now only runs after a successful save. - TemplateViewModel / DecoTemplateViewModel carry Version (header-only, not echoed in the response body), populated in Mapper. - TemplateController / DecoTemplateController: GET items emits a strong ETag of the root's version; PUT parses If-Match (400 on malformed), threads the expected version into the command, and returns the new ETag from the refreshed root on success. Both PUT actions now use the handler's returned item list directly instead of re-querying items. - SPA: templates.ts / decoTemplates.ts gain getXItemsWithMeta and an If-Match-aware replaceX; TemplateEditor / DecoTemplateEditor hold the ETag in a ref, read items-with-meta first on load, and open a "changed elsewhere" ConfirmDialog on a 412 instead of navigating away. Tests: new ReplaceTemplateItemsHandlerConcurrencyTests / ReplaceDecoTemplateItemsHandlerConcurrencyTests mirror the Block concurrency contract tests (stale/matching/absent If-Match, no-op bump, racing-save 412, non-vacuous backstop). TemplateControllerTests / DecoTemplateControllerTests gain ETag/If-Match/412 coverage. TemplatesScreen.test.tsx / DecoTemplatesScreen.test.tsx gain a 412 conflict-dialog test mirroring BlocksScreen's. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/ReplaceDecoTemplateItems.cs | 3 +- .../ReplaceDecoTemplateItemsHandler.cs | 75 +++++--- .../Commands/ReplaceTemplateItems.cs | 7 +- .../Commands/ReplaceTemplateItemsHandler.cs | 39 ++++- .../Scheduling/DecoTemplateViewModel.cs | 2 +- ErsatzTV.Application/Scheduling/Mapper.cs | 5 +- .../Scheduling/TemplateViewModel.cs | 2 +- ...ecoTemplateItemsHandlerConcurrencyTests.cs | 164 ++++++++++++++++++ ...aceTemplateItemsHandlerConcurrencyTests.cs | 163 +++++++++++++++++ .../DecoTemplateControllerTests.cs | 108 ++++++++++-- .../Controllers/PlayoutControllerTests.cs | 8 +- .../Controllers/TemplateControllerTests.cs | 108 ++++++++++-- .../Controllers/Api/DecoTemplateController.cs | 39 ++++- .../Requests/ReplaceDecoTemplateRequest.cs | 8 +- .../Api/Requests/ReplaceTemplateRequest.cs | 5 +- .../Controllers/Api/TemplateController.cs | 36 +++- web/src/api/decoTemplates.ts | 23 ++- web/src/api/templates.ts | 23 ++- web/src/screens/DecoTemplatesScreen.test.tsx | 38 ++++ web/src/screens/DecoTemplatesScreen.tsx | 83 ++++++--- web/src/screens/TemplatesScreen.test.tsx | 37 ++++ web/src/screens/TemplatesScreen.tsx | 70 ++++++-- 22 files changed, 926 insertions(+), 120 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Scheduling/ReplaceDecoTemplateItemsHandlerConcurrencyTests.cs create mode 100644 ErsatzTV.Tests/Application/Scheduling/ReplaceTemplateItemsHandlerConcurrencyTests.cs diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItems.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItems.cs index 9669e5223..85c178b2d 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItems.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItems.cs @@ -6,5 +6,6 @@ public record ReplaceDecoTemplateItems( int DecoTemplateId, int DecoTemplateGroupId, string Name, - List Items) + List Items, + Option ExpectedVersion = default) : IRequest>>; diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs index 52fccc29d..d765ac468 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceDecoTemplateItemsHandler.cs @@ -20,10 +20,19 @@ public class ReplaceDecoTemplateItemsHandler( { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); Validation validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(ps => Persist(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(decoTemplate => decoTemplate.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: decoTemplate => Persist(dbContext, request, decoTemplate, cancellationToken), + Left: error => Task.FromResult>>(error)); } - private async Task> Persist( + private async Task>> Persist( TvContext dbContext, ReplaceDecoTemplateItems request, DecoTemplate decoTemplate, @@ -36,33 +45,49 @@ public class ReplaceDecoTemplateItemsHandler( decoTemplate.Items = request.Items.Map(i => BuildItem(decoTemplate, i)).ToList(); - await dbContext.SaveChangesAsync(cancellationToken); + // Unconditional bump: EF emits the root UPDATE only when a scalar actually differs, so a + // same-value/no-op save would otherwise write no root row and neither fire the concurrency + // token nor rotate other clients' ETags. Bumping guarantees both on every save (issue #253). + decoTemplate.Version++; - // Deco/break/default-filler content is only (re)applied during a Reset build (a Continue keeps the - // frozen DecoDefault filler items), and BlockKey change-detection has no deco dimension, so nothing - // self-heals a deco-template edit — the editor returned 200 but built filler stayed stale until a - // manual Reset (#251). Enqueue a Reset for every playout that references this deco template. This - // whole post-commit invalidation runs with CancellationToken.None (audit #22 policy): once the edit - // is committed, a late request cancellation must not be able to abort the affected-playout query OR - // the enqueue and leave content stale. - List playoutIds = await dbContext.PlayoutTemplates - .Where(pt => pt.DecoTemplateId == decoTemplate.Id) - .Select(pt => pt.PlayoutId) - .Distinct() - .ToListAsync(CancellationToken.None); + // 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. + Either saved = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + return await saved.Match( + Right: async _ => + { + // Deco/break/default-filler content is only (re)applied during a Reset build (a Continue keeps + // the frozen DecoDefault filler items), and BlockKey change-detection has no deco dimension, so + // nothing self-heals a deco-template edit — the editor returned 200 but built filler stayed + // stale until a manual Reset (#251). Enqueue a Reset for every playout that references this + // deco template. This whole post-commit invalidation runs with CancellationToken.None (audit + // #22 policy): once the edit is committed, a late request cancellation must not be able to + // abort the affected-playout query OR the enqueue and leave content stale. Only runs after a + // successful save (issue #253) — a 412/422 must not enqueue a Reset for content that was never + // persisted. + List playoutIds = await dbContext.PlayoutTemplates + .Where(pt => pt.DecoTemplateId == decoTemplate.Id) + .Select(pt => pt.PlayoutId) + .Distinct() + .ToListAsync(CancellationToken.None); - foreach (int playoutId in playoutIds) - { - await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Reset), CancellationToken.None); - } + foreach (int playoutId in playoutIds) + { + await channel.WriteAsync( + new BuildPlayout(playoutId, PlayoutBuildMode.Reset), + CancellationToken.None); + } - await dbContext.Entry(decoTemplate) - .Collection(t => t.Items) - .Query() - .Include(i => i.Deco) - .LoadAsync(cancellationToken); + await dbContext.Entry(decoTemplate) + .Collection(t => t.Items) + .Query() + .Include(i => i.Deco) + .LoadAsync(cancellationToken); - return decoTemplate.Items.Map(Mapper.ProjectToViewModel).ToList(); + return Right>( + decoTemplate.Items.Map(Mapper.ProjectToViewModel).ToList()); + }, + Left: error => Task.FromResult(Left>(error))); } private static DecoTemplateItem BuildItem(DecoTemplate decoTemplate, ReplaceDecoTemplateItem item) => diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItems.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItems.cs index e00cfef5f..f8ff2c1a8 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItems.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItems.cs @@ -2,5 +2,10 @@ using ErsatzTV.Core; namespace ErsatzTV.Application.Scheduling; -public record ReplaceTemplateItems(int TemplateGroupId, int TemplateId, string Name, List Items) +public record ReplaceTemplateItems( + int TemplateGroupId, + int TemplateId, + string Name, + List Items, + Option ExpectedVersion = default) : IRequest>>; diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs index 055f3c0e4..bc67427e2 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplaceTemplateItemsHandler.cs @@ -15,10 +15,19 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory dbContextF { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); Validation validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(ps => Persist(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(template => template.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: template => Persist(dbContext, request, template, cancellationToken), + Left: error => Task.FromResult>>(error)); } - private static async Task> Persist( + private static async Task>> Persist( TvContext dbContext, ReplaceTemplateItems request, Template template, @@ -30,7 +39,10 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory dbContextF dbContext.RemoveRange(template.Items); template.Items = request.Items.Map(i => BuildItem(template, i)).ToList(); - await dbContext.SaveChangesAsync(cancellationToken); + // Unconditional bump: EF emits the root UPDATE only when a scalar actually differs, so a + // same-value/no-op save would otherwise write no root row and neither fire the concurrency + // token nor rotate other clients' ETags. Bumping guarantees both on every save (issue #253). + template.Version++; // TODO: refresh any playouts that use this schedule // foreach (Playout playout in programSchedule.Playouts) @@ -38,13 +50,22 @@ public class ReplaceTemplateItemsHandler(IDbContextFactory dbContextF // await _channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh)); // } - await dbContext.Entry(template) - .Collection(t => t.Items) - .Query() - .Include(i => i.Block) - .LoadAsync(cancellationToken); + // 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. + Either saved = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + return await saved.Match( + Right: async _ => + { + await dbContext.Entry(template) + .Collection(t => t.Items) + .Query() + .Include(i => i.Block) + .LoadAsync(cancellationToken); - return template.Items.Map(Mapper.ProjectToViewModel).ToList(); + return Right>( + template.Items.Map(Mapper.ProjectToViewModel).ToList()); + }, + Left: error => Task.FromResult(Left>(error))); } private static TemplateItem BuildItem(Template template, ReplaceTemplateItem item) => diff --git a/ErsatzTV.Application/Scheduling/DecoTemplateViewModel.cs b/ErsatzTV.Application/Scheduling/DecoTemplateViewModel.cs index c9543d63c..d1517d1c2 100644 --- a/ErsatzTV.Application/Scheduling/DecoTemplateViewModel.cs +++ b/ErsatzTV.Application/Scheduling/DecoTemplateViewModel.cs @@ -1,3 +1,3 @@ namespace ErsatzTV.Application.Scheduling; -public record DecoTemplateViewModel(int Id, int DecoTemplateGroupId, string GroupName, string Name); +public record DecoTemplateViewModel(int Id, int DecoTemplateGroupId, string GroupName, string Name, int Version); diff --git a/ErsatzTV.Application/Scheduling/Mapper.cs b/ErsatzTV.Application/Scheduling/Mapper.cs index d82d9325f..91e6356b5 100644 --- a/ErsatzTV.Application/Scheduling/Mapper.cs +++ b/ErsatzTV.Application/Scheduling/Mapper.cs @@ -65,7 +65,7 @@ internal static class Mapper new(templateGroup.Id, templateGroup.Name, templateGroup.Templates.Count); internal static TemplateViewModel ProjectToViewModel(Template template) => - new(template.Id, template.TemplateGroupId, template.TemplateGroup.Name, template.Name); + new(template.Id, template.TemplateGroupId, template.TemplateGroup.Name, template.Name, template.Version); internal static TemplateItemViewModel ProjectToViewModel(TemplateItem templateItem) { @@ -168,7 +168,8 @@ internal static class Mapper decoTemplate.Id, decoTemplate.DecoTemplateGroupId, decoTemplate.DecoTemplateGroup.Name, - decoTemplate.Name); + decoTemplate.Name, + decoTemplate.Version); } internal static DecoTemplateItemViewModel ProjectToViewModel(DecoTemplateItem decoTemplateItem) diff --git a/ErsatzTV.Application/Scheduling/TemplateViewModel.cs b/ErsatzTV.Application/Scheduling/TemplateViewModel.cs index cef2789bf..1e72952b0 100644 --- a/ErsatzTV.Application/Scheduling/TemplateViewModel.cs +++ b/ErsatzTV.Application/Scheduling/TemplateViewModel.cs @@ -1,3 +1,3 @@ namespace ErsatzTV.Application.Scheduling; -public record TemplateViewModel(int Id, int TemplateGroupId, string GroupName, string Name); +public record TemplateViewModel(int Id, int TemplateGroupId, string GroupName, string Name, int Version); diff --git a/ErsatzTV.Tests/Application/Scheduling/ReplaceDecoTemplateItemsHandlerConcurrencyTests.cs b/ErsatzTV.Tests/Application/Scheduling/ReplaceDecoTemplateItemsHandlerConcurrencyTests.cs new file mode 100644 index 000000000..3d265d0c8 --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/ReplaceDecoTemplateItemsHandlerConcurrencyTests.cs @@ -0,0 +1,164 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Playouts; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Errors; +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.Scheduling; + +/// +/// Contract tests for the #253 optimistic-concurrency mechanic on the DecoTemplate aggregate, +/// mirroring (the Block reference +/// implementation): the handler pre-check (stale If-Match → 412), the force-write path (no +/// If-Match), the unconditional Version bump on every save, 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 DecoTemplate and the losing save +/// silently succeeds instead of mapping to a . +/// +[TestFixture] +public class ReplaceDecoTemplateItemsHandlerConcurrencyTests +{ + private InMemoryTvContext _db = null!; + private Channel _channel = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _channel = System.Threading.Channels.Channel.CreateUnbounded(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private async Task SeedDecoTemplateAsync(int version) + { + await using TvContext ctx = _db.CreateContext(); + ctx.Decos.Add(new Deco { Id = 10, DecoGroupId = 1, Name = "D" }); + ctx.DecoTemplates.Add( + new DecoTemplate + { + Id = 1, + DecoTemplateGroupId = 1, + Name = "Weekday", + Version = version, + Items = new List() + }); + await ctx.SaveChangesAsync(); + } + + private ReplaceDecoTemplateItemsHandler CreateHandler() => new(_db.Factory, _channel.Writer); + + private static ReplaceDecoTemplateItems Command(Option expectedVersion) => + new( + DecoTemplateId: 1, + DecoTemplateGroupId: 1, + Name: "Weekday", + Items: [new ReplaceDecoTemplateItem(DecoId: 10, StartTime: TimeSpan.Zero, EndTime: TimeSpan.FromHours(1))], + ExpectedVersion: expectedVersion); + + private async Task ReadVersionAsync() + { + await using TvContext ctx = _db.CreateContext(); + return await ctx.DecoTemplates.Where(t => t.Id == 1).Select(t => t.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 SeedDecoTemplateAsync(version: 2); + ReplaceDecoTemplateItemsHandler handler = CreateHandler(); + + Either> result = + await handler.Handle(Command(Some(1)), CancellationToken.None); + + LeftOrNull(result).ShouldBeOfType(); + + // The pre-check runs before any mutation: version unchanged, no items written. + (await ReadVersionAsync()).ShouldBe(2); + await using TvContext ctx = _db.CreateContext(); + (await ctx.DecoTemplateItems.CountAsync(i => i.DecoTemplateId == 1)).ShouldBe(0); + } + + [Test] + public async Task Matching_If_Match_Should_Succeed_And_Bump_Version() + { + await SeedDecoTemplateAsync(version: 2); + ReplaceDecoTemplateItemsHandler handler = CreateHandler(); + + 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 SeedDecoTemplateAsync(version: 2); + ReplaceDecoTemplateItemsHandler handler = CreateHandler(); + + // 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 Save_Should_Bump_Version_Even_When_Content_Is_Unchanged() + { + await SeedDecoTemplateAsync(version: 5); + ReplaceDecoTemplateItemsHandler handler = CreateHandler(); + + // Same content twice: the unconditional bump (M1) must still rotate the version each time, + // otherwise a no-op PUT-back would not fire the token or 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 SeedDecoTemplateAsync(version: 1); + + // Two writers load the same version, then both bump-and-save. The IsConcurrencyToken guard on + // DecoTemplate 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(); + + DecoTemplate winner = await ctxWinner.DecoTemplates.SingleAsync(t => t.Id == 1); + DecoTemplate loser = await ctxLoser.DecoTemplates.SingleAsync(t => t.Id == 1); + + winner.Version++; + Either winnerResult = await ctxWinner.SaveChangesWithConcurrencyGuard(CancellationToken.None); + winnerResult.IsRight.ShouldBeTrue(); + + loser.Version++; + Either loserResult = await ctxLoser.SaveChangesWithConcurrencyGuard(CancellationToken.None); + + LeftOrNull(loserResult).ShouldBeOfType(); + + // Non-vacuous: the winner's write stuck at exactly its +1; the loser did not overwrite it. + (await ReadVersionAsync()).ShouldBe(2); + } +} diff --git a/ErsatzTV.Tests/Application/Scheduling/ReplaceTemplateItemsHandlerConcurrencyTests.cs b/ErsatzTV.Tests/Application/Scheduling/ReplaceTemplateItemsHandlerConcurrencyTests.cs new file mode 100644 index 000000000..4e71c0a91 --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/ReplaceTemplateItemsHandlerConcurrencyTests.cs @@ -0,0 +1,163 @@ +using ErsatzTV.Application; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Errors; +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.Scheduling; + +/// +/// Contract tests for the #253 optimistic-concurrency mechanic on the Template aggregate, +/// mirroring (the Block reference +/// implementation): the handler pre-check (stale If-Match → 412), the force-write path (no +/// If-Match), the unconditional Version bump on every save, 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 Template and the losing save +/// silently succeeds instead of mapping to a . +/// +[TestFixture] +public class ReplaceTemplateItemsHandlerConcurrencyTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private async Task SeedTemplateAsync(int version) + { + await using TvContext ctx = _db.CreateContext(); + ctx.Blocks.Add( + new Block + { + Id = 10, + BlockGroupId = 1, + Name = "Morning", + Minutes = 30, + StopScheduling = BlockStopScheduling.AfterDurationEnd, + Items = new List() + }); + ctx.Templates.Add( + new Template + { + Id = 1, + TemplateGroupId = 1, + Name = "Weekday", + Version = version, + Items = new List() + }); + await ctx.SaveChangesAsync(); + } + + private static ReplaceTemplateItems Command(Option expectedVersion) => + new( + 1, + 1, + "Weekday", + new List { new(10, TimeSpan.Zero) }, + expectedVersion); + + private async Task ReadVersionAsync() + { + await using TvContext ctx = _db.CreateContext(); + return await ctx.Templates.Where(t => t.Id == 1).Select(t => t.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 SeedTemplateAsync(version: 2); + var handler = new ReplaceTemplateItemsHandler(_db.Factory); + + Either> result = + await handler.Handle(Command(Some(1)), CancellationToken.None); + + LeftOrNull(result).ShouldBeOfType(); + + // The pre-check runs before any mutation: version unchanged, no items written. + (await ReadVersionAsync()).ShouldBe(2); + await using TvContext ctx = _db.CreateContext(); + (await ctx.TemplateItems.CountAsync(i => i.TemplateId == 1)).ShouldBe(0); + } + + [Test] + public async Task Matching_If_Match_Should_Succeed_And_Bump_Version() + { + await SeedTemplateAsync(version: 2); + var handler = new ReplaceTemplateItemsHandler(_db.Factory); + + 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 SeedTemplateAsync(version: 2); + var handler = new ReplaceTemplateItemsHandler(_db.Factory); + + // 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 Save_Should_Bump_Version_Even_When_Content_Is_Unchanged() + { + await SeedTemplateAsync(version: 5); + var handler = new ReplaceTemplateItemsHandler(_db.Factory); + + // Same content twice: the unconditional bump (M1) must still rotate the version each time, + // otherwise a no-op PUT-back would not fire the token or 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 SeedTemplateAsync(version: 1); + + // Two writers load the same version, then both bump-and-save. The IsConcurrencyToken guard on + // Template 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(); + + Template winner = await ctxWinner.Templates.SingleAsync(t => t.Id == 1); + Template loser = await ctxLoser.Templates.SingleAsync(t => t.Id == 1); + + winner.Version++; + Either winnerResult = await ctxWinner.SaveChangesWithConcurrencyGuard(CancellationToken.None); + winnerResult.IsRight.ShouldBeTrue(); + + loser.Version++; + Either loserResult = await ctxLoser.SaveChangesWithConcurrencyGuard(CancellationToken.None); + + LeftOrNull(loserResult).ShouldBeOfType(); + + // Non-vacuous: the winner's write stuck at exactly its +1; the loser did not overwrite it. + (await ReadVersionAsync()).ShouldBe(2); + } +} diff --git a/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs b/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs index 6ebfe6754..044c83450 100644 --- a/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/DecoTemplateControllerTests.cs @@ -4,8 +4,10 @@ using ErsatzTV.Controllers.Api; using ErsatzTV.Controllers.Api.Requests; using ErsatzTV.Core; using ErsatzTV.Core.Api.Scheduling; +using ErsatzTV.Core.Errors; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -25,7 +27,12 @@ public class DecoTemplateControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new DecoTemplateController(_mediator); + _controller = new DecoTemplateController(_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] @@ -259,12 +266,8 @@ public class DecoTemplateControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.Some(MakeDecoTemplate(4, 7, "Morning"))); _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Right>([])); - _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(new List - { - MakeItem(10, "Morning Bumper", TimeSpan.FromHours(6), TimeSpan.FromHours(7)) - }); + .Returns(Right>( + [MakeItem(10, "Morning Bumper", TimeSpan.FromHours(6), TimeSpan.FromHours(7))])); IActionResult result = await _controller.Replace( 4, @@ -318,11 +321,96 @@ public class DecoTemplateControllerTests CancellationToken.None); result.ShouldBeOfType(); - await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); } - private static DecoTemplateViewModel MakeDecoTemplate(int id, int groupId, string name) => - new(id, groupId, "Group", name); + [Test] + public async Task GetItems_Should_Set_ETag_From_DecoTemplate_Version() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeDecoTemplate(4, 2, "Morning", version: 9))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); + + await _controller.GetItems(4, CancellationToken.None); + + _controller.Response.Headers.ETag.ToString().ShouldBe("\"9\""); + } + + [Test] + public async Task Replace_Should_Return_400_On_Malformed_If_Match() + { + _controller.Request.Headers.IfMatch = "not-an-etag"; + + IActionResult result = await _controller.Replace( + 4, + new ReplaceDecoTemplateRequest("Morning", []), + CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } + + [Test] + public async Task Replace_Should_Thread_If_Match_Version_Into_Command() + { + _controller.Request.Headers.IfMatch = "\"3\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeDecoTemplate(4, 7, "Morning", version: 4))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>([])); + + IActionResult result = await _controller.Replace( + 4, + new ReplaceDecoTemplateRequest("Morning", []), + CancellationToken.None); + + result.ShouldBeOfType(); + // On success the response carries the refreshed deco template'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 Replace_Without_If_Match_Should_Force_Write() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeDecoTemplate(4, 7, "Morning"))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>([])); + + await _controller.Replace( + 4, + new ReplaceDecoTemplateRequest("Morning", []), + CancellationToken.None); + + await _mediator.Received(1).Send( + Arg.Is(c => c.ExpectedVersion == Option.None), + Arg.Any()); + } + + [Test] + public async Task Replace_Should_Return_412_On_Precondition_Failed() + { + _controller.Request.Headers.IfMatch = "\"2\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeDecoTemplate(4, 7, "Morning", version: 5))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Left>(new PreconditionFailedError("stale"))); + + IActionResult result = await _controller.Replace( + 4, + new ReplaceDecoTemplateRequest("Morning", []), + CancellationToken.None); + + var objectResult = result.ShouldBeOfType(); + objectResult.StatusCode.ShouldBe(StatusCodes.Status412PreconditionFailed); + } + + private static DecoTemplateViewModel MakeDecoTemplate(int id, int groupId, string name, int version = 1) => + new(id, groupId, "Group", name, version); private static DecoTemplateItemViewModel MakeItem(int decoId, string decoName, TimeSpan startTime, TimeSpan endTime) { diff --git a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs index d0af315c4..a93e0c1a4 100644 --- a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs @@ -1108,7 +1108,7 @@ public class PlayoutControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns([MakeTemplateViewModel(7), MakeTemplateViewModel(8)]); _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Option.Some(new DecoTemplateViewModel(5, 1, "G", "DT"))); + .Returns(Option.Some(new DecoTemplateViewModel(5, 1, "G", "DT", 0))); _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.None); _mediator.Send(Arg.Any(), Arg.Any()) @@ -1250,13 +1250,13 @@ public class PlayoutControllerTests new(id, $"Schedule {id}", false, false, false, false, FixedStartTimeBehavior.Strict); private static TemplateViewModel MakeTemplateViewModel(int id) => - new(id, 1, "Group", $"Template {id}"); + new(id, 1, "Group", $"Template {id}", 0); private static PlayoutTemplateViewModel MakeTemplateVm(int id, int index, int templateId, int? decoTemplateId) => new( id, - new TemplateViewModel(templateId, 1, "Group", $"Template {templateId}"), - decoTemplateId is { } dtId ? new DecoTemplateViewModel(dtId, 1, "DGroup", $"Deco {dtId}") : null, + new TemplateViewModel(templateId, 1, "Group", $"Template {templateId}", 0), + decoTemplateId is { } dtId ? new DecoTemplateViewModel(dtId, 1, "DGroup", $"Deco {dtId}", 0) : null, index, [], [], diff --git a/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs b/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs index a56f47017..4a5f9c595 100644 --- a/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/TemplateControllerTests.cs @@ -4,8 +4,10 @@ using ErsatzTV.Controllers.Api; using ErsatzTV.Controllers.Api.Requests; using ErsatzTV.Core; using ErsatzTV.Core.Api.Scheduling; +using ErsatzTV.Core.Errors; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -25,7 +27,12 @@ public class TemplateControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new TemplateController(_mediator); + _controller = new TemplateController(_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] @@ -248,12 +255,8 @@ public class TemplateControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(Option.Some(MakeTemplate(4, 7, "Morning"))); _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(Right>([])); - _mediator.Send(Arg.Any(), Arg.Any()) - .Returns(new List - { - MakeItem(10, "Cartoons", TimeSpan.FromHours(6), 60) - }); + .Returns(Right>( + [MakeItem(10, "Cartoons", TimeSpan.FromHours(6), 60)])); IActionResult result = await _controller.Replace( 4, @@ -305,7 +308,92 @@ public class TemplateControllerTests CancellationToken.None); result.ShouldBeOfType(); - await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } + + [Test] + public async Task GetItems_Should_Set_ETag_From_Template_Version() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeTemplate(4, 2, "Morning", version: 9))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new List()); + + await _controller.GetItems(4, CancellationToken.None); + + _controller.Response.Headers.ETag.ToString().ShouldBe("\"9\""); + } + + [Test] + public async Task Replace_Should_Return_400_On_Malformed_If_Match() + { + _controller.Request.Headers.IfMatch = "not-an-etag"; + + IActionResult result = await _controller.Replace( + 4, + new ReplaceTemplateRequest("Morning", []), + CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } + + [Test] + public async Task Replace_Should_Thread_If_Match_Version_Into_Command() + { + _controller.Request.Headers.IfMatch = "\"3\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeTemplate(4, 7, "Morning", version: 4))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>([])); + + IActionResult result = await _controller.Replace( + 4, + new ReplaceTemplateRequest("Morning", []), + CancellationToken.None); + + result.ShouldBeOfType(); + // On success the response carries the refreshed template'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 Replace_Without_If_Match_Should_Force_Write() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeTemplate(4, 7, "Morning"))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right>([])); + + await _controller.Replace( + 4, + new ReplaceTemplateRequest("Morning", []), + CancellationToken.None); + + await _mediator.Received(1).Send( + Arg.Is(c => c.ExpectedVersion == Option.None), + Arg.Any()); + } + + [Test] + public async Task Replace_Should_Return_412_On_Precondition_Failed() + { + _controller.Request.Headers.IfMatch = "\"2\""; + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(MakeTemplate(4, 7, "Morning", version: 5))); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Left>(new PreconditionFailedError("stale"))); + + IActionResult result = await _controller.Replace( + 4, + new ReplaceTemplateRequest("Morning", []), + CancellationToken.None); + + var objectResult = result.ShouldBeOfType(); + objectResult.StatusCode.ShouldBe(StatusCodes.Status412PreconditionFailed); } [Test] @@ -359,8 +447,8 @@ public class TemplateControllerTests result.ShouldBeOfType(); } - private static TemplateViewModel MakeTemplate(int id, int groupId, string name) => - new(id, groupId, "Group", name); + private static TemplateViewModel MakeTemplate(int id, int groupId, string name, int version = 1) => + new(id, groupId, "Group", name, version); private static TemplateItemViewModel MakeItem(int blockId, string blockName, TimeSpan startTime, int minutes) { diff --git a/ErsatzTV/Controllers/Api/DecoTemplateController.cs b/ErsatzTV/Controllers/Api/DecoTemplateController.cs index 82a316239..1c8dc80fb 100644 --- a/ErsatzTV/Controllers/Api/DecoTemplateController.cs +++ b/ErsatzTV/Controllers/Api/DecoTemplateController.cs @@ -143,6 +143,9 @@ public class DecoTemplateController(IMediator mediator) : ControllerBase [HttpGet("/api/deco-templates/{id:int}/items")] [Tags("DecoTemplates")] [EndpointSummary("Get deco template items")] + [EndpointDescription( + "Returns the deco template's items and a strong ETag of the deco template's version. Pass that ETag " + + "back as If-Match on the replace (PUT) to detect a concurrent edit (issue #253).")] [EndpointGroupName("general")] [ProducesResponseType(typeof(List), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] @@ -154,6 +157,9 @@ public class DecoTemplateController(IMediator mediator) : ControllerBase return ApiResults.NotFoundProblem(); } + // The items GET returns children, not the root, so read the deco template's version for the ETag. + ConcurrencyHeaders.SetETag(Response, decoTemplate.Map(t => t.Version).IfNone(0)); + List items = await mediator.Send(new GetDecoTemplateItems(id), cancellationToken); return new OkObjectResult( items.OrderBy(i => i.StartTime.TimeOfDay).Map(ProjectToResponseModel).ToList()); @@ -164,16 +170,32 @@ public class DecoTemplateController(IMediator mediator) : ControllerBase [EndpointSummary("Replace a deco template and its items")] [EndpointDescription( "Replaces the deco template's name and its full item list. Each item assigns a deco to a start/end time " + - "of day; an end time of 00:00:00 means the item runs to the end of the day. Items must not overlap.")] + "of day; an end time of 00:00:00 means the item runs to the end of the day. Items must not overlap. " + + "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(DecoTemplateWithItemsResponseModel), 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 Replace( int id, [Required] [FromBody] ReplaceDecoTemplateRequest 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 \"*\"." + }); + } + Option maybeDecoTemplate = await mediator.Send(new GetDecoTemplateById(id), cancellationToken); if (maybeDecoTemplate.IsNone) @@ -184,18 +206,23 @@ public class DecoTemplateController(IMediator mediator) : ControllerBase int decoTemplateGroupId = maybeDecoTemplate.Map(t => t.DecoTemplateGroupId).IfNone(0); Either> result = - await mediator.Send(request.ToCommand(decoTemplateGroupId, id), cancellationToken); + await mediator.Send( + request.ToCommand(decoTemplateGroupId, id, ifMatch.ExpectedVersion), + cancellationToken); return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), - Right: async _ => + Right: async items => { Option refreshed = await mediator.Send(new GetDecoTemplateById(id), cancellationToken); - List items = - await mediator.Send(new GetDecoTemplateItems(id), cancellationToken); return refreshed.Match( - Some: vm => (IActionResult)new OkObjectResult(ProjectToWithItemsResponseModel(vm, items)), + Some: vm => + { + // Return the new ETag so a same-tab second save doesn't 412 against its own write. + ConcurrencyHeaders.SetETag(Response, vm.Version); + return (IActionResult)new OkObjectResult(ProjectToWithItemsResponseModel(vm, items)); + }, None: () => ApiResults.NotFoundProblem()); }); } diff --git a/ErsatzTV/Controllers/Api/Requests/ReplaceDecoTemplateRequest.cs b/ErsatzTV/Controllers/Api/Requests/ReplaceDecoTemplateRequest.cs index e649a6a1e..734655928 100644 --- a/ErsatzTV/Controllers/Api/Requests/ReplaceDecoTemplateRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ReplaceDecoTemplateRequest.cs @@ -4,10 +4,14 @@ namespace ErsatzTV.Controllers.Api.Requests; public record ReplaceDecoTemplateRequest(string Name, List Items) { - public ReplaceDecoTemplateItems ToCommand(int decoTemplateGroupId, int decoTemplateId) => + public ReplaceDecoTemplateItems ToCommand( + int decoTemplateGroupId, + int decoTemplateId, + Option expectedVersion = default) => new( decoTemplateId, decoTemplateGroupId, Name, - (Items ?? []).Select(item => item.ToReplaceItem()).ToList()); + (Items ?? []).Select(item => item.ToReplaceItem()).ToList(), + expectedVersion); } diff --git a/ErsatzTV/Controllers/Api/Requests/ReplaceTemplateRequest.cs b/ErsatzTV/Controllers/Api/Requests/ReplaceTemplateRequest.cs index f2c5f20e0..330a76176 100644 --- a/ErsatzTV/Controllers/Api/Requests/ReplaceTemplateRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ReplaceTemplateRequest.cs @@ -4,10 +4,11 @@ namespace ErsatzTV.Controllers.Api.Requests; public record ReplaceTemplateRequest(string Name, List Items) { - public ReplaceTemplateItems ToCommand(int templateGroupId, int templateId) => + public ReplaceTemplateItems ToCommand(int templateGroupId, int templateId, Option expectedVersion = default) => new( templateGroupId, templateId, Name, - (Items ?? []).Select(item => item.ToReplaceItem()).ToList()); + (Items ?? []).Select(item => item.ToReplaceItem()).ToList(), + expectedVersion); } diff --git a/ErsatzTV/Controllers/Api/TemplateController.cs b/ErsatzTV/Controllers/Api/TemplateController.cs index e02f87209..624320e38 100644 --- a/ErsatzTV/Controllers/Api/TemplateController.cs +++ b/ErsatzTV/Controllers/Api/TemplateController.cs @@ -134,6 +134,9 @@ public class TemplateController(IMediator mediator) : ControllerBase [HttpGet("/api/templates/{id:int}/items")] [Tags("Templates")] [EndpointSummary("Get template items")] + [EndpointDescription( + "Returns the template's items and a strong ETag of the template's version. Pass that ETag back as " + + "If-Match on the replace (PUT) to detect a concurrent edit (issue #253).")] [EndpointGroupName("general")] [ProducesResponseType(typeof(List), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] @@ -145,6 +148,9 @@ public class TemplateController(IMediator mediator) : ControllerBase return ApiResults.NotFoundProblem(); } + // The items GET returns children, not the root, so read the template's version for the ETag. + ConcurrencyHeaders.SetETag(Response, template.Map(t => t.Version).IfNone(0)); + List items = await mediator.Send(new GetTemplateItems(id), cancellationToken); return new OkObjectResult( items.OrderBy(i => i.StartTime.TimeOfDay).Map(ProjectToResponseModel).ToList()); @@ -155,16 +161,32 @@ public class TemplateController(IMediator mediator) : ControllerBase [EndpointSummary("Replace a template and its items")] [EndpointDescription( "Replaces the template's name and its full item list. Each item assigns a block to a start time of day; " + - "items must not overlap (an item's end time is its start time plus the assigned block's duration).")] + "items must not overlap (an item's end time is its start time plus the assigned block's duration). " + + "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(TemplateWithItemsResponseModel), 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 Replace( int id, [Required] [FromBody] ReplaceTemplateRequest 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 \"*\"." + }); + } + Option maybeTemplate = await mediator.Send(new GetTemplateById(id), cancellationToken); if (maybeTemplate.IsNone) { @@ -174,16 +196,20 @@ public class TemplateController(IMediator mediator) : ControllerBase int templateGroupId = maybeTemplate.Map(t => t.TemplateGroupId).IfNone(0); Either> result = - await mediator.Send(request.ToCommand(templateGroupId, id), cancellationToken); + await mediator.Send(request.ToCommand(templateGroupId, id, ifMatch.ExpectedVersion), cancellationToken); return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), - Right: async _ => + Right: async items => { Option refreshed = await mediator.Send(new GetTemplateById(id), cancellationToken); - List items = await mediator.Send(new GetTemplateItems(id), cancellationToken); return refreshed.Match( - Some: vm => (IActionResult)new OkObjectResult(ProjectToWithItemsResponseModel(vm, items)), + Some: vm => + { + // Return the new ETag so a same-tab second save doesn't 412 against its own write. + ConcurrencyHeaders.SetETag(Response, vm.Version); + return (IActionResult)new OkObjectResult(ProjectToWithItemsResponseModel(vm, items)); + }, None: () => ApiResults.NotFoundProblem()); }); } diff --git a/web/src/api/decoTemplates.ts b/web/src/api/decoTemplates.ts index 3e88a15aa..09d01739a 100644 --- a/web/src/api/decoTemplates.ts +++ b/web/src/api/decoTemplates.ts @@ -1,4 +1,4 @@ -import { ApiError, request } from './client'; +import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client'; import type { components } from './generated/v1'; export type DecoTemplateGroup = components['schemas']['DecoTemplateGroupResponseModel']; @@ -46,8 +46,25 @@ export function getDecoTemplateItems(id: number): Promise { return request(`/api/deco-templates/${id}/items`); } -export function replaceDecoTemplate(id: number, body: ReplaceDecoTemplateRequest): Promise { - return request(`/api/deco-templates/${id}`, { body, method: 'PUT' }); +/** Load deco template items together with the deco template's concurrency ETag (issue #253). */ +export function getDecoTemplateItemsWithMeta(id: number): Promise> { + return requestWithMeta(`/api/deco-templates/${id}/items`); +} + +/** + * Replace a deco template. 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). + */ +export function replaceDecoTemplate( + id: number, + body: ReplaceDecoTemplateRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/deco-templates/${id}`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } export function messageFromDecoTemplateError(error: unknown, fallback = 'Unable to load deco templates'): string { diff --git a/web/src/api/templates.ts b/web/src/api/templates.ts index d4788d03f..c91c28e9d 100644 --- a/web/src/api/templates.ts +++ b/web/src/api/templates.ts @@ -1,4 +1,4 @@ -import { ApiError, request } from './client'; +import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client'; import type { components } from './generated/v1'; export type TemplateGroup = components['schemas']['TemplateGroupResponseModel']; @@ -47,8 +47,25 @@ export function getTemplateItems(id: number): Promise { return request(`/api/templates/${id}/items`); } -export function replaceTemplate(id: number, body: ReplaceTemplateRequest): Promise { - return request(`/api/templates/${id}`, { body, method: 'PUT' }); +/** Load template items together with the template's concurrency ETag (issue #253). */ +export function getTemplateItemsWithMeta(id: number): Promise> { + return requestWithMeta(`/api/templates/${id}/items`); +} + +/** + * Replace a template. 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). + */ +export function replaceTemplate( + id: number, + body: ReplaceTemplateRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/templates/${id}`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } export function copyTemplate(id: number, body: CopyTemplateRequest): Promise