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.
This commit is contained in:
@@ -32,6 +32,10 @@ public class AddEpisodeToPlaylistHandler(IDbContextFactory<TvContext> 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;
|
||||
}
|
||||
|
||||
@@ -69,6 +69,9 @@ public class AddItemsToPlaylistHandler : IRequestHandler<AddItemsToPlaylist, Eit
|
||||
}
|
||||
}
|
||||
|
||||
// Mutates the playlist's editor-visible item list, so bump the concurrency token (issue #253).
|
||||
playlist.Version++;
|
||||
|
||||
await dbContext.SaveChangesAsync(cancellationToken);
|
||||
|
||||
return Unit.Default;
|
||||
|
||||
@@ -32,6 +32,10 @@ public class AddMovieToPlaylistHandler(IDbContextFactory<TvContext> 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;
|
||||
}
|
||||
|
||||
@@ -32,6 +32,10 @@ public class AddSeasonToPlaylistHandler(IDbContextFactory<TvContext> 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;
|
||||
}
|
||||
|
||||
@@ -32,6 +32,10 @@ public class AddShowToPlaylistHandler(IDbContextFactory<TvContext> 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;
|
||||
}
|
||||
|
||||
@@ -2,5 +2,9 @@ using ErsatzTV.Core;
|
||||
|
||||
namespace ErsatzTV.Application.MediaCollections;
|
||||
|
||||
public record ReplacePlaylistItems(int PlaylistId, string Name, List<ReplacePlaylistItem> Items)
|
||||
public record ReplacePlaylistItems(
|
||||
int PlaylistId,
|
||||
string Name,
|
||||
List<ReplacePlaylistItem> Items,
|
||||
Option<int> ExpectedVersion = default)
|
||||
: IRequest<Either<BaseError, List<PlaylistItemViewModel>>>;
|
||||
|
||||
@@ -15,10 +15,21 @@ public class ReplacePlaylistItemsHandler(IDbContextFactory<TvContext> dbContextF
|
||||
{
|
||||
await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
|
||||
Validation<BaseError, Playlist> 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<BaseError> to a single BaseError (the native
|
||||
// Validation.ToEither() would keep Seq and is called explicitly here to avoid that shadow).
|
||||
Either<BaseError, Playlist> 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<Either<BaseError, List<PlaylistItemViewModel>>>(error));
|
||||
}
|
||||
|
||||
private static async Task<List<PlaylistItemViewModel>> Persist(
|
||||
private static async Task<Either<BaseError, List<PlaylistItemViewModel>>> Persist(
|
||||
TvContext dbContext,
|
||||
ReplacePlaylistItems request,
|
||||
Playlist playlist,
|
||||
@@ -30,9 +41,15 @@ public class ReplacePlaylistItemsHandler(IDbContextFactory<TvContext> 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<BaseError, Unit> saved = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken);
|
||||
return saved.Map(_ => playlist.Items.Map(Mapper.ProjectToViewModel).ToList());
|
||||
}
|
||||
|
||||
private static PlaylistItem BuildItem(Playlist playlist, int index, ReplacePlaylistItem item) =>
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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);
|
||||
|
||||
+155
@@ -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;
|
||||
|
||||
/// <summary>
|
||||
/// 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
|
||||
/// <c>IsConcurrencyToken()</c> config on Playlist and the losing save silently succeeds instead of
|
||||
/// mapping to a <see cref="PreconditionFailedError" />.
|
||||
/// </summary>
|
||||
[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<PlaylistItem>()
|
||||
});
|
||||
await ctx.SaveChangesAsync();
|
||||
}
|
||||
|
||||
private static ReplacePlaylistItems Command(Option<int> expectedVersion) =>
|
||||
new(
|
||||
1,
|
||||
"Kids",
|
||||
new List<ReplacePlaylistItem>
|
||||
{
|
||||
new(0, CollectionType.Movie, null, null, null, 55, PlaybackOrder.Shuffle, null, false, true)
|
||||
},
|
||||
expectedVersion);
|
||||
|
||||
private async Task<int> 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<T>(Either<BaseError, T> result) =>
|
||||
result.Match<BaseError?>(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<BaseError, List<PlaylistItemViewModel>> result =
|
||||
await handler.Handle(Command(Some(1)), CancellationToken.None);
|
||||
|
||||
LeftOrNull(result).ShouldBeOfType<PreconditionFailedError>();
|
||||
|
||||
// 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<BaseError, List<PlaylistItemViewModel>> 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<BaseError, List<PlaylistItemViewModel>> 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<BaseError, Unit> winnerResult = await ctxWinner.SaveChangesWithConcurrencyGuard(CancellationToken.None);
|
||||
winnerResult.IsRight.ShouldBeTrue();
|
||||
|
||||
loser.Version++;
|
||||
Either<BaseError, Unit> loserResult = await ctxLoser.SaveChangesWithConcurrencyGuard(CancellationToken.None);
|
||||
|
||||
LeftOrNull(loserResult).ShouldBeOfType<PreconditionFailedError>();
|
||||
|
||||
// Non-vacuous: the winner's write stuck at exactly its +1; the loser did not overwrite it.
|
||||
(await ReadVersionAsync()).ShouldBe(2);
|
||||
}
|
||||
}
|
||||
@@ -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<GetDecoById>(), Arg.Any<CancellationToken>())
|
||||
|
||||
@@ -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<IMediator>();
|
||||
_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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(Option<PlaylistViewModel>.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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)));
|
||||
_mediator.Send(Arg.Any<GetPlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(new List<PlaylistItemViewModel>
|
||||
{
|
||||
@@ -280,11 +287,24 @@ public class PlaylistControllerTests
|
||||
await _mediator.DidNotReceive().Send(Arg.Any<GetPlaylistItems>(), Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task GetItems_Should_Set_ETag_From_Playlist_Version()
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 9)));
|
||||
_mediator.Send(Arg.Any<GetPlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(new List<PlaylistItemViewModel>());
|
||||
|
||||
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<CreatePlaylist>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, PlaylistViewModel>(new PlaylistViewModel(9, 1, "Kids", false)));
|
||||
.Returns(Right<BaseError, PlaylistViewModel>(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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(
|
||||
Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)),
|
||||
Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 2)));
|
||||
_mediator.Send(Arg.Any<ReplacePlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, List<PlaylistItemViewModel>>(new List<PlaylistItemViewModel>
|
||||
{
|
||||
@@ -346,6 +370,8 @@ public class PlaylistControllerTests
|
||||
|
||||
result.ShouldBeOfType<OkObjectResult>().Value.ShouldBeOfType<List<PlaylistItemResponseModel>>().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<ReplacePlaylistItems>(c =>
|
||||
c.PlaylistId == 4 &&
|
||||
@@ -358,6 +384,77 @@ public class PlaylistControllerTests
|
||||
Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
[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<PlaylistItemRequest>()),
|
||||
CancellationToken.None);
|
||||
|
||||
result.ShouldBeOfType<BadRequestObjectResult>();
|
||||
await _mediator.DidNotReceive().Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>());
|
||||
await _mediator.DidNotReceive().Send(Arg.Any<ReplacePlaylistItems>(), Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Update_Should_Thread_If_Match_Version_Into_Command()
|
||||
{
|
||||
_controller.Request.Headers.IfMatch = "\"3\"";
|
||||
_mediator.Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 3)));
|
||||
_mediator.Send(Arg.Any<ReplacePlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, List<PlaylistItemViewModel>>(new List<PlaylistItemViewModel>()));
|
||||
|
||||
IActionResult result = await _controller.Update(
|
||||
4,
|
||||
new ReplacePlaylistRequest("Kids", new List<PlaylistItemRequest>()),
|
||||
CancellationToken.None);
|
||||
|
||||
result.ShouldBeOfType<OkObjectResult>();
|
||||
await _mediator.Received(1).Send(
|
||||
Arg.Is<ReplacePlaylistItems>(c => c.ExpectedVersion == Option<int>.Some(3)),
|
||||
Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Update_Without_If_Match_Should_Force_Write()
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)));
|
||||
_mediator.Send(Arg.Any<ReplacePlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, List<PlaylistItemViewModel>>(new List<PlaylistItemViewModel>()));
|
||||
|
||||
await _controller.Update(
|
||||
4,
|
||||
new ReplacePlaylistRequest("Kids", new List<PlaylistItemRequest>()),
|
||||
CancellationToken.None);
|
||||
|
||||
await _mediator.Received(1).Send(
|
||||
Arg.Is<ReplacePlaylistItems>(c => c.ExpectedVersion == Option<int>.None),
|
||||
Arg.Any<CancellationToken>());
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Update_Should_Return_412_On_Precondition_Failed()
|
||||
{
|
||||
_controller.Request.Headers.IfMatch = "\"2\"";
|
||||
_mediator.Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 5)));
|
||||
_mediator.Send(Arg.Any<ReplacePlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, List<PlaylistItemViewModel>>(new PreconditionFailedError("stale")));
|
||||
|
||||
IActionResult result = await _controller.Update(
|
||||
4,
|
||||
new ReplacePlaylistRequest("Kids", new List<PlaylistItemRequest>()),
|
||||
CancellationToken.None);
|
||||
|
||||
var objectResult = result.ShouldBeOfType<ObjectResult>();
|
||||
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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)));
|
||||
_mediator.Send(Arg.Any<ReplacePlaylistItems>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, List<PlaylistItemViewModel>>(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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "System", true)));
|
||||
.Returns(Option<PlaylistViewModel>.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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)));
|
||||
_mediator.Send(Arg.Any<DeletePlaylist>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<BaseError>.None);
|
||||
|
||||
@@ -438,7 +535,7 @@ public class PlaylistControllerTests
|
||||
public async Task Delete_Should_Return_422_On_System_Playlist()
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "System", true)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "System", true, 1)));
|
||||
_mediator.Send(Arg.Any<DeletePlaylist>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<BaseError>.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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)));
|
||||
_mediator.Send(Arg.Any<AddItemsToPlaylist>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Right<BaseError, Unit>(Unit.Default));
|
||||
|
||||
@@ -488,7 +585,7 @@ public class PlaylistControllerTests
|
||||
public async Task AddItems_Should_Return_422_On_System_Playlist()
|
||||
{
|
||||
_mediator.Send(Arg.Any<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "System", true)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "System", true, 1)));
|
||||
_mediator.Send(Arg.Any<AddItemsToPlaylist>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, Unit>(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<GetPlaylistById>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false)));
|
||||
.Returns(Option<PlaylistViewModel>.Some(new PlaylistViewModel(4, 1, "Kids", false, 1)));
|
||||
_mediator.Send(Arg.Any<AddItemsToPlaylist>(), Arg.Any<CancellationToken>())
|
||||
.Returns(Left<BaseError, Unit>(BaseError.New("Movie does not exist")));
|
||||
|
||||
|
||||
@@ -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<PlaylistItemResponseModel>), 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<PlaylistItemViewModel> 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<PlaylistItemResponseModel>), 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<IActionResult> 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<PlaylistViewModel> maybePlaylist = await mediator.Send(new GetPlaylistById(id), cancellationToken);
|
||||
if (maybePlaylist.IsNone)
|
||||
{
|
||||
@@ -195,10 +219,16 @@ public class PlaylistController(IMediator mediator) : ControllerBase
|
||||
}
|
||||
|
||||
Either<BaseError, List<PlaylistItemViewModel>> 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<PlaylistViewModel> 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")]
|
||||
|
||||
@@ -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<PlaylistItemRequest>? Items)
|
||||
{
|
||||
public ReplacePlaylistItems ToCommand(int id) =>
|
||||
new(id, Name ?? string.Empty, BuildItems());
|
||||
public ReplacePlaylistItems ToCommand(int id, Option<int> 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() =>
|
||||
|
||||
@@ -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<PlaylistItem[]> {
|
||||
return request<PlaylistItem[]>(`/api/playlists/${id}/items`);
|
||||
}
|
||||
|
||||
/** Load playlist items together with the playlist's concurrency ETag (issue #253). */
|
||||
export function getPlaylistItemsWithMeta(id: number): Promise<ResponseWithMeta<PlaylistItem[]>> {
|
||||
return requestWithMeta<PlaylistItem[]>(`/api/playlists/${id}/items`);
|
||||
}
|
||||
|
||||
export function createPlaylist(body: CreatePlaylistRequest): Promise<Playlist> {
|
||||
return request<Playlist>('/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<PlaylistItem[]> {
|
||||
return request<PlaylistItem[]>(`/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<ResponseWithMeta<PlaylistItem[]>> {
|
||||
return requestWithMeta<PlaylistItem[]>(`/api/playlists/${id}`, {
|
||||
body,
|
||||
method: 'PUT',
|
||||
headers: ifMatch ? { 'If-Match': ifMatch } : undefined
|
||||
});
|
||||
}
|
||||
|
||||
export function deletePlaylist(id: number): Promise<void> {
|
||||
|
||||
@@ -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(<PlaylistsScreen />);
|
||||
|
||||
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(<PlaylistsScreen />);
|
||||
|
||||
@@ -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<PlaylistPreviewItem[] | null>(null);
|
||||
const [previewMessage, setPreviewMessage] = useState<string | null>(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<string | null>(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
|
||||
)}
|
||||
</Card>
|
||||
)}
|
||||
|
||||
<ConfirmDialog
|
||||
cancelLabel="Keep editing"
|
||||
confirmLabel="Reload"
|
||||
message="This playlist was changed elsewhere since you opened it. Reload to get the latest version — your unsaved changes will be discarded."
|
||||
onCancel={() => setConflictOpen(false)}
|
||||
onConfirm={reloadAfterConflict}
|
||||
open={conflictOpen}
|
||||
title="Playlist changed elsewhere"
|
||||
tone="danger"
|
||||
/>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user