From 5e5f0af68476eccff5d1f3b2dc766e6ea088b515 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 17:54:21 +0200 Subject: [PATCH 01/17] fix(235-A): normalize async-op error contracts on Maintenance + Troubleshoot controllers (#235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slice A of the async-op API contract normalization. MaintenanceController: - EmptyTrash error path: was 500 text/plain (error.ToString()); now maps the BaseError Left through ApiResults.ToErrorResult() -> 404 (NotFoundError) / 422 ProblemDetails. Success stays 200 OkResult. Added ProducesResponseType 200 + 422. - CleanArtwork: fire-and-forget enqueue of DeleteOrphanedArtwork was a silent 200; now returns 202 Accepted (AcceptedResult) since it queues background work. Added ProducesResponseType 202. (Controller does not derive from ControllerBase, so results are built directly as before.) TroubleshootController.TroubleshootPlayback (GET|HEAD /api/troubleshoot/playback.m3u8): - Two bare body-less NotFound() call sites conflated "not found" with "prepare/ playback failure". Both now return a ProblemDetails body: * prepare-failure (result.IsLeft): mapped through error.ToErrorResult() -> 404 for NotFoundError (unknown media item/channel) else 422 for a validation BaseError. * terminal fall-through (prepare ok but no playable output): kept 404 with a distinguishing ApiResults.NotFoundProblem(...) detail. - Added ProducesResponseType 404 + 422 (409 already present). Consumer check: the SPA (PlaybackTroubleshootingScreen) feeds the playback.m3u8 URL straight to hls.js via HlsPlayer, which never inspects the HTTP status code — playback state is surfaced via the separate /api/troubleshoot/playback/status poll. So the 404->422 split for the validation subcase is safe; no player code branches on the status code. Tests: MaintenanceControllerTests (200/422/202 + enqueue assertion), TroubleshootControllerTests (prepare 404 NotFoundError, 422 validation). All green; Api error-metadata/contract/security scans still pass. Note: OpenAPI artifacts (v1.json / v1.d.ts) intentionally NOT regenerated here — the orchestrator regenerates once after all #235 slices merge. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Controllers/MaintenanceControllerTests.cs | 67 +++++++++++++++++++ .../TroubleshootControllerTests.cs | 50 ++++++++++++++ .../Controllers/Api/MaintenanceController.cs | 13 ++-- .../Controllers/Api/TroubleshootController.cs | 19 +++++- 4 files changed, 139 insertions(+), 10 deletions(-) create mode 100644 ErsatzTV.Tests/Controllers/MaintenanceControllerTests.cs diff --git a/ErsatzTV.Tests/Controllers/MaintenanceControllerTests.cs b/ErsatzTV.Tests/Controllers/MaintenanceControllerTests.cs new file mode 100644 index 000000000..35c8416d1 --- /dev/null +++ b/ErsatzTV.Tests/Controllers/MaintenanceControllerTests.cs @@ -0,0 +1,67 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Maintenance; +using ErsatzTV.Controllers.Api; +using ErsatzTV.Core; +using LanguageExt; +using MediatR; +using Microsoft.AspNetCore.Mvc; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using static LanguageExt.Prelude; + +namespace ErsatzTV.Tests.Controllers; + +[TestFixture] +public class MaintenanceControllerTests +{ + private MaintenanceController _controller = null!; + private IMediator _mediator = null!; + private Channel _workerChannel = null!; + + [SetUp] + public void SetUp() + { + _mediator = Substitute.For(); + _workerChannel = Channel.CreateUnbounded(); + _controller = new MaintenanceController(_mediator, _workerChannel.Writer); + } + + [Test] + public async Task EmptyTrash_Should_Return_200_On_Success() + { + _mediator.Send(Arg.Any()) + .Returns(Right(unit)); + + IActionResult result = await _controller.EmptyTrash(); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send(Arg.Any()); + } + + [Test] + public async Task EmptyTrash_Should_Return_422_ProblemDetails_On_Error() + { + _mediator.Send(Arg.Any()) + .Returns(Left(BaseError.New("Failed to empty trash"))); + + IActionResult result = await _controller.EmptyTrash(); + + var unprocessable = result.ShouldBeOfType(); + var problem = unprocessable.Value.ShouldBeOfType(); + problem.Status.ShouldBe(422); + problem.Detail.ShouldBe("Failed to empty trash"); + } + + [Test] + public async Task CleanArtwork_Should_Return_202_And_Enqueue_DeleteOrphanedArtwork() + { + IActionResult result = await _controller.CleanArtwork(CancellationToken.None); + + result.ShouldBeOfType(); + + _workerChannel.Reader.TryRead(out IBackgroundServiceRequest? enqueued).ShouldBeTrue(); + enqueued.ShouldBeOfType(); + } +} diff --git a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs index e04ac59cf..31d470256 100644 --- a/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/TroubleshootControllerTests.cs @@ -12,6 +12,7 @@ using ErsatzTV.Controllers.Api.Requests; using ErsatzTV.Core; using ErsatzTV.Core.Api.Troubleshooting; using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.FFmpeg; using ErsatzTV.Core.Interfaces.Locking; using ErsatzTV.Core.Interfaces.Repositories; using ErsatzTV.Core.Interfaces.Troubleshooting; @@ -274,6 +275,55 @@ public class TroubleshootControllerTests .Send(Arg.Any(), Arg.Any()); } + [Test] + public async Task TroubleshootPlayback_Should_Return_404_ProblemDetails_When_Prepare_Not_Found() + { + _entityLocker.IsTroubleshootingPlaybackLocked().Returns(false); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Either.Left( + new ErsatzTV.Core.Errors.NotFoundError("no such media item"))); + + IActionResult result = await _controller.TroubleshootPlayback( + mediaItem: 999, + channel: 0, + ffmpegProfile: 1, + StreamingMode.HttpLiveStreamingSegmenter, + watermark: [], + graphicsElement: [], + streamSelector: string.Empty, + subtitleId: null, + seekSeconds: 0, + start: null, + CancellationToken.None); + + var notFound = result.ShouldBeOfType(); + notFound.Value.ShouldBeOfType().Status.ShouldBe(404); + } + + [Test] + public async Task TroubleshootPlayback_Should_Return_422_ProblemDetails_When_Prepare_Fails_Validation() + { + _entityLocker.IsTroubleshootingPlaybackLocked().Returns(false); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Either.Left(BaseError.New("unable to prepare"))); + + IActionResult result = await _controller.TroubleshootPlayback( + mediaItem: 1, + channel: 0, + ffmpegProfile: 1, + StreamingMode.HttpLiveStreamingSegmenter, + watermark: [], + graphicsElement: [], + streamSelector: string.Empty, + subtitleId: null, + seekSeconds: 0, + start: null, + CancellationToken.None); + + var unprocessable = result.ShouldBeOfType(); + unprocessable.Value.ShouldBeOfType().Status.ShouldBe(422); + } + [Test] public async Task GetPlaybackStatus_Should_Report_Idle_When_No_Result_And_Unlocked() { diff --git a/ErsatzTV/Controllers/Api/MaintenanceController.cs b/ErsatzTV/Controllers/Api/MaintenanceController.cs index b6009e32a..2093207b7 100644 --- a/ErsatzTV/Controllers/Api/MaintenanceController.cs +++ b/ErsatzTV/Controllers/Api/MaintenanceController.cs @@ -2,6 +2,7 @@ using System.Threading.Channels; using ErsatzTV.Application; using ErsatzTV.Application.Maintenance; using ErsatzTV.Core; +using ErsatzTV.Extensions; using MediatR; using Microsoft.AspNetCore.Mvc; @@ -23,17 +24,14 @@ public class MaintenanceController(IMediator mediator, ChannelWriter EmptyTrash() { Either result = await mediator.Send(new EmptyTrash()); foreach (BaseError error in result.LeftToSeq()) { - return new ContentResult - { - StatusCode = StatusCodes.Status500InternalServerError, - Content = error.ToString(), - ContentType = "text/plain" - }; + return error.ToErrorResult(); } return new OkResult(); @@ -42,9 +40,10 @@ public class MaintenanceController(IMediator mediator, ChannelWriter CleanArtwork(CancellationToken cancellationToken) { await workerChannel.WriteAsync(new DeleteOrphanedArtwork(), cancellationToken); - return new OkResult(); + return new AcceptedResult(); } } diff --git a/ErsatzTV/Controllers/Api/TroubleshootController.cs b/ErsatzTV/Controllers/Api/TroubleshootController.cs index 129c077f5..7a523f3f7 100644 --- a/ErsatzTV/Controllers/Api/TroubleshootController.cs +++ b/ErsatzTV/Controllers/Api/TroubleshootController.cs @@ -114,7 +114,9 @@ public class TroubleshootController( [Tags("Troubleshooting")] [EndpointSummary("Start a troubleshooting playback session")] [EndpointGroupName("general")] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task TroubleshootPlayback( [FromQuery] int mediaItem, @@ -174,9 +176,15 @@ public class TroubleshootController( Optional(start)), cancellationToken); - if (result.IsLeft) + // Distinguish "prepare failed" from the later "no playable output" fall-through: map the + // handler's BaseError through the standard helper (404 for NotFoundError — e.g. an unknown + // media item/channel — else 422 for a validation failure) with a ProblemDetails body, + // instead of a bare body-less 404. The SPA feeds this URL straight to hls.js (HlsPlayer) + // and never inspects the status code — failures surface via the /status poll — so the + // 404→422 split for validation errors is safe. + foreach (BaseError error in result.LeftToSeq()) { - return NotFound(); + return error.ToErrorResult(); } // Prepare returned a process, so the handler holds the troubleshooting lock now @@ -273,7 +281,12 @@ public class TroubleshootController( } } - return NotFound(); + // Terminal fall-through: Prepare succeeded but no playable output was produced (playback + // failed to start, was cancelled, or the segmenter never wrote segments). Keep the 404 status + // the SPA player already tolerates, but attach a distinguishing ProblemDetails body rather + // than a bare NotFound() so the response is self-describing. + return ApiResults.NotFoundProblem( + "Troubleshooting playback did not produce any output. It may have failed to start or been cancelled."); } [HttpHead("api/troubleshoot/playback/archive")] From d02f953922e8d30d3265e040e7404107456e62ac Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 17:57:41 +0200 Subject: [PATCH 02/17] fix(235-F7): release leaked Trakt lock on worker shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The global Trakt lock is acquired by SchedulerService.RefreshTraktLists / MatchTraktLists (and TraktController) and released only when the *terminal* message of a batch — the one carrying Unlock: true (list == traktLists.Last()) — is processed by WorkerService, whose handler (AddTraktListHandler / MatchTraktListItemsHandler) calls IEntityLocker.UnlockTrakt() in a finally. WorkerService.ExecuteAsync breaks out of the read loop on stoppingToken.IsCancellationRequested (and exits on channel completion / reader cancellation) BEFORE processing the next message. If shutdown lands after a batch is enqueued but before its terminal Unlock: true message is handled, UnlockTrakt() never runs and the in-memory Trakt lock leaks for the rest of the process lifetime (subsequent Trakt operations 409 forever). Fix (option a): make the batch-release loss-tolerant with a compensating release in a finally around the read loop — if the Trakt lock is still held when the worker stops, release it. Chosen over tracking pending ownership (b) because the lock is a global singleton and WorkerService is its sole batch-release site, so "held at shutdown" unambiguously means "the terminal release was lost"; covers all three exit paths (break / channel completion / cancellation) in one place. Same lock-lifecycle class as #231/#233/#234. Regression test: WorkerServiceTests gates the first (non-terminal) batch message on the stopping token, then StopAsync-cancels so the worker breaks before the terminal Unlock: true message — asserts the lock is released and the terminal message was never processed. Proven non-vacuous: inverting the finally condition fails the test. Backend-only; no controller/DTO/SPA/OpenAPI impact. Co-Authored-By: Claude Opus 4.8 (1M context) --- ErsatzTV.Tests/Services/WorkerServiceTests.cs | 101 ++++++++++++++++++ ErsatzTV/Services/WorkerService.cs | 18 ++++ 2 files changed, 119 insertions(+) create mode 100644 ErsatzTV.Tests/Services/WorkerServiceTests.cs diff --git a/ErsatzTV.Tests/Services/WorkerServiceTests.cs b/ErsatzTV.Tests/Services/WorkerServiceTests.cs new file mode 100644 index 000000000..96c6cd404 --- /dev/null +++ b/ErsatzTV.Tests/Services/WorkerServiceTests.cs @@ -0,0 +1,101 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Core; +using ErsatzTV.Core.Interfaces.Locking; +using ErsatzTV.Services; +using LanguageExt; +using MediatR; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using Unit = LanguageExt.Unit; + +namespace ErsatzTV.Tests.Services; + +[TestFixture] +public class WorkerServiceTests +{ + // F7 regression (issue #235): the global Trakt lock is acquired by SchedulerService and released + // only when the *terminal* (Unlock: true) message of a batch is processed by WorkerService. If the + // worker stops before reaching that terminal message (shutdown break / channel completion / + // cancellation), the release never fires and the in-memory Trakt lock leaks for the life of the + // process. WorkerService must release a held Trakt lock as a compensating action on shutdown. + [Test] + public async Task Should_Release_Held_Trakt_Lock_When_Worker_Stops_Before_Terminal_Message() + { + var channel = Channel.CreateUnbounded(); + + // A Trakt batch: only the terminal message carries Unlock: true. If the worker never processes + // it, the handler-side UnlockTrakt() never runs. + AddTraktList nonTerminal = AddTraktList.Existing("user", "list-1", false); + AddTraktList terminal = AddTraktList.Existing("user", "list-2", true); + await channel.Writer.WriteAsync(nonTerminal); + await channel.Writer.WriteAsync(terminal); + + // Stateful stand-in for the singleton EntityLocker: the lock starts held (a batch acquired it). + var traktLocked = 1; + var locker = Substitute.For(); + locker.IsTraktLocked().Returns(_ => Volatile.Read(ref traktLocked) == 1); + locker.UnlockTrakt().Returns(_ => Interlocked.Exchange(ref traktLocked, 0) == 1); + + // Gate: processing the first (non-terminal) message parks on the stopping token, guaranteeing + // the worker never reaches the terminal (Unlock: true) message before it is stopped. + var firstSeen = new TaskCompletionSource(); + var processed = new List(); + var mediator = Substitute.For(); + mediator.Send(Arg.Any(), Arg.Any()) + .Returns(async call => + { + lock (processed) + { + processed.Add(call.Arg()); + } + + firstSeen.TrySetResult(); + + // Block on the stopping token so the loop parks here until StopAsync cancels it. + await Task.Delay(Timeout.Infinite, call.Arg()); + return (Either)Unit.Default; + }); + + var provider = Substitute.For(); + provider.GetService(typeof(IMediator)).Returns(mediator); + var scope = Substitute.For(); + scope.ServiceProvider.Returns(provider); + var scopeFactory = Substitute.For(); + scopeFactory.CreateScope().Returns(scope); + + var worker = new WorkerService( + channel.Reader, + scopeFactory, + locker, + NullLogger.Instance); + + await worker.StartAsync(CancellationToken.None); + + // Wait until the first message is actively being processed (parked on the stopping token). + await firstSeen.Task.WaitAsync(TimeSpan.FromSeconds(5)); + + // Shut the worker down: cancels the stopping token -> parked Delay throws -> loop breaks + // before the terminal message is ever processed. + await worker.StopAsync(CancellationToken.None); + + // The compensating release must have fired even though the terminal message was never handled. + locker.IsTraktLocked().ShouldBeFalse(); + locker.Received(1).UnlockTrakt(); + + // Prove the leak scenario is genuine: we stopped after the non-terminal message but before the + // terminal (Unlock: true) one, so the handler-side release could not have run. + List seen; + lock (processed) + { + seen = processed.ToList(); + } + + seen.ShouldContain(nonTerminal); + seen.ShouldNotContain(terminal); + } +} diff --git a/ErsatzTV/Services/WorkerService.cs b/ErsatzTV/Services/WorkerService.cs index 5e3ebfd00..1cefb8eb2 100644 --- a/ErsatzTV/Services/WorkerService.cs +++ b/ErsatzTV/Services/WorkerService.cs @@ -9,6 +9,7 @@ using ErsatzTV.Application.MediaCollections; using ErsatzTV.Application.Playouts; using ErsatzTV.Application.Subtitles; using ErsatzTV.Core; +using ErsatzTV.Core.Interfaces.Locking; using MediatR; namespace ErsatzTV.Services; @@ -16,16 +17,19 @@ namespace ErsatzTV.Services; public class WorkerService : BackgroundService { private readonly ChannelReader _channel; + private readonly IEntityLocker _entityLocker; private readonly ILogger _logger; private readonly IServiceScopeFactory _serviceScopeFactory; public WorkerService( ChannelReader channel, IServiceScopeFactory serviceScopeFactory, + IEntityLocker entityLocker, ILogger logger) { _channel = channel; _serviceScopeFactory = serviceScopeFactory; + _entityLocker = entityLocker; _logger = logger; } @@ -143,5 +147,19 @@ public class WorkerService : BackgroundService { _logger.LogInformation("Worker service shutting down"); } + finally + { + // The global Trakt lock is acquired by SchedulerService/TraktController and released only + // when the *terminal* (Unlock: true) message of a batch is processed here. If this loop + // stops before reaching that message - shutdown break above, channel completion, or the + // reader throwing on cancellation - the release never fires and the in-memory Trakt lock + // leaks for the remaining life of the process (subsequent Trakt operations 409 forever). + // Make the batch-release loss-tolerant with a compensating release on worker shutdown. + if (_entityLocker.IsTraktLocked()) + { + _logger.LogDebug("Releasing held Trakt lock during worker shutdown"); + _entityLocker.UnlockTrakt(); + } + } } } From 9b73b62527c111c3987503f2bb945d1012ce8932 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 17:58:15 +0200 Subject: [PATCH 03/17] =?UTF-8?q?feat(235):=20async-op=20API=20contract=20?= =?UTF-8?q?normalization=20=E2=80=94=20playouts=20slice=20C=20(#235)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Slice C of the async-op contract normalization: - channel reset (POST /api/channels/{channelNumber}/playout/reset) now returns 202 Accepted (was 200 Ok) — it only queues a background rebuild - reset-all (POST /api/playouts/reset-all) still 202 but now returns a ResetAllPlayoutsResponseModel body reporting QueuedPlayoutIds / SkippedLocked / SkippedUnsupported instead of silently swallowing skips; handler returns a new ResetAllPlayoutsResult record - single-playout GET (GET /api/playouts/{id}) now exposes IsLocked on PlayoutResponseModel, set from IEntityLocker.IsPlayoutLocked mirroring the list projection — gives a polling client the lock flag Tests: channel reset asserts 202; reset-all asserts 202 + skipped-body shape; single GET asserts IsLocked; new ResetAllPlayoutsHandlerTests (in-memory SQLite) asserts locked/ExternalJson/None land in skipped lists and eligible playouts in queued. docs/api-conventions.md §3a updated. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Playouts/Commands/ResetAllPlayouts.cs | 2 +- .../Commands/ResetAllPlayoutsHandler.cs | 27 +++++- .../Playouts/ResetAllPlayoutsResult.cs | 6 ++ .../Api/Playouts/PlayoutResponseModel.cs | 9 +- .../Playouts/ResetAllPlayoutsResponseModel.cs | 7 ++ .../Playouts/ResetAllPlayoutsHandlerTests.cs | 82 +++++++++++++++++++ .../Controllers/ChannelControllerTests.cs | 4 +- .../Controllers/PlayoutControllerTests.cs | 30 ++++++- ErsatzTV/Controllers/Api/ChannelController.cs | 4 +- ErsatzTV/Controllers/Api/PlayoutController.cs | 29 ++++--- docs/api-conventions.md | 17 +++- 11 files changed, 188 insertions(+), 29 deletions(-) create mode 100644 ErsatzTV.Application/Playouts/ResetAllPlayoutsResult.cs create mode 100644 ErsatzTV.Core/Api/Playouts/ResetAllPlayoutsResponseModel.cs create mode 100644 ErsatzTV.Tests/Application/Playouts/ResetAllPlayoutsHandlerTests.cs diff --git a/ErsatzTV.Application/Playouts/Commands/ResetAllPlayouts.cs b/ErsatzTV.Application/Playouts/Commands/ResetAllPlayouts.cs index dbf41139d..79967ab48 100644 --- a/ErsatzTV.Application/Playouts/Commands/ResetAllPlayouts.cs +++ b/ErsatzTV.Application/Playouts/Commands/ResetAllPlayouts.cs @@ -1,3 +1,3 @@ namespace ErsatzTV.Application.Playouts; -public record ResetAllPlayouts : IRequest; +public record ResetAllPlayouts : IRequest; diff --git a/ErsatzTV.Application/Playouts/Commands/ResetAllPlayoutsHandler.cs b/ErsatzTV.Application/Playouts/Commands/ResetAllPlayoutsHandler.cs index c99a127f6..e997bedb7 100644 --- a/ErsatzTV.Application/Playouts/Commands/ResetAllPlayoutsHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/ResetAllPlayoutsHandler.cs @@ -11,33 +11,49 @@ public class ResetAllPlayoutsHandler( IEntityLocker locker, ChannelWriter channel, IDbContextFactory dbContextFactory) - : IRequestHandler + : IRequestHandler { - public async Task Handle(ResetAllPlayouts request, CancellationToken cancellationToken) + public async Task Handle( + ResetAllPlayouts request, + CancellationToken cancellationToken) { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); + var queued = new List(); + var skippedLocked = new List(); + var skippedUnsupported = new List(); + foreach (Playout playout in await dbContext.Playouts.ToListAsync(cancellationToken)) { switch (playout.ScheduleKind) { case PlayoutScheduleKind.Classic: - if (!locker.IsPlayoutLocked(playout.Id)) + if (locker.IsPlayoutLocked(playout.Id)) + { + skippedLocked.Add(playout.Id); + } + else { await channel.WriteAsync( new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), cancellationToken); + queued.Add(playout.Id); } break; case PlayoutScheduleKind.Block: case PlayoutScheduleKind.Sequential: case PlayoutScheduleKind.Scripted: - if (!locker.IsPlayoutLocked(playout.Id)) + if (locker.IsPlayoutLocked(playout.Id)) + { + skippedLocked.Add(playout.Id); + } + else { await channel.WriteAsync( new BuildPlayout(playout.Id, PlayoutBuildMode.Reset), cancellationToken); + queued.Add(playout.Id); } break; @@ -45,8 +61,11 @@ public class ResetAllPlayoutsHandler( case PlayoutScheduleKind.None: default: // external json cannot be reset + skippedUnsupported.Add(playout.Id); continue; } } + + return new ResetAllPlayoutsResult(queued, skippedLocked, skippedUnsupported); } } diff --git a/ErsatzTV.Application/Playouts/ResetAllPlayoutsResult.cs b/ErsatzTV.Application/Playouts/ResetAllPlayoutsResult.cs new file mode 100644 index 000000000..0d5fd9633 --- /dev/null +++ b/ErsatzTV.Application/Playouts/ResetAllPlayoutsResult.cs @@ -0,0 +1,6 @@ +namespace ErsatzTV.Application.Playouts; + +public record ResetAllPlayoutsResult( + List QueuedPlayoutIds, + List SkippedLocked, + List SkippedUnsupported); diff --git a/ErsatzTV.Core/Api/Playouts/PlayoutResponseModel.cs b/ErsatzTV.Core/Api/Playouts/PlayoutResponseModel.cs index 63b08b96b..3439da4f9 100644 --- a/ErsatzTV.Core/Api/Playouts/PlayoutResponseModel.cs +++ b/ErsatzTV.Core/Api/Playouts/PlayoutResponseModel.cs @@ -14,7 +14,8 @@ public record PlayoutResponseModel( TimeSpan? DailyRebuildTime, PlayoutBuildStatusResponseModel? BuildStatus, int? DecoId, - string? DecoName) + string? DecoName, + bool IsLocked) { public static PlayoutResponseModel From( int id, @@ -27,7 +28,8 @@ public record PlayoutResponseModel( TimeSpan? dailyRebuildTime, PlayoutBuildStatusResponseModel? buildStatus, int? decoId, - string? decoName) => + string? decoName, + bool isLocked) => new( id, scheduleKind, @@ -39,5 +41,6 @@ public record PlayoutResponseModel( dailyRebuildTime, buildStatus, decoId, - decoName); + decoName, + isLocked); } diff --git a/ErsatzTV.Core/Api/Playouts/ResetAllPlayoutsResponseModel.cs b/ErsatzTV.Core/Api/Playouts/ResetAllPlayoutsResponseModel.cs new file mode 100644 index 000000000..830b2bb15 --- /dev/null +++ b/ErsatzTV.Core/Api/Playouts/ResetAllPlayoutsResponseModel.cs @@ -0,0 +1,7 @@ +#nullable enable +namespace ErsatzTV.Core.Api.Playouts; + +public record ResetAllPlayoutsResponseModel( + List QueuedPlayoutIds, + List SkippedLocked, + List SkippedUnsupported); diff --git a/ErsatzTV.Tests/Application/Playouts/ResetAllPlayoutsHandlerTests.cs b/ErsatzTV.Tests/Application/Playouts/ResetAllPlayoutsHandlerTests.cs new file mode 100644 index 000000000..e7b6ffab1 --- /dev/null +++ b/ErsatzTV.Tests/Application/Playouts/ResetAllPlayoutsHandlerTests.cs @@ -0,0 +1,82 @@ +using System.Collections.Generic; +using System.Threading; +using System.Threading.Channels; +using System.Threading.Tasks; +using ErsatzTV.Application; +using ErsatzTV.Application.Playouts; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Locking; +using ErsatzTV.Core.Scheduling; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using Channel = System.Threading.Channels.Channel; + +namespace ErsatzTV.Tests.Application.Playouts; + +[TestFixture] +public class ResetAllPlayoutsHandlerTests +{ + private InMemoryTvContext _db = null!; + private Channel _worker = null!; + private IEntityLocker _entityLocker = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _worker = Channel.CreateUnbounded(); + _entityLocker = Substitute.For(); + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + private ResetAllPlayoutsHandler CreateHandler() => + new(_entityLocker, _worker.Writer, _db.Factory); + + private async Task SeedPlayout(PlayoutScheduleKind kind) + { + await using TvContext context = _db.CreateContext(); + var playout = new Playout { ChannelId = 0, ScheduleKind = kind }; + context.Playouts.Add(playout); + await context.SaveChangesAsync(); + return playout.Id; + } + + [Test] + public async Task Handle_Should_Queue_Eligible_And_Report_Skipped() + { + int classicId = await SeedPlayout(PlayoutScheduleKind.Classic); + int blockId = await SeedPlayout(PlayoutScheduleKind.Block); + int lockedId = await SeedPlayout(PlayoutScheduleKind.Sequential); + int externalJsonId = await SeedPlayout(PlayoutScheduleKind.ExternalJson); + int noneId = await SeedPlayout(PlayoutScheduleKind.None); + + _entityLocker.IsPlayoutLocked(lockedId).Returns(true); + + ResetAllPlayoutsResult result = + await CreateHandler().Handle(new ResetAllPlayouts(), CancellationToken.None); + + // eligible, unlocked playouts are queued + result.QueuedPlayoutIds.ShouldBe(new List { classicId, blockId }, ignoreOrder: true); + + // locked playout lands in SkippedLocked, not queued + result.SkippedLocked.ShouldBe(new List { lockedId }); + + // ExternalJson + None land in SkippedUnsupported + result.SkippedUnsupported.ShouldBe(new List { externalJsonId, noneId }, ignoreOrder: true); + + // exactly one BuildPlayout message per queued playout was enqueued + var enqueued = new List(); + while (_worker.Reader.TryRead(out IBackgroundServiceRequest? request)) + { + var build = request.ShouldBeOfType(); + enqueued.Add(build.PlayoutId); + } + + enqueued.ShouldBe(new List { classicId, blockId }, ignoreOrder: true); + } +} diff --git a/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs b/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs index 67b84c74e..5e1deb4e3 100644 --- a/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs @@ -407,7 +407,7 @@ public class ChannelControllerTests IActionResult result = await _controller.ResetPlayout("5", mode: null, CancellationToken.None); - result.ShouldBeOfType(); + result.ShouldBeOfType().StatusCode.ShouldBe(202); _workerChannel.Reader.TryRead(out IBackgroundServiceRequest? request).ShouldBeTrue(); var buildPlayout = request.ShouldBeOfType(); buildPlayout.PlayoutId.ShouldBe(9); @@ -436,7 +436,7 @@ public class ChannelControllerTests IActionResult result = await _controller.ResetPlayout("5", PlayoutBuildMode.Continue, CancellationToken.None); - result.ShouldBeOfType(); + result.ShouldBeOfType().StatusCode.ShouldBe(202); _workerChannel.Reader.TryRead(out IBackgroundServiceRequest? request).ShouldBeTrue(); request.ShouldBeOfType().Mode.ShouldBe(PlayoutBuildMode.Continue); await _mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); diff --git a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs index 69b5171ef..0fc9e3bc5 100644 --- a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs @@ -527,6 +527,21 @@ public class PlayoutControllerTests result.ShouldBeOfType().Value.ShouldBe(ToResponse(vm)); } + [Test] + public async Task GetById_Should_Expose_IsLocked_From_EntityLocker() + { + PlayoutNameViewModel vm = MakePlayout(9); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(vm)); + _entityLocker.IsPlayoutLocked(9).Returns(true); + + IActionResult result = await _controller.GetById(9, CancellationToken.None); + + var body = result.ShouldBeOfType().Value.ShouldBeOfType(); + body.IsLocked.ShouldBeTrue(); + body.ShouldBe(ToResponse(vm, isLocked: true)); + } + [Test] public async Task GetById_Should_Return_404_For_None_With_ProblemDetails() { @@ -693,9 +708,17 @@ public class PlayoutControllerTests [Test] public async Task ResetAll_Should_Return_202_And_Send_Command() { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(new ResetAllPlayoutsResult([1, 2], [3], [4])); + IActionResult result = await _controller.ResetAll(CancellationToken.None); - result.ShouldBeOfType(); + var accepted = result.ShouldBeOfType(); + accepted.StatusCode.ShouldBe(202); + var body = accepted.Value.ShouldBeOfType(); + body.QueuedPlayoutIds.ShouldBe(new List { 1, 2 }); + body.SkippedLocked.ShouldBe(new List { 3 }); + body.SkippedUnsupported.ShouldBe(new List { 4 }); await _mediator.Received(1).Send(Arg.Any(), Arg.Any()); } @@ -1286,7 +1309,7 @@ public class PlayoutControllerTests null, null); - private static PlayoutResponseModel ToResponse(PlayoutNameViewModel vm) => + private static PlayoutResponseModel ToResponse(PlayoutNameViewModel vm, bool isLocked = false) => PlayoutResponseModel.From( vm.PlayoutId, vm.ScheduleKind, @@ -1303,7 +1326,8 @@ public class PlayoutControllerTests vm.BuildStatus.Success, vm.BuildStatus.Message), vm.DecoId, - vm.DecoName); + vm.DecoName, + isLocked); private static void ShouldHaveActionRoute(string actionName, string httpMethod, string route) { diff --git a/ErsatzTV/Controllers/Api/ChannelController.cs b/ErsatzTV/Controllers/Api/ChannelController.cs index e70f1a833..d2357e9e7 100644 --- a/ErsatzTV/Controllers/Api/ChannelController.cs +++ b/ErsatzTV/Controllers/Api/ChannelController.cs @@ -211,7 +211,7 @@ public class ChannelController( "progress) and all other playout kinds use Reset (rebuild from scratch), matching the Blazor UI. " + "Pass mode to force a specific PlayoutBuildMode.")] [EndpointGroupName("general")] - [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status202Accepted)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] public async Task ResetPlayout( @@ -233,7 +233,7 @@ public class ChannelController( PlayoutBuildMode buildMode = mode ?? await DefaultResetMode(playoutId, cancellationToken); await workerChannel.WriteAsync(new BuildPlayout(playoutId, buildMode), cancellationToken); - return new OkResult(); + return new AcceptedResult(); } return ApiResults.NotFoundProblem(); diff --git a/ErsatzTV/Controllers/Api/PlayoutController.cs b/ErsatzTV/Controllers/Api/PlayoutController.cs index a0bff515f..c023135d7 100644 --- a/ErsatzTV/Controllers/Api/PlayoutController.cs +++ b/ErsatzTV/Controllers/Api/PlayoutController.cs @@ -69,7 +69,7 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : public async Task GetById(int id, CancellationToken cancellationToken) { Option result = await mediator.Send(new GetPlayoutById(id), cancellationToken); - return result.Map(ToResponse).ToGetResult(); + return result.Map(vm => ToResponse(vm, entityLocker.IsPlayoutLocked(id))).ToGetResult(); } [HttpGet("/api/playouts/{id:int}/items", Name = "GetPlayoutItems")] @@ -128,7 +128,9 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : Option playout = await mediator.Send(new GetPlayoutById(created.PlayoutId), cancellationToken); return playout.Match( - Some: vm => (IActionResult)new CreatedResult($"/api/playouts/{vm.PlayoutId}", ToResponse(vm)), + Some: vm => (IActionResult)new CreatedResult( + $"/api/playouts/{vm.PlayoutId}", + ToResponse(vm, entityLocker.IsPlayoutLocked(vm.PlayoutId))), None: () => ApiResults.NotFoundProblem()); }); }); @@ -194,7 +196,8 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : return result.Match( Left: error => error.ToErrorResult(), - Right: playout => (IActionResult)new OkObjectResult(ToResponse(playout))); + Right: playout => (IActionResult)new OkObjectResult( + ToResponse(playout, entityLocker.IsPlayoutLocked(id)))); } private async Task> UpdateScheduleFile( @@ -260,7 +263,7 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : Option refreshed = await mediator.Send(new GetPlayoutById(id), cancellationToken); return refreshed.Match( - Some: vm => (IActionResult)new OkObjectResult(ToResponse(vm)), + Some: vm => (IActionResult)new OkObjectResult(ToResponse(vm, entityLocker.IsPlayoutLocked(id))), None: () => ApiResults.NotFoundProblem()); } @@ -550,14 +553,19 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : [Tags("Playouts")] [EndpointSummary("Reset all playouts")] [EndpointGroupName("general")] - [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(typeof(ResetAllPlayoutsResponseModel), StatusCodes.Status202Accepted)] // No 409 lock guard here (unlike the id-keyed mutations): ResetAllPlayoutsHandler already // skips any playout whose build lock is held, matching Blazor. It is a fire-and-forget - // bulk enqueue, so it always accepts. See docs/decisions.md 2026-07-10. + // bulk enqueue, so it always accepts — the 202 body reports which playouts were queued and + // which were skipped (locked, or an unsupported ExternalJson/None kind). See docs/decisions.md 2026-07-10. public async Task ResetAll(CancellationToken cancellationToken) { - await mediator.Send(new ResetAllPlayouts(), cancellationToken); - return Accepted(); + ResetAllPlayoutsResult result = await mediator.Send(new ResetAllPlayouts(), cancellationToken); + var body = new ResetAllPlayoutsResponseModel( + result.QueuedPlayoutIds, + result.SkippedLocked, + result.SkippedUnsupported); + return new AcceptedResult((string)null, body); } [HttpPost("/api/playouts/{id:int}/erase-items", Name = "ErasePlayoutItems")] @@ -728,7 +736,7 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : private static PlayoutHistoryDetailsResponseModel ToDetailsResponse(PlayoutHistoryDetailsViewModel vm) => new(vm.PlaybackOrder, vm.CollectionType, vm.Name, vm.MediaItemType, vm.MediaItemTitle); - private static PlayoutResponseModel ToResponse(PlayoutNameViewModel vm) => + private static PlayoutResponseModel ToResponse(PlayoutNameViewModel vm, bool isLocked) => PlayoutResponseModel.From( vm.PlayoutId, vm.ScheduleKind, @@ -740,7 +748,8 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : vm.DbDailyRebuildTime, ToBuildStatus(vm.BuildStatus), vm.DecoId, - vm.DecoName); + vm.DecoName, + isLocked); private static PlayoutAlternateScheduleResponseModel ToResponse(PlayoutAlternateScheduleViewModel vm) => new( diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 38a944ce1..14861a905 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -112,12 +112,21 @@ Established by issue #215 (`PlayoutController` + `ChannelController.ResetPlayout `[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)]` to each guarded action. Precedent for the 409 shape: `TraktController` (its private `ConflictProblem()`). Two nuances: - **Fire-and-forget bulk operations don't 409** — `POST /api/playouts/reset-all` stays 202; its - handler (`ResetAllPlayoutsHandler`) already *skips* locked playouts, matching Blazor + the handler - semantics. Only per-id mutations 409. + handler (`ResetAllPlayoutsHandler`) *skips* locked playouts, matching Blazor + the handler + semantics. Only per-id mutations 409. As of #235 the handler returns a `ResetAllPlayoutsResult` + (`QueuedPlayoutIds` / `SkippedLocked` / `SkippedUnsupported`) and the controller returns the 202 + **with a `ResetAllPlayoutsResponseModel` body** reporting what was queued vs. skipped (locked, or + an unsupported `ExternalJson`/`None` kind) — a fire-and-forget bulk op still reports its outcome + rather than silently swallowing skips. - **Surface the lock state to clients** so they can pre-disable the buttons: stamp an `IsLocked` boolean onto the list DTO (`PlayoutListItemResponseModel`, set from `IsPlayoutLocked` in the - controller's list projection) rather than adding a push channel. The SPA reads it and, on a 409, - refreshes the list to pick up the flag. + controller's list projection) rather than adding a push channel — and (as of #235) onto the + single-playout GET DTO (`PlayoutResponseModel.IsLocked`, set the same way in every action that + maps it) so a client polling one playout has the same flag. The SPA reads it and, on a 409, + refreshes to pick up the flag. +- **Async-op success is 202, not 200** — an endpoint whose success path only *queues* a background + rebuild returns **202 Accepted**, not 200 (#235: `POST /api/channels/{channelNumber}/playout/reset` + queues a `BuildPlayout` → `AcceptedResult`). Reserve 200 for a synchronous durable result. ### 3b. Map a "queue a background job" outcome to status codes with an enum, not a `bool` From 628c9d722818ecb0fd603d182549fcada4613356 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:02:17 +0200 Subject: [PATCH 04/17] =?UTF-8?q?feat(235):=20F9=20API=20parity=20?= =?UTF-8?q?=E2=80=94=20library=20deep-scan,=20external-collections=20scan,?= =?UTF-8?q?=20scan-show=20outcome=20enum=20(#235=20slice=20B)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the two F9 Libraries.razor parity gaps and normalizes scan-show error mapping to ProblemDetails. TASK 1 — library-wide deep scan: - QueueLibraryScanByLibraryId gains optional `bool DeepScan = false`; handler threads it into ForceSynchronize{Plex,Jellyfin,Emby}LibraryById. - POST /api/libraries/{id}/scan?deep=false binds it via [FromQuery]. TASK 2 — external-collections scan (new endpoints): - POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=false acquires the per-source collections lock (§3b: lock IS the running scan → 409), enqueues Synchronize{X}Collections(id, ForceScan:true, deep) to the scanner channel, returns 202; compensating-unlock on enqueue throw. TASK 3 — scan-show normalization: - New QueueShowScanResult enum; handler returns it instead of bool. - POST /api/libraries/{id}/scan-show now maps 202/404/409/422 (all errors ProblemDetails) instead of 200/404/400-anonymous-object. - Updated the lone Blazor caller (TelevisionSeasonList.razor). Tests: LibrariesController (scan deep=true, scan-show enum→status), the three media-source controllers (scan-collections route/404/409/202/compensating-unlock), and handler tests for both changed handlers (deep threading + show-scan outcomes). Docs: api-conventions §3b exemplar + blazor-route-parity §5 F9 gate. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Commands/QueueLibraryScanByLibraryId.cs | 2 +- .../QueueLibraryScanByLibraryIdHandler.cs | 6 +- .../Commands/QueueShowScanByLibraryId.cs | 13 +- .../QueueShowScanByLibraryIdHandler.cs | 26 ++- ...QueueLibraryScanByLibraryIdHandlerTests.cs | 32 ++- .../QueueShowScanByLibraryIdHandlerTests.cs | 214 ++++++++++++++++++ .../EmbyMediaSourcesControllerTests.cs | 46 ++++ .../JellyfinMediaSourcesControllerTests.cs | 46 ++++ .../Controllers/LibrariesControllerTests.cs | 91 +++++++- .../PlexMediaSourcesControllerTests.cs | 62 +++++ .../Api/EmbyMediaSourcesController.cs | 47 ++++ .../Api/JellyfinMediaSourcesController.cs | 47 ++++ .../Controllers/Api/LibrariesController.cs | 49 +++- .../Api/PlexMediaSourcesController.cs | 45 ++++ ErsatzTV/Pages/TelevisionSeasonList.razor | 5 +- docs/api-conventions.md | 9 + docs/blazor-route-parity.md | 6 +- 17 files changed, 704 insertions(+), 42 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Libraries/QueueShowScanByLibraryIdHandlerTests.cs diff --git a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs index 024b47be3..4b64fd73b 100644 --- a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs +++ b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryId.cs @@ -8,4 +8,4 @@ public enum QueueLibraryScanResult AlreadyScanning } -public record QueueLibraryScanByLibraryId(int LibraryId) : IRequest; +public record QueueLibraryScanByLibraryId(int LibraryId, bool DeepScan = false) : IRequest; diff --git a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs index 6bb6a5f4b..8782e6631 100644 --- a/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs +++ b/ErsatzTV.Application/Libraries/Commands/QueueLibraryScanByLibraryIdHandler.cs @@ -66,7 +66,7 @@ public class QueueLibraryScanByLibraryIdHandler( new SynchronizePlexLibraries(library.MediaSourceId), cancellationToken); await scannerWorker.WriteAsync( - new ForceSynchronizePlexLibraryById(library.Id, false), + new ForceSynchronizePlexLibraryById(library.Id, request.DeepScan), cancellationToken); break; case JellyfinLibrary: @@ -74,7 +74,7 @@ public class QueueLibraryScanByLibraryIdHandler( new SynchronizeJellyfinLibraries(library.MediaSourceId), cancellationToken); await scannerWorker.WriteAsync( - new ForceSynchronizeJellyfinLibraryById(library.Id, false), + new ForceSynchronizeJellyfinLibraryById(library.Id, request.DeepScan), cancellationToken); break; case EmbyLibrary: @@ -82,7 +82,7 @@ public class QueueLibraryScanByLibraryIdHandler( new SynchronizeEmbyLibraries(library.MediaSourceId), cancellationToken); await scannerWorker.WriteAsync( - new ForceSynchronizeEmbyLibraryById(library.Id, false), + new ForceSynchronizeEmbyLibraryById(library.Id, request.DeepScan), cancellationToken); break; } diff --git a/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryId.cs b/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryId.cs index e4b1bf194..886a76d19 100644 --- a/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryId.cs +++ b/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryId.cs @@ -1,3 +1,14 @@ namespace ErsatzTV.Application.Libraries; -public record QueueShowScanByLibraryId(int LibraryId, int ShowId, string ShowTitle, bool DeepScan) : IRequest; +public enum QueueShowScanResult +{ + Queued, + NotFound, + Unsupported, + SyncDisabled, + AlreadyScanning, + ScanFailed +} + +public record QueueShowScanByLibraryId(int LibraryId, int ShowId, string ShowTitle, bool DeepScan) + : IRequest; diff --git a/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryIdHandler.cs b/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryIdHandler.cs index 281606797..5d3c93f3c 100644 --- a/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryIdHandler.cs +++ b/ErsatzTV.Application/Libraries/Commands/QueueShowScanByLibraryIdHandler.cs @@ -19,9 +19,9 @@ public class QueueShowScanByLibraryIdHandler( IMediator mediator, ChannelWriter workerChannel, ILogger logger) - : IRequestHandler + : IRequestHandler { - public async Task Handle(QueueShowScanByLibraryId request, CancellationToken cancellationToken) + public async Task Handle(QueueShowScanByLibraryId request, CancellationToken cancellationToken) { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); @@ -42,14 +42,14 @@ public class QueueShowScanByLibraryIdHandler( if (!shouldSyncItems) { logger.LogWarning("Library sync is disabled for library id {Id}", library.Id); - return false; + return QueueShowScanResult.SyncDisabled; } - // Check if library is already being scanned - return false if locked + // A false from LockLibrary means a scan is already in progress; we own no release. if (!locker.LockLibrary(library.Id)) { logger.LogWarning("Library {Id} is already being scanned, cannot scan individual show", library.Id); - return false; + return QueueShowScanResult.AlreadyScanning; } logger.LogDebug( @@ -60,41 +60,43 @@ public class QueueShowScanByLibraryIdHandler( try { - var success = false; + QueueShowScanResult outcome; switch (library) { case PlexLibrary: Either plexResult = await mediator.Send( new SynchronizePlexShowById(library.Id, request.ShowId, request.DeepScan), cancellationToken); - success = plexResult.IsRight; + outcome = plexResult.IsRight ? QueueShowScanResult.Queued : QueueShowScanResult.ScanFailed; break; case JellyfinLibrary: Either jellyfinResult = await mediator.Send( new SynchronizeJellyfinShowById(library.Id, request.ShowId, request.DeepScan), cancellationToken); - success = jellyfinResult.IsRight; + outcome = jellyfinResult.IsRight ? QueueShowScanResult.Queued : QueueShowScanResult.ScanFailed; break; case EmbyLibrary: Either embyResult = await mediator.Send( new SynchronizeEmbyShowById(library.Id, request.ShowId, request.DeepScan), cancellationToken); - success = embyResult.IsRight; + outcome = embyResult.IsRight ? QueueShowScanResult.Queued : QueueShowScanResult.ScanFailed; break; case LocalLibrary: logger.LogWarning("Single show scanning is not supported for local libraries"); + outcome = QueueShowScanResult.Unsupported; break; default: logger.LogWarning("Unknown library type for library {Id}", library.Id); + outcome = QueueShowScanResult.Unsupported; break; } - if (success && request.DeepScan) + if (outcome == QueueShowScanResult.Queued && request.DeepScan) { await workerChannel.WriteAsync(new ExtractEmbeddedShowSubtitles(request.ShowId), cancellationToken); } - return success; + return outcome; } finally { @@ -103,6 +105,6 @@ public class QueueShowScanByLibraryIdHandler( } } - return false; + return QueueShowScanResult.NotFound; } } diff --git a/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs b/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs index c25cde87b..61fd248ef 100644 --- a/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Libraries/QueueLibraryScanByLibraryIdHandlerTests.cs @@ -164,6 +164,30 @@ public class QueueLibraryScanByLibraryIdHandlerTests locker.Received(1).UnlockLibrary(libraryId); } + [Test] + public async Task Handle_Should_Thread_DeepScan_Into_Plex_ForceSynchronize() + { + int libraryId = await SeedSyncEnabledPlexLibrary(); + + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + Channel channel = ThreadingChannel.CreateUnbounded(); + + QueueLibraryScanByLibraryIdHandler handler = CreateHandler(locker, channel.Writer); + + QueueLibraryScanResult result = await handler.Handle( + new QueueLibraryScanByLibraryId(libraryId, DeepScan: true), + CancellationToken.None); + + result.ShouldBe(QueueLibraryScanResult.Queued); + + // first message refreshes the library list, second is the deep force-sync carrying DeepScan == true + channel.Reader.TryRead(out IScannerBackgroundServiceRequest? first).ShouldBeTrue(); + first.ShouldBeOfType(); + channel.Reader.TryRead(out IScannerBackgroundServiceRequest? second).ShouldBeTrue(); + second.ShouldBeOfType().DeepScan.ShouldBeTrue(); + } + private QueueLibraryScanByLibraryIdHandler CreateHandler( IEntityLocker locker, ChannelWriter writer) => @@ -193,7 +217,11 @@ public class QueueLibraryScanByLibraryIdHandlerTests return source.Libraries[0].Id; } - private async Task SeedSyncDisabledPlexLibrary() + private async Task SeedSyncDisabledPlexLibrary() => await SeedPlexLibrary(shouldSyncItems: false); + + private async Task SeedSyncEnabledPlexLibrary() => await SeedPlexLibrary(shouldSyncItems: true); + + private async Task SeedPlexLibrary(bool shouldSyncItems) { await using TvContext context = _db.CreateContext(); var source = new PlexMediaSource @@ -212,7 +240,7 @@ public class QueueLibraryScanByLibraryIdHandlerTests Name = "Plex Movies", MediaKind = LibraryMediaKind.Movies, Key = "1", - ShouldSyncItems = false, + ShouldSyncItems = shouldSyncItems, Paths = [] } ] diff --git a/ErsatzTV.Tests/Application/Libraries/QueueShowScanByLibraryIdHandlerTests.cs b/ErsatzTV.Tests/Application/Libraries/QueueShowScanByLibraryIdHandlerTests.cs new file mode 100644 index 000000000..e9aba9722 --- /dev/null +++ b/ErsatzTV.Tests/Application/Libraries/QueueShowScanByLibraryIdHandlerTests.cs @@ -0,0 +1,214 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.Libraries; +using ErsatzTV.Application.Plex; +using ErsatzTV.Application.Subtitles; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Locking; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using MediatR; +using Microsoft.Extensions.Logging.Abstractions; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using static LanguageExt.Prelude; +using ThreadingChannel = System.Threading.Channels.Channel; + +namespace ErsatzTV.Tests.Application.Libraries; + +[TestFixture] +public class QueueShowScanByLibraryIdHandlerTests +{ + private InMemoryTvContext _db = null!; + + [SetUp] + public async Task SetUp() => _db = await InMemoryTvContext.CreateAsync(); + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + [Test] + public async Task Handle_Should_Return_NotFound_When_Library_Missing() + { + var locker = Substitute.For(); + var mediator = Substitute.For(); + + QueueShowScanByLibraryIdHandler handler = CreateHandler(locker, mediator, out _); + + QueueShowScanResult result = await handler.Handle( + new QueueShowScanByLibraryId(9999, 1, "Show", false), + CancellationToken.None); + + result.ShouldBe(QueueShowScanResult.NotFound); + locker.DidNotReceive().LockLibrary(Arg.Any()); + } + + [Test] + public async Task Handle_Should_Return_SyncDisabled_When_Item_Sync_Off() + { + int libraryId = await SeedPlexLibrary(shouldSyncItems: false); + var locker = Substitute.For(); + var mediator = Substitute.For(); + + QueueShowScanByLibraryIdHandler handler = CreateHandler(locker, mediator, out _); + + QueueShowScanResult result = await handler.Handle( + new QueueShowScanByLibraryId(libraryId, 1, "Show", false), + CancellationToken.None); + + result.ShouldBe(QueueShowScanResult.SyncDisabled); + locker.DidNotReceive().LockLibrary(Arg.Any()); + } + + [Test] + public async Task Handle_Should_Return_AlreadyScanning_When_Lock_Not_Acquired() + { + int libraryId = await SeedPlexLibrary(shouldSyncItems: true); + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(false); + var mediator = Substitute.For(); + + QueueShowScanByLibraryIdHandler handler = CreateHandler(locker, mediator, out _); + + QueueShowScanResult result = await handler.Handle( + new QueueShowScanByLibraryId(libraryId, 1, "Show", false), + CancellationToken.None); + + result.ShouldBe(QueueShowScanResult.AlreadyScanning); + await mediator.DidNotReceive().Send(Arg.Any(), Arg.Any()); + } + + [Test] + public async Task Handle_Should_Return_Unsupported_For_Local_Library() + { + int libraryId = await SeedLocalLibrary(); + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + var mediator = Substitute.For(); + + QueueShowScanByLibraryIdHandler handler = CreateHandler(locker, mediator, out _); + + QueueShowScanResult result = await handler.Handle( + new QueueShowScanByLibraryId(libraryId, 1, "Show", false), + CancellationToken.None); + + result.ShouldBe(QueueShowScanResult.Unsupported); + locker.Received(1).UnlockLibrary(libraryId); + } + + [Test] + public async Task Handle_Should_Return_Queued_And_Extract_Subtitles_On_Deep_Success() + { + int libraryId = await SeedPlexLibrary(shouldSyncItems: true); + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + var mediator = Substitute.For(); + mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Right("ok")); + + QueueShowScanByLibraryIdHandler handler = CreateHandler( + locker, + mediator, + out Channel worker); + + QueueShowScanResult result = await handler.Handle( + new QueueShowScanByLibraryId(libraryId, 42, "Show", DeepScan: true), + CancellationToken.None); + + result.ShouldBe(QueueShowScanResult.Queued); + locker.Received(1).UnlockLibrary(libraryId); + worker.Reader.TryRead(out IBackgroundServiceRequest? request).ShouldBeTrue(); + request.ShouldBeOfType().ShowId.ShouldBe(42); + } + + [Test] + public async Task Handle_Should_Return_ScanFailed_When_SubScan_Left() + { + int libraryId = await SeedPlexLibrary(shouldSyncItems: true); + var locker = Substitute.For(); + locker.LockLibrary(libraryId).Returns(true); + var mediator = Substitute.For(); + mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Left(BaseError.New("scan error"))); + + QueueShowScanByLibraryIdHandler handler = CreateHandler( + locker, + mediator, + out Channel worker); + + QueueShowScanResult result = await handler.Handle( + new QueueShowScanByLibraryId(libraryId, 42, "Show", DeepScan: true), + CancellationToken.None); + + result.ShouldBe(QueueShowScanResult.ScanFailed); + locker.Received(1).UnlockLibrary(libraryId); + // no subtitle extraction on a failed scan + worker.Reader.TryRead(out _).ShouldBeFalse(); + } + + private QueueShowScanByLibraryIdHandler CreateHandler( + IEntityLocker locker, + IMediator mediator, + out Channel worker) + { + worker = ThreadingChannel.CreateUnbounded(); + return new QueueShowScanByLibraryIdHandler( + _db.Factory, + locker, + mediator, + worker.Writer, + NullLogger.Instance); + } + + private async Task SeedLocalLibrary() + { + await using TvContext context = _db.CreateContext(); + var source = new LocalMediaSource + { + Libraries = + [ + new LocalLibrary + { + Name = "Local Movies", + MediaKind = LibraryMediaKind.Movies, + Paths = [] + } + ] + }; + await context.LocalMediaSources.AddAsync(source); + await context.SaveChangesAsync(); + return source.Libraries[0].Id; + } + + private async Task SeedPlexLibrary(bool shouldSyncItems) + { + await using TvContext context = _db.CreateContext(); + var source = new PlexMediaSource + { + ServerName = "Plex Server", + ProductVersion = "1", + Platform = "Linux", + PlatformVersion = "1", + ClientIdentifier = "plex", + Connections = [], + PathReplacements = [], + Libraries = + [ + new PlexLibrary + { + Name = "Plex Shows", + MediaKind = LibraryMediaKind.Shows, + Key = "1", + ShouldSyncItems = shouldSyncItems, + Paths = [] + } + ] + }; + await context.PlexMediaSources.AddAsync(source); + await context.SaveChangesAsync(); + return source.Libraries[0].Id; + } +} diff --git a/ErsatzTV.Tests/Controllers/EmbyMediaSourcesControllerTests.cs b/ErsatzTV.Tests/Controllers/EmbyMediaSourcesControllerTests.cs index d5a672677..cd838dcd8 100644 --- a/ErsatzTV.Tests/Controllers/EmbyMediaSourcesControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/EmbyMediaSourcesControllerTests.cs @@ -75,6 +75,10 @@ public class EmbyMediaSourcesControllerTests nameof(EmbyMediaSourcesController.RefreshLibraries), "POST", "/api/media-sources/emby/{id:int}/refresh-libraries"); + ShouldHaveActionRoute( + nameof(EmbyMediaSourcesController.ScanCollections), + "POST", + "/api/media-sources/emby/{id:int}/scan-collections"); } [Test] @@ -414,6 +418,48 @@ public class EmbyMediaSourcesControllerTests request.ShouldBeOfType().EmbyMediaSourceId.ShouldBe(1); } + [Test] + public async Task ScanCollections_Should_Return_404_When_Source_Missing() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.None); + + IActionResult result = await _controller.ScanCollections(99, cancellationToken: CancellationToken.None); + + result.ShouldBeOfType(); + _entityLocker.DidNotReceive().LockEmbyCollections(); + } + + [Test] + public async Task ScanCollections_Should_Return_409_When_Collections_Locked() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new EmbyMediaSourceViewModel(1, "s", "a"))); + _entityLocker.LockEmbyCollections().Returns(false); + + IActionResult result = await _controller.ScanCollections(1, cancellationToken: CancellationToken.None); + + result.ShouldBeOfType().Value.ShouldBeOfType().Status.ShouldBe(409); + _scannerChannel.Reader.TryRead(out _).ShouldBeFalse(); + } + + [Test] + public async Task ScanCollections_Should_Enqueue_And_Return_202() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new EmbyMediaSourceViewModel(1, "s", "a"))); + _entityLocker.LockEmbyCollections().Returns(true); + + IActionResult result = await _controller.ScanCollections(1, deep: true, CancellationToken.None); + + result.ShouldBeOfType(); + _scannerChannel.Reader.TryRead(out IScannerBackgroundServiceRequest? request).ShouldBeTrue(); + var command = request.ShouldBeOfType(); + command.EmbyMediaSourceId.ShouldBe(1); + command.ForceScan.ShouldBeTrue(); + command.DeepScan.ShouldBeTrue(); + } + private static void ShouldHaveActionRoute(string actionName, string httpMethod, string route) { MethodInfo action = typeof(EmbyMediaSourcesController).GetMethod(actionName) diff --git a/ErsatzTV.Tests/Controllers/JellyfinMediaSourcesControllerTests.cs b/ErsatzTV.Tests/Controllers/JellyfinMediaSourcesControllerTests.cs index 5623b5dfa..a891570bf 100644 --- a/ErsatzTV.Tests/Controllers/JellyfinMediaSourcesControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/JellyfinMediaSourcesControllerTests.cs @@ -75,6 +75,10 @@ public class JellyfinMediaSourcesControllerTests nameof(JellyfinMediaSourcesController.RefreshLibraries), "POST", "/api/media-sources/jellyfin/{id:int}/refresh-libraries"); + ShouldHaveActionRoute( + nameof(JellyfinMediaSourcesController.ScanCollections), + "POST", + "/api/media-sources/jellyfin/{id:int}/scan-collections"); } [Test] @@ -414,6 +418,48 @@ public class JellyfinMediaSourcesControllerTests request.ShouldBeOfType().JellyfinMediaSourceId.ShouldBe(1); } + [Test] + public async Task ScanCollections_Should_Return_404_When_Source_Missing() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.None); + + IActionResult result = await _controller.ScanCollections(99, cancellationToken: CancellationToken.None); + + result.ShouldBeOfType(); + _entityLocker.DidNotReceive().LockJellyfinCollections(); + } + + [Test] + public async Task ScanCollections_Should_Return_409_When_Collections_Locked() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new JellyfinMediaSourceViewModel(1, "s", "a"))); + _entityLocker.LockJellyfinCollections().Returns(false); + + IActionResult result = await _controller.ScanCollections(1, cancellationToken: CancellationToken.None); + + result.ShouldBeOfType().Value.ShouldBeOfType().Status.ShouldBe(409); + _scannerChannel.Reader.TryRead(out _).ShouldBeFalse(); + } + + [Test] + public async Task ScanCollections_Should_Enqueue_And_Return_202() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(Option.Some(new JellyfinMediaSourceViewModel(1, "s", "a"))); + _entityLocker.LockJellyfinCollections().Returns(true); + + IActionResult result = await _controller.ScanCollections(1, deep: true, CancellationToken.None); + + result.ShouldBeOfType(); + _scannerChannel.Reader.TryRead(out IScannerBackgroundServiceRequest? request).ShouldBeTrue(); + var command = request.ShouldBeOfType(); + command.JellyfinMediaSourceId.ShouldBe(1); + command.ForceScan.ShouldBeTrue(); + command.DeepScan.ShouldBeTrue(); + } + private static void ShouldHaveActionRoute(string actionName, string httpMethod, string route) { MethodInfo action = typeof(JellyfinMediaSourcesController).GetMethod(actionName) diff --git a/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs b/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs index 95ebbdf6b..476758752 100644 --- a/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/LibrariesControllerTests.cs @@ -71,14 +71,15 @@ public class LibrariesControllerTests } [Test] - public async Task ScanShow_Should_Queue_Scan_By_Show_Id_When_Show_Belongs_To_Library() + public async Task ScanShow_Should_Return_202_And_Queue_Scan_By_Show_Id_When_Show_Belongs_To_Library() { _televisionRepository.GetShowTitle(3, 42).Returns(Option.Some("The Office")); - _mediator.Send(Arg.Any(), Arg.Any()).Returns(true); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueShowScanResult.Queued); IActionResult result = await _controller.ScanShow(3, new ScanShowRequest(42, DeepScan: true)); - result.ShouldBeOfType(); + result.ShouldBeOfType(); await _mediator.Received(1).Send( Arg.Is(r => r.LibraryId == 3 && r.ShowId == 42 && r.ShowTitle == "The Office" && r.DeepScan), @@ -86,14 +87,68 @@ public class LibrariesControllerTests } [Test] - public async Task ScanShow_Should_Return_BadRequest_When_Mediator_Fails_To_Queue() + public async Task ScanShow_Should_Return_409_When_AlreadyScanning() { _televisionRepository.GetShowTitle(3, 42).Returns(Option.Some("The Office")); - _mediator.Send(Arg.Any(), Arg.Any()).Returns(false); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueShowScanResult.AlreadyScanning); IActionResult result = await _controller.ScanShow(3, new ScanShowRequest(42)); - result.ShouldBeOfType(); + var conflict = result.ShouldBeOfType(); + conflict.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status409Conflict); + } + + [Test] + public async Task ScanShow_Should_Return_422_When_SyncDisabled() + { + _televisionRepository.GetShowTitle(3, 42).Returns(Option.Some("The Office")); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueShowScanResult.SyncDisabled); + + IActionResult result = await _controller.ScanShow(3, new ScanShowRequest(42)); + + var unprocessable = result.ShouldBeOfType(); + unprocessable.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status422UnprocessableEntity); + } + + [Test] + public async Task ScanShow_Should_Return_422_When_Unsupported() + { + _televisionRepository.GetShowTitle(3, 42).Returns(Option.Some("The Office")); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueShowScanResult.Unsupported); + + IActionResult result = await _controller.ScanShow(3, new ScanShowRequest(42)); + + var unprocessable = result.ShouldBeOfType(); + unprocessable.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status422UnprocessableEntity); + } + + [Test] + public async Task ScanShow_Should_Return_422_When_ScanFailed() + { + _televisionRepository.GetShowTitle(3, 42).Returns(Option.Some("The Office")); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueShowScanResult.ScanFailed); + + IActionResult result = await _controller.ScanShow(3, new ScanShowRequest(42)); + + var unprocessable = result.ShouldBeOfType(); + unprocessable.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status422UnprocessableEntity); + } + + [Test] + public async Task ScanShow_Should_Return_404_When_Handler_Reports_NotFound() + { + _televisionRepository.GetShowTitle(3, 42).Returns(Option.Some("The Office")); + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueShowScanResult.NotFound); + + IActionResult result = await _controller.ScanShow(3, new ScanShowRequest(42)); + + var notFound = result.ShouldBeOfType(); + notFound.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status404NotFound); } [Test] @@ -102,11 +157,25 @@ public class LibrariesControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(QueueLibraryScanResult.Queued); - IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + IActionResult result = await _controller.ScanLibrary(7, cancellationToken: CancellationToken.None); result.ShouldBeOfType(); await _mediator.Received(1).Send( - Arg.Is(r => r.LibraryId == 7), + Arg.Is(r => r.LibraryId == 7 && !r.DeepScan), + Arg.Any()); + } + + [Test] + public async Task ScanLibrary_Should_Pass_DeepScan_When_Deep_Query_True() + { + _mediator.Send(Arg.Any(), Arg.Any()) + .Returns(QueueLibraryScanResult.Queued); + + IActionResult result = await _controller.ScanLibrary(7, deep: true, CancellationToken.None); + + result.ShouldBeOfType(); + await _mediator.Received(1).Send( + Arg.Is(r => r.LibraryId == 7 && r.DeepScan), Arg.Any()); } @@ -116,7 +185,7 @@ public class LibrariesControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(QueueLibraryScanResult.NotFound); - IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + IActionResult result = await _controller.ScanLibrary(7, cancellationToken: CancellationToken.None); var notFound = result.ShouldBeOfType(); notFound.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status404NotFound); @@ -128,7 +197,7 @@ public class LibrariesControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(QueueLibraryScanResult.AlreadyScanning); - IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + IActionResult result = await _controller.ScanLibrary(7, cancellationToken: CancellationToken.None); var conflict = result.ShouldBeOfType(); conflict.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status409Conflict); @@ -140,7 +209,7 @@ public class LibrariesControllerTests _mediator.Send(Arg.Any(), Arg.Any()) .Returns(QueueLibraryScanResult.SyncDisabled); - IActionResult result = await _controller.ScanLibrary(7, CancellationToken.None); + IActionResult result = await _controller.ScanLibrary(7, cancellationToken: CancellationToken.None); var unprocessable = result.ShouldBeOfType(); unprocessable.Value.ShouldBeOfType().Status.ShouldBe(StatusCodes.Status422UnprocessableEntity); diff --git a/ErsatzTV.Tests/Controllers/PlexMediaSourcesControllerTests.cs b/ErsatzTV.Tests/Controllers/PlexMediaSourcesControllerTests.cs index 4c66dacd1..a2a9a4ec2 100644 --- a/ErsatzTV.Tests/Controllers/PlexMediaSourcesControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlexMediaSourcesControllerTests.cs @@ -70,6 +70,10 @@ public class PlexMediaSourcesControllerTests nameof(PlexMediaSourcesController.RefreshLibraries), "POST", "/api/media-sources/plex/{id:int}/refresh-libraries"); + ShouldHaveActionRoute( + nameof(PlexMediaSourcesController.ScanCollections), + "POST", + "/api/media-sources/plex/{id:int}/scan-collections"); } // ----- P1 GetState ----- @@ -452,6 +456,64 @@ public class PlexMediaSourcesControllerTests Arg.Any()); } + // ----- P9 ScanCollections ----- + + [Test] + public async Task ScanCollections_Should_Return_404_When_Source_Missing() + { + SourceExists(false); + + IActionResult result = await _controller.ScanCollections(9, cancellationToken: CancellationToken.None); + + result.ShouldBeOfType(); + _entityLocker.DidNotReceive().LockPlexCollections(); + await _channel.DidNotReceive().WriteAsync( + Arg.Any(), + Arg.Any()); + } + + [Test] + public async Task ScanCollections_Should_Return_409_When_Collections_Locked() + { + SourceExists(true); + _entityLocker.LockPlexCollections().Returns(false); + + IActionResult result = await _controller.ScanCollections(3, cancellationToken: CancellationToken.None); + + result.ShouldBeOfType().Value.ShouldBeOfType().Status.ShouldBe(409); + await _channel.DidNotReceive().WriteAsync( + Arg.Any(), + Arg.Any()); + } + + [Test] + public async Task ScanCollections_Should_Return_202_And_Enqueue() + { + SourceExists(true); + _entityLocker.LockPlexCollections().Returns(true); + + IActionResult result = await _controller.ScanCollections(3, deep: true, CancellationToken.None); + + result.ShouldBeOfType(); + await _channel.Received(1).WriteAsync( + Arg.Is(s => s.PlexMediaSourceId == 3 && s.ForceScan && s.DeepScan), + Arg.Any()); + } + + [Test] + public async Task ScanCollections_Should_Compensate_Unlock_When_Enqueue_Throws() + { + SourceExists(true); + _entityLocker.LockPlexCollections().Returns(true); + _channel.WriteAsync(Arg.Any(), Arg.Any()) + .Returns(_ => throw new InvalidOperationException("channel closed")); + + await Should.ThrowAsync( + () => _controller.ScanCollections(3, cancellationToken: CancellationToken.None)); + + _entityLocker.Received(1).UnlockPlexCollections(); + } + private void SourceExists(bool exists) => _mediator.Send(Arg.Any(), Arg.Any()) .Returns(exists diff --git a/ErsatzTV/Controllers/Api/EmbyMediaSourcesController.cs b/ErsatzTV/Controllers/Api/EmbyMediaSourcesController.cs index 9780e20f4..a63450fda 100644 --- a/ErsatzTV/Controllers/Api/EmbyMediaSourcesController.cs +++ b/ErsatzTV/Controllers/Api/EmbyMediaSourcesController.cs @@ -278,6 +278,53 @@ public class EmbyMediaSourcesController( return new AcceptedResult(); } + [HttpPost("/api/media-sources/emby/{id:int}/scan-collections", Name = "ScanEmbyCollections")] + [Tags("Emby")] + [EndpointSummary("Scan an Emby source's collections")] + [EndpointDescription( + "Queues a synchronization of the source's collections (fire-and-forget). Pass ?deep=true for a deep " + + "scan. Returns 409 while an Emby collections scan is already in progress.")] + [EndpointGroupName("general")] + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + public async Task ScanCollections( + int id, + [FromQuery] bool deep = false, + CancellationToken cancellationToken = default) + { + Option maybeSource = + await mediator.Send(new GetEmbyMediaSourceById(id), cancellationToken); + if (maybeSource.IsNone) + { + return ApiResults.NotFoundProblem(); + } + + // The collections lock IS the running scan (§3b): fail to acquire = a scan is already active → 409. + if (!entityLocker.LockEmbyCollections()) + { + return ApiResults.ConflictProblem( + "Emby collections scan in progress", + "An Emby collections scan is already in progress; try again once it completes."); + } + + try + { + await scannerWorkerChannel.WriteAsync( + new SynchronizeEmbyCollections(id, true, deep), + cancellationToken); + } + catch + { + // the scanner releases the lock when it processes the message; if the enqueue throws after we + // acquired the lock, release it here (EnqueueWithTraktLock compensating-unlock, §3b) + entityLocker.UnlockEmbyCollections(); + throw; + } + + return new AcceptedResult(); + } + // §C7: LockLibrary then enqueue the sync pair; a throw from the enqueue compensates by unlocking // (EnqueueWithTraktLock pattern) — a locked library is silently skipped (Blazor parity). private async Task EnqueueLibrarySync(int sourceId, int libraryId, CancellationToken cancellationToken) diff --git a/ErsatzTV/Controllers/Api/JellyfinMediaSourcesController.cs b/ErsatzTV/Controllers/Api/JellyfinMediaSourcesController.cs index aa767ed14..d3a96d296 100644 --- a/ErsatzTV/Controllers/Api/JellyfinMediaSourcesController.cs +++ b/ErsatzTV/Controllers/Api/JellyfinMediaSourcesController.cs @@ -278,6 +278,53 @@ public class JellyfinMediaSourcesController( return new AcceptedResult(); } + [HttpPost("/api/media-sources/jellyfin/{id:int}/scan-collections", Name = "ScanJellyfinCollections")] + [Tags("Jellyfin")] + [EndpointSummary("Scan a Jellyfin source's collections")] + [EndpointDescription( + "Queues a synchronization of the source's collections (fire-and-forget). Pass ?deep=true for a deep " + + "scan. Returns 409 while a Jellyfin collections scan is already in progress.")] + [EndpointGroupName("general")] + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + public async Task ScanCollections( + int id, + [FromQuery] bool deep = false, + CancellationToken cancellationToken = default) + { + Option maybeSource = + await mediator.Send(new GetJellyfinMediaSourceById(id), cancellationToken); + if (maybeSource.IsNone) + { + return ApiResults.NotFoundProblem(); + } + + // The collections lock IS the running scan (§3b): fail to acquire = a scan is already active → 409. + if (!entityLocker.LockJellyfinCollections()) + { + return ApiResults.ConflictProblem( + "Jellyfin collections scan in progress", + "A Jellyfin collections scan is already in progress; try again once it completes."); + } + + try + { + await scannerWorkerChannel.WriteAsync( + new SynchronizeJellyfinCollections(id, true, deep), + cancellationToken); + } + catch + { + // the scanner releases the lock when it processes the message; if the enqueue throws after we + // acquired the lock, release it here (EnqueueWithTraktLock compensating-unlock, §3b) + entityLocker.UnlockJellyfinCollections(); + throw; + } + + return new AcceptedResult(); + } + // §C7: LockLibrary then enqueue the sync pair; a throw from the enqueue compensates by unlocking // (EnqueueWithTraktLock pattern) — a locked library is silently skipped (Blazor parity). private async Task EnqueueLibrarySync(int sourceId, int libraryId, CancellationToken cancellationToken) diff --git a/ErsatzTV/Controllers/Api/LibrariesController.cs b/ErsatzTV/Controllers/Api/LibrariesController.cs index d8525a52d..d75a11a5e 100644 --- a/ErsatzTV/Controllers/Api/LibrariesController.cs +++ b/ErsatzTV/Controllers/Api/LibrariesController.cs @@ -22,13 +22,18 @@ public class LibrariesController(ITelevisionRepository televisionRepository, IMe [HttpPost("/api/libraries/{id:int}/scan")] [Tags("Libraries")] [EndpointSummary("Scan library")] + [EndpointDescription("Queues a scan of the whole library. Pass ?deep=true for a deep (metadata-refresh) scan.")] [ProducesResponseType(StatusCodes.Status202Accepted)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] - public async Task ScanLibrary(int id, CancellationToken cancellationToken) + public async Task ScanLibrary( + int id, + [FromQuery] bool deep = false, + CancellationToken cancellationToken = default) { - QueueLibraryScanResult result = await mediator.Send(new QueueLibraryScanByLibraryId(id), cancellationToken); + QueueLibraryScanResult result = + await mediator.Send(new QueueLibraryScanByLibraryId(id, deep), cancellationToken); return result switch { QueueLibraryScanResult.Queued => new AcceptedResult(), @@ -49,19 +54,47 @@ public class LibrariesController(ITelevisionRepository televisionRepository, IMe [HttpPost("/api/libraries/{id:int}/scan-show")] [Tags("Libraries")] [EndpointSummary("Scan show")] - [ProducesResponseType(StatusCodes.Status200OK)] + [ProducesResponseType(StatusCodes.Status202Accepted)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] - [ProducesResponseType(StatusCodes.Status400BadRequest)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task ScanShow(int id, [FromBody] ScanShowRequest request) { Option maybeTitle = await televisionRepository.GetShowTitle(id, request.ShowId); foreach (string title in maybeTitle) { - bool result = await mediator.Send(new QueueShowScanByLibraryId(id, request.ShowId, title, request.DeepScan)); + QueueShowScanResult result = + await mediator.Send(new QueueShowScanByLibraryId(id, request.ShowId, title, request.DeepScan)); - return result - ? new OkResult() - : new BadRequestObjectResult(new { error = "Unable to queue show scan. Library may not exist, may not support single show scanning, or may already be scanning." }); + return result switch + { + QueueShowScanResult.Queued => new AcceptedResult(), + QueueShowScanResult.AlreadyScanning => ApiResults.ConflictProblem( + "Library scan in progress", + $"A scan for library {id} is already in progress; cannot scan an individual show."), + QueueShowScanResult.SyncDisabled => new UnprocessableEntityObjectResult( + new ProblemDetails + { + Status = StatusCodes.Status422UnprocessableEntity, + Title = "Library sync is disabled", + Detail = $"Item sync is disabled for library {id}." + }), + QueueShowScanResult.Unsupported => new UnprocessableEntityObjectResult( + new ProblemDetails + { + Status = StatusCodes.Status422UnprocessableEntity, + Title = "Single show scanning is not supported", + Detail = $"Library {id} does not support scanning an individual show." + }), + QueueShowScanResult.ScanFailed => new UnprocessableEntityObjectResult( + new ProblemDetails + { + Status = StatusCodes.Status422UnprocessableEntity, + Title = "Unable to scan show", + Detail = $"The scan for show {request.ShowId} in library {id} could not be completed." + }), + _ => ApiResults.NotFoundProblem($"Library {id} does not exist.") + }; } return ApiResults.NotFoundProblem($"Show {request.ShowId} does not exist in library {id}."); diff --git a/ErsatzTV/Controllers/Api/PlexMediaSourcesController.cs b/ErsatzTV/Controllers/Api/PlexMediaSourcesController.cs index e1cc451d6..ff6a48098 100644 --- a/ErsatzTV/Controllers/Api/PlexMediaSourcesController.cs +++ b/ErsatzTV/Controllers/Api/PlexMediaSourcesController.cs @@ -277,6 +277,51 @@ public class PlexMediaSourcesController( return new AcceptedResult(); } + [HttpPost("/api/media-sources/plex/{id:int}/scan-collections", Name = "ScanPlexCollections")] + [Tags("Plex")] + [EndpointSummary("Scan a Plex server's collections")] + [EndpointDescription( + "Queues a synchronization of the server's collections (fire-and-forget). Pass ?deep=true for a deep " + + "scan. Returns 409 while a Plex collections scan is already in progress.")] + [EndpointGroupName("general")] + [ProducesResponseType(StatusCodes.Status202Accepted)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + public async Task ScanCollections( + int id, + [FromQuery] bool deep = false, + CancellationToken cancellationToken = default) + { + if (!await PlexSourceExists(id, cancellationToken)) + { + return ApiResults.NotFoundProblem(); + } + + // The collections lock IS the running scan (§3b): fail to acquire = a scan is already active → 409. + if (!entityLocker.LockPlexCollections()) + { + return ApiResults.ConflictProblem( + "Plex collections scan in progress", + "A Plex collections scan is already in progress; try again once it completes."); + } + + try + { + await scannerWorkerChannel.WriteAsync( + new SynchronizePlexCollections(id, true, deep), + cancellationToken); + } + catch + { + // the scanner releases the lock when it processes the message; if the enqueue throws after we + // acquired the lock, release it here (EnqueueWithTraktLock compensating-unlock, §3b) + entityLocker.UnlockPlexCollections(); + throw; + } + + return new AcceptedResult(); + } + private async Task PlexSourceExists(int id, CancellationToken cancellationToken) => (await mediator.Send(new GetPlexMediaSourceById(id), cancellationToken)).IsSome; diff --git a/ErsatzTV/Pages/TelevisionSeasonList.razor b/ErsatzTV/Pages/TelevisionSeasonList.razor index 40016caf6..af9e0f4c7 100644 --- a/ErsatzTV/Pages/TelevisionSeasonList.razor +++ b/ErsatzTV/Pages/TelevisionSeasonList.razor @@ -307,8 +307,9 @@ private async Task ScanShow(bool deepScan) { - bool result = await Mediator.Send(new QueueShowScanByLibraryId(_show.LibraryId, _show.Id, _show.Title, deepScan)); - if (!result) + QueueShowScanResult result = + await Mediator.Send(new QueueShowScanByLibraryId(_show.LibraryId, _show.Id, _show.Title, deepScan)); + if (result != QueueShowScanResult.Queued) { Snackbar.Add($"Unable to scan show {_show.Title}", Severity.Error); } diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 38a944ce1..639868c98 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -135,6 +135,15 @@ operation itself, not a mutation racing it). Add `[ProducesResponseType]` for 20 `EnqueueWithTraktLock` compensating-unlock pattern (`TraktController`): if a `WriteAsync` throws after a successful `Lock*`, `Unlock*` in a `catch` and rethrow — one lock ⇄ exactly one release. +A second exemplar (issue #235 slice B), where the lock lives on the **controller** rather than in a +handler: `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=` acquires the +per-source collections lock (`entityLocker.LockPlexCollections()` etc.) — the lock IS the running +collections scan, so a `false` = 409 — then `WriteAsync`es `Synchronize{X}Collections(id, ForceScan: +true, deep)` to the scanner channel and returns **202**. `ScannerService` releases that lock in a +`finally` when it processes the message; the controller compensating-unlocks in a `catch` if the +enqueue throws. `POST /api/libraries/{id}/scan?deep=` similarly threads an optional `[FromQuery] bool +deep` into `QueueLibraryScanByLibraryId(id, DeepScan)`. + `NotFoundError : BaseError` lives in `ErsatzTV.Core/Errors/NotFoundError.cs` — return it from a handler's validation when a lookup fails, so the controller-side mapping falls out for free. diff --git a/docs/blazor-route-parity.md b/docs/blazor-route-parity.md index 6ae11a304..1addc1409 100644 --- a/docs/blazor-route-parity.md +++ b/docs/blazor-route-parity.md @@ -270,8 +270,10 @@ intentional exit ramp until phase (b) removes Blazor entirely. ## Section 5 — Removal execution runbook (#91 phase b, Step 2/3) The removal PR is **gated** — it starts only after these clear: ~~#202 (media-source write API + SPA)~~ -**DONE 2026-07-11**, #235 F9 (deep-scan + external-collections-scan API — no API today, so `Libraries.razor` -can't be deleted yet), and the **mandatory cold adversarial pass** (#91 comment 2026-07-09). (#204's id-carrying pattern +**DONE 2026-07-11**, ~~#235 F9 (deep-scan + external-collections-scan API)~~ **API DONE (#235 slice B)**: +`POST /api/libraries/{id}/scan?deep=` now threads deep-scan, and `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=` +covers external-collections scan (the two `Libraries.razor` parity gaps) — the SPA `Libraries.razor` port can now +proceed, and the **mandatory cold adversarial pass** (#91 comment 2026-07-09). (#204's id-carrying pattern redirects landed 2026-07-11; its catch-all fallback that replaces `MapFallbackToPage("/_Host")` is folded into Step 2 below, since it can only ship when `_Host` is deleted.) When those close, the removal-PR routine executes, in order: From a27cfc475e0453dd75fb2cb0710ffd33630c5f54 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:04:33 +0200 Subject: [PATCH 05/17] chore(235): regenerate OpenAPI artifacts + endpoint index after slice merge Co-Authored-By: Claude Opus 4.8 (1M context) --- ErsatzTV/wwwroot/openapi/v1.json | 423 ++++++++++++++++++++++++++++++- docs/endpoint-index.md | 5 +- web/src/api/generated/v1.d.ts | 6 + 3 files changed, 423 insertions(+), 11 deletions(-) diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 21666f098..01ed64660 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -1822,8 +1822,8 @@ } ], "responses": { - "200": { - "description": "OK" + "202": { + "description": "Accepted" }, "404": { "description": "Not Found", @@ -4798,6 +4798,80 @@ } } }, + "/api/media-sources/emby/{id}/scan-collections": { + "post": { + "tags": [ + "Emby" + ], + "summary": "Scan an Emby source's collections", + "description": "Queues a synchronization of the source's collections (fire-and-forget). Pass ?deep=true for a deep scan. Returns 409 while an Emby collections scan is already in progress.", + "operationId": "ScanEmbyCollections", + "parameters": [ + { + "name": "id", + "in": "path", + "required": true, + "schema": { + "type": "integer", + "format": "int32" + } + }, + { + "name": "deep", + "in": "query", + "schema": { + "type": "boolean", + "default": false + } + } + ], + "responses": { + "202": { + "description": "Accepted" + }, + "404": { + "description": "Not Found", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + } + } + } + }, "/api/ffmpeg/profiles": { "get": { "tags": [ @@ -6576,6 +6650,80 @@ } } }, + "/api/media-sources/jellyfin/{id}/scan-collections": { + "post": { + "tags": [ + "Jellyfin" + ], + "summary": "Scan a Jellyfin source's collections", + "description": "Queues a synchronization of the source's collections (fire-and-forget). Pass ?deep=true for a deep scan. Returns 409 while a Jellyfin collections scan is already in progress.", + "operationId": "ScanJellyfinCollections", + "parameters": [ + { + "name": "id", + "in": "path", + "required": true, + "schema": { + "type": "integer", + "format": "int32" + } + }, + { + "name": "deep", + "in": "query", + "schema": { + "type": "boolean", + "default": false + } + } + ], + "responses": { + "202": { + "description": "Accepted" + }, + "404": { + "description": "Not Found", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + } + } + } + }, "/api/languages": { "get": { "tags": [ @@ -6662,6 +6810,7 @@ "Libraries" ], "summary": "Scan library", + "description": "Queues a scan of the whole library. Pass ?deep=true for a deep (metadata-refresh) scan.", "parameters": [ { "name": "id", @@ -6671,6 +6820,14 @@ "type": "integer", "format": "int32" } + }, + { + "name": "deep", + "in": "query", + "schema": { + "type": "boolean", + "default": false + } } ], "responses": { @@ -6783,8 +6940,8 @@ "required": true }, "responses": { - "200": { - "description": "OK" + "202": { + "description": "Accepted" }, "404": { "description": "Not Found", @@ -6806,8 +6963,28 @@ } } }, - "400": { - "description": "Bad Request", + "409": { + "description": "Conflict", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "422": { + "description": "Unprocessable Entity", "content": { "text/plain": { "schema": { @@ -7548,6 +7725,26 @@ "responses": { "200": { "description": "OK" + }, + "422": { + "description": "Unprocessable Entity", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } } } } @@ -7559,8 +7756,8 @@ ], "summary": "Clean artwork cache", "responses": { - "200": { - "description": "OK" + "202": { + "description": "Accepted" } } } @@ -10458,7 +10655,24 @@ "operationId": "ResetAllPlayouts", "responses": { "202": { - "description": "Accepted" + "description": "Accepted", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ResetAllPlayoutsResponseModel" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ResetAllPlayoutsResponseModel" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ResetAllPlayoutsResponseModel" + } + } + } } } } @@ -11275,6 +11489,80 @@ } } }, + "/api/media-sources/plex/{id}/scan-collections": { + "post": { + "tags": [ + "Plex" + ], + "summary": "Scan a Plex server's collections", + "description": "Queues a synchronization of the server's collections (fire-and-forget). Pass ?deep=true for a deep scan. Returns 409 while a Plex collections scan is already in progress.", + "operationId": "ScanPlexCollections", + "parameters": [ + { + "name": "id", + "in": "path", + "required": true, + "schema": { + "type": "integer", + "format": "int32" + } + }, + { + "name": "deep", + "in": "query", + "schema": { + "type": "boolean", + "default": false + } + } + ], + "responses": { + "202": { + "description": "Accepted" + }, + "404": { + "description": "Not Found", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "409": { + "description": "Conflict", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + } + } + } + }, "/api/rerun-collections": { "get": { "tags": [ @@ -15846,6 +16134,26 @@ } ], "responses": { + "404": { + "description": "Not Found", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "409": { "description": "Conflict", "content": { @@ -15865,6 +16173,26 @@ } } } + }, + "422": { + "description": "Unprocessable Entity", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } } } }, @@ -15960,6 +16288,26 @@ } ], "responses": { + "404": { + "description": "Not Found", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "409": { "description": "Conflict", "content": { @@ -15979,6 +16327,26 @@ } } } + }, + "422": { + "description": "Unprocessable Entity", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } } } } @@ -22533,7 +22901,8 @@ "dailyRebuildTime", "buildStatus", "decoId", - "decoName" + "decoName", + "isLocked" ], "type": "object", "properties": { @@ -22591,6 +22960,9 @@ "null", "string" ] + }, + "isLocked": { + "type": "boolean" } } }, @@ -23422,6 +23794,37 @@ } } }, + "ResetAllPlayoutsResponseModel": { + "required": [ + "queuedPlayoutIds", + "skippedLocked", + "skippedUnsupported" + ], + "type": "object", + "properties": { + "queuedPlayoutIds": { + "type": "array", + "items": { + "type": "integer", + "format": "int32" + } + }, + "skippedLocked": { + "type": "array", + "items": { + "type": "integer", + "format": "int32" + } + }, + "skippedUnsupported": { + "type": "array", + "items": { + "type": "integer", + "format": "int32" + } + } + } + }, "ResolutionResponseModel": { "required": [ "id", diff --git a/docs/endpoint-index.md b/docs/endpoint-index.md index d1b1ad3e5..7f833e36d 100644 --- a/docs/endpoint-index.md +++ b/docs/endpoint-index.md @@ -2,7 +2,7 @@ *Generated by `scripts/generate-endpoint-index.py` from `ErsatzTV/wwwroot/openapi/v1.json`. Do not edit by hand -- regenerated by `scripts/update-openapi.sh`.* -155 endpoints, 240 operations. +158 endpoints, 243 operations. ## Artists @@ -122,6 +122,7 @@ | GET | `/api/media-sources/emby/{id}/path-replacements` | GetEmbyPathReplacements | Get an Emby source's path replacements | | PUT | `/api/media-sources/emby/{id}/path-replacements` | ReplaceEmbyPathReplacements | Replace an Emby source's path replacements | | POST | `/api/media-sources/emby/{id}/refresh-libraries` | RefreshEmbyLibraries | Refresh an Emby source's libraries | +| POST | `/api/media-sources/emby/{id}/scan-collections` | ScanEmbyCollections | Scan an Emby source's collections | ## FFmpeg Profiles @@ -176,6 +177,7 @@ | GET | `/api/media-sources/jellyfin/{id}/path-replacements` | GetJellyfinPathReplacements | Get a Jellyfin source's path replacements | | PUT | `/api/media-sources/jellyfin/{id}/path-replacements` | ReplaceJellyfinPathReplacements | Replace a Jellyfin source's path replacements | | POST | `/api/media-sources/jellyfin/{id}/refresh-libraries` | RefreshJellyfinLibraries | Refresh a Jellyfin source's libraries | +| POST | `/api/media-sources/jellyfin/{id}/scan-collections` | ScanJellyfinCollections | Scan a Jellyfin source's collections | ## Languages @@ -295,6 +297,7 @@ | GET | `/api/media-sources/plex/{id}/path-replacements` | GetPlexPathReplacements | Get a Plex server's path replacements | | PUT | `/api/media-sources/plex/{id}/path-replacements` | ReplacePlexPathReplacements | Replace a Plex server's path replacements | | POST | `/api/media-sources/plex/{id}/refresh-libraries` | RefreshPlexLibraries | Refresh a Plex server's libraries | +| POST | `/api/media-sources/plex/{id}/scan-collections` | ScanPlexCollections | Scan a Plex server's collections | ## Rerun Collections diff --git a/web/src/api/generated/v1.d.ts b/web/src/api/generated/v1.d.ts index b03c5e51d..d4ee00744 100644 --- a/web/src/api/generated/v1.d.ts +++ b/web/src/api/generated/v1.d.ts @@ -1093,6 +1093,7 @@ export interface components { "buildStatus": null | components["schemas"]["PlayoutBuildStatusResponseModel"]; "decoId": null | number; "decoName": null | string; + "isLocked": boolean; }; "PlayoutScheduleKind": "None" | "Classic" | "Block" | "Sequential" | "Scripted" | "ExternalJson"; "PlayoutSettingsResponseModel": { @@ -1243,6 +1244,11 @@ export interface components { "selectedName": null | string; "firstRunPlaybackOrder": components["schemas"]["PlaybackOrder"]; "rerunPlaybackOrder": components["schemas"]["PlaybackOrder"]; + }; + "ResetAllPlayoutsResponseModel": { + "queuedPlayoutIds": Array; + "skippedLocked": Array; + "skippedUnsupported": Array; }; "ResolutionResponseModel": { "id": number; From d32ca976f72f881efe5d0426cd83977b7be4d56a Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:10:02 +0200 Subject: [PATCH 06/17] feat(235): SPA clients for deep/collections scan + typed reset-all; docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - libraries.ts: scanLibrary(id, deep), new scanCollections(source, id, deep), corrected stale scanShow status-code comment (400 -> 202/404/409/422) - playouts.ts: resetAllPlayouts returns typed ResetAllPlayoutsResponseModel body - libraries.test.ts: deep-scan + scanCollections client tests - decisions.md: #235 async-op contract + F9 endpoints + accepted-by-design channels note - blazor-route-parity.md §5: F9 API gate closed; SPA deep/collections buttons = removal-PR work Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/blazor-route-parity.md | 6 +++- docs/decisions.md | 55 +++++++++++++++++++++++++++++++++++ web/src/api/libraries.test.ts | 21 ++++++++++++- web/src/api/libraries.ts | 28 ++++++++++++++---- web/src/api/playouts.ts | 9 ++++-- 5 files changed, 110 insertions(+), 9 deletions(-) diff --git a/docs/blazor-route-parity.md b/docs/blazor-route-parity.md index 1addc1409..ed95b8e5e 100644 --- a/docs/blazor-route-parity.md +++ b/docs/blazor-route-parity.md @@ -273,7 +273,11 @@ The removal PR is **gated** — it starts only after these clear: ~~#202 (media- **DONE 2026-07-11**, ~~#235 F9 (deep-scan + external-collections-scan API)~~ **API DONE (#235 slice B)**: `POST /api/libraries/{id}/scan?deep=` now threads deep-scan, and `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=` covers external-collections scan (the two `Libraries.razor` parity gaps) — the SPA `Libraries.razor` port can now -proceed, and the **mandatory cold adversarial pass** (#91 comment 2026-07-09). (#204's id-carrying pattern +proceed, and the **mandatory cold adversarial pass** (#91 comment 2026-07-09). **Remaining SPA affordance for +the removal PR** (API + thin `web/src/api/libraries.ts` clients — `scanLibrary(id, deep)`, `scanCollections` — +already shipped by #235): `LibrariesScreen` currently exposes only quick-scan; the removal PR must add the +**deep-scan** and **external-collections-scan** buttons (wiring the shipped clients) before deleting +`Libraries.razor`, or that capability is lost. (#204's id-carrying pattern redirects landed 2026-07-11; its catch-all fallback that replaces `MapFallbackToPage("/_Host")` is folded into Step 2 below, since it can only ship when `_Host` is deleted.) When those close, the removal-PR routine executes, in order: diff --git a/docs/decisions.md b/docs/decisions.md index ce6d41b94..0d7a1d738 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -649,3 +649,58 @@ action of the Step 2 deletion PR merge** (not before — `main` moves until then cut. Not cut this session — `main` still carries Blazor and will advance before the removal PR. + +## 2026-07-11 — Async-op API contract normalization + playout build observability + F9 scan endpoints (#235) + +Reviewer#20 F7/F8/F9. Normalizes the queue-triggering `/api/*` endpoints onto one contract, closes the two +F9 `Libraries.razor` parity gaps, and hardens the Trakt batch-lock lifecycle. Much of the F8 surface was +**already normalized** by #232 (library scan → `QueueLibraryScanResult` 202/404/409/422) and #215 (per-id +playout mutations + reset → 409 lock guard) — this issue finished the remaining outliers. + +**Normalized async-op contract** (queue-triggering endpoints): **202 Accepted** = work queued; **404 +ProblemDetails** = entity missing (controller pre-check); **409 ProblemDetails** = lock held (the running +job, or a mutation racing it — §3a/§3b); **422 ProblemDetails** = domain precondition (sync disabled / +unsupported / start failed). Trakt was the reference implementation. Changes made: +- `MaintenanceController.EmptyTrash` — error path **500 text/plain → 404/422 ProblemDetails** (`ToErrorResult`). +- `MaintenanceController.CleanArtwork` — silent **200 → 202** (fire-and-forget enqueue). No SPA consumer. +- `LibrariesController.ScanShow` — conflated **400 `{error}` → 202/404/409/422** via a new + `QueueShowScanResult` enum (6 outcomes incl. an honest `ScanFailed`→422, distinct from `Unsupported`). +- `ChannelController.ResetPlayout` — **200 → 202** (queue-triggering; 404/409 unchanged). +- `PlayoutController.ResetAll` — **202 (no body) → 202 + `ResetAllPlayoutsResponseModel`** reporting + `queuedPlayoutIds` / `skippedLocked` / `skippedUnsupported` (replaces the silent skip; still 202, still + skips locked/ExternalJson by design per §3a — now it *reports* what it skipped). +- `TroubleshootController.TroubleshootPlayback` — bare body-less `NotFound()` → **404/422 ProblemDetails** + with distinguishing detail. **Status codes the SPA HLS player depends on were preserved** — verified + `HlsPlayer.tsx` never branches on this endpoint's status (playback state comes from the separate + `/api/troubleshoot/playback/status` poll); only the error *body* was enriched. + +**Playout build observability**: the list endpoint (`GET /api/playouts`) already stamped `isLocked` + +`BuildStatus` on `PlayoutListItemResponseModel` (#215); this issue adds **`isLocked` to the single-playout +`GET /api/playouts/{id}`** (`PlayoutResponseModel`), so the detail poll surface carries the §3a lock flag +too. No dedicated `GET /api/playouts/{id}/status` push channel was added — the flag on the existing GETs is +the HTTP-observable substitute for Blazor's live lock event, matching the `GET /api/trakt/status` precedent. + +**F9 parity endpoints** (the `Libraries.razor` deletion gate — #202 did NOT close these): +- **Deep scan**: `POST /api/libraries/{id}/scan` gains `?deep=false`, threaded through + `QueueLibraryScanByLibraryId(LibraryId, DeepScan=false)` into `ForceSynchronize{Plex,Jellyfin,Emby}LibraryById(id, deep)` + (was hardcoded `false`). Non-breaking: existing callers omit it. +- **External-collections scan**: new `POST /api/media-sources/{plex|jellyfin|emby}/{id}/scan-collections?deep=false` + on the three #202 media-source controllers, dispatching `Synchronize{X}Collections(id, ForceScan:true, deep)`. + Each pre-checks source existence (404), acquires the per-source **collections** lock (`Lock{X}Collections()` — + the lock *is* the running scan, so a false = **409**), then enqueues and returns 202; the controller + compensating-unlocks in a `catch` if the enqueue throws (§3b), and `ScannerService` releases in its `finally`. + Thin SPA clients shipped (`scanLibrary(id, deep)`, `scanCollections`); **the SPA deep-scan / collections + buttons are the removal PR's remaining parity work** (parity doc §5). + +**F7 Trakt batch-lock leak fix**: the global Trakt lock was released only when the *terminal* batch message +(`Unlock: true`) was processed; a `WorkerService` shutdown/cancellation before that message leaked the lock +permanently (subsequent Trakt ops 409 until restart — same class as #231/#233/#234). Fix: `WorkerService` +now releases the Trakt lock in a `finally` on read-loop exit if still held. Non-vacuous regression test proven +against an inverted-condition control. + +**Accepted-by-design** (per the issue's decision-record ask): the worker's channels are **unbounded** and +there is **no shutdown drain** — messages still queued at process exit are dropped. This is acceptable because +the entity locks are **in-memory singletons that die with the process**, so a dropped message can't strand a +lock across restarts (the F7 `finally` covers the *within-process* shutdown-break leak, which is the only way +a lock outlives its batch while the process keeps running). Adding a bounded-channel backpressure / graceful +drain is out of scope and would not fix a correctness bug. diff --git a/web/src/api/libraries.test.ts b/web/src/api/libraries.test.ts index 349f26bb0..2cb3745cd 100644 --- a/web/src/api/libraries.test.ts +++ b/web/src/api/libraries.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { scanLibrary, scanShow } from './libraries'; +import { scanCollections, scanLibrary, scanShow } from './libraries'; function noContent(): Response { return new Response(null, { status: 200 }); @@ -22,6 +22,25 @@ describe('libraries api client', () => { expect(fetchMock).toHaveBeenCalledWith('/api/libraries/4/scan', expect.objectContaining({ method: 'POST' })); }); + it('scanLibrary appends ?deep=true for a deep scan', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(noContent()); + await scanLibrary(4, true); + expect(fetchMock).toHaveBeenCalledWith('/api/libraries/4/scan?deep=true', expect.objectContaining({ method: 'POST' })); + }); + + it('scanCollections POSTs to the media-source scan-collections endpoint', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(noContent()); + await scanCollections('jellyfin', 7); + expect(fetchMock).toHaveBeenCalledWith('/api/media-sources/jellyfin/7/scan-collections', expect.objectContaining({ method: 'POST' })); + }); + + it('scanCollections appends ?deep=true for a deep scan', async () => { + const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(noContent()); + await scanCollections('plex', 3, true); + const { url } = lastCall(fetchMock); + expect(url).toBe('/api/media-sources/plex/3/scan-collections?deep=true'); + }); + it('scanShow POSTs the show id and deepScan flag', async () => { const fetchMock = vi.spyOn(window, 'fetch').mockResolvedValue(noContent()); await scanShow(4, { deepScan: true, showId: 42 }); diff --git a/web/src/api/libraries.ts b/web/src/api/libraries.ts index 66361eae9..97b66ad98 100644 --- a/web/src/api/libraries.ts +++ b/web/src/api/libraries.ts @@ -42,8 +42,24 @@ export function getLibraryScanStatus(): Promise { ); } -export function scanLibrary(libraryId: number): Promise { - return request(`/api/libraries/${libraryId}/scan`, { method: 'POST' }); +// Queues a library scan. `deep` requests a deep (force-metadata) scan — the API equivalent of the +// legacy Blazor "Deep Scan Library" button (#235 F9); omit or pass false for a quick scan. Returns +// 202 when queued, 404 (missing), 409 (already scanning), 422 (sync disabled) — see §3b. +export function scanLibrary(libraryId: number, deep = false): Promise { + const query = deep ? '?deep=true' : ''; + return request(`/api/libraries/${libraryId}/scan${query}`, { method: 'POST' }); +} + +// Media-source families that support an external-collections scan (#235 F9). Matches the three +// media-source controllers #202 introduced. +export type CollectionsScanSource = 'emby' | 'jellyfin' | 'plex'; + +// Queues an external-collections scan for a Plex/Jellyfin/Emby media source — the API equivalent of +// the legacy Blazor "Scan Collections" button (#235 F9). `deep` requests a deep scan. Returns 202 +// when queued, 404 (source missing), 409 (a collections scan is already running for that source). +export function scanCollections(source: CollectionsScanSource, sourceId: number, deep = false): Promise { + const query = deep ? '?deep=true' : ''; + return request(`/api/media-sources/${source}/${sourceId}/scan-collections${query}`, { method: 'POST' }); } export interface ScanShowParams { @@ -51,9 +67,11 @@ export interface ScanShowParams { deepScan?: boolean; } -// Queues a scan of a single show (by id) within a library. Returns 200 on success, 404 when the -// show id doesn't exist in the library, 400 when the library doesn't support single-show -// scanning. Body keys are `showId` and `deepScan` (see LibrariesController.ScanShowRequest). +// Queues a scan of a single show (by id) within a library. Returns 202 when queued, 404 when the +// show id doesn't exist in the library, 409 when a scan is already running, and 422 when the +// library doesn't support single-show scanning / sync is disabled / the scan failed to start (all +// error bodies are ProblemDetails; #235 normalized the old conflated 400). Body keys are `showId` +// and `deepScan` (see LibrariesController.ScanShowRequest). export function scanShow(libraryId: number, params: ScanShowParams): Promise { return request(`/api/libraries/${libraryId}/scan-show`, { body: { deepScan: params.deepScan ?? false, showId: params.showId }, diff --git a/web/src/api/playouts.ts b/web/src/api/playouts.ts index 839281c3d..529800061 100644 --- a/web/src/api/playouts.ts +++ b/web/src/api/playouts.ts @@ -108,8 +108,13 @@ export function getPlayoutChannelStates(): Promise { return request('/api/channels/state'); } -export function resetAllPlayouts(): Promise { - return request('/api/playouts/reset-all', { method: 'POST' }); +export type ResetAllPlayoutsResult = components['schemas']['ResetAllPlayoutsResponseModel']; + +// Queues a reset of every eligible playout. Returns 202 with a body reporting which playouts were +// queued vs skipped (locked, or an unsupported ExternalJson/None schedule kind) — #235 replaced the +// old silent skip. The caller may surface `skipped*` to explain why some playouts didn't reset. +export function resetAllPlayouts(): Promise { + return request('/api/playouts/reset-all', { method: 'POST' }); } export function deletePlayout(playoutId: number): Promise { From 1a8c0f60defa184e2eb2f6328ede326836c0fc41 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sat, 11 Jul 2026 18:33:40 +0200 Subject: [PATCH 07/17] 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 08/17] 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 09/17] 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