diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrder.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrder.cs index 0e7c44b6b..6ae9d26c0 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrder.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrder.cs @@ -4,6 +4,7 @@ namespace ErsatzTV.Application.MediaCollections; public record UpdateCollectionCustomOrder( int CollectionId, - List MediaItemCustomOrders) : IRequest>; + List MediaItemCustomOrders, + Option ExpectedVersion = default) : IRequest>; public record MediaItemCustomOrder(int MediaItemId, int CustomIndex); diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrderHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrderHandler.cs index c8333f71b..50c20b11b 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrderHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateCollectionCustomOrderHandler.cs @@ -32,13 +32,22 @@ public class UpdateCollectionCustomOrderHandler : IRequestHandler validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(c => ApplyUpdateRequest(dbContext, c, request)); + + // Optimistic-concurrency check as a standalone Either AFTER validation, never via Apply (which + // Join()s the error Seq and would flatten PreconditionFailedError to a 422) — issue #253 §7a. + Either validated = LanguageExtensions.ToEither(validation) + .Bind(c => c.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: c => ApplyUpdateRequest(dbContext, c, request, cancellationToken), + Left: error => Task.FromResult>(error)); } - private async Task ApplyUpdateRequest( + private async Task> ApplyUpdateRequest( TvContext dbContext, Collection c, - UpdateCollectionCustomOrder request) + UpdateCollectionCustomOrder request, + CancellationToken cancellationToken) { foreach (MediaItemCustomOrder updateItem in request.MediaItemCustomOrders) { @@ -51,14 +60,24 @@ public class UpdateCollectionCustomOrderHandler : IRequestHandler 0) + // Unconditional bump (issue #253 §7a / M1) then guarded save (→ 412 on a lost race). + c.Version++; + Either saveResult = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + if (saveResult.IsLeft) { - // refresh all playouts that use this collection - foreach (int playoutId in await _mediaCollectionRepository - .PlayoutIdsUsingCollection(request.CollectionId)) - { - await _channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh)); - } + return saveResult; + } + + // Refresh all playouts that use this collection. The old `SaveChangesAsync() > 0` gate is always + // true once the version bumps unconditionally (M2), so run the refresh on any successful save. + // Post-commit enqueue on CancellationToken.None so a late cancellation can't drop the rebuild + // after the commit landed (#254 / §7b). + foreach (int playoutId in await _mediaCollectionRepository + .PlayoutIdsUsingCollection(request.CollectionId)) + { + await _channel.WriteAsync( + new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), + CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollection.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollection.cs index 9aca8d253..13e156667 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollection.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollection.cs @@ -12,4 +12,5 @@ public record UpdateMultiCollectionItem( public record UpdateMultiCollection( int MultiCollectionId, string Name, - List Items) : IRequest>; + List Items, + Option ExpectedVersion = default) : IRequest>; diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs index f4af1f30a..877ad55a6 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateMultiCollectionHandler.cs @@ -36,10 +36,18 @@ public class UpdateMultiCollectionHandler : IRequestHandler validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(c => ApplyUpdateRequest(dbContext, c, request, cancellationToken)); + + // Optimistic-concurrency check as a standalone Either AFTER validation, never via Apply (which + // Join()s the error Seq and would flatten PreconditionFailedError to a 422) — issue #253 §7a. + Either validated = LanguageExtensions.ToEither(validation) + .Bind(c => c.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: c => ApplyUpdateRequest(dbContext, c, request, cancellationToken), + Left: error => Task.FromResult>(error)); } - private async Task ApplyUpdateRequest( + private async Task> ApplyUpdateRequest( TvContext dbContext, MultiCollection c, UpdateMultiCollection request, @@ -47,8 +55,16 @@ public class UpdateMultiCollectionHandler : IRequestHandler nameSave = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + if (nameSave.IsLeft) + { + return nameSave; + } var toAdd = request.Items .Filter(i => i.CollectionId.HasValue) diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollection.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollection.cs index 089593d00..11adece2d 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollection.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollection.cs @@ -13,5 +13,6 @@ public record UpdateRerunCollection( SmartCollectionViewModel SmartCollection, NamedMediaItemViewModel MediaItem, PlaybackOrder FirstRunPlaybackOrder, - PlaybackOrder RerunPlaybackOrder) + PlaybackOrder RerunPlaybackOrder, + Option ExpectedVersion = default) : IRequest>; diff --git a/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollectionHandler.cs b/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollectionHandler.cs index 3f2c3620f..7a9a68f99 100644 --- a/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollectionHandler.cs +++ b/ErsatzTV.Application/MediaCollections/Commands/UpdateRerunCollectionHandler.cs @@ -22,10 +22,18 @@ public class UpdateRerunCollectionHandler( { await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); Validation validation = await Validate(dbContext, request, cancellationToken); - return await validation.Apply(c => ApplyUpdateRequest(dbContext, c, request, cancellationToken)); + + // Optimistic-concurrency check as a standalone Either AFTER validation, never via Apply (which + // Join()s the error Seq and would flatten PreconditionFailedError to a 422) — issue #253 §7a. + Either validated = LanguageExtensions.ToEither(validation) + .Bind(c => c.CheckVersion(request.ExpectedVersion)); + + return await validated.Match( + Right: c => ApplyUpdateRequest(dbContext, c, request, cancellationToken), + Left: error => Task.FromResult>(error)); } - private async Task ApplyUpdateRequest( + private async Task> ApplyUpdateRequest( TvContext dbContext, RerunCollection c, UpdateRerunCollection request, @@ -40,17 +48,23 @@ public class UpdateRerunCollectionHandler( c.FirstRunPlaybackOrder = request.FirstRunPlaybackOrder; c.RerunPlaybackOrder = request.RerunPlaybackOrder; - // rebuild playouts - if (await dbContext.SaveChangesAsync(cancellationToken) > 0) + // Unconditional bump (issue #253 §7a / M1) then guarded save, which maps a racing + // DbUpdateConcurrencyException to PreconditionFailedError (→ 412). + c.Version++; + Either saveResult = await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + if (saveResult.IsLeft) { - // post-commit side effect runs on CancellationToken.None so a late request cancellation - // can't abort it after the commit landed (#254) - // refresh all playouts that use this rerun collection - foreach (int playoutId in await mediaCollectionRepository.PlayoutIdsUsingRerunCollection( - request.RerunCollectionId)) - { - await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); - } + return saveResult; + } + + // Refresh all playouts that use this rerun collection. The old `SaveChangesAsync() > 0` gate is + // always true once the version bumps unconditionally (M2), so run the refresh on any successful + // save. Post-commit enqueue on CancellationToken.None so a late cancellation can't drop the + // rebuild after the commit landed (#254). + foreach (int playoutId in await mediaCollectionRepository.PlayoutIdsUsingRerunCollection( + request.RerunCollectionId)) + { + await channel.WriteAsync(new BuildPlayout(playoutId, PlayoutBuildMode.Refresh), CancellationToken.None); } return Unit.Default; diff --git a/ErsatzTV.Application/MediaCollections/Mapper.cs b/ErsatzTV.Application/MediaCollections/Mapper.cs index 05271e474..930864e67 100644 --- a/ErsatzTV.Application/MediaCollections/Mapper.cs +++ b/ErsatzTV.Application/MediaCollections/Mapper.cs @@ -12,14 +12,16 @@ internal static class Mapper collection.Id, collection.Name, collection.UseCustomPlaybackOrder, - MediaItemState.Normal); + MediaItemState.Normal, + collection.Version); internal static MultiCollectionViewModel ProjectToViewModel(MultiCollection multiCollection) => new( multiCollection.Id, multiCollection.Name, Optional(multiCollection.MultiCollectionItems).Flatten().Map(ProjectToViewModel).ToList(), - Optional(multiCollection.MultiCollectionSmartItems).Flatten().Map(ProjectToViewModel).ToList()); + Optional(multiCollection.MultiCollectionSmartItems).Flatten().Map(ProjectToViewModel).ToList(), + multiCollection.Version); internal static SmartCollectionViewModel ProjectToViewModel(SmartCollection collection) => new(collection.Id, collection.Name, collection.Query); @@ -49,7 +51,8 @@ internal static class Mapper _ => null }, collection.FirstRunPlaybackOrder, - collection.RerunPlaybackOrder); + collection.RerunPlaybackOrder, + collection.Version); internal static TraktListViewModel ProjectToViewModel(TraktList traktList) => new( diff --git a/ErsatzTV.Application/MediaCollections/MediaCollectionViewModel.cs b/ErsatzTV.Application/MediaCollections/MediaCollectionViewModel.cs index 023842a04..8bd43b956 100644 --- a/ErsatzTV.Application/MediaCollections/MediaCollectionViewModel.cs +++ b/ErsatzTV.Application/MediaCollections/MediaCollectionViewModel.cs @@ -8,7 +8,10 @@ public record MediaCollectionViewModel( int Id, string Name, bool UseCustomPlaybackOrder, - MediaItemState State) : MediaCardViewModel( + MediaItemState State, + // Optimistic-concurrency token (issue #253), header-only via ETag; 0 for the selection/placeholder + // constructions that don't read a real collection. Set from the entity by the Mapper projection. + int Version = 0) : MediaCardViewModel( Id, Name, string.Empty, diff --git a/ErsatzTV.Application/MediaCollections/MultiCollectionViewModel.cs b/ErsatzTV.Application/MediaCollections/MultiCollectionViewModel.cs index 507826fd7..e90ebc916 100644 --- a/ErsatzTV.Application/MediaCollections/MultiCollectionViewModel.cs +++ b/ErsatzTV.Application/MediaCollections/MultiCollectionViewModel.cs @@ -4,4 +4,6 @@ public record MultiCollectionViewModel( int Id, string Name, List Items, - List SmartItems); + List SmartItems, + // Optimistic-concurrency token (issue #253), header-only via ETag; 0 for selection placeholders. + int Version = 0); diff --git a/ErsatzTV.Application/MediaCollections/RerunCollectionViewModel.cs b/ErsatzTV.Application/MediaCollections/RerunCollectionViewModel.cs index 3be4e1497..27d1415ff 100644 --- a/ErsatzTV.Application/MediaCollections/RerunCollectionViewModel.cs +++ b/ErsatzTV.Application/MediaCollections/RerunCollectionViewModel.cs @@ -12,4 +12,6 @@ public record RerunCollectionViewModel( SmartCollectionViewModel SmartCollection, NamedMediaItemViewModel MediaItem, PlaybackOrder FirstRunPlaybackOrder, - PlaybackOrder RerunPlaybackOrder); + PlaybackOrder RerunPlaybackOrder, + // Optimistic-concurrency token (issue #253), header-only via ETag. + int Version = 0); diff --git a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItems.cs b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItems.cs index dd5b7fc78..7c398fceb 100644 --- a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItems.cs +++ b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItems.cs @@ -2,5 +2,8 @@ using ErsatzTV.Core; namespace ErsatzTV.Application.Playouts; -public record ReplacePlayoutAlternateScheduleItems(int PlayoutId, List Items) +public record ReplacePlayoutAlternateScheduleItems( + int PlayoutId, + List Items, + Option ExpectedVersion = default) : IRequest>; diff --git a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs index 434a0e439..8cc641b74 100644 --- a/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/ReplacePlayoutAlternateScheduleItemsHandler.cs @@ -40,6 +40,18 @@ public class ReplacePlayoutAlternateScheduleItemsHandler( foreach (Playout playout in maybePlayout) { + // Optimistic-concurrency pre-check (issue #253 §7a): reject a stale If-Match with a + // PreconditionFailedError (→ 412) before any mutation. Introduced as a standalone Either, + // and returned directly (a value, not a throw) so it escapes the catch(Exception) below + // rather than being reshaped into a bare 422 (§9/H1). + Either versionCheck = playout.CheckVersion(request.ExpectedVersion); + if (versionCheck.IsLeft) + { + return versionCheck.Match>( + Left: error => error, + Right: _ => Unit.Default); + } + var existingScheduleMap = new Dictionary(); var daysToCheck = new List(); @@ -127,7 +139,20 @@ public class ReplacePlayoutAlternateScheduleItemsHandler( playout.ProgramScheduleId = highest.ProgramScheduleId; } - await dbContext.SaveChangesAsync(cancellationToken); + // Unconditional bump (issue #253 §7a / M1): EF emits the root UPDATE only when a scalar + // actually differs, so a no-op PUT-back would otherwise neither fire the concurrency token + // nor rotate other clients' ETags. + playout.Version++; + + // Guarded save maps a racing DbUpdateConcurrencyException to PreconditionFailedError (→ 412) + // as a return value, so a lost race short-circuits here before the post-commit refresh block + // and escapes the catch(Exception) below as a 412, not a 422 (§9/H1). + Either saveResult = + await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + if (saveResult.IsLeft) + { + return saveResult; + } if (hasDefaultScheduleChange) { diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs index c94770f57..943a96cdd 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateExternalJsonPlayoutHandler.cs @@ -58,7 +58,8 @@ public class playout.DailyRebuildTime, playout.BuildStatus, playout.DecoId, - playout.Deco?.Name); + playout.Deco?.Name, + playout.Version); } private static Task> Validate( diff --git a/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs index 1ca83a979..3a90dc9d9 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdatePlayoutHandler.cs @@ -48,7 +48,8 @@ public class UpdatePlayoutHandler : IRequestHandler> Validate( diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs index 9ef7c1c58..cadc2eb64 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateScriptedPlayoutHandler.cs @@ -53,7 +53,8 @@ public class playout.DailyRebuildTime, playout.BuildStatus, playout.DecoId, - playout.Deco?.Name); + playout.Deco?.Name, + playout.Version); } private async Task> Validate( diff --git a/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs b/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs index e81cb6ab9..974967629 100644 --- a/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs +++ b/ErsatzTV.Application/Playouts/Commands/UpdateSequentialPlayoutHandler.cs @@ -58,7 +58,8 @@ public class playout.DailyRebuildTime, playout.BuildStatus, playout.DecoId, - playout.Deco?.Name); + playout.Deco?.Name, + playout.Version); } private static Task> Validate( diff --git a/ErsatzTV.Application/Playouts/Mapper.cs b/ErsatzTV.Application/Playouts/Mapper.cs index da82809b5..9f05feaae 100644 --- a/ErsatzTV.Application/Playouts/Mapper.cs +++ b/ErsatzTV.Application/Playouts/Mapper.cs @@ -19,7 +19,8 @@ internal static class Mapper playout.DecoId, // the paged-playouts query does not eager-load Deco (the list response does not surface // the default deco); GetPlayoutById includes it for the detail response - playout.Deco?.Name); + playout.Deco?.Name, + playout.Version); internal static PlayoutItemViewModel ProjectToViewModel(PlayoutItem playoutItem) => new( diff --git a/ErsatzTV.Application/Playouts/PlayoutNameViewModel.cs b/ErsatzTV.Application/Playouts/PlayoutNameViewModel.cs index 6ee27fbf0..82a1d1bcd 100644 --- a/ErsatzTV.Application/Playouts/PlayoutNameViewModel.cs +++ b/ErsatzTV.Application/Playouts/PlayoutNameViewModel.cs @@ -13,7 +13,8 @@ public record PlayoutNameViewModel( TimeSpan? DbDailyRebuildTime, PlayoutBuildStatus BuildStatus, int? DecoId, - string DecoName) + string DecoName, + int Version) { public Option DailyRebuildTime => Optional(DbDailyRebuildTime); diff --git a/ErsatzTV.Application/Playouts/Queries/GetPlayoutByIdHandler.cs b/ErsatzTV.Application/Playouts/Queries/GetPlayoutByIdHandler.cs index 8583f91ac..00439caaf 100644 --- a/ErsatzTV.Application/Playouts/Queries/GetPlayoutByIdHandler.cs +++ b/ErsatzTV.Application/Playouts/Queries/GetPlayoutByIdHandler.cs @@ -30,6 +30,7 @@ public class GetPlayoutByIdHandler(IDbContextFactory dbContextFactory p.DailyRebuildTime, p.BuildStatus, p.DecoId, - p.DecoId == null ? null : p.Deco.Name)); + p.DecoId == null ? null : p.Deco.Name, + p.Version)); } } diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItems.cs b/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItems.cs index 221d19cbf..00888c406 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItems.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItems.cs @@ -2,5 +2,8 @@ using ErsatzTV.Core; namespace ErsatzTV.Application.Scheduling; -public record ReplacePlayoutTemplateItems(int PlayoutId, List Items) +public record ReplacePlayoutTemplateItems( + int PlayoutId, + List Items, + Option ExpectedVersion = default) : IRequest>; diff --git a/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs b/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs index a9c33ffdd..11975e491 100644 --- a/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/ReplacePlayoutTemplateItemsHandler.cs @@ -29,6 +29,17 @@ public class ReplacePlayoutTemplateItemsHandler( foreach (Playout playout in maybePlayout) { + // Optimistic-concurrency pre-check (issue #253 §7a): reject a stale If-Match with a + // PreconditionFailedError (→ 412) before any mutation. Returned directly so it escapes the + // catch(Exception) below as a 412 rather than being reshaped into a bare 422 (§9/H1). + Either versionCheck = playout.CheckVersion(request.ExpectedVersion); + if (versionCheck.IsLeft) + { + return versionCheck.Match>( + Left: error => error, + Right: _ => Option.None); + } + PlayoutTemplate[] existing = playout.Templates.ToArray(); List incoming = request.Items; @@ -88,7 +99,20 @@ public class ReplacePlayoutTemplateItemsHandler( } } - await dbContext.SaveChangesAsync(cancellationToken); + // Unconditional bump (issue #253 §7a / M1): #8 writes no root scalar at all (only the + // Templates child collection), so without this the root UPDATE never fires and the token + // never rotates. Bump then save through the guard, which maps a racing + // DbUpdateConcurrencyException to PreconditionFailedError (→ 412) as a return value. + playout.Version++; + + Either saveResult = + await dbContext.SaveChangesWithConcurrencyGuard(cancellationToken); + if (saveResult.IsLeft) + { + return saveResult.Match>( + Left: error => error, + Right: _ => Option.None); + } } return Option.None; diff --git a/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs b/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs index de31cfd49..2e3620ac4 100644 --- a/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs +++ b/ErsatzTV.Application/Scheduling/Commands/UpdateDefaultDecoHandler.cs @@ -20,7 +20,14 @@ public class UpdateDefaultDecoHandler( int updated = await dbContext.Playouts .Where(p => p.Id == request.PlayoutId) - .ExecuteUpdateAsync(u => u.SetProperty(p => p.DecoId, p => request.DecoId), cancellationToken); + .ExecuteUpdateAsync( + u => u + .SetProperty(p => p.DecoId, p => request.DecoId) + // Bump the shared Playout.Version so any open alternate-schedule/template editor tab + // (which round-trips this same token) detects the deco change and reloads (#253 §7a). + // Bulk ExecuteUpdate bypasses change-tracking, so bump via SetProperty, not Version++. + .SetProperty(p => p.Version, p => p.Version + 1), + cancellationToken); // Changing a playout's default deco only takes effect on a Reset build (deco content is applied // during Reset, and BlockKey has no deco dimension to self-heal) — same stale-until-Reset class diff --git a/ErsatzTV.Tests/Application/MediaCollections/CollectionCustomOrderConcurrencyTests.cs b/ErsatzTV.Tests/Application/MediaCollections/CollectionCustomOrderConcurrencyTests.cs new file mode 100644 index 000000000..a549bcb52 --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/CollectionCustomOrderConcurrencyTests.cs @@ -0,0 +1,82 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Errors; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using Unit = LanguageExt.Unit; +using static LanguageExt.Prelude; + +namespace ErsatzTV.Tests.Application.MediaCollections; + +/// +/// #253 optimistic-concurrency contract on the Collection custom-order handler (#6): the pre-check +/// 412 (standalone Either after validation, not flattened to 422) and the unconditional Version bump. +/// +[TestFixture] +public class CollectionCustomOrderConcurrencyTests : MediaCollectionHandlerTestBase +{ + private async Task SeedCollectionWithVersion(int id, int version, string name = "Collection") + { + await using TvContext context = Db.CreateContext(); + context.Collections.Add(new Collection + { + Id = id, + Name = name, + Version = version, + MediaItems = [], + CollectionItems = [] + }); + await context.SaveChangesAsync(); + } + + private async Task ReadVersion(int id) + { + await using TvContext context = Db.CreateContext(); + return await context.Collections.Where(c => c.Id == id).Select(c => c.Version).SingleAsync(); + } + + private UpdateCollectionCustomOrderHandler MakeHandler() + { + IMediaCollectionRepository repo = Substitute.For(); + repo.PlayoutIdsUsingCollection(Arg.Any()).Returns([]); + return new UpdateCollectionCustomOrderHandler(Db.Factory, repo, Worker); + } + + private static BaseError LeftOf(Either either) => + either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); + + [Test] + public async Task Stale_If_Match_Should_Fail_Precondition_And_Not_Mutate() + { + await SeedCollectionWithVersion(1, version: 2); + + Either result = await MakeHandler().Handle( + new UpdateCollectionCustomOrder(1, [], Some(1)), + CancellationToken.None); + + LeftOf(result).ShouldBeOfType(); + (await ReadVersion(1)).ShouldBe(2); + } + + [Test] + public async Task Matching_If_Match_Should_Succeed_And_Bump() + { + await SeedCollectionWithVersion(1, version: 2); + + Either result = await MakeHandler().Handle( + new UpdateCollectionCustomOrder(1, [], Some(2)), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(3); + } +} diff --git a/ErsatzTV.Tests/Application/MediaCollections/MultiCollectionConcurrencyTests.cs b/ErsatzTV.Tests/Application/MediaCollections/MultiCollectionConcurrencyTests.cs new file mode 100644 index 000000000..14029bbf2 --- /dev/null +++ b/ErsatzTV.Tests/Application/MediaCollections/MultiCollectionConcurrencyTests.cs @@ -0,0 +1,107 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.MediaCollections; +using ErsatzTV.Core; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Errors; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using Microsoft.EntityFrameworkCore; +using NSubstitute; +using NUnit.Framework; +using Shouldly; +using Unit = LanguageExt.Unit; +using static LanguageExt.Prelude; + +namespace ErsatzTV.Tests.Application.MediaCollections; + +/// +/// #253 optimistic-concurrency contract on the MultiCollection replace handler (#9): pre-check 412 +/// (standalone Either, not flattened to 422), the unconditional Version bump, and the M2 rework of +/// the old SaveChangesAsync() > 0 gate — a name-only change still saves (and bumps) but must +/// NOT rebuild playouts, since the version bump rides the first (name) save while the item save stays +/// gated on real item changes. +/// +[TestFixture] +public class MultiCollectionConcurrencyTests : MediaCollectionHandlerTestBase +{ + private static UpdateMultiCollection Update(int id, string name, Option expectedVersion) => + new(id, name, [], expectedVersion); + + private async Task SeedMultiCollection(int id, int version, string name = "Multi") + { + await using TvContext context = Db.CreateContext(); + context.MultiCollections.Add(new MultiCollection + { + Id = id, + Name = name, + Version = version, + MultiCollectionItems = [], + MultiCollectionSmartItems = [] + }); + await context.SaveChangesAsync(); + } + + private async Task ReadVersion(int id) + { + await using TvContext context = Db.CreateContext(); + return await context.MultiCollections.Where(c => c.Id == id).Select(c => c.Version).SingleAsync(); + } + + private UpdateMultiCollectionHandler MakeHandler(IMediaCollectionRepository repo) => + new(Db.Factory, repo, Worker, SearchTargets); + + private static IMediaCollectionRepository EmptyRepo() + { + IMediaCollectionRepository repo = Substitute.For(); + repo.PlayoutIdsUsingMultiCollection(Arg.Any()).Returns([]); + return repo; + } + + private static BaseError LeftOf(Either either) => + either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); + + [Test] + public async Task Update_Stale_If_Match_Should_Fail_Precondition_And_Not_Mutate() + { + await SeedMultiCollection(1, version: 2); + + Either result = + await MakeHandler(EmptyRepo()).Handle(Update(1, "Renamed", Some(1)), CancellationToken.None); + + LeftOf(result).ShouldBeOfType(); + (await ReadVersion(1)).ShouldBe(2); + } + + [Test] + public async Task Update_Matching_If_Match_Should_Succeed_And_Bump() + { + await SeedMultiCollection(1, version: 2); + + Either result = + await MakeHandler(EmptyRepo()).Handle(Update(1, "Renamed", Some(2)), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(3); + } + + [Test] + public async Task Name_Only_Change_Should_Bump_But_Not_Rebuild_Playouts() + { + await SeedMultiCollection(1, version: 1, name: "Before"); + IMediaCollectionRepository repo = EmptyRepo(); + + Either result = + await MakeHandler(repo).Handle(Update(1, "After", None), CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadVersion(1)).ShouldBe(2); + + // M2: the version bump rides the first (name) save, so the second (items) save writes nothing → + // no rebuild is enqueued and the search index is not signalled for a name-only edit. + SearchTargets.DidNotReceive().SearchTargetsChanged(); + await repo.DidNotReceive().PlayoutIdsUsingMultiCollection(Arg.Any()); + } +} diff --git a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionHandlerTests.cs b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionHandlerTests.cs index b1998754d..25e059e0e 100644 --- a/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionHandlerTests.cs +++ b/ErsatzTV.Tests/Application/MediaCollections/RerunCollectionHandlerTests.cs @@ -8,10 +8,12 @@ using ErsatzTV.Core.Interfaces.Repositories; using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; using LanguageExt; +using Microsoft.EntityFrameworkCore; using NSubstitute; using NUnit.Framework; using Shouldly; using Unit = LanguageExt.Unit; +using static LanguageExt.Prelude; namespace ErsatzTV.Tests.Application.MediaCollections; @@ -150,7 +152,49 @@ public class RerunCollectionHandlerTests : MediaCollectionHandlerTestBase await context.SaveChangesAsync(); } - private async Task SeedRerunCollection(int id, CollectionType collectionType, string name = "Rerun") + [Test] + public async Task Update_Stale_If_Match_Should_Fail_Precondition() + { + await SeedRerunCollection(1, CollectionType.SmartCollection, version: 2); + await SeedSmartCollection(2); + IMediaCollectionRepository repo = Substitute.For(); + repo.PlayoutIdsUsingRerunCollection(Arg.Any()).Returns([]); + var handler = new UpdateRerunCollectionHandler(Db.Factory, repo, Worker); + + Either result = await handler.Handle( + MakeUpdate(1, CollectionType.SmartCollection, smartCollection: new SmartCollectionViewModel(2, "", "")) + with { ExpectedVersion = Some(1) }, + CancellationToken.None); + + LeftOf(result).ShouldBeOfType(); + (await ReadVersion(1)).ShouldBe(2); + } + + [Test] + public async Task Update_Matching_If_Match_Should_Succeed_And_Bump() + { + await SeedRerunCollection(1, CollectionType.SmartCollection, version: 2); + await SeedSmartCollection(2); + IMediaCollectionRepository repo = Substitute.For(); + repo.PlayoutIdsUsingRerunCollection(Arg.Any()).Returns([]); + var handler = new UpdateRerunCollectionHandler(Db.Factory, repo, Worker); + + Either result = await handler.Handle( + MakeUpdate(1, CollectionType.SmartCollection, smartCollection: new SmartCollectionViewModel(2, "", "")) + with { ExpectedVersion = Some(2) }, + CancellationToken.None); + + RightOf(result); + (await ReadVersion(1)).ShouldBe(3); + } + + private async Task ReadVersion(int id) + { + await using TvContext context = Db.CreateContext(); + return await context.RerunCollections.Where(c => c.Id == id).Select(c => c.Version).SingleAsync(); + } + + private async Task SeedRerunCollection(int id, CollectionType collectionType, string name = "Rerun", int version = 0) { await using TvContext context = Db.CreateContext(); context.RerunCollections.Add(new RerunCollection @@ -158,6 +202,7 @@ public class RerunCollectionHandlerTests : MediaCollectionHandlerTestBase Id = id, Name = name, CollectionType = collectionType, + Version = version, FirstRunPlaybackOrder = PlaybackOrder.Chronological, RerunPlaybackOrder = PlaybackOrder.Chronological }); diff --git a/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs b/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs index 76a6fd8cc..ce769c8b2 100644 --- a/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Playouts/PlayoutHandlerTests.cs @@ -1,6 +1,7 @@ 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.Errors; @@ -9,6 +10,7 @@ using ErsatzTV.Infrastructure.Data; using ErsatzTV.Tests.Support; using LanguageExt; using MediatR; +using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging.Abstractions; using NSubstitute; using NUnit.Framework; @@ -16,6 +18,7 @@ using Shouldly; using Testably.Abstractions.Testing; using Unit = LanguageExt.Unit; using Channel = System.Threading.Channels.Channel; +using static LanguageExt.Prelude; namespace ErsatzTV.Tests.Application.Playouts; @@ -118,6 +121,149 @@ public class PlayoutHandlerTests LeftOf(result).Value.ShouldContain("must not be empty"); } + // ---- #253 optimistic concurrency (alternate schedules #7 + templates #8, shared Playout.Version) ---- + + private static ReplacePlayoutAlternateSchedule AltItem(int programScheduleId = 10) => + new(0, 0, programScheduleId, [], [], [], false, 1, 1, null, 12, 31, null); + + private static ReplacePlayoutTemplate TemplateItem(int templateId = 20) => + new(0, 0, templateId, null, [], [], [], false, 1, 1, null, 12, 31, null); + + private async Task SeedPlayout(int id, int version, int? programScheduleId = 10) + { + await using TvContext context = _db.CreateContext(); + context.Playouts.Add( + new Playout + { + Id = id, + ChannelId = 1, + ProgramScheduleId = programScheduleId, + Version = version, + Items = [], + ProgramScheduleAlternates = [], + Templates = [] + }); + await context.SaveChangesAsync(); + } + + private async Task ReadPlayoutVersion(int id) + { + await using TvContext context = _db.CreateContext(); + return await context.Playouts.Where(p => p.Id == id).Select(p => p.Version).SingleAsync(); + } + + [Test] + public async Task ReplaceAlternateSchedules_Stale_If_Match_Should_Fail_Precondition_And_Not_Mutate() + { + await SeedPlayout(1, version: 2); + var handler = new ReplacePlayoutAlternateScheduleItemsHandler( + _db.Factory, + _worker, + NullLogger.Instance); + + Either result = await handler.Handle( + new ReplacePlayoutAlternateScheduleItems(1, [AltItem()], Some(1)), + CancellationToken.None); + + // Standalone-Either pre-check (not via Apply) preserves the subtype → 412 rather than a flattened 422. + LeftOf(result).ShouldBeOfType(); + (await ReadPlayoutVersion(1)).ShouldBe(2); + } + + [Test] + public async Task ReplaceAlternateSchedules_Matching_If_Match_Should_Succeed_And_Bump() + { + await SeedPlayout(1, version: 2); + var handler = new ReplacePlayoutAlternateScheduleItemsHandler( + _db.Factory, + _worker, + NullLogger.Instance); + + Either result = await handler.Handle( + new ReplacePlayoutAlternateScheduleItems(1, [AltItem()], Some(2)), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadPlayoutVersion(1)).ShouldBe(3); + } + + [Test] + public async Task ReplaceAlternateSchedules_Absent_If_Match_Should_Force_Write_And_Bump() + { + await SeedPlayout(1, version: 2); + var handler = new ReplacePlayoutAlternateScheduleItemsHandler( + _db.Factory, + _worker, + NullLogger.Instance); + + Either result = await handler.Handle( + new ReplacePlayoutAlternateScheduleItems(1, [AltItem()], None), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(); + (await ReadPlayoutVersion(1)).ShouldBe(3); + } + + [Test] + public async Task ReplaceTemplates_Stale_If_Match_Should_Fail_Precondition_And_Not_Mutate() + { + await SeedPlayout(1, version: 2); + var handler = new ReplacePlayoutTemplateItemsHandler( + _db.Factory, + NullLogger.Instance); + + Option result = await handler.Handle( + new ReplacePlayoutTemplateItems(1, [TemplateItem()], Some(1)), + CancellationToken.None); + + // Some(PreconditionFailedError): the pre-check escapes the handler's catch(Exception) as a 412 + // rather than being reshaped into a bare BaseError → 422 (§9/H1). + result.IfNone(() => throw new AssertionException("Expected a Some(error)")) + .ShouldBeOfType(); + (await ReadPlayoutVersion(1)).ShouldBe(2); + } + + [Test] + public async Task ReplaceTemplates_Matching_If_Match_Should_Succeed_And_Bump() + { + await SeedPlayout(1, version: 2); + var handler = new ReplacePlayoutTemplateItemsHandler( + _db.Factory, + NullLogger.Instance); + + Option result = await handler.Handle( + new ReplacePlayoutTemplateItems(1, [TemplateItem()], Some(2)), + CancellationToken.None); + + result.IsNone.ShouldBeTrue(); + (await ReadPlayoutVersion(1)).ShouldBe(3); + } + + [Test] + public async Task Racing_Playout_Save_Should_Map_Concurrency_Failure_To_412() + { + await SeedPlayout(1, version: 1); + + // Backstop mechanic on the Playout token: two writers load the same version and both bump-and-save; + // IsConcurrencyToken on Playout keys the loser's UPDATE on the original version → 0 rows → + // DbUpdateConcurrencyException → PreconditionFailedError. Non-vacuous: drop .IsConcurrencyToken() + // on Playout and the loser silently overwrites the winner instead. + await using TvContext ctxWinner = _db.CreateContext(); + await using TvContext ctxLoser = _db.CreateContext(); + + Playout winner = await ctxWinner.Playouts.SingleAsync(p => p.Id == 1); + Playout loser = await ctxLoser.Playouts.SingleAsync(p => p.Id == 1); + + winner.Version++; + (await ctxWinner.SaveChangesWithConcurrencyGuard(CancellationToken.None)).IsRight.ShouldBeTrue(); + + loser.Version++; + Either loserResult = await ctxLoser.SaveChangesWithConcurrencyGuard(CancellationToken.None); + + loserResult.Match(Right: _ => null, Left: e => e).ShouldBeOfType(); + (await ReadPlayoutVersion(1)).ShouldBe(2); + } + private static BaseError LeftOf(Either either) => either.Match(Left: e => e, Right: _ => throw new AssertionException("Expected a Left result")); diff --git a/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs b/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs index 67b84c74e..3e300c819 100644 --- a/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/ChannelControllerTests.cs @@ -454,7 +454,8 @@ public class ChannelControllerTests null, null, null, - null); + null, + 0); private static ChannelViewModel MakeVm(int id) => new( diff --git a/ErsatzTV.Tests/Controllers/CollectionControllerTests.cs b/ErsatzTV.Tests/Controllers/CollectionControllerTests.cs index fff04b2be..9914e4e2d 100644 --- a/ErsatzTV.Tests/Controllers/CollectionControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/CollectionControllerTests.cs @@ -8,6 +8,7 @@ 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; @@ -28,7 +29,11 @@ public class CollectionControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new CollectionController(_mediator); + _controller = new CollectionController(_mediator) + { + // Real HttpContext so the #253 ETag/If-Match concurrency headers can be read/written. + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() } + }; } [Test] diff --git a/ErsatzTV.Tests/Controllers/MultiCollectionControllerTests.cs b/ErsatzTV.Tests/Controllers/MultiCollectionControllerTests.cs index d40215341..d9e491943 100644 --- a/ErsatzTV.Tests/Controllers/MultiCollectionControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/MultiCollectionControllerTests.cs @@ -7,6 +7,7 @@ using ErsatzTV.Core.Api.MediaCollections; using ErsatzTV.Core.Domain; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -24,7 +25,11 @@ public class MultiCollectionControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new MultiCollectionController(_mediator); + _controller = new MultiCollectionController(_mediator) + { + // Real HttpContext so the #253 ETag/If-Match concurrency headers can be read/written. + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() } + }; } private MultiCollectionController _controller = null!; diff --git a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs index d0af315c4..9f99c254d 100644 --- a/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/PlayoutControllerTests.cs @@ -17,6 +17,7 @@ using ErsatzTV.Core.Interfaces.Locking; using ErsatzTV.Core.Scheduling; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -39,7 +40,11 @@ public class PlayoutControllerTests { _mediator = Substitute.For(); _entityLocker = Substitute.For(); - _controller = new PlayoutController(_mediator, _entityLocker); + _controller = new PlayoutController(_mediator, _entityLocker) + { + // Real HttpContext so the #253 ETag/If-Match concurrency headers can be read/written. + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() } + }; } [Test] @@ -1284,7 +1289,8 @@ public class PlayoutControllerTests null, new PlayoutBuildStatus(), null, - null); + null, + 0); private static PlayoutResponseModel ToResponse(PlayoutNameViewModel vm) => PlayoutResponseModel.From( diff --git a/ErsatzTV.Tests/Controllers/RerunCollectionControllerTests.cs b/ErsatzTV.Tests/Controllers/RerunCollectionControllerTests.cs index 9a6430775..6f4cf53ab 100644 --- a/ErsatzTV.Tests/Controllers/RerunCollectionControllerTests.cs +++ b/ErsatzTV.Tests/Controllers/RerunCollectionControllerTests.cs @@ -7,6 +7,7 @@ using ErsatzTV.Core.Api.MediaCollections; using ErsatzTV.Core.Domain; using LanguageExt; using MediatR; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Mvc.Routing; using NSubstitute; @@ -24,7 +25,11 @@ public class RerunCollectionControllerTests public void SetUp() { _mediator = Substitute.For(); - _controller = new RerunCollectionController(_mediator); + _controller = new RerunCollectionController(_mediator) + { + // Real HttpContext so the #253 ETag/If-Match concurrency headers can be read/written. + ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() } + }; } private RerunCollectionController _controller = null!; diff --git a/ErsatzTV/Controllers/Api/CollectionController.cs b/ErsatzTV/Controllers/Api/CollectionController.cs index 67e9b77d1..81031f2e7 100644 --- a/ErsatzTV/Controllers/Api/CollectionController.cs +++ b/ErsatzTV/Controllers/Api/CollectionController.cs @@ -49,6 +49,15 @@ public class CollectionController(IMediator mediator) : ControllerBase int clampedPageNum = Math.Max(0, pageNum); int clampedPageSize = Math.Clamp(pageSize, 1, 100); + // The items GET is the reorder editor's load endpoint: emit the collection's version as the + // concurrency ETag (issue #253). The response carries children, so read the root separately. + Option maybeCollection = + await mediator.Send(new GetCollectionById(id), cancellationToken); + foreach (MediaCollectionViewModel collection in maybeCollection) + { + ConcurrencyHeaders.SetETag(Response, collection.Version); + } + Either result = await mediator.Send( new GetCollectionItems(id, clampedPageNum, clampedPageSize), cancellationToken); @@ -105,13 +114,21 @@ public class CollectionController(IMediator mediator) : ControllerBase "UseCustomPlaybackOrder on the collection for this order to take effect.")] [EndpointGroupName("general")] [ProducesResponseType(StatusCodes.Status204NoContent)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status412PreconditionFailed)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task UpdateCustomOrder( int id, [Required] [FromBody] UpdateCollectionCustomOrderRequest request, CancellationToken cancellationToken) { + IfMatchCondition ifMatch = ConcurrencyHeaders.ParseIfMatch(Request); + if (ifMatch.Kind is IfMatchKind.Malformed) + { + return ConcurrencyHeaders.MalformedIfMatchProblem(); + } + Option maybeCollection = await mediator.Send(new GetCollectionById(id), cancellationToken); if (maybeCollection.IsNone) @@ -119,8 +136,18 @@ public class CollectionController(IMediator mediator) : ControllerBase return ApiResults.NotFoundProblem(); } - Either result = await mediator.Send(request.ToCommand(id), cancellationToken); - return result.ToDeletedResult(); + Either result = + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); + return await result.Match( + Left: error => Task.FromResult(error.ToErrorResult()), + Right: async _ => + { + // Emit the new ETag on the bodyless 204 so a same-tab second save doesn't 412 (#253). + Option refreshed = + await mediator.Send(new GetCollectionById(id), cancellationToken); + ConcurrencyHeaders.SetETag(Response, refreshed.Map(c => c.Version).IfNone(0)); + return (IActionResult)new NoContentResult(); + }); } [HttpDelete("/api/collections/{id:int}")] diff --git a/ErsatzTV/Controllers/Api/MultiCollectionController.cs b/ErsatzTV/Controllers/Api/MultiCollectionController.cs index 6fa0b2e6e..d666d7586 100644 --- a/ErsatzTV/Controllers/Api/MultiCollectionController.cs +++ b/ErsatzTV/Controllers/Api/MultiCollectionController.cs @@ -48,6 +48,13 @@ public class MultiCollectionController(IMediator mediator) : ControllerBase { Option result = await mediator.Send(new GetMultiCollectionById(id), cancellationToken); + + // This by-id GET is the editor's load endpoint: emit the concurrency ETag (issue #253). + foreach (MultiCollectionViewModel vm in result) + { + ConcurrencyHeaders.SetETag(Response, vm.Version); + } + return result.Map(ProjectToResponseModel).ToGetResult(); } @@ -74,14 +81,23 @@ public class MultiCollectionController(IMediator mediator) : ControllerBase [EndpointSummary("Update a multi collection")] [EndpointGroupName("general")] [ProducesResponseType(typeof(MultiCollectionResponseModel), 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] UpdateMultiCollectionRequest request, CancellationToken cancellationToken) { - Either result = await mediator.Send(request.ToCommand(id), cancellationToken); + IfMatchCondition ifMatch = ConcurrencyHeaders.ParseIfMatch(Request); + if (ifMatch.Kind is IfMatchKind.Malformed) + { + return ConcurrencyHeaders.MalformedIfMatchProblem(); + } + + Either result = + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), Right: async _ => @@ -89,7 +105,12 @@ public class MultiCollectionController(IMediator mediator) : ControllerBase Option multiCollection = await mediator.Send(new GetMultiCollectionById(id), cancellationToken); return multiCollection.Match( - Some: vm => (IActionResult)new OkObjectResult(ProjectToResponseModel(vm)), + Some: vm => + { + // Emit the new ETag so a same-tab second save doesn't 412 against its own write (#253). + ConcurrencyHeaders.SetETag(Response, vm.Version); + return (IActionResult)new OkObjectResult(ProjectToResponseModel(vm)); + }, None: () => ApiResults.NotFoundProblem()); }); } diff --git a/ErsatzTV/Controllers/Api/PlayoutController.cs b/ErsatzTV/Controllers/Api/PlayoutController.cs index a0bff515f..0e144e342 100644 --- a/ErsatzTV/Controllers/Api/PlayoutController.cs +++ b/ErsatzTV/Controllers/Api/PlayoutController.cs @@ -292,6 +292,9 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : } } + // The GET returns children, so read the playout's version for the concurrency ETag (issue #253). + ConcurrencyHeaders.SetETag(Response, maybePlayout.Map(p => p.Version).IfNone(0)); + List items = await mediator.Send(new GetPlayoutAlternateSchedules(id), cancellationToken); return new OkObjectResult(items.OrderBy(i => i.Index).Select(ToResponse).ToList()); @@ -307,14 +310,22 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : "at least one item, and every ProgramScheduleId must exist. Only valid for Classic playouts.")] [EndpointGroupName("general")] [ProducesResponseType(typeof(List), StatusCodes.Status200OK)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status412PreconditionFailed)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task ReplaceAlternateSchedules( int id, [Required] [FromBody] ReplacePlayoutAlternateSchedulesRequest request, CancellationToken cancellationToken) { + IfMatchCondition ifMatch = ConcurrencyHeaders.ParseIfMatch(Request); + if (ifMatch.Kind is IfMatchKind.Malformed) + { + return ConcurrencyHeaders.MalformedIfMatchProblem(); + } + if (entityLocker.IsPlayoutLocked(id)) { return PlayoutLockedProblem(); @@ -355,11 +366,17 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : return BaseError.New($"[ProgramScheduleId] {missingScheduleIds[0]} does not exist").ToErrorResult(); } - Either result = await mediator.Send(request.ToCommand(id), cancellationToken); + Either result = + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), Right: async _ => { + // Emit the new ETag (post-bump) so a same-tab second save doesn't 412 against its own write (#253). + Option refreshedPlayout = + await mediator.Send(new GetPlayoutById(id), cancellationToken); + ConcurrencyHeaders.SetETag(Response, refreshedPlayout.Map(p => p.Version).IfNone(0)); + List refreshed = await mediator.Send(new GetPlayoutAlternateSchedules(id), cancellationToken); return (IActionResult)new OkObjectResult(refreshed.OrderBy(i => i.Index).Select(ToResponse).ToList()); @@ -392,6 +409,9 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : } } + // The GET returns children, so read the playout's version for the concurrency ETag (issue #253). + ConcurrencyHeaders.SetETag(Response, maybePlayout.Map(p => p.Version).IfNone(0)); + List items = await mediator.Send(new GetPlayoutTemplates(id), cancellationToken); return new OkObjectResult(items.OrderBy(i => i.Index).Select(ToResponse).ToList()); } @@ -405,14 +425,22 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : "supplied DecoTemplateId must exist. An empty list clears all templates. Only valid for Block playouts.")] [EndpointGroupName("general")] [ProducesResponseType(typeof(List), StatusCodes.Status200OK)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status400BadRequest)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status409Conflict)] + [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status412PreconditionFailed)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status422UnprocessableEntity)] public async Task ReplaceTemplates( int id, [Required] [FromBody] ReplacePlayoutTemplatesRequest request, CancellationToken cancellationToken) { + IfMatchCondition ifMatch = ConcurrencyHeaders.ParseIfMatch(Request); + if (ifMatch.Kind is IfMatchKind.Malformed) + { + return ConcurrencyHeaders.MalformedIfMatchProblem(); + } + if (entityLocker.IsPlayoutLocked(id)) { return PlayoutLockedProblem(); @@ -460,12 +488,17 @@ public class PlayoutController(IMediator mediator, IEntityLocker entityLocker) : } } - Option result = await mediator.Send(request.ToCommand(id), cancellationToken); + Option result = + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); foreach (BaseError error in result) { return error.ToErrorResult(); } + // Emit the new ETag (post-bump) so a same-tab second save doesn't 412 against its own write (#253). + Option refreshedPlayout = await mediator.Send(new GetPlayoutById(id), cancellationToken); + ConcurrencyHeaders.SetETag(Response, refreshedPlayout.Map(p => p.Version).IfNone(0)); + List refreshed = await mediator.Send(new GetPlayoutTemplates(id), cancellationToken); return new OkObjectResult(refreshed.OrderBy(i => i.Index).Select(ToResponse).ToList()); } diff --git a/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutAlternateSchedulesRequest.cs b/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutAlternateSchedulesRequest.cs index b94b3ef89..fac2c6c53 100644 --- a/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutAlternateSchedulesRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutAlternateSchedulesRequest.cs @@ -8,10 +8,11 @@ public record ReplacePlayoutAlternateSchedulesRequest(List + public ReplacePlayoutAlternateScheduleItems ToCommand(int playoutId, Option expectedVersion = default) => new( playoutId, (Items ?? []) .Select((item, index) => item.ToReplaceItem(index)) - .ToList()); + .ToList(), + expectedVersion); } diff --git a/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutTemplatesRequest.cs b/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutTemplatesRequest.cs index 73bce1d8c..ee0321a74 100644 --- a/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutTemplatesRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/ReplacePlayoutTemplatesRequest.cs @@ -5,10 +5,11 @@ namespace ErsatzTV.Controllers.Api.Requests; public record ReplacePlayoutTemplatesRequest(List Items) { // Index is assigned from array order (top-to-bottom priority), mirroring the Blazor editor. - public ReplacePlayoutTemplateItems ToCommand(int playoutId) => + public ReplacePlayoutTemplateItems ToCommand(int playoutId, Option expectedVersion = default) => new( playoutId, (Items ?? []) .Select((item, index) => item.ToReplaceItem(index)) - .ToList()); + .ToList(), + expectedVersion); } diff --git a/ErsatzTV/Controllers/Api/Requests/UpdateCollectionCustomOrderRequest.cs b/ErsatzTV/Controllers/Api/Requests/UpdateCollectionCustomOrderRequest.cs index faaa63714..a8b51e6a8 100644 --- a/ErsatzTV/Controllers/Api/Requests/UpdateCollectionCustomOrderRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/UpdateCollectionCustomOrderRequest.cs @@ -4,10 +4,11 @@ namespace ErsatzTV.Controllers.Api.Requests; public record UpdateCollectionCustomOrderRequest(List MediaItemIds) { - public UpdateCollectionCustomOrder ToCommand(int collectionId) => + public UpdateCollectionCustomOrder ToCommand(int collectionId, Option expectedVersion = default) => new( collectionId, (MediaItemIds ?? []) .Select((mediaItemId, index) => new MediaItemCustomOrder(mediaItemId, index)) - .ToList()); + .ToList(), + expectedVersion); } diff --git a/ErsatzTV/Controllers/Api/Requests/UpdateMultiCollectionRequest.cs b/ErsatzTV/Controllers/Api/Requests/UpdateMultiCollectionRequest.cs index 07bb99d3c..b5d017df7 100644 --- a/ErsatzTV/Controllers/Api/Requests/UpdateMultiCollectionRequest.cs +++ b/ErsatzTV/Controllers/Api/Requests/UpdateMultiCollectionRequest.cs @@ -6,7 +6,7 @@ namespace ErsatzTV.Controllers.Api.Requests; public record UpdateMultiCollectionRequest(string Name, List Items) { - public UpdateMultiCollection ToCommand(int id) => + public UpdateMultiCollection ToCommand(int id, Option expectedVersion = default) => new( id, Name, @@ -16,5 +16,6 @@ public record UpdateMultiCollectionRequest(string Name, List expectedVersion = default) { (MediaCollectionViewModel collection, MultiCollectionViewModel multiCollection, @@ -28,6 +28,7 @@ public record UpdateRerunCollectionRequest( smartCollection, mediaItem, FirstRunPlaybackOrder, - RerunPlaybackOrder); + RerunPlaybackOrder, + expectedVersion); } } diff --git a/ErsatzTV/Controllers/Api/RerunCollectionController.cs b/ErsatzTV/Controllers/Api/RerunCollectionController.cs index 6faa54031..380f857d1 100644 --- a/ErsatzTV/Controllers/Api/RerunCollectionController.cs +++ b/ErsatzTV/Controllers/Api/RerunCollectionController.cs @@ -48,6 +48,13 @@ public class RerunCollectionController(IMediator mediator) : ControllerBase { Option result = await mediator.Send(new GetRerunCollectionById(id), cancellationToken); + + // This by-id GET is the editor's load endpoint: emit the concurrency ETag (issue #253). + foreach (RerunCollectionViewModel vm in result) + { + ConcurrencyHeaders.SetETag(Response, vm.Version); + } + return result.Map(ProjectToResponseModel).ToGetResult(); } @@ -81,13 +88,21 @@ public class RerunCollectionController(IMediator mediator) : ControllerBase [EndpointSummary("Update a rerun collection")] [EndpointGroupName("general")] [ProducesResponseType(typeof(RerunCollectionResponseModel), 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] UpdateRerunCollectionRequest request, CancellationToken cancellationToken) { + IfMatchCondition ifMatch = ConcurrencyHeaders.ParseIfMatch(Request); + if (ifMatch.Kind is IfMatchKind.Malformed) + { + return ConcurrencyHeaders.MalformedIfMatchProblem(); + } + if (!RerunCollectionRequestMapping.IsSupportedSelectionType(request.CollectionType)) { return BaseError.New( @@ -95,7 +110,8 @@ public class RerunCollectionController(IMediator mediator) : ControllerBase .ToErrorResult(); } - Either result = await mediator.Send(request.ToCommand(id), cancellationToken); + Either result = + await mediator.Send(request.ToCommand(id, ifMatch.ExpectedVersion), cancellationToken); return await result.Match( Left: error => Task.FromResult(error.ToErrorResult()), Right: async _ => @@ -103,7 +119,12 @@ public class RerunCollectionController(IMediator mediator) : ControllerBase Option rerunCollection = await mediator.Send(new GetRerunCollectionById(id), cancellationToken); return rerunCollection.Match( - Some: vm => (IActionResult)new OkObjectResult(ProjectToResponseModel(vm)), + Some: vm => + { + // Emit the new ETag so a same-tab second save doesn't 412 against its own write (#253). + ConcurrencyHeaders.SetETag(Response, vm.Version); + return (IActionResult)new OkObjectResult(ProjectToResponseModel(vm)); + }, None: () => ApiResults.NotFoundProblem()); }); } diff --git a/ErsatzTV/Extensions/ConcurrencyHeaders.cs b/ErsatzTV/Extensions/ConcurrencyHeaders.cs index c3dafae8d..3735cb120 100644 --- a/ErsatzTV/Extensions/ConcurrencyHeaders.cs +++ b/ErsatzTV/Extensions/ConcurrencyHeaders.cs @@ -1,6 +1,7 @@ using System.Globalization; using LanguageExt; using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; using Microsoft.Extensions.Primitives; namespace ErsatzTV.Extensions; @@ -74,4 +75,18 @@ public static class ConcurrencyHeaders public static void SetETag(HttpResponse response, int version) => response.Headers.ETag = $"\"{version}\""; + + /// + /// The 400 response for a syntactically-invalid If-Match header + /// (). Shared by the replace-all PUT endpoints so the + /// fail-safe rejection wording stays consistent (issue #253 §7a). + /// + public static IActionResult MalformedIfMatchProblem() => + 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 \"*\"." + }); } diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 94cffae88..5e75fce0d 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -2981,6 +2981,26 @@ "204": { "description": "No Content" }, + "400": { + "description": "Bad Request", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "404": { "description": "Not Found", "content": { @@ -3001,6 +3021,26 @@ } } }, + "412": { + "description": "Precondition Failed", + "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": { @@ -8124,6 +8164,26 @@ } } }, + "400": { + "description": "Bad Request", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "404": { "description": "Not Found", "content": { @@ -8144,6 +8204,26 @@ } } }, + "412": { + "description": "Precondition Failed", + "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": { @@ -9956,6 +10036,26 @@ } } }, + "400": { + "description": "Bad Request", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "404": { "description": "Not Found", "content": { @@ -9996,6 +10096,26 @@ } } }, + "412": { + "description": "Precondition Failed", + "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": { @@ -10182,6 +10302,26 @@ } } }, + "400": { + "description": "Bad Request", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "404": { "description": "Not Found", "content": { @@ -10222,6 +10362,26 @@ } } }, + "412": { + "description": "Precondition Failed", + "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": { @@ -11591,6 +11751,26 @@ } } }, + "400": { + "description": "Bad Request", + "content": { + "text/plain": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + }, + "text/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, "404": { "description": "Not Found", "content": { @@ -11611,6 +11791,26 @@ } } }, + "412": { + "description": "Precondition Failed", + "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": { @@ -21055,6 +21255,11 @@ "useCustomPlaybackOrder": { "type": "boolean" }, + "version": { + "type": "integer", + "format": "int32", + "default": 0 + }, "mediaItemId": { "type": "integer", "format": "int32" diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 715e14cfa..81d93284e 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -343,6 +343,24 @@ save → 412 (prove it non-vacuous by dropping `.IsConcurrencyToken()` and watch Phase 2 (a later PR) flips a missing `If-Match` from force-write to **428 Precondition Required** once every editor echoes and one release soaks. +**Fan-out status.** Block (#2) is the reference. **PR2** wired the RR + Reconcile aggregates +(Template #3, DecoTemplate #4, Playlist #5, schedule-items #1). **PR3** wired the Diff + Scalar aggregates: +Collection custom-order #6, Playout alternate-schedules #7 and templates #8 (both share `Playout.Version`; +their `catch(Exception)`→422 handlers were restructured so the guard's `PreconditionFailedError` Left +returns before the catch — §9/H1), MultiCollection #9, and RerunCollection #10. Two M2 gate notes for the +`SaveChangesAsync() > 0` handlers: the RerunCollection/Collection-custom-order refresh now runs on any +successful save (the bump makes the gate always-true); MultiCollection keeps its "name-only change → no +playout rebuild" optimization by bumping on the **first** (name) save so the **second** (items) save's +`> 0` still means "items changed". The one bulk writer in scope, `UpdateDefaultDecoHandler`, bumps via +`.SetProperty(x => x.Version, x => x.Version + 1)` (bulk `ExecuteUpdate` can't throw the concurrency +exception, so it needs no guard). **Deferred (tracked follow-up):** the *other* same-root config +writers — non-bulk siblings like `UpdateCollectionHandler`/`RemoveItemsFromCollectionHandler`, +`UpdatePlayoutHandler` and the `ScheduleFile` handlers, and the repository-mediated `Add*ToCollection` +family — do **not** yet rotate their aggregate's ETag. The primary endpoints' own bump+guard fully cover +the two-tab lost-update this contract targets; the deferred writers only affect cross-editor ETag rotation, +and adding an unconditional bump to a handler that uses plain `SaveChangesAsync` (not the guard) would open +a new `DbUpdateConcurrencyException`→500 path — so they need a uniform guard+bump pass of their own. + ## 7b. Post-commit side effects run on `CancellationToken.None` Once a command handler's `await dbContext.SaveChangesAsync(cancellationToken)` (or repository upsert) diff --git a/docs/decisions.md b/docs/decisions.md index e980eb8c7..2c5c25191 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -723,3 +723,38 @@ the controller pre-guard) so a direct caller can't trip the `Max()`-on-empty cra the versioned aggregates — several of which this sweep also touched (post-commit token, a different line region). Low git-conflict risk, but merge `main` in and expect to see the `CancellationToken.None` convention already present on the post-commit enqueues. + +## 2026-07-11 — #253 PR3: Diff + Scalar concurrency fan-out (Collection / Playout×2 / MultiCollection / RerunCollection) + +**Context.** PR3 of the #253 optimistic-concurrency arc fans the frozen Block recipe (api-conventions §7a) +across the five Diff/Scalar aggregates. Three judgment calls beyond the mechanical copy: + +**H1 — Playout `catch(Exception)`→422.** The two Playout replace handlers wrap `SaveChangesAsync` in a +`catch(Exception)` that maps any exception to a bare `BaseError` (→ 422). Rather than let the guard's +concurrency failure be reshaped into a 422, the guarded save (`SaveChangesWithConcurrencyGuard`) returns a +`PreconditionFailedError` **Left as a value** and the handler returns it before the post-commit block — +so it never reaches the catch. Proven by the pre-check-subtype tests (a `.Apply` flatten would fail +`ShouldBeOfType`) plus a non-vacuous Playout racing-save test. + +**M2 — the `SaveChangesAsync() > 0` gates.** RerunCollection and Collection-custom-order run their +playout-refresh **unconditionally** on a successful save (the unconditional `Version++` makes the old gate +always-true; the "nothing changed" branch is dead). MultiCollection is the exception: it saved the name +first specifically so a name-only change wouldn't rebuild playouts, so we bump `Version` on that **first** +save and leave the **second** (items) save's `> 0` gate intact — a name-only edit still bumps + rotates the +ETag but does not rebuild. Enumerating every behavior the gate provided before reworking it (the #232 lesson). + +**Sibling-writer scope (deferred).** §7a's config-only boundary says every writer of an aggregate's +editor-visible config bumps `Version`. PR3 ships the five primary endpoints' full contract + the one +design-named bulk writer (`UpdateDefaultDecoHandler`, safe via `.SetProperty`). It **defers** the other +same-root non-bulk config writers (`UpdateCollectionHandler`, `RemoveItemsFromCollectionHandler`, +`UpdatePlayoutHandler`, the `ScheduleFile` handlers) and the repository-mediated `Add*ToCollection` family. +Rationale: the primary endpoints' own bump+guard fully cover the two-tab lost-update the issue targets; +the deferred writers only affect cross-editor ETag *rotation*, and adding an unconditional bump to a handler +that uses plain `SaveChangesAsync` (not the guard) converts a latent lost-update into a **new 500** +(`DbUpdateConcurrencyException`) — doing it safely needs a uniform guard+bump+412 pass of its own, better +done with the #197 contract work. Tracked as a follow-up issue. + +**VMs.** `Playout.Version` surfaces via `PlayoutNameViewModel` (required arg); the three collection VMs +(`MediaCollectionViewModel`, `MultiCollectionViewModel`, `RerunCollectionViewModel`) carry `int Version = 0` +(defaulted — 0 for the selection-placeholder constructions, real value from the Mapper projection). +Header-only via ETag, never echoed in a response body (the Block precedent). diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index e272858da..50de7523e 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -153,6 +153,12 @@ fresher edit: "changed elsewhere — reload (unsaved changes discarded)" `ConfirmDialog` (Reload bumps `reloadKey`), distinct from a 409 ("build in progress — retry shortly"). All other errors stay the generic save-error path. Reference: `web/src/screens/BlocksScreen.tsx` `BlockEditor`. +- **List-derived editors** (Multi/Rerun collections open the editor from a paged-list row, not a per-record + GET): fetch the single record via the `…WithMeta` by-id helper (`getMultiCollectionWithMeta` / + `getRerunCollectionWithMeta`) when the editor mounts, and build the draft from **that** response — so the + ETag and the draft data come from one read and stay consistent (the fail-safe ordering; don't pair a + list-row draft with a separately-fetched ETag). Editors that navigate back to the list on save (Playout, + Multi/Rerun, Collection reorder) need only the 412 branch — no post-save ETag rotation. ## 5. Artwork rendering diff --git a/web/src/api/collections.ts b/web/src/api/collections.ts index f22d4b7ca..ca3b0ee71 100644 --- a/web/src/api/collections.ts +++ b/web/src/api/collections.ts @@ -1,4 +1,4 @@ -import { ApiError, request } from './client'; +import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client'; import type { components } from './generated/v1'; import type { LibraryBrowseItem, PagedLibraryBrowseItems } from './libraryBrowse'; @@ -57,12 +57,36 @@ export function getCollectionItems( return request(`/api/collections/${id}/items?${params.toString()}`); } +/** Load a page of collection items together with the collection's concurrency ETag (issue #253). */ +export function getCollectionItemsWithMeta( + id: number, + pageNum = 0, + pageSize = 100 +): Promise> { + const params = new URLSearchParams({ + pageNum: String(pageNum), + pageSize: String(pageSize) + }); + return requestWithMeta(`/api/collections/${id}/items?${params.toString()}`); +} + // Replaces a manual collection's custom order wholesale: the CustomIndex of each media item is // derived from its position in `mediaItemIds`, so a partial array silently drops the items left // out of it (#211). Callers must submit the full ordered id list. -export function updateCollectionCustomOrder(id: number, mediaItemIds: number[]): Promise { +// +// 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 updateCollectionCustomOrder( + id: number, + mediaItemIds: number[], + ifMatch?: string | null +): Promise> { const body: UpdateCollectionCustomOrderRequest = { mediaItemIds }; - return request(`/api/collections/${id}/custom-order`, { body, method: 'PUT' }); + return requestWithMeta(`/api/collections/${id}/custom-order`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } /* ---------- smart collections ---------- */ diff --git a/web/src/api/generated/v1.d.ts b/web/src/api/generated/v1.d.ts index b03c5e51d..ed3c8192a 100644 --- a/web/src/api/generated/v1.d.ts +++ b/web/src/api/generated/v1.d.ts @@ -801,6 +801,7 @@ export interface components { "id": number; "name": null | string; "useCustomPlaybackOrder": boolean; + "version"?: number; "mediaItemId"?: number; "title"?: null | string; "subtitle"?: null | string; diff --git a/web/src/api/multiCollections.ts b/web/src/api/multiCollections.ts index 848ec03f9..9f6660304 100644 --- a/web/src/api/multiCollections.ts +++ b/web/src/api/multiCollections.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 MultiCollection = components['schemas']['MultiCollectionResponseModel']; @@ -38,12 +38,29 @@ export function getMultiCollection(id: number): Promise { return request(`/api/multi-collections/${id}`); } +/** Load a single multi-collection together with its concurrency ETag (issue #253). */ +export function getMultiCollectionWithMeta(id: number): Promise> { + return requestWithMeta(`/api/multi-collections/${id}`); +} + export function createMultiCollection(body: CreateMultiCollectionRequest): Promise { return request('/api/multi-collections', { body, method: 'POST' }); } -export function updateMultiCollection(id: number, body: UpdateMultiCollectionRequest): Promise { - return request(`/api/multi-collections/${id}`, { body, method: 'PUT' }); +/** + * Update a multi-collection. 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 updateMultiCollection( + id: number, + body: UpdateMultiCollectionRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/multi-collections/${id}`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } export function deleteMultiCollection(id: number): Promise { diff --git a/web/src/api/playouts.ts b/web/src/api/playouts.ts index 839281c3d..7aa49967f 100644 --- a/web/src/api/playouts.ts +++ b/web/src/api/playouts.ts @@ -1,5 +1,5 @@ import { useCallback, useEffect, useRef, useState } from 'react'; -import { ApiError, request } from './client'; +import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client'; import type { components } from './generated/v1'; export type PlayoutSummary = components['schemas']['PlayoutListItemResponseModel']; @@ -159,13 +159,26 @@ export function getAlternateSchedules(playoutId: number): Promise(`/api/playouts/${playoutId}/alternate-schedules`); } +/** Load alternate schedules together with the playout's concurrency ETag (issue #253). */ +export function getAlternateSchedulesWithMeta( + playoutId: number +): Promise> { + return requestWithMeta(`/api/playouts/${playoutId}/alternate-schedules`); +} + +/** + * Replace a playout's alternate schedules. 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 replaceAlternateSchedules( playoutId: number, - body: ReplacePlayoutAlternateSchedulesRequest -): Promise { - return request(`/api/playouts/${playoutId}/alternate-schedules`, { + body: ReplacePlayoutAlternateSchedulesRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/playouts/${playoutId}/alternate-schedules`, { body, - method: 'PUT' + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined }); } @@ -173,13 +186,24 @@ export function getPlayoutTemplates(playoutId: number): Promise(`/api/playouts/${playoutId}/templates`); } +/** Load playout templates together with the playout's concurrency ETag (issue #253). */ +export function getPlayoutTemplatesWithMeta(playoutId: number): Promise> { + return requestWithMeta(`/api/playouts/${playoutId}/templates`); +} + +/** + * Replace a playout's templates. 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 replacePlayoutTemplates( playoutId: number, - body: ReplacePlayoutTemplatesRequest -): Promise { - return request(`/api/playouts/${playoutId}/templates`, { + body: ReplacePlayoutTemplatesRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/playouts/${playoutId}/templates`, { body, - method: 'PUT' + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined }); } diff --git a/web/src/api/rerunCollections.ts b/web/src/api/rerunCollections.ts index c49da59e4..45d025497 100644 --- a/web/src/api/rerunCollections.ts +++ b/web/src/api/rerunCollections.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 RerunCollection = components['schemas']['RerunCollectionResponseModel']; @@ -36,12 +36,29 @@ export function getRerunCollection(id: number): Promise { return request(`/api/rerun-collections/${id}`); } +/** Load a single rerun collection together with its concurrency ETag (issue #253). */ +export function getRerunCollectionWithMeta(id: number): Promise> { + return requestWithMeta(`/api/rerun-collections/${id}`); +} + export function createRerunCollection(body: CreateRerunCollectionRequest): Promise { return request('/api/rerun-collections', { body, method: 'POST' }); } -export function updateRerunCollection(id: number, body: UpdateRerunCollectionRequest): Promise { - return request(`/api/rerun-collections/${id}`, { body, method: 'PUT' }); +/** + * Update a rerun collection. 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 updateRerunCollection( + id: number, + body: UpdateRerunCollectionRequest, + ifMatch?: string | null +): Promise> { + return requestWithMeta(`/api/rerun-collections/${id}`, { + body, + method: 'PUT', + headers: ifMatch ? { 'If-Match': ifMatch } : undefined + }); } export function deleteRerunCollection(id: number): Promise { diff --git a/web/src/screens/CollectionsScreen.tsx b/web/src/screens/CollectionsScreen.tsx index 4e4b68e87..b4400d36d 100644 --- a/web/src/screens/CollectionsScreen.tsx +++ b/web/src/screens/CollectionsScreen.tsx @@ -27,11 +27,13 @@ import { } from '../components'; import { addItemsToCollection, + ApiError, createCollection, createSmartCollection, deleteCollection, deleteSmartCollection, getCollectionItems, + getCollectionItemsWithMeta, getCollections, getLibraryBrowseItems, getSmartCollections, @@ -513,7 +515,11 @@ function ManualItemsView({ const [reorderItems, setReorderItems] = useState(null); const [reorderLoading, setReorderLoading] = useState(false); const [reorderSaving, setReorderSaving] = useState(false); + const [conflictOpen, setConflictOpen] = useState(false); const activeRef = useRef(true); + // Concurrency ETag (issue #253): captured from the first page of the reorder-mode items GET, + // sent as If-Match on the custom-order save. + const etagRef = useRef(null); // No synchronous setState here: `loading` starts true and flips false in `finally`, so // this is safe to call from an effect. Reloads (after add/remove) reset to the first page. @@ -586,9 +592,9 @@ function ManualItemsView({ setError(null); try { - const first = await getCollectionItems(collection.id, 0, PAGE_SIZE); - let all = first.page ?? []; - const totalCount = first.totalCount ?? all.length; + const first = await getCollectionItemsWithMeta(collection.id, 0, PAGE_SIZE); + let all = first.data.page ?? []; + const totalCount = first.data.totalCount ?? all.length; let pageNum = 1; while (all.length < totalCount) { @@ -604,6 +610,7 @@ function ManualItemsView({ } if (activeRef.current) { + etagRef.current = first.etag; setReorderItems(all); setReordering(true); } @@ -651,7 +658,7 @@ function ManualItemsView({ try { const mediaItemIds = reorderItems.map((item) => item.mediaItemId ?? item.id); - await updateCollectionCustomOrder(collection.id, mediaItemIds); + await updateCollectionCustomOrder(collection.id, mediaItemIds, etagRef.current); if (activeRef.current) { setReordering(false); @@ -660,7 +667,13 @@ function ManualItemsView({ } } catch (saveError) { if (activeRef.current) { - setError(messageFromCollectionError(saveError, 'Unable to save order')); + if (saveError instanceof ApiError && saveError.status === 412) { + // Another edit landed since we loaded the reorder view — force a reload rather than + // overwriting it (#253). + setConflictOpen(true); + } else { + setError(messageFromCollectionError(saveError, 'Unable to save order')); + } } } finally { if (activeRef.current) { @@ -669,6 +682,14 @@ function ManualItemsView({ } }; + const reloadAfterConflict = () => { + setConflictOpen(false); + setError(null); + setReordering(false); + setReorderItems(null); + void enterReorder(); + }; + return (
@@ -808,6 +829,17 @@ function ManualItemsView({ onClose={() => setPickerOpen(false)} open={pickerOpen} /> + + setConflictOpen(false)} + onConfirm={reloadAfterConflict} + open={conflictOpen} + title="Order changed elsewhere" + tone="danger" + />
); } diff --git a/web/src/screens/MultiCollectionsScreen.test.tsx b/web/src/screens/MultiCollectionsScreen.test.tsx index 28083bf7f..3b104e2aa 100644 --- a/web/src/screens/MultiCollectionsScreen.test.tsx +++ b/web/src/screens/MultiCollectionsScreen.test.tsx @@ -47,6 +47,14 @@ function mockApi(options: MockOptions = {}) { } } + // Single-record GET (the editor loads it on open for the #253 concurrency ETag) — must be + // matched before the list handler, which would otherwise swallow it with the paged shape. + const multiById = url.match(/^\/api\/multi-collections\/(\d+)$/); + if (multiById && method === 'GET') { + const found = (multi as Array<{ id: number }>).find((m) => String(m.id) === multiById[1]) ?? multi[0]; + return Promise.resolve(jsonResponse(found)); + } + if (url.startsWith('/api/multi-collections') && method === 'GET') { return Promise.resolve(jsonResponse({ page: multi, totalCount: multi.length })); } diff --git a/web/src/screens/MultiCollectionsScreen.tsx b/web/src/screens/MultiCollectionsScreen.tsx index 89170f6f8..d7a92c31a 100644 --- a/web/src/screens/MultiCollectionsScreen.tsx +++ b/web/src/screens/MultiCollectionsScreen.tsx @@ -2,9 +2,11 @@ import { useCallback, useEffect, useRef, useState } from 'react'; import { ArrowLeft, Check, Layers, Plus, Trash2, TriangleAlert } from 'lucide-react'; import { Badge, Button, Card, ConfirmDialog, IconButton, Input, Select, Spinner, Switch } from '../components'; import { + ApiError, createMultiCollection, deleteMultiCollection, getCollections, + getMultiCollectionWithMeta, getMultiCollections, getSmartCollections, messageFromMultiCollectionError, @@ -111,15 +113,30 @@ function MultiCollectionEditor({ const [loadError, setLoadError] = useState(null); const [saveError, setSaveError] = useState(null); const [saving, setSaving] = useState(false); + const [conflictOpen, setConflictOpen] = useState(false); + const [reloadKey, setReloadKey] = useState(0); const activeRef = useRef(true); + // Concurrency ETag (issue #253): captured from the single-record GET when editing an existing + // multi-collection, sent as If-Match on save. There's no rotation-on-save because the editor + // always returns to the list after a successful save (see onSaved below). + const etagRef = useRef(null); useEffect(() => { activeRef.current = true; - Promise.all([getCollections(), getSmartCollections()]) - .then(([manual, smart]) => { + Promise.all([ + getCollections(), + getSmartCollections(), + initial ? getMultiCollectionWithMeta(initial.id) : Promise.resolve(null) + ]) + .then(([manual, smart, mcMeta]) => { if (activeRef.current) { setCollections(manual); setSmartCollections(smart); + if (mcMeta) { + etagRef.current = mcMeta.etag; + setName(mcMeta.data.name ?? ''); + setItems(itemsFromMultiCollection(mcMeta.data)); + } setLoadError(null); } }) @@ -132,7 +149,7 @@ function MultiCollectionEditor({ return () => { activeRef.current = false; }; - }, []); + }, [initial, reloadKey]); const hasManual = (id: number) => items.some((item) => item.kind === 'manual' && item.id === id); const hasSmart = (id: number) => items.some((item) => item.kind === 'smart' && item.id === id); @@ -182,19 +199,30 @@ function MultiCollectionEditor({ try { const body = { items: items.map(toItemRequest), name: trimmedName }; if (initial) { - await updateMultiCollection(initial.id, body); + await updateMultiCollection(initial.id, body, etagRef.current); } else { await createMultiCollection(body); } onSaved(); } catch (error) { - setSaveError(messageFromMultiCollectionError(error, 'Unable to save multi-collection')); + 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(messageFromMultiCollectionError(error, 'Unable to save multi-collection')); + } } finally { setSaving(false); } }; + const reloadAfterConflict = () => { + setConflictOpen(false); + setSaveError(null); + setReloadKey((key) => key + 1); + }; + const collectionOptions = [ { label: 'Select a collection…', value: '' }, ...collections @@ -316,6 +344,17 @@ function MultiCollectionEditor({ )) )} + + setConflictOpen(false)} + onConfirm={reloadAfterConflict} + open={conflictOpen} + title="Multi-collection changed elsewhere" + tone="danger" + />
); } diff --git a/web/src/screens/PlayoutScheduleEditors.tsx b/web/src/screens/PlayoutScheduleEditors.tsx index 546e28f5b..d87287e85 100644 --- a/web/src/screens/PlayoutScheduleEditors.tsx +++ b/web/src/screens/PlayoutScheduleEditors.tsx @@ -1,14 +1,15 @@ import { useEffect, useRef, useState } from 'react'; import { ArrowDown, ArrowLeft, ArrowRight, ArrowUp, CalendarDays, Check, Plus, Trash2, TriangleAlert } from 'lucide-react'; import { navigateToPath } from '../routing'; -import { Badge, Button, Card, Checkbox, IconButton, Select, Spinner } from '../components'; +import { Badge, Button, Card, Checkbox, ConfirmDialog, IconButton, Select, Spinner } from '../components'; import { daysInMonth, firstMatchingIndex } from './playoutTemplateCalendar'; import { + ApiError, DAYS_OF_WEEK, - getAlternateSchedules, + getAlternateSchedulesWithMeta, getDecoTemplates, getPlayout, - getPlayoutTemplates, + getPlayoutTemplatesWithMeta, getSchedules, getTemplates, messageFromPlayoutClientError, @@ -428,18 +429,24 @@ export function PlayoutAlternateSchedulesScreen({ playoutId }: { playoutId: numb const [loadError, setLoadError] = useState(null); const [saveError, setSaveError] = useState(null); const [saving, setSaving] = useState(false); + const [conflictOpen, setConflictOpen] = useState(false); + const [reloadKey, setReloadKey] = useState(0); const activeRef = useRef(true); + // Concurrency ETag (issue #253): captured from the alternate-schedules GET, sent as If-Match on + // save. Both screens navigate away on a successful save, so there's no response ETag to rotate in. + const etagRef = useRef(null); useEffect(() => { activeRef.current = true; - Promise.all([getPlayout(playoutId), getAlternateSchedules(playoutId), getSchedules()]) - .then(([playout, alternates, allSchedules]) => { + Promise.all([getPlayout(playoutId), getAlternateSchedulesWithMeta(playoutId), getSchedules()]) + .then(([playout, alternatesMeta, allSchedules]) => { if (!activeRef.current) { return; } + etagRef.current = alternatesMeta.etag; setChannelName(playout.channelName); setSchedules(allSchedules); - const drafts = alternates.map((alternate) => altToDraft(alternate, allSchedules)); + const drafts = alternatesMeta.data.map((alternate) => altToDraft(alternate, allSchedules)); setItems(drafts); setSelectedKey(drafts.length === 1 ? drafts[0].key : null); }) @@ -451,7 +458,7 @@ export function PlayoutAlternateSchedulesScreen({ playoutId }: { playoutId: numb return () => { activeRef.current = false; }; - }, [playoutId]); + }, [playoutId, reloadKey]); if (loadError) { return ; @@ -516,24 +523,43 @@ export function PlayoutAlternateSchedulesScreen({ playoutId }: { playoutId: numb } setSaving(true); setSaveError(null); - replaceAlternateSchedules(playoutId, { - items: items.map((item) => ({ - id: item.id, - programScheduleId: item.programScheduleId, - ...toRequestRecurrence(item) - })) - }) + replaceAlternateSchedules( + playoutId, + { + items: items.map((item) => ({ + id: item.id, + programScheduleId: item.programScheduleId, + ...toRequestRecurrence(item) + })) + }, + etagRef.current + ) .then(() => { navigateToPath(PLAYOUTS_PATH); }) .catch((error: unknown) => { - if (activeRef.current) { + if (!activeRef.current) { + return; + } + if (error instanceof ApiError && error.status === 412) { + // Another edit landed since we loaded — force a reload rather than overwriting it (#253). + setConflictOpen(true); + setSaving(false); + } else { setSaveError(messageFromPlayoutClientError(error, 'Unable to save alternate schedules')); setSaving(false); } }); }; + const reloadAfterConflict = () => { + setConflictOpen(false); + setSaveError(null); + setItems(null); + setSelectedKey(null); + setReloadKey((key) => key + 1); + }; + const scheduleNameOf = (id: number) => schedules.find((schedule) => schedule.id === id)?.name ?? '(none)'; // note: '(none)' also covers a schedule whose name is null @@ -643,6 +669,17 @@ export function PlayoutAlternateSchedulesScreen({ playoutId }: { playoutId: numb /> )} + + setConflictOpen(false)} + onConfirm={reloadAfterConflict} + open={conflictOpen} + title="Playout changed elsewhere" + tone="danger" + /> ); } @@ -684,19 +721,25 @@ export function PlayoutTemplatesEditorScreen({ playoutId }: { playoutId: number const [saveError, setSaveError] = useState(null); const [saving, setSaving] = useState(false); const [showPreview, setShowPreview] = useState(false); + const [conflictOpen, setConflictOpen] = useState(false); + const [reloadKey, setReloadKey] = useState(0); const activeRef = useRef(true); + // Concurrency ETag (issue #253): captured from the templates GET, sent as If-Match on save. Both + // screens navigate away on a successful save, so there's no response ETag to rotate in. + const etagRef = useRef(null); useEffect(() => { activeRef.current = true; - Promise.all([getPlayout(playoutId), getPlayoutTemplates(playoutId), getTemplates(), getDecoTemplates()]) - .then(([playout, playoutTemplates, allTemplates, allDecoTemplates]) => { + Promise.all([getPlayout(playoutId), getPlayoutTemplatesWithMeta(playoutId), getTemplates(), getDecoTemplates()]) + .then(([playout, templatesMeta, allTemplates, allDecoTemplates]) => { if (!activeRef.current) { return; } + etagRef.current = templatesMeta.etag; setChannelName(playout.channelName); setTemplates(allTemplates); setDecoTemplates(allDecoTemplates); - const drafts = playoutTemplates.map(templateToDraft); + const drafts = templatesMeta.data.map(templateToDraft); setItems(drafts); setSelectedKey(drafts.length === 1 ? drafts[0].key : null); }) @@ -708,7 +751,7 @@ export function PlayoutTemplatesEditorScreen({ playoutId }: { playoutId: number return () => { activeRef.current = false; }; - }, [playoutId]); + }, [playoutId, reloadKey]); if (loadError) { return ; @@ -784,25 +827,44 @@ export function PlayoutTemplatesEditorScreen({ playoutId }: { playoutId: number } setSaving(true); setSaveError(null); - replacePlayoutTemplates(playoutId, { - items: items.map((item) => ({ - id: item.id, - templateId: item.templateId, - decoTemplateId: item.decoTemplateId, - ...toRequestRecurrence(item) - })) - }) + replacePlayoutTemplates( + playoutId, + { + items: items.map((item) => ({ + id: item.id, + templateId: item.templateId, + decoTemplateId: item.decoTemplateId, + ...toRequestRecurrence(item) + })) + }, + etagRef.current + ) .then(() => { navigateToPath(PLAYOUTS_PATH); }) .catch((error: unknown) => { - if (activeRef.current) { + if (!activeRef.current) { + return; + } + if (error instanceof ApiError && error.status === 412) { + // Another edit landed since we loaded — force a reload rather than overwriting it (#253). + setConflictOpen(true); + setSaving(false); + } else { setSaveError(messageFromPlayoutClientError(error, 'Unable to save playout templates')); setSaving(false); } }); }; + const reloadAfterConflict = () => { + setConflictOpen(false); + setSaveError(null); + setItems(null); + setSelectedKey(null); + setReloadKey((key) => key + 1); + }; + const templateNameOf = (id: number) => templates.find((template) => template.id === id)?.name ?? '(none)'; return ( @@ -916,6 +978,17 @@ export function PlayoutTemplatesEditorScreen({ playoutId }: { playoutId: number )} + + setConflictOpen(false)} + onConfirm={reloadAfterConflict} + open={conflictOpen} + title="Playout changed elsewhere" + tone="danger" + /> ); } diff --git a/web/src/screens/RerunCollectionsScreen.test.tsx b/web/src/screens/RerunCollectionsScreen.test.tsx index 18fa1eb1a..76d7cd50f 100644 --- a/web/src/screens/RerunCollectionsScreen.test.tsx +++ b/web/src/screens/RerunCollectionsScreen.test.tsx @@ -47,6 +47,14 @@ function mockApi(options: MockOptions = {}) { } } + // Single-record GET (the editor loads it on open for the #253 concurrency ETag) — must be + // matched before the list handler, which would otherwise swallow it with the paged shape. + const rerunById = url.match(/^\/api\/rerun-collections\/(\d+)$/); + if (rerunById && method === 'GET') { + const found = (list as Array<{ id: number }>).find((r) => String(r.id) === rerunById[1]) ?? list[0]; + return Promise.resolve(jsonResponse(found)); + } + if (url.startsWith('/api/rerun-collections') && method === 'GET') { return Promise.resolve(jsonResponse({ page: list, totalCount: list.length })); } diff --git a/web/src/screens/RerunCollectionsScreen.tsx b/web/src/screens/RerunCollectionsScreen.tsx index 0dd504567..79f26445d 100644 --- a/web/src/screens/RerunCollectionsScreen.tsx +++ b/web/src/screens/RerunCollectionsScreen.tsx @@ -3,11 +3,13 @@ import { ArrowLeft, Check, Plus, Repeat, Trash2, TriangleAlert } from 'lucide-re import { Badge, Button, Card, ConfirmDialog, IconButton, Input, Select, Spinner } from '../components'; import type { SelectOption } from '../components'; import { + ApiError, createRerunCollection, deleteRerunCollection, getCollections, getLibraryBrowseItems, getMultiCollections, + getRerunCollectionWithMeta, getRerunCollections, getSmartCollections, messageFromRerunCollectionError, @@ -208,9 +210,42 @@ function RerunCollectionEditor({ const [pickerError, setPickerError] = useState(null); const [saveError, setSaveError] = useState(null); const [saving, setSaving] = useState(false); + const [conflictOpen, setConflictOpen] = useState(false); + const [reloadKey, setReloadKey] = useState(0); + // Concurrency ETag (issue #253): captured from the single-record GET when editing an existing + // rerun collection, sent as If-Match on save. There's no rotation-on-save because the editor + // always returns to the list after a successful save (see onSaved below). + const etagRef = useRef(null); const { collectionType } = draft; + // Re-fetch the record's ETag (and current data, on a conflict reload) when editing an existing + // rerun collection. Runs once on mount and again after reloadAfterConflict bumps reloadKey. + useEffect(() => { + if (!initial) { + return; + } + + let active = true; + + getRerunCollectionWithMeta(initial.id) + .then((meta) => { + if (active) { + etagRef.current = meta.etag; + setDraft(draftFromRerun(meta.data)); + } + }) + .catch((error: unknown) => { + if (active) { + setSaveError(messageFromRerunCollectionError(error, 'Unable to load rerun collection')); + } + }); + + return () => { + active = false; + }; + }, [initial, reloadKey]); + // Load the picker list for the active type. Resets only in the async callbacks (never // synchronously in the effect body) per spa-conventions §3. useEffect(() => { @@ -275,19 +310,30 @@ function RerunCollectionEditor({ }; if (initial) { - await updateRerunCollection(initial.id, body); + await updateRerunCollection(initial.id, body, etagRef.current); } else { await createRerunCollection(body); } onSaved(); } catch (error) { - setSaveError(messageFromRerunCollectionError(error, 'Unable to save rerun collection')); + 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(messageFromRerunCollectionError(error, 'Unable to save rerun collection')); + } } finally { setSaving(false); } }; + const reloadAfterConflict = () => { + setConflictOpen(false); + setSaveError(null); + setReloadKey((key) => key + 1); + }; + // Ensure the currently-selected item is always an option even if it isn't in the fetched // page (e.g. a large library where browse returned a different slice). const selectedInList = draft.selectedId != null && pickerItems.some((item) => item.id === draft.selectedId); @@ -378,6 +424,17 @@ function RerunCollectionEditor({ /> + + setConflictOpen(false)} + onConfirm={reloadAfterConflict} + open={conflictOpen} + title="Rerun collection changed elsewhere" + tone="danger" + /> ); }