fix(671): resolve rerun-collection selections through one shared include chain
The paged list handler eager-loaded nothing, so `ProjectToViewModel` read four unloaded navigations and every row of every collection type projected a null selection. Because the selected id and the display name are read off the SAME navigation, this dropped the id too -- the harm is not an unlabelled badge but an editor that round-trips a null and clears the user's stored selection. The by-id handler loaded metadata for only four of the ten selectable media types: Song/OtherVideo/Image/RemoteStream returned a null-ish selection and Episode/MusicVideo threw an NRE that surfaced as a 500. Fixed at the boundary rather than per call site: - `RerunCollectionQueryExtensions.IncludeSelectionDetails()` is now the single include chain, called by both handlers, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` precedent (#229). Artwork legs are deliberately omitted -- this projection reads only ids and titles. - The media-item switch was duplicated verbatim for RerunCollection and PlaylistItem; both now call one `ProjectMediaItemToViewModel`, which handles `RemoteStream` (via a new `ProjectToNamedViewModel`, since the existing `ProjectToViewModel(RemoteStream)` returns an unrelated type) and never falls through to null -- an unknown subtype keeps its id and takes a conspicuous name, because throwing would fail a whole paged GET over one bad row. - Every metadata navigation in `MediaItems.Mapper` is now read through `Optional(...).Flatten()`, so an un-included nav degrades to "???" instead of being a latent 500 for whichever caller loads least. Tests enumerate all 13 supported CollectionTypes for both handlers, with the matrix derived from `IsSupportedSelectionType` so a newly-supported type joins it automatically, plus a completeness guard on the set. Each mechanism was removed in turn and confirmed red first. fixes #671 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -37,23 +37,43 @@ internal static class Mapper
|
||||
collection.Collection is not null ? ProjectToViewModel(collection.Collection) : null,
|
||||
collection.MultiCollection is not null ? ProjectToViewModel(collection.MultiCollection) : null,
|
||||
collection.SmartCollection is not null ? ProjectToViewModel(collection.SmartCollection) : null,
|
||||
collection.MediaItem switch
|
||||
{
|
||||
Show show => MediaItems.Mapper.ProjectToViewModel(show),
|
||||
Season season => MediaItems.Mapper.ProjectToViewModel(season),
|
||||
Artist artist => MediaItems.Mapper.ProjectToViewModel(artist),
|
||||
Movie movie => MediaItems.Mapper.ProjectToViewModel(movie),
|
||||
Episode episode => MediaItems.Mapper.ProjectToViewModel(episode),
|
||||
MusicVideo musicVideo => MediaItems.Mapper.ProjectToViewModel(musicVideo),
|
||||
OtherVideo otherVideo => MediaItems.Mapper.ProjectToViewModel(otherVideo),
|
||||
Song song => MediaItems.Mapper.ProjectToViewModel(song),
|
||||
Image image => MediaItems.Mapper.ProjectToViewModel(image),
|
||||
_ => null
|
||||
},
|
||||
ProjectMediaItemToViewModel(collection.MediaItem),
|
||||
collection.FirstRunPlaybackOrder,
|
||||
collection.RerunPlaybackOrder,
|
||||
collection.Version);
|
||||
|
||||
/// <summary>
|
||||
/// Flattens the <see cref="MediaItem" /> half of a selection tagged union to a named view model.
|
||||
/// Shared by <see cref="RerunCollection" /> and <see cref="PlaylistItem" />, which select from an
|
||||
/// identical set of media types; one copy is what stops the two drifting apart again (issue #671
|
||||
/// — the same rationale as <c>ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails</c>
|
||||
/// on the query side).
|
||||
/// A null <paramref name="mediaItem" /> is the legitimate "this selection is not a media item"
|
||||
/// case (the selection is a Collection/MultiCollection/SmartCollection instead) and maps to null.
|
||||
/// An unrecognized non-null subtype keeps its id and takes a deliberately conspicuous name rather
|
||||
/// than falling through to null: the id is what the editor round-trips, so returning null there
|
||||
/// silently clears the user's stored selection — while throwing would fail an entire paged GET
|
||||
/// over one unreadable row.
|
||||
/// </summary>
|
||||
private static MediaItems.NamedMediaItemViewModel ProjectMediaItemToViewModel(MediaItem mediaItem) =>
|
||||
mediaItem switch
|
||||
{
|
||||
null => null,
|
||||
Show show => MediaItems.Mapper.ProjectToViewModel(show),
|
||||
Season season => MediaItems.Mapper.ProjectToViewModel(season),
|
||||
Artist artist => MediaItems.Mapper.ProjectToViewModel(artist),
|
||||
Movie movie => MediaItems.Mapper.ProjectToViewModel(movie),
|
||||
Episode episode => MediaItems.Mapper.ProjectToViewModel(episode),
|
||||
MusicVideo musicVideo => MediaItems.Mapper.ProjectToViewModel(musicVideo),
|
||||
OtherVideo otherVideo => MediaItems.Mapper.ProjectToViewModel(otherVideo),
|
||||
Song song => MediaItems.Mapper.ProjectToViewModel(song),
|
||||
Image image => MediaItems.Mapper.ProjectToViewModel(image),
|
||||
RemoteStream remoteStream => MediaItems.Mapper.ProjectToNamedViewModel(remoteStream),
|
||||
_ => new MediaItems.NamedMediaItemViewModel(
|
||||
mediaItem.Id,
|
||||
$"[unsupported media type: {mediaItem.GetType().Name}]")
|
||||
};
|
||||
|
||||
internal static TraktListViewModel ProjectToViewModel(TraktList traktList) =>
|
||||
new(
|
||||
traktList.Id,
|
||||
@@ -108,19 +128,7 @@ internal static class Mapper
|
||||
playlistItem.SmartCollection is not null
|
||||
? ProjectToViewModel(playlistItem.SmartCollection)
|
||||
: null,
|
||||
playlistItem.MediaItem switch
|
||||
{
|
||||
Show show => MediaItems.Mapper.ProjectToViewModel(show),
|
||||
Season season => MediaItems.Mapper.ProjectToViewModel(season),
|
||||
Artist artist => MediaItems.Mapper.ProjectToViewModel(artist),
|
||||
Movie movie => MediaItems.Mapper.ProjectToViewModel(movie),
|
||||
Episode episode => MediaItems.Mapper.ProjectToViewModel(episode),
|
||||
MusicVideo musicVideo => MediaItems.Mapper.ProjectToViewModel(musicVideo),
|
||||
OtherVideo otherVideo => MediaItems.Mapper.ProjectToViewModel(otherVideo),
|
||||
Song song => MediaItems.Mapper.ProjectToViewModel(song),
|
||||
Image image => MediaItems.Mapper.ProjectToViewModel(image),
|
||||
_ => null
|
||||
},
|
||||
ProjectMediaItemToViewModel(playlistItem.MediaItem),
|
||||
playlistItem.PlaybackOrder,
|
||||
playlistItem.Count,
|
||||
playlistItem.PlayAll,
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using static ErsatzTV.Application.MediaCollections.Mapper;
|
||||
@@ -15,13 +15,15 @@ public class GetPagedRerunCollectionsHandler(IDbContextFactory<TvContext> dbCont
|
||||
await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
|
||||
int count = await dbContext.RerunCollections.CountAsync(cancellationToken);
|
||||
|
||||
IQueryable<RerunCollection> query = dbContext.RerunCollections.AsNoTracking();
|
||||
IQueryable<RerunCollection> query = dbContext.RerunCollections.AsNoTracking().IncludeSelectionDetails();
|
||||
|
||||
if (!string.IsNullOrWhiteSpace(request.Query))
|
||||
{
|
||||
query = query.Where(rc => EF.Functions.Like(rc.Name, $"%{request.Query}%"));
|
||||
}
|
||||
|
||||
// EF applies the includes to the paged subquery, so the selection graph is loaded for at most
|
||||
// PageSize rows — the per-request cost is bounded by the page, not by the table (issue #671).
|
||||
List<RerunCollectionViewModel> page = await query
|
||||
.OrderBy(rc => rc.Name)
|
||||
.Skip(request.PageNum * request.PageSize)
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Infrastructure.Extensions;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
@@ -16,20 +16,7 @@ public class GetRerunCollectionByIdHandler(IDbContextFactory<TvContext> dbContex
|
||||
await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
|
||||
return await dbContext.RerunCollections
|
||||
.AsNoTracking()
|
||||
.Include(c => c.Collection)
|
||||
.Include(c => c.MultiCollection)
|
||||
.Include(c => c.SmartCollection)
|
||||
.Include(i => i.MediaItem)
|
||||
.ThenInclude(i => (i as Movie).MovieMetadata)
|
||||
.Include(i => i.MediaItem)
|
||||
.ThenInclude(i => (i as Season).SeasonMetadata)
|
||||
.Include(i => i.MediaItem)
|
||||
.ThenInclude(i => (i as Season).Show)
|
||||
.ThenInclude(s => s.ShowMetadata)
|
||||
.Include(i => i.MediaItem)
|
||||
.ThenInclude(i => (i as Show).ShowMetadata)
|
||||
.Include(i => i.MediaItem)
|
||||
.ThenInclude(i => (i as Artist).ArtistMetadata)
|
||||
.IncludeSelectionDetails()
|
||||
.SelectOneAsync(c => c.Id, c => c.Id == request.Id, cancellationToken)
|
||||
.MapT(ProjectToViewModel);
|
||||
}
|
||||
|
||||
@@ -0,0 +1,57 @@
|
||||
using ErsatzTV.Core.Domain;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
|
||||
namespace ErsatzTV.Application.MediaCollections;
|
||||
|
||||
internal static class RerunCollectionQueryExtensions
|
||||
{
|
||||
/// <summary>
|
||||
/// The single source of truth for the navigation graph a <see cref="RerunCollection" /> needs before it
|
||||
/// can be projected via <see cref="Mapper.ProjectToViewModel(RerunCollection)" />. Both the paged-list
|
||||
/// and by-id handlers reload through this chain so the two cannot drift apart again (see #671 — the list
|
||||
/// handler had no includes at all, so every row projected a null selection, while the by-id handler
|
||||
/// covered only Movie/Season/Show/Artist and so returned a null selection for Song/OtherVideo/Image and
|
||||
/// a 500 for Episode/MusicVideo).
|
||||
/// Because the id and the display name are both read off these navigations, an un-included type does not
|
||||
/// merely lose its label — it loses the selected id too, which is what silently cleared a stored
|
||||
/// selection in the editor.
|
||||
/// Deliberately narrower than the analogous playlist-item chain in <c>GetPlaylistItemsHandler</c>: the
|
||||
/// rerun projection reads only each selection's id and title, never its artwork, so the
|
||||
/// <c>.ThenInclude(… => …Artwork)</c> legs are omitted rather than paid for on every page.
|
||||
/// </summary>
|
||||
public static IQueryable<RerunCollection> IncludeSelectionDetails(this IQueryable<RerunCollection> query) =>
|
||||
query
|
||||
.Include(c => c.Collection)
|
||||
.Include(c => c.MultiCollection)
|
||||
.Include(c => c.SmartCollection)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Movie).MovieMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Show).ShowMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Season).SeasonMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Season).Show)
|
||||
.ThenInclude(s => s.ShowMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Artist).ArtistMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Episode).EpisodeMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Episode).Season)
|
||||
.ThenInclude(s => s.Show)
|
||||
.ThenInclude(s => s.ShowMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as MusicVideo).MusicVideoMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as MusicVideo).Artist)
|
||||
.ThenInclude(a => a.ArtistMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as OtherVideo).OtherVideoMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Song).SongMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as Image).ImageMetadata)
|
||||
.Include(c => c.MediaItem)
|
||||
.ThenInclude(i => (i as RemoteStream).RemoteStreamMetadata);
|
||||
}
|
||||
@@ -1,18 +1,24 @@
|
||||
using System.Globalization;
|
||||
using System.Globalization;
|
||||
using ErsatzTV.Core.Domain;
|
||||
|
||||
namespace ErsatzTV.Application.MediaItems;
|
||||
|
||||
internal static class Mapper
|
||||
{
|
||||
// Every metadata navigation below is read through Optional(...).Flatten() rather than a bare
|
||||
// dereference: these projections are reached from several handlers whose Include chains differ,
|
||||
// and an un-included navigation must degrade to the "???" placeholder instead of throwing an
|
||||
// NRE that surfaces as a 500 on a GET (issue #671).
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(Show show) =>
|
||||
new(show.Id, show.ShowMetadata.HeadOrNone().Map(sm => $"{sm?.Title} ({sm?.Year})").IfNone("???"));
|
||||
new(
|
||||
show.Id,
|
||||
Optional(show.ShowMetadata).Flatten().HeadOrNone().Map(sm => $"{sm?.Title} ({sm?.Year})").IfNone("???"));
|
||||
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(Season season) =>
|
||||
new(season.Id, $"{ShowTitle(season)} - {SeasonDescription(season)}");
|
||||
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(Artist artist) =>
|
||||
new(artist.Id, artist.ArtistMetadata.HeadOrNone().Match(am => am.Title, () => "???"));
|
||||
new(artist.Id, Optional(artist.ArtistMetadata).Flatten().HeadOrNone().Match(am => am.Title, () => "???"));
|
||||
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(Movie movie) =>
|
||||
new(movie.Id, MovieTitle(movie));
|
||||
@@ -24,23 +30,37 @@ internal static class Mapper
|
||||
new(musicVideo.Id, MusicVideoTitle(musicVideo));
|
||||
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(OtherVideo otherVideo) =>
|
||||
new(otherVideo.Id, otherVideo.OtherVideoMetadata.HeadOrNone().Match(ov => ov.Title, () => "???"));
|
||||
new(
|
||||
otherVideo.Id,
|
||||
Optional(otherVideo.OtherVideoMetadata).Flatten().HeadOrNone().Match(ov => ov.Title, () => "???"));
|
||||
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(Song song) =>
|
||||
new(song.Id, SongTitle(song));
|
||||
|
||||
internal static NamedMediaItemViewModel ProjectToViewModel(Image image) =>
|
||||
new(image.Id, image.ImageMetadata.HeadOrNone().Match(i => i.Title, () => "???"));
|
||||
new(image.Id, Optional(image.ImageMetadata).Flatten().HeadOrNone().Match(i => i.Title, () => "???"));
|
||||
|
||||
internal static RemoteStreamViewModel ProjectToViewModel(RemoteStream remoteStream) =>
|
||||
new(remoteStream.Id, remoteStream.Url, remoteStream.Script);
|
||||
|
||||
/// <summary>
|
||||
/// The named projection for a <see cref="RemoteStream" />. This cannot be an overload of
|
||||
/// <see cref="ProjectToViewModel(RemoteStream)" /> — that one already exists and returns a
|
||||
/// <see cref="RemoteStreamViewModel" />, and C# will not overload on return type alone. Its
|
||||
/// absence is why every selection-flattening switch dropped <c>RemoteStream</c> through a
|
||||
/// <c>_ => null</c> arm (issue #671).
|
||||
/// </summary>
|
||||
internal static NamedMediaItemViewModel ProjectToNamedViewModel(RemoteStream remoteStream) =>
|
||||
new(
|
||||
remoteStream.Id,
|
||||
Optional(remoteStream.RemoteStreamMetadata).Flatten().HeadOrNone().Match(rsm => rsm.Title, () => "???"));
|
||||
|
||||
private static string MovieTitle(Movie movie)
|
||||
{
|
||||
var title = "???";
|
||||
var year = "???";
|
||||
|
||||
foreach (MovieMetadata movieMetadata in movie.MovieMetadata.HeadOrNone())
|
||||
foreach (MovieMetadata movieMetadata in Optional(movie.MovieMetadata).Flatten().HeadOrNone())
|
||||
{
|
||||
title = movieMetadata.Title;
|
||||
foreach (int y in Optional(movieMetadata.Year))
|
||||
@@ -57,7 +77,10 @@ internal static class Mapper
|
||||
var title = "???";
|
||||
var year = "???";
|
||||
|
||||
foreach (ShowMetadata show in season.Show.ShowMetadata.HeadOrNone())
|
||||
// Season.Show and Show.ShowMetadata are only populated when the caller eager-loaded them.
|
||||
// An un-included navigation must degrade to the "???" placeholder these helpers already
|
||||
// produce for missing metadata — never an NRE, which surfaced as a 500 (issue #671).
|
||||
foreach (ShowMetadata show in Optional(season.Show?.ShowMetadata).Flatten().HeadOrNone())
|
||||
{
|
||||
title = show.Title;
|
||||
foreach (int y in Optional(show.Year))
|
||||
@@ -74,10 +97,10 @@ internal static class Mapper
|
||||
|
||||
private static string EpisodeTitle(Episode e)
|
||||
{
|
||||
string showTitle = e.Season.Show.ShowMetadata.HeadOrNone()
|
||||
string showTitle = Optional(e.Season?.Show?.ShowMetadata).Flatten().HeadOrNone()
|
||||
.Map(sm => $"{sm.Title} - ").IfNone(string.Empty);
|
||||
var episodeNumbers = e.EpisodeMetadata.Map(em => em.EpisodeNumber).ToList();
|
||||
var episodeTitles = e.EpisodeMetadata.Map(em => em.Title).ToList();
|
||||
var episodeNumbers = Optional(e.EpisodeMetadata).Flatten().Map(em => em.EpisodeNumber).ToList();
|
||||
var episodeTitles = Optional(e.EpisodeMetadata).Flatten().Map(em => em.Title).ToList();
|
||||
if (episodeNumbers.Count == 0 || episodeTitles.Count == 0)
|
||||
{
|
||||
return "[unknown episode]";
|
||||
@@ -86,24 +109,24 @@ internal static class Mapper
|
||||
var numbersString = $"e{string.Join('e', episodeNumbers.Map(n => $"{n:00}"))}";
|
||||
var titlesString = $"{string.Join('/', episodeTitles)}";
|
||||
|
||||
return $"{showTitle}s{e.Season.SeasonNumber:00}{numbersString} - {titlesString}";
|
||||
return $"{showTitle}s{e.Season?.SeasonNumber ?? 0:00}{numbersString} - {titlesString}";
|
||||
}
|
||||
|
||||
private static string MusicVideoTitle(MusicVideo mv)
|
||||
{
|
||||
string artistName = mv.Artist.ArtistMetadata.HeadOrNone()
|
||||
string artistName = Optional(mv.Artist?.ArtistMetadata).Flatten().HeadOrNone()
|
||||
.Map(am => $"{am.Title} - ").IfNone(string.Empty);
|
||||
return mv.MusicVideoMetadata.HeadOrNone()
|
||||
return Optional(mv.MusicVideoMetadata).Flatten().HeadOrNone()
|
||||
.Map(mvm => $"{artistName}{mvm.Title}")
|
||||
.IfNone("[unknown music video]");
|
||||
}
|
||||
|
||||
private static string SongTitle(Song s)
|
||||
{
|
||||
string songArtist = s.SongMetadata.HeadOrNone()
|
||||
string songArtist = Optional(s.SongMetadata).Flatten().HeadOrNone()
|
||||
.Map(sm => $"{string.Join(", ", sm.Artists)} - ")
|
||||
.IfNone(string.Empty);
|
||||
return s.SongMetadata.HeadOrNone()
|
||||
return Optional(s.SongMetadata).Flatten().HeadOrNone()
|
||||
.Map(sm => $"{songArtist}{sm.Title ?? string.Empty}")
|
||||
.IfNone("[unknown song]");
|
||||
}
|
||||
|
||||
@@ -0,0 +1,278 @@
|
||||
using ErsatzTV.Application.MediaCollections;
|
||||
using ErsatzTV.Controllers.Api.Requests;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using LanguageExt;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.MediaCollections;
|
||||
|
||||
/// <summary>
|
||||
/// Read-path coverage for the two rerun-collection query handlers (issue #671). The defect was
|
||||
/// precisely that nobody enumerated the selection types: the list handler eager-loaded nothing, and
|
||||
/// the by-id handler loaded metadata for only four of the ten media types. So the matrix here is
|
||||
/// derived from the production predicate rather than hand-listed — a newly-supported
|
||||
/// <see cref="CollectionType" /> joins it automatically and fails loudly in <c>SeedSelection</c>
|
||||
/// until someone teaches the suite how to seed it.
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
|
||||
{
|
||||
private const int SelectedId = 42;
|
||||
|
||||
private static IEnumerable<CollectionType> SupportedSelectionTypes =>
|
||||
Enum.GetValues<CollectionType>().Where(RerunCollectionRequestMapping.IsSupportedSelectionType);
|
||||
|
||||
/// <summary>
|
||||
/// Completeness guard. Without it, a change that narrowed <c>IsSupportedSelectionType</c> would
|
||||
/// shrink the matrix silently and every remaining case would still pass — the "filters on the
|
||||
/// property it asserts" failure mode.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public void Supported_Selection_Types_Should_Be_The_Full_Documented_Set()
|
||||
{
|
||||
SupportedSelectionTypes.ShouldBe(
|
||||
[
|
||||
CollectionType.Collection,
|
||||
CollectionType.TelevisionShow,
|
||||
CollectionType.TelevisionSeason,
|
||||
CollectionType.Artist,
|
||||
CollectionType.MultiCollection,
|
||||
CollectionType.SmartCollection,
|
||||
CollectionType.Movie,
|
||||
CollectionType.Episode,
|
||||
CollectionType.MusicVideo,
|
||||
CollectionType.OtherVideo,
|
||||
CollectionType.Song,
|
||||
CollectionType.Image,
|
||||
CollectionType.RemoteStream
|
||||
],
|
||||
ignoreOrder: true);
|
||||
}
|
||||
|
||||
[TestCaseSource(nameof(SupportedSelectionTypes))]
|
||||
public async Task GetById_Should_Resolve_The_Selection(CollectionType collectionType)
|
||||
{
|
||||
await SeedSelection(collectionType);
|
||||
await SeedRerunCollection(1, collectionType);
|
||||
|
||||
var handler = new GetRerunCollectionByIdHandler(Db.Factory);
|
||||
|
||||
Option<RerunCollectionViewModel> result =
|
||||
await handler.Handle(new GetRerunCollectionById(1), CancellationToken.None);
|
||||
|
||||
RerunCollectionViewModel vm = result.IfNone(() => throw new AssertionException("Expected a result"));
|
||||
AssertSelectionResolved(vm, collectionType);
|
||||
}
|
||||
|
||||
[TestCaseSource(nameof(SupportedSelectionTypes))]
|
||||
public async Task GetPaged_Should_Resolve_The_Selection(CollectionType collectionType)
|
||||
{
|
||||
await SeedSelection(collectionType);
|
||||
await SeedRerunCollection(1, collectionType);
|
||||
|
||||
var handler = new GetPagedRerunCollectionsHandler(Db.Factory);
|
||||
|
||||
PagedRerunCollectionsViewModel result = await handler.Handle(
|
||||
new GetPagedRerunCollections(string.Empty, 0, 10),
|
||||
CancellationToken.None);
|
||||
|
||||
result.Page.Count.ShouldBe(1);
|
||||
AssertSelectionResolved(result.Page[0], collectionType);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Mirrors <c>RerunCollectionController.ProjectToResponseModel</c>, which flattens the tagged
|
||||
/// union to the single <c>selectedId</c> / <c>selectedName</c> pair the SPA consumes. The id is
|
||||
/// the load-bearing half: the editor round-trips it, so a null there silently clears the user's
|
||||
/// stored selection.
|
||||
/// </summary>
|
||||
private static void AssertSelectionResolved(RerunCollectionViewModel vm, CollectionType collectionType)
|
||||
{
|
||||
int? selectedId = vm.Collection?.Id
|
||||
?? vm.MultiCollection?.Id
|
||||
?? vm.SmartCollection?.Id
|
||||
?? vm.MediaItem?.MediaItemId;
|
||||
|
||||
string selectedName = vm.Collection?.Name
|
||||
?? vm.MultiCollection?.Name
|
||||
?? vm.SmartCollection?.Name
|
||||
?? vm.MediaItem?.Name;
|
||||
|
||||
selectedId.ShouldBe(SelectedId, $"{collectionType} lost its selected id");
|
||||
|
||||
selectedName.ShouldNotBeNullOrWhiteSpace($"{collectionType} lost its selected name");
|
||||
|
||||
// The placeholders the mappers emit when metadata is missing. Asserting merely "not null" would
|
||||
// pass on "???" — i.e. on a selection whose navigation was never loaded.
|
||||
selectedName.ShouldNotContain("???", customMessage: $"{collectionType} resolved to a placeholder name");
|
||||
selectedName.ShouldNotStartWith("[unknown", customMessage: $"{collectionType} resolved to a placeholder");
|
||||
selectedName.ShouldNotStartWith(
|
||||
"[unsupported media type",
|
||||
customMessage: $"{collectionType} fell through the media-item switch");
|
||||
}
|
||||
|
||||
private async Task SeedSelection(CollectionType collectionType)
|
||||
{
|
||||
await using TvContext context = Db.CreateContext();
|
||||
|
||||
switch (collectionType)
|
||||
{
|
||||
case CollectionType.Collection:
|
||||
context.Collections.Add(new Collection
|
||||
{
|
||||
Id = SelectedId,
|
||||
Name = "Selected collection",
|
||||
MediaItems = []
|
||||
});
|
||||
break;
|
||||
case CollectionType.MultiCollection:
|
||||
context.MultiCollections.Add(new MultiCollection
|
||||
{
|
||||
Id = SelectedId,
|
||||
Name = "Selected multi collection"
|
||||
});
|
||||
break;
|
||||
case CollectionType.SmartCollection:
|
||||
context.SmartCollections.Add(new SmartCollection
|
||||
{
|
||||
Id = SelectedId,
|
||||
Name = "Selected smart collection",
|
||||
Query = "tag:family"
|
||||
});
|
||||
break;
|
||||
case CollectionType.TelevisionShow:
|
||||
context.Shows.Add(new Show
|
||||
{
|
||||
Id = SelectedId,
|
||||
ShowMetadata = [new ShowMetadata { Title = "Selected show", Year = 2020 }]
|
||||
});
|
||||
break;
|
||||
case CollectionType.TelevisionSeason:
|
||||
context.Seasons.Add(new Season
|
||||
{
|
||||
Id = SelectedId,
|
||||
SeasonNumber = 3,
|
||||
Show = new Show
|
||||
{
|
||||
Id = 900,
|
||||
ShowMetadata = [new ShowMetadata { Title = "Parent show", Year = 2020 }]
|
||||
}
|
||||
});
|
||||
break;
|
||||
case CollectionType.Artist:
|
||||
context.Artists.Add(new Artist
|
||||
{
|
||||
Id = SelectedId,
|
||||
ArtistMetadata = [new ArtistMetadata { Title = "Selected artist" }]
|
||||
});
|
||||
break;
|
||||
case CollectionType.Movie:
|
||||
context.Movies.Add(new Movie
|
||||
{
|
||||
Id = SelectedId,
|
||||
MovieMetadata = [new MovieMetadata { Title = "Selected movie", Year = 2019 }]
|
||||
});
|
||||
break;
|
||||
case CollectionType.Episode:
|
||||
context.Episodes.Add(new Episode
|
||||
{
|
||||
Id = SelectedId,
|
||||
EpisodeMetadata = [new EpisodeMetadata { Title = "Selected episode", EpisodeNumber = 4 }],
|
||||
Season = new Season
|
||||
{
|
||||
Id = 901,
|
||||
SeasonNumber = 2,
|
||||
Show = new Show
|
||||
{
|
||||
Id = 902,
|
||||
ShowMetadata = [new ShowMetadata { Title = "Episode's show", Year = 2018 }]
|
||||
}
|
||||
}
|
||||
});
|
||||
break;
|
||||
case CollectionType.MusicVideo:
|
||||
context.MusicVideos.Add(new MusicVideo
|
||||
{
|
||||
Id = SelectedId,
|
||||
MusicVideoMetadata = [new MusicVideoMetadata { Title = "Selected music video" }],
|
||||
Artist = new Artist
|
||||
{
|
||||
Id = 903,
|
||||
ArtistMetadata = [new ArtistMetadata { Title = "Video's artist" }]
|
||||
}
|
||||
});
|
||||
break;
|
||||
case CollectionType.OtherVideo:
|
||||
context.OtherVideos.Add(new OtherVideo
|
||||
{
|
||||
Id = SelectedId,
|
||||
OtherVideoMetadata = [new OtherVideoMetadata { Title = "Selected other video" }]
|
||||
});
|
||||
break;
|
||||
case CollectionType.Song:
|
||||
context.Songs.Add(new Song
|
||||
{
|
||||
Id = SelectedId,
|
||||
SongMetadata = [new SongMetadata { Title = "Selected song", Artists = [] }]
|
||||
});
|
||||
break;
|
||||
case CollectionType.Image:
|
||||
context.Images.Add(new Image
|
||||
{
|
||||
Id = SelectedId,
|
||||
ImageMetadata = [new ImageMetadata { Title = "Selected image" }]
|
||||
});
|
||||
break;
|
||||
case CollectionType.RemoteStream:
|
||||
context.RemoteStreams.Add(new RemoteStream
|
||||
{
|
||||
Id = SelectedId,
|
||||
Url = "http://example.invalid/stream",
|
||||
RemoteStreamMetadata = [new RemoteStreamMetadata { Title = "Selected remote stream" }]
|
||||
});
|
||||
break;
|
||||
default:
|
||||
throw new AssertionException(
|
||||
$"{collectionType} is a supported rerun-collection selection type but this suite does " +
|
||||
"not know how to seed it — teach SeedSelection about it rather than narrowing the matrix.");
|
||||
}
|
||||
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
private async Task SeedRerunCollection(int id, CollectionType collectionType)
|
||||
{
|
||||
await using TvContext context = Db.CreateContext();
|
||||
|
||||
var rerunCollection = new RerunCollection
|
||||
{
|
||||
Id = id,
|
||||
Name = "Rerun",
|
||||
CollectionType = collectionType,
|
||||
FirstRunPlaybackOrder = PlaybackOrder.Chronological,
|
||||
RerunPlaybackOrder = PlaybackOrder.Chronological
|
||||
};
|
||||
|
||||
switch (collectionType)
|
||||
{
|
||||
case CollectionType.Collection:
|
||||
rerunCollection.CollectionId = SelectedId;
|
||||
break;
|
||||
case CollectionType.MultiCollection:
|
||||
rerunCollection.MultiCollectionId = SelectedId;
|
||||
break;
|
||||
case CollectionType.SmartCollection:
|
||||
rerunCollection.SmartCollectionId = SelectedId;
|
||||
break;
|
||||
default:
|
||||
rerunCollection.MediaItemId = SelectedId;
|
||||
break;
|
||||
}
|
||||
|
||||
context.RerunCollections.Add(rerunCollection);
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
}
|
||||
@@ -146,6 +146,34 @@ Exemplars:
|
||||
`Brief`. `Remediation.Kind` is a mapped **string** ("ExternalDoc"/"AppRoute"), not a wire enum —
|
||||
same pattern as `Status`. See `decisions.md` 2026-07-17 (#164).
|
||||
|
||||
### 2a. Flattening a tagged-union selection (read path)
|
||||
|
||||
Several DTOs flatten a "exactly one of these navigations is populated" tagged union to a single
|
||||
`selectedId` + `selectedName` pair (`RerunCollectionResponseModel`, and the playlist-item shape).
|
||||
Two rules, both learned from #671, where the list endpoint returned a null selection for **every**
|
||||
row and the detail GET 500'd for two of its media types:
|
||||
|
||||
- **One include chain per projected aggregate, shared by every handler that projects it.** Put it in
|
||||
a `<Aggregate>QueryExtensions` extension method and call it from the list handler *and* the by-id
|
||||
handler. Exemplars: `RerunCollectionQueryExtensions.IncludeSelectionDetails()`,
|
||||
`ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()`. Two hand-maintained chains
|
||||
drift, and the one that drifts is usually the paged list, whose rows are individually less
|
||||
obviously wrong. Applying it before `Skip`/`Take` is fine — EF applies the includes to the paged
|
||||
subquery, so the cost is bounded by `PageSize`, not by the table.
|
||||
- **The id and the name must not share a single point of failure.** When both are read off the same
|
||||
eager-loaded navigation, the id is only ever as available as the name — so an un-included type
|
||||
doesn't merely render an unlabelled badge, it drops the selected id, and an editor that
|
||||
round-trips that id silently clears the user's stored selection. Accordingly a media-item
|
||||
flattening switch never ends in `_ => null`: an unrecognized subtype keeps its id and takes a
|
||||
conspicuous `[unsupported media type: X]` name. Throwing is the wrong lever — it would fail an
|
||||
entire paged GET over one unreadable row. The shared switch is
|
||||
`MediaCollections.Mapper.ProjectMediaItemToViewModel`.
|
||||
|
||||
Corollary for the mappers themselves: `MediaItems.Mapper`'s projections are reached from handlers
|
||||
whose include chains differ, so every metadata navigation is read through `Optional(...).Flatten()`
|
||||
and degrades to the `"???"` placeholder rather than throwing. A bare `x.Season.Show.ShowMetadata`
|
||||
inside a projection is a latent 500 on some other caller's GET.
|
||||
|
||||
## 3. Error mapping
|
||||
|
||||
Central helper: `ErsatzTV/Extensions/ApiResults.cs`. Use these extension methods instead of
|
||||
|
||||
@@ -31,6 +31,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `api.search-field-values-sources` | `GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for a narrow allow-list of catalog fields (never the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source (`title`, `show_title` only); `limit` clamped to `[1, 50]` (default 50). The FINAL filter, dedup and ordering applied to the response are ORDINAL (`OrdinalIgnoreCase` / `StringComparer.Ordinal`), never current-culture, because `UseRequestLocalization` makes the culture caller-controlled — scoped to the in-memory stages on purpose: a field sourced by a plain EF query is filtered and truncated by the DATABASE collation first (SQLite's `LOWER()` is ASCII-only), which ordinal semantics downstream cannot undo (ersatztv#668). A field whose values live in an EF **primitive collection** (one JSON array per row in a single column: `SongMetadata.Artists`, `SongMetadata.AlbumArtists`) is served, not 404'd, as bounded best-effort, and its rows are read by a keyset page carrying **NO RESIDUAL predicate** — `SELECT Id, <col> AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch`, no `LIKE`, no `LOWER`, not even `IS NOT NULL`. The cursor is itself a predicate, but a SEEKABLE one on the ordering key: it positions the scan and never discards a row. A RESIDUAL predicate discards rows the engine already produced, and `LIMIT` truncates only the survivors — so with one present it bounds the OUTPUT rather than the row count. All selectivity is in memory. The guarantee is scoped: **at most 20,000 LOGICAL rows returned/materialized and at most 10 round trips (11 for `artist`)** — NOT bounded physical work and NOT bounded bytes, because MySQL traverses deleted-but-unpurged index records and the `TEXT`/`longtext` payload width is unrestricted. The walk pages 2,000 rows at a time, stopping on the first of enough distinct matches, a short page, or the ceiling. | 2026-07-26 | [link](records/api/search-field-values-sources.md) |
|
||||
| `api.search-field-values-unicode-fold` | The EF-sourced facet fields (`genre`, `show_genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`'s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite's `LOWER()` folds ASCII only (`lower('Édith')` is `'Édith'` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware, so `LOWER('Édith')` really is `'édith'` and the existing predicate reaches the row unaided. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT <col> AS Value FROM <table> WHERE [<discriminator> AND] etv_upper(<col>) LIKE @Pattern ESCAPE '\' ORDER BY <col> LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today's EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record's "Known limitation inherited, not introduced" clause; everything else it settles still holds. | 2026-07-27 | [link](records/api/search-field-values-unicode-fold.md) |
|
||||
| `api.search-paging-cap` | Search stays capped at 100 items per media kind; an overflowing kind's "See all" reuses library-browse paging instead of adding new API surface. | 2026-07-11 | [link](records/api/search-paging-cap.md) |
|
||||
| `api.selection-projection-include-chain` | Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `<Aggregate>QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET. | 2026-07-28 | [link](records/api/selection-projection-include-chain.md) |
|
||||
| `api.versioning-v1` | The entire `/api` surface is versioned to `/api/v1` uniformly (no unversioned corner); legacy unversioned callers are rewritten in-pipeline (not redirected) with Deprecation/Link/Sunset headers, and post-freeze `/api/v1` is additive-only — a breaking change requires `/api/v2`. | 2026-07-13 | [link](records/api/versioning-v1.md) |
|
||||
| `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) |
|
||||
| `blazor.ui-removed` | The legacy Blazor Server UI (`Pages/`, `Shared/`, `ViewModels/`, `Validators/`, MudBlazor + 8 other packages, Blazor Startup wiring) is fully deleted now that the SPA has parity; the legacy `MapWhen` branch is kept only for controllers/docs/OpenAPI/`LegacyUiRedirects`, and the catch-all fallback 302s any unmatched non-api/artwork/docs/openapi path to `/app`. | 2026-07-11 | [link](records/blazor/ui-removed.md) |
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
---
|
||||
key: api.selection-projection-include-chain
|
||||
title: '2026-07-28 — A tagged-union selection is projected through one shared include chain, and its flattening switch never falls through to null (#671)'
|
||||
status: active
|
||||
since: '2026-07-28'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'Every handler that projects an aggregate carrying a tagged-union selection loads it through ONE shared `<Aggregate>QueryExtensions` include chain — `RerunCollectionQueryExtensions.IncludeSelectionDetails()`, joining the existing `ProgramScheduleItemQueryExtensions.IncludeScheduleItemDetails()` — called by the paged-list handler and the by-id handler alike, so the two cannot drift. The media-item flattening switch is likewise ONE shared helper, `MediaCollections.Mapper.ProjectMediaItemToViewModel`, covering all ten selectable media types including `RemoteStream`, whose named projection is `MediaItems.Mapper.ProjectToNamedViewModel` (it cannot be an overload of `ProjectToViewModel(RemoteStream)`, which already exists returning the unrelated `RemoteStreamViewModel`; C# will not overload on return type). That switch NEVER ends in `_ => null`: a null MediaItem is the legitimate not-a-media-item case, while an unrecognized non-null subtype keeps its id and takes a conspicuous `[unsupported media type: X]` name. Fail-soft is deliberate — throwing would fail an entire paged GET over one unreadable row. Finally, every metadata navigation inside `MediaItems.Mapper` is read through `Optional(...).Flatten()` and degrades to the `"???"` placeholder, because those projections are reached from handlers whose include chains differ and a bare `x.Season.Show.ShowMetadata` is a latent 500 on some other caller GET.'
|
||||
signals: 'rerun collection null selection, selectedId null for every row, list badge renders Collection with no name, detail GET 500 on Episode, detail GET 500 on MusicVideo, RemoteStream dropped by the mapper, underscore arrow null fallthrough, AsNoTracking suppresses navigation fixup, eager load missing on paged list, Include after Skip Take, EpisodeTitle NullReferenceException, MusicVideoTitle bare Artist deref, ShowTitle bare Show deref, id only as available as the name, editor silently clears stored selection · paths: `ErsatzTV.Application/MediaCollections/RerunCollectionQueryExtensions.cs`, `ErsatzTV.Application/MediaCollections/Mapper.cs`, `ErsatzTV.Application/MediaItems/Mapper.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetPagedRerunCollectionsHandler.cs`, `ErsatzTV.Application/MediaCollections/Queries/GetRerunCollectionByIdHandler.cs`, `docs/api-conventions.md` §2a · issues: #671, #651, #229'
|
||||
mechanics: '`RerunCollectionQueryExtensions.IncludeSelectionDetails`; `Mapper.ProjectMediaItemToViewModel`; `MediaItems.Mapper.ProjectToNamedViewModel`; `RerunCollectionQueryHandlerTests` (`GetById_Should_Resolve_The_Selection`, `GetPaged_Should_Resolve_The_Selection`, `Supported_Selection_Types_Should_Be_The_Full_Documented_Set`); `RerunCollectionRequestMapping.IsSupportedSelectionType`'
|
||||
---
|
||||
|
||||
Applies the `#229` shared-include-chain remedy to the READ path. That record framed the rule as a
|
||||
write-path concern (project the mutation response through the include chain the GET uses); #671 is
|
||||
its mirror image, where the GET itself under-loaded.
|
||||
|
||||
## The coupling that hid the bug
|
||||
|
||||
The id and the display name are read off the SAME eager-loaded navigation, so the id is only ever as
|
||||
available as the name. There is no state where the API knows WHICH item is selected but not what it
|
||||
is called. That is why the client-side symptom looked like a naming problem: an unlabelled badge.
|
||||
The real harm is one level down — the selected id is null too, and an editor that round-trips it
|
||||
silently clears the stored selection. #651 added a client-side merge-instead-of-replace guard that
|
||||
made this survivable; that guard stays, but it was a client-side patch over a server-side defect.
|
||||
|
||||
The corollary is the rule: never let the id and the name share a single point of failure. Hence the
|
||||
`_ => null` ban — an unrecognized subtype surrenders its NAME, never its ID.
|
||||
|
||||
## Why fail-soft rather than a throw
|
||||
|
||||
The Done-when asked for the unhandled case to be handled or made loud. A throw is loud but fails an
|
||||
entire paged GET over one bad row, on a read path. A conspicuous placeholder name is loud in the UI,
|
||||
greppable in logs, and preserves the id — the thing that actually matters. Recorded because a future
|
||||
session could reasonably reach for `NotSupportedException` here, as the sibling
|
||||
`ProgramSchedules.Mapper` item-type switch does; that one dispatches on the ITEM type, an internal
|
||||
closed set, where a throw is correct.
|
||||
|
||||
## Scope deliberately not widened
|
||||
|
||||
Nine further media-item switches exist (four in `ProgramSchedules.Mapper`, five in
|
||||
`Scheduling.Mapper`) and all handle only Show/Season/Artist. That is NOT the same oversight: those
|
||||
call sites genuinely restrict selection to those three types, and their handlers load a matching
|
||||
chain. Only the RerunCollection and PlaylistItem switches span the full set, which is why exactly
|
||||
those two were merged. `GetPlaylistItemsHandler` already loaded the full graph, so PlaylistItem was
|
||||
never broken in production — it shared the latent `RemoteStream` gap and now shares the fix.
|
||||
|
||||
## Verification worth repeating
|
||||
|
||||
Each mechanism was removed in turn and the suite quoted red before the fix was restored: stripping
|
||||
the list include chain failed all 13 types with "lost its selected id"; restoring the original
|
||||
four-type by-id chain failed exactly the six the issue named; and reverting only the three bare
|
||||
dereferences reproduced the `NullReferenceException` for Episode and MusicVideo. A green new test
|
||||
over a read path proves little until the mechanism it covers has been shown to fail without it.
|
||||
Reference in New Issue
Block a user