From 1a8c0f60defa184e2eb2f6328ede326836c0fc41 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:33:40 +0200 Subject: [PATCH] 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" + /> ); }