diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs index ab4da04d9..1ec7ceb71 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/AddProgramScheduleItemHandler.cs @@ -60,7 +60,15 @@ public class AddProgramScheduleItemHandler : ProgramScheduleItemCommandBase, await _channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), cancellationToken); } - return ProjectToViewModel(item); + // reload with the full navigation graph before projecting: BuildItem creates the watermark/graphics + // join rows with only their foreign-key ids set, so the tracked entities have null Watermark / + // GraphicsElement navs that ProjectToViewModel dereferences (would 500 on POST — see #229). + ProgramScheduleItem persisted = await dbContext.ProgramScheduleItems + .Filter(psi => psi.Id == item.Id) + .IncludeScheduleItemDetails() + .SingleAsync(cancellationToken); + + return ProjectToViewModel(persisted); } private static async Task> Validate( diff --git a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs index b149a2bec..b24a8a038 100644 --- a/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs @@ -56,7 +56,16 @@ public class ReplaceProgramScheduleItemsHandler( await channel.WriteAsync(new BuildPlayout(playout.Id, PlayoutBuildMode.Refresh), cancellationToken); } - return programSchedule.Items.Map(ProjectToViewModel); + // reload with the full navigation graph before projecting: BuildItem creates the watermark/graphics + // join rows with only their foreign-key ids set, so the tracked entities have null Watermark / + // GraphicsElement navs that ProjectToViewModel dereferences (would 500 on PUT — see #229). + List persisted = await dbContext.ProgramScheduleItems + .Filter(psi => psi.ProgramScheduleId == programSchedule.Id) + .IncludeScheduleItemDetails() + .OrderBy(i => i.Index) + .ToListAsync(cancellationToken); + + return persisted.Map(ProjectToViewModel).ToList(); } private static async Task> Validate( diff --git a/ErsatzTV.Application/ProgramSchedules/ProgramScheduleItemQueryExtensions.cs b/ErsatzTV.Application/ProgramSchedules/ProgramScheduleItemQueryExtensions.cs new file mode 100644 index 000000000..141cb1510 --- /dev/null +++ b/ErsatzTV.Application/ProgramSchedules/ProgramScheduleItemQueryExtensions.cs @@ -0,0 +1,48 @@ +using ErsatzTV.Core.Domain; +using Microsoft.EntityFrameworkCore; + +namespace ErsatzTV.Application.ProgramSchedules; + +internal static class ProgramScheduleItemQueryExtensions +{ + /// + /// The single source of truth for the navigation graph a needs before + /// it can be projected via . The mapper dereferences the + /// Watermark / GraphicsElement navs of each join row without a null guard, so any handler + /// that projects freshly-persisted items MUST reload through this include chain (see #229 — the write + /// path used to project the tracked-but-unloaded graph and threw a NullReferenceException, surfacing as a + /// 500 on PUT/POST whenever an item carried a watermark or graphics element). + /// + public static IQueryable IncludeScheduleItemDetails(this IQueryable query) => + query + .Include(i => i.Collection) + .Include(i => i.MultiCollection) + .Include(i => i.SmartCollection) + .Include(i => i.RerunCollection) + .Include(i => i.Playlist) + .Include(i => i.MediaItem) + .ThenInclude(i => (i as Movie).MovieMetadata) + .ThenInclude(mm => mm.Artwork) + .Include(i => i.MediaItem) + .ThenInclude(i => (i as Season).SeasonMetadata) + .ThenInclude(sm => sm.Artwork) + .Include(i => i.MediaItem) + .ThenInclude(i => (i as Season).Show) + .ThenInclude(s => s.ShowMetadata) + .ThenInclude(sm => sm.Artwork) + .Include(i => i.MediaItem) + .ThenInclude(i => (i as Show).ShowMetadata) + .ThenInclude(sm => sm.Artwork) + .Include(i => i.MediaItem) + .ThenInclude(i => (i as Artist).ArtistMetadata) + .ThenInclude(am => am.Artwork) + .Include(i => i.PreRollFiller) + .Include(i => i.MidRollFiller) + .Include(i => i.PostRollFiller) + .Include(i => i.TailFiller) + .Include(i => i.FallbackFiller) + .Include(i => i.ProgramScheduleItemWatermarks) + .ThenInclude(i => i.Watermark) + .Include(i => i.ProgramScheduleItemGraphicsElements) + .ThenInclude(i => i.GraphicsElement); +} diff --git a/ErsatzTV.Application/ProgramSchedules/Queries/GetProgramScheduleItemsHandler.cs b/ErsatzTV.Application/ProgramSchedules/Queries/GetProgramScheduleItemsHandler.cs index 1d4c5e7ef..9006003b6 100644 --- a/ErsatzTV.Application/ProgramSchedules/Queries/GetProgramScheduleItemsHandler.cs +++ b/ErsatzTV.Application/ProgramSchedules/Queries/GetProgramScheduleItemsHandler.cs @@ -20,36 +20,8 @@ public class GetProgramScheduleItemsHandler(IDbContextFactory dbConte return await dbContext.ProgramScheduleItems .Filter(psi => psi.ProgramScheduleId == request.Id) - .Include(i => i.Collection) - .Include(i => i.MultiCollection) - .Include(i => i.SmartCollection) - .Include(i => i.RerunCollection) - .Include(i => i.Playlist) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Movie).MovieMetadata) - .ThenInclude(mm => mm.Artwork) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Season).SeasonMetadata) - .ThenInclude(sm => sm.Artwork) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Season).Show) - .ThenInclude(s => s.ShowMetadata) - .ThenInclude(sm => sm.Artwork) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Show).ShowMetadata) - .ThenInclude(sm => sm.Artwork) - .Include(i => i.MediaItem) - .ThenInclude(i => (i as Artist).ArtistMetadata) - .ThenInclude(am => am.Artwork) - .Include(i => i.PreRollFiller) - .Include(i => i.MidRollFiller) - .Include(i => i.PostRollFiller) - .Include(i => i.TailFiller) - .Include(i => i.FallbackFiller) - .Include(i => i.ProgramScheduleItemWatermarks) - .ThenInclude(i => i.Watermark) - .Include(i => i.ProgramScheduleItemGraphicsElements) - .ThenInclude(i => i.GraphicsElement) + .IncludeScheduleItemDetails() + .OrderBy(i => i.Index) .ToListAsync(cancellationToken) .Map(programScheduleItems => programScheduleItems.Map(ProjectToViewModel) .Map(psi => EnforceProperties(maybeProgramSchedule, psi)).ToList()); diff --git a/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs new file mode 100644 index 000000000..6602fc198 --- /dev/null +++ b/ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemWriteProjectionTests.cs @@ -0,0 +1,167 @@ +using System.Threading.Channels; +using ErsatzTV.Application; +using ErsatzTV.Application.ProgramSchedules; +using ErsatzTV.Core; +using ErsatzTV.Core.Api.Scheduling; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Scheduling; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Tests.Support; +using LanguageExt; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.ProgramSchedules; + +/// +/// Regression tests for #229. The write path (Replace = PUT, Add = POST) used to project freshly-persisted +/// items whose watermark/graphics join rows only carry foreign-key ids — the Watermark / GraphicsElement navs +/// are null, and Mapper.ProjectToViewModel dereferences them without a guard, throwing an NRE that +/// surfaced as a 500. The pre-existing round-trip test masked this because the Replace handler returned a +/// lazy LanguageExt Map sequence and that test only checked .IsRight — it never enumerated +/// the projected items, so the deferred NRE never fired. These tests force enumeration of the write-path +/// response (exactly as the controller's .ToList() / single-item serialization does) and seed the +/// watermark/graphics entities through a separate context so the handler's fresh factory context has +/// nothing pre-tracked (no accidental change-tracker fix-up). +/// +[TestFixture] +public class ScheduleItemWriteProjectionTests +{ + private InMemoryTvContext _db = null!; + private ChannelWriter _worker = null!; + + [SetUp] + public async Task SetUp() + { + _db = await InMemoryTvContext.CreateAsync(); + _worker = System.Threading.Channels.Channel.CreateUnbounded().Writer; + } + + [TearDown] + public async Task TearDown() => await _db.DisposeAsync(); + + [Test] + public async Task Replace_PutResponse_Should_Carry_Watermark_And_Graphics_Names() + { + int scheduleId = await SeedReferences(); + + var handler = new ReplaceProgramScheduleItemsHandler(_db.Factory, _worker); + Either> result = await handler.Handle( + new ReplaceProgramScheduleItems(scheduleId, [ItemWithGraphics(0)]), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(result.LeftToSeq().HeadOrNone().Match(e => e.Value, () => "unknown")); + + // Force enumeration exactly like the controller (`items.Select(...).ToList()`) — this is what threw the NRE. + List items = result.RightToSeq().Head().ToList(); + ProgramScheduleItemViewModel item = items.Single(); + item.Watermarks.Select(w => w.Name).ShouldBe(["WM One", "WM Two"]); + item.GraphicsElements.Count.ShouldBe(2); + item.GraphicsElements.Select(g => g.Name).ShouldAllBe(n => !string.IsNullOrWhiteSpace(n)); + } + + [Test] + public async Task Add_PostResponse_Should_Carry_Watermark_And_Graphics_Names() + { + int scheduleId = await SeedReferences(); + + var handler = new AddProgramScheduleItemHandler(_db.Factory, _worker); + Either result = await handler.Handle( + AddItemWithGraphics(scheduleId), + CancellationToken.None); + + result.IsRight.ShouldBeTrue(result.LeftToSeq().HeadOrNone().Match(e => e.Value, () => "unknown")); + + ProgramScheduleItemViewModel item = result.RightToSeq().Head(); + item.Watermarks.Select(w => w.Name).ShouldBe(["WM One", "WM Two"]); + item.GraphicsElements.Count.ShouldBe(2); + } + + [Test] + public async Task Get_Should_Return_Items_In_Index_Order_Regardless_Of_Id_Order() + { + int scheduleId; + await using (TvContext ctx = _db.CreateContext()) + { + var schedule = new ProgramSchedule + { + Name = "Shuffled", Items = [], Playouts = [], ProgramScheduleAlternates = [] + }; + ctx.ProgramSchedules.Add(schedule); + await ctx.SaveChangesAsync(); + scheduleId = schedule.Id; + + // Insert with ids deliberately out of Index order: id 10 -> Index 2, id 11 -> Index 0, id 12 -> Index 1. + // Before the fix the GET returned id order (2, 0, 1); after, it must return Index order (0, 1, 2). + ctx.ProgramScheduleItems.Add(OneItemRow(10, scheduleId, index: 2)); + ctx.ProgramScheduleItems.Add(OneItemRow(11, scheduleId, index: 0)); + ctx.ProgramScheduleItems.Add(OneItemRow(12, scheduleId, index: 1)); + await ctx.SaveChangesAsync(); + } + + var getHandler = new GetProgramScheduleItemsHandler(_db.Factory); + List items = + await getHandler.Handle(new GetProgramScheduleItems(scheduleId), CancellationToken.None); + + items.Select(i => i.Index).ShouldBe([0, 1, 2]); + } + + private static ProgramScheduleItemOne OneItemRow(int id, int scheduleId, int index) => + new() + { + Id = id, + ProgramScheduleId = scheduleId, + Index = index, + CollectionType = CollectionType.Collection, + CollectionId = 1, + PlaybackOrder = PlaybackOrder.Shuffle, + ProgramScheduleItemWatermarks = [], + ProgramScheduleItemGraphicsElements = [] + }; + + private async Task SeedReferences() + { + await using TvContext ctx = _db.CreateContext(); + ctx.Collections.Add(new Collection { Id = 1, Name = "First Collection" }); + ctx.ChannelWatermarks.Add(new ChannelWatermark { Id = 1, Name = "WM One" }); + ctx.ChannelWatermarks.Add(new ChannelWatermark { Id = 2, Name = "WM Two" }); + ctx.GraphicsElements.Add(new GraphicsElement { Id = 1, Name = "GE One", Path = "/ge/1", Kind = GraphicsElementKind.Image }); + ctx.GraphicsElements.Add(new GraphicsElement { Id = 2, Name = "GE Two", Path = "/ge/2", Kind = GraphicsElementKind.Image }); + + var schedule = new ProgramSchedule + { + Name = "WM Schedule", Items = [], Playouts = [], ProgramScheduleAlternates = [] + }; + ctx.ProgramSchedules.Add(schedule); + await ctx.SaveChangesAsync(); + return schedule.Id; + } + + private static ReplaceProgramScheduleItem ItemWithGraphics(int index) => + new( + index, StartType.Dynamic, StartTime: null, FixedStartTimeBehavior: null, PlayoutMode.One, + CollectionType.Collection, CollectionId: 1, MultiCollectionId: null, SmartCollectionId: null, + RerunCollectionId: null, MediaItemId: null, PlaylistId: null, SearchTitle: null, SearchQuery: null, + PlaybackOrder: PlaybackOrder.Shuffle, MarathonGroupBy: MarathonGroupBy.None, + MarathonShuffleGroups: false, MarathonShuffleItems: false, MarathonBatchSize: null, + FillWithGroupMode: FillWithGroupMode.None, MultipleMode: MultipleMode.Count, MultipleCount: null, + PlayoutDuration: null, TailMode: TailMode.None, DiscardToFillAttempts: null, CustomTitle: null, + GuideMode: GuideMode.Normal, PreRollFillerId: null, MidRollFillerId: null, PostRollFillerId: null, + TailFillerId: null, FallbackFillerId: null, WatermarkIds: [1, 2], GraphicsElementIds: [1, 2], + PreferredAudioLanguageCode: null, PreferredAudioTitle: null, PreferredSubtitleLanguageCode: null, + SubtitleMode: null); + + private static AddProgramScheduleItem AddItemWithGraphics(int scheduleId) => + new( + scheduleId, StartType.Dynamic, StartTime: null, FixedStartTimeBehavior: null, PlayoutMode.One, + CollectionType.Collection, CollectionId: 1, MultiCollectionId: null, SmartCollectionId: null, + RerunCollectionId: null, MediaItemId: null, PlaylistId: null, SearchTitle: null, SearchQuery: null, + PlaybackOrder: PlaybackOrder.Shuffle, MarathonGroupBy: MarathonGroupBy.None, + MarathonShuffleGroups: false, MarathonShuffleItems: false, MarathonBatchSize: null, + FillWithGroupMode: FillWithGroupMode.None, MultipleMode: MultipleMode.Count, MultipleCount: null, + PlayoutDuration: null, TailMode: TailMode.None, DiscardToFillAttempts: null, CustomTitle: null, + GuideMode: GuideMode.Normal, PreRollFillerId: null, MidRollFillerId: null, PostRollFillerId: null, + TailFillerId: null, FallbackFillerId: null, WatermarkIds: [1, 2], GraphicsElementIds: [1, 2], + PreferredAudioLanguageCode: null, PreferredAudioTitle: null, PreferredSubtitleLanguageCode: null, + SubtitleMode: null); +} diff --git a/docs/api-conventions.md b/docs/api-conventions.md index e3b89e566..40b999cee 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -197,6 +197,17 @@ items), index items from **array order** in the request body rather than trustin index/order field. Exemplar: `ReplaceScheduleItemsRequest.ToCommand(scheduleId)` — `Items.Select((item, index) => item.ToReplaceCommand(index))`. +**Project the write-path response through the same include chain the GET uses — never off the +freshly-built graph.** After `SaveChanges`, a command's entities carry only the foreign-key ids you set +(e.g. `ProgramScheduleItemWatermark.WatermarkId`); their reference navs are null, and any mapper that +dereferences one unguarded throws an NRE that surfaces as a 500. Reload with the read-side includes +before mapping. Exemplars: `ReplaceProgramScheduleItemsHandler` / `AddProgramScheduleItemHandler` reload +via `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` (the one include chain shared with +`GetProgramScheduleItemsHandler`). Also beware LanguageExt `Map` is **lazy** — returning +`items.Map(ProjectToViewModel)` defers the projection, so a test that only checks `.IsRight` won't catch +the NRE; the controller's `.ToList()`/serialization does (regression: `ScheduleItemWriteProjectionTests`). +GET handlers that feed an ordered list must also `.OrderBy(i => i.Index)` — id order is not index order. + ## 8. Known API warts (don't "fix" without discussion — they're deliberate synthesized rows) `GET /api/blocks` and `GET /api/templates` (via `GetAllBlocksHandler` / `GetAllTemplatesHandler` in