fix(671): review round 2 -- guard Song.Artists, cover the second consumer
Cold independent review of 017ef988d. Three findings taken, one filed out.
The important one is a regression this branch INTRODUCED. `SongMetadata.Artists`
is a nullable EF primitive collection (JSON in one column, not a navigation)
that FallbackMetadataProvider leaves unassigned when a song's tags fail to read,
and `string.Join` throws ArgumentNullException on a null sequence. The rerun
list previously did not load SongMetadata at all, so the throw was unreachable
there; adding the include promoted it to a live 500 that would have failed the
whole page. Confirmed by reverting the guard: ArgumentNullException, parameter
'values'. The file header claiming every member was guarded was false.
The empty case is filtered too, so an artist-less song loses its bare " - ".
Second: `GetPlaylistItemsHandler` had no handler-level test at all (its
controller tests stub the mediator), so the RemoteStream include added last
round was discharged by inspection -- the same method that produced #671. It
now runs the same 13-type matrix via a shared SelectionSeedData; removing the
include fails that matrix.
Third: dropped the dead `(i as Season).SeasonMetadata` include leg -- the Season
projection reads Show.ShowMetadata and the scalar SeasonNumber, never
SeasonMetadata.
Filed #690 for the pre-existing, out-of-scope finding: the paged TotalCount
ignores the search query, so the SPA renders empty pages.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -28,8 +28,8 @@ internal static class RerunCollectionQueryExtensions
|
|||||||
.ThenInclude(i => (i as Movie).MovieMetadata)
|
.ThenInclude(i => (i as Movie).MovieMetadata)
|
||||||
.Include(c => c.MediaItem)
|
.Include(c => c.MediaItem)
|
||||||
.ThenInclude(i => (i as Show).ShowMetadata)
|
.ThenInclude(i => (i as Show).ShowMetadata)
|
||||||
.Include(c => c.MediaItem)
|
// No (i as Season).SeasonMetadata leg on purpose: ProjectToViewModel(Season) builds its name
|
||||||
.ThenInclude(i => (i as Season).SeasonMetadata)
|
// from Show.ShowMetadata and the scalar SeasonNumber, and never reads SeasonMetadata.
|
||||||
.Include(c => c.MediaItem)
|
.Include(c => c.MediaItem)
|
||||||
.ThenInclude(i => (i as Season).Show)
|
.ThenInclude(i => (i as Season).Show)
|
||||||
.ThenInclude(s => s.ShowMetadata)
|
.ThenInclude(s => s.ShowMetadata)
|
||||||
|
|||||||
@@ -127,8 +127,14 @@ internal static class Mapper
|
|||||||
|
|
||||||
private static string SongTitle(Song s)
|
private static string SongTitle(Song s)
|
||||||
{
|
{
|
||||||
|
// Artists is a NULLABLE primitive collection, not a navigation: a song whose tags failed to read
|
||||||
|
// is persisted by FallbackMetadataProvider with Artists never assigned, and string.Join throws
|
||||||
|
// ArgumentNullException on a null sequence. Filtering the empty case too avoids prefixing an
|
||||||
|
// artist-less song with a bare " - ".
|
||||||
string songArtist = Optional(s.SongMetadata).Flatten().HeadOrNone()
|
string songArtist = Optional(s.SongMetadata).Flatten().HeadOrNone()
|
||||||
.Map(sm => $"{string.Join(", ", sm.Artists)} - ")
|
.Map(sm => Optional(sm.Artists).Flatten().ToList())
|
||||||
|
.Filter(artists => artists.Count > 0)
|
||||||
|
.Map(artists => $"{string.Join(", ", artists)} - ")
|
||||||
.IfNone(string.Empty);
|
.IfNone(string.Empty);
|
||||||
return Optional(s.SongMetadata).Flatten().HeadOrNone()
|
return Optional(s.SongMetadata).Flatten().HeadOrNone()
|
||||||
.Map(sm => $"{songArtist}{sm.Title ?? string.Empty}")
|
.Map(sm => $"{songArtist}{sm.Title ?? string.Empty}")
|
||||||
|
|||||||
@@ -0,0 +1,89 @@
|
|||||||
|
using ErsatzTV.Application.MediaCollections;
|
||||||
|
using ErsatzTV.Core.Domain;
|
||||||
|
using ErsatzTV.Infrastructure.Data;
|
||||||
|
using ErsatzTV.Tests.Support;
|
||||||
|
using NUnit.Framework;
|
||||||
|
using Shouldly;
|
||||||
|
|
||||||
|
namespace ErsatzTV.Tests.Application.MediaCollections;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The second consumer of the shared <c>ProjectMediaItemToViewModel</c> switch (issue #671).
|
||||||
|
/// <c>GetPlaylistItemsHandler</c> had no handler-level test — the controller tests stub the mediator
|
||||||
|
/// and never execute the query — so the only symptom of a missing include here was a silent "???"
|
||||||
|
/// name that nothing in the suite could see. Widening the shared switch with a RemoteStream arm
|
||||||
|
/// obliged this handler to gain a matching include; proving that by inspection would have repeated
|
||||||
|
/// the very method that produced #671, so it gets the same full matrix the rerun handlers get.
|
||||||
|
/// </summary>
|
||||||
|
[TestFixture]
|
||||||
|
public class GetPlaylistItemsHandlerTests : MediaCollectionHandlerTestBase
|
||||||
|
{
|
||||||
|
private static IEnumerable<CollectionType> SupportedSelectionTypes => SelectionSeedData.SupportedSelectionTypes;
|
||||||
|
|
||||||
|
[TestCaseSource(nameof(SupportedSelectionTypes))]
|
||||||
|
public async Task GetPlaylistItems_Should_Resolve_The_Selection(CollectionType collectionType)
|
||||||
|
{
|
||||||
|
await SeedSelection(collectionType);
|
||||||
|
await SeedPlaylistItem(collectionType);
|
||||||
|
|
||||||
|
var handler = new GetPlaylistItemsHandler(Db.Factory);
|
||||||
|
|
||||||
|
List<PlaylistItemViewModel> items =
|
||||||
|
await handler.Handle(new GetPlaylistItems(1), CancellationToken.None);
|
||||||
|
|
||||||
|
items.Count.ShouldBe(1);
|
||||||
|
|
||||||
|
PlaylistItemViewModel item = items[0];
|
||||||
|
|
||||||
|
int? selectedId = item.Collection?.Id
|
||||||
|
?? item.MultiCollection?.Id
|
||||||
|
?? item.SmartCollection?.Id
|
||||||
|
?? item.MediaItem?.MediaItemId;
|
||||||
|
|
||||||
|
string selectedName = item.Collection?.Name
|
||||||
|
?? item.MultiCollection?.Name
|
||||||
|
?? item.SmartCollection?.Name
|
||||||
|
?? item.MediaItem?.Name;
|
||||||
|
|
||||||
|
selectedId.ShouldBe(SelectionSeedData.SelectedId, $"{collectionType} lost its selected id");
|
||||||
|
selectedName.ShouldBe(
|
||||||
|
SelectionSeedData.ExpectedName(collectionType),
|
||||||
|
$"{collectionType} projected the wrong name");
|
||||||
|
}
|
||||||
|
|
||||||
|
private async Task SeedSelection(CollectionType collectionType)
|
||||||
|
{
|
||||||
|
await using TvContext context = Db.CreateContext();
|
||||||
|
await SelectionSeedData.SeedSelection(context, collectionType);
|
||||||
|
}
|
||||||
|
|
||||||
|
private async Task SeedPlaylistItem(CollectionType collectionType)
|
||||||
|
{
|
||||||
|
await using TvContext context = Db.CreateContext();
|
||||||
|
|
||||||
|
var item = new PlaylistItem
|
||||||
|
{
|
||||||
|
Id = 1,
|
||||||
|
Index = 0,
|
||||||
|
PlaylistId = 1,
|
||||||
|
CollectionType = collectionType,
|
||||||
|
PlaybackOrder = PlaybackOrder.Chronological
|
||||||
|
};
|
||||||
|
|
||||||
|
SelectionSeedData.ApplySelection(
|
||||||
|
collectionType,
|
||||||
|
v => item.CollectionId = v,
|
||||||
|
v => item.MultiCollectionId = v,
|
||||||
|
v => item.SmartCollectionId = v,
|
||||||
|
v => item.MediaItemId = v);
|
||||||
|
|
||||||
|
context.Playlists.Add(new Playlist
|
||||||
|
{
|
||||||
|
Id = 1,
|
||||||
|
Name = "Playlist",
|
||||||
|
Items = [item]
|
||||||
|
});
|
||||||
|
|
||||||
|
await context.SaveChangesAsync();
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -1,5 +1,4 @@
|
|||||||
using ErsatzTV.Application.MediaCollections;
|
using ErsatzTV.Application.MediaCollections;
|
||||||
using ErsatzTV.Controllers.Api.Requests;
|
|
||||||
using ErsatzTV.Core.Domain;
|
using ErsatzTV.Core.Domain;
|
||||||
using ErsatzTV.Infrastructure.Data;
|
using ErsatzTV.Infrastructure.Data;
|
||||||
using ErsatzTV.Tests.Support;
|
using ErsatzTV.Tests.Support;
|
||||||
@@ -12,23 +11,18 @@ namespace ErsatzTV.Tests.Application.MediaCollections;
|
|||||||
/// <summary>
|
/// <summary>
|
||||||
/// Read-path coverage for the two rerun-collection query handlers (issue #671). The defect was
|
/// 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
|
/// 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
|
/// the by-id handler loaded metadata for only four of the ten media types. So the matrix is derived
|
||||||
/// derived from the production predicate rather than hand-listed — a newly-supported
|
/// from the production predicate (see <see cref="SelectionSeedData" />) rather than hand-listed.
|
||||||
/// <see cref="CollectionType" /> joins it automatically and fails loudly in <c>SeedSelection</c>
|
|
||||||
/// until someone teaches the suite how to seed it.
|
|
||||||
/// </summary>
|
/// </summary>
|
||||||
[TestFixture]
|
[TestFixture]
|
||||||
public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
|
public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
|
||||||
{
|
{
|
||||||
private const int SelectedId = 42;
|
private static IEnumerable<CollectionType> SupportedSelectionTypes => SelectionSeedData.SupportedSelectionTypes;
|
||||||
|
|
||||||
private static IEnumerable<CollectionType> SupportedSelectionTypes =>
|
|
||||||
Enum.GetValues<CollectionType>().Where(RerunCollectionRequestMapping.IsSupportedSelectionType);
|
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Completeness guard. Without it, a change that narrowed <c>IsSupportedSelectionType</c> would
|
/// 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
|
/// shrink the matrix silently and every remaining case would still pass — the "filters on the
|
||||||
/// property it asserts" failure mode.
|
/// property it asserts" failure mode. Set equality, so it fails on widening too.
|
||||||
/// </summary>
|
/// </summary>
|
||||||
[Test]
|
[Test]
|
||||||
public void Supported_Selection_Types_Should_Be_The_Full_Documented_Set()
|
public void Supported_Selection_Types_Should_Be_The_Full_Documented_Set()
|
||||||
@@ -84,30 +78,37 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// The exact projected name per type. Pinning the whole string — rather than merely asserting
|
/// <c>SongMetadata.Artists</c> is a NULLABLE primitive collection, and a song whose tags failed to
|
||||||
/// "not a placeholder" — is what makes a missing NESTED include leg visible: dropping
|
/// read is persisted with it never assigned. Before #671 the rerun list did not load SongMetadata
|
||||||
/// Episode → Season → Show still yields the non-placeholder "s00e04 - Selected episode", and
|
/// at all, so this was unreachable there; eager-loading it made a latent `string.Join` throw into a
|
||||||
/// dropping MusicVideo → Artist still yields "Selected music video". Both would sail past a
|
/// live 500 that would take down the whole page.
|
||||||
/// looser assertion while having lost real data.
|
|
||||||
/// </summary>
|
/// </summary>
|
||||||
private static string ExpectedName(CollectionType collectionType) =>
|
[TestCase(null, "Selected song", TestName = "GetById_Song_With_Null_Artists_Should_Not_Throw")]
|
||||||
collectionType switch
|
[TestCase(new string[] { }, "Selected song", TestName = "GetById_Song_With_No_Artists_Should_Not_Prefix")]
|
||||||
|
public async Task GetById_Should_Tolerate_Song_Artists(string[] artists, string expectedName)
|
||||||
|
{
|
||||||
|
await using (TvContext context = Db.CreateContext())
|
||||||
{
|
{
|
||||||
CollectionType.Collection => "Selected collection",
|
context.Songs.Add(new Song
|
||||||
CollectionType.MultiCollection => "Selected multi collection",
|
{
|
||||||
CollectionType.SmartCollection => "Selected smart collection",
|
Id = SelectionSeedData.SelectedId,
|
||||||
CollectionType.TelevisionShow => "Selected show (2020)",
|
SongMetadata = [new SongMetadata { Title = "Selected song", Artists = artists?.ToList() }]
|
||||||
CollectionType.TelevisionSeason => "Parent show (2020) - Season 3",
|
});
|
||||||
CollectionType.Artist => "Selected artist",
|
await context.SaveChangesAsync();
|
||||||
CollectionType.Movie => "Selected movie (2019)",
|
}
|
||||||
CollectionType.Episode => "Episode's show - s02e04 - Selected episode",
|
|
||||||
CollectionType.MusicVideo => "Video's artist - Selected music video",
|
await SeedRerunCollection(1, CollectionType.Song);
|
||||||
CollectionType.OtherVideo => "Selected other video",
|
|
||||||
CollectionType.Song => "Song artist - Selected song",
|
var handler = new GetRerunCollectionByIdHandler(Db.Factory);
|
||||||
CollectionType.Image => "Selected image",
|
|
||||||
CollectionType.RemoteStream => "Selected remote stream",
|
Option<RerunCollectionViewModel> result =
|
||||||
_ => throw new AssertionException($"No expected name pinned for {collectionType}")
|
await handler.Handle(new GetRerunCollectionById(1), CancellationToken.None);
|
||||||
};
|
|
||||||
|
RerunCollectionViewModel vm = result.IfNone(() => throw new AssertionException("Expected a result"));
|
||||||
|
vm.MediaItem.ShouldNotBeNull();
|
||||||
|
vm.MediaItem.MediaItemId.ShouldBe(SelectionSeedData.SelectedId);
|
||||||
|
vm.MediaItem.Name.ShouldBe(expectedName);
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// Mirrors <c>RerunCollectionController.ProjectToResponseModel</c>, which flattens the tagged
|
/// Mirrors <c>RerunCollectionController.ProjectToResponseModel</c>, which flattens the tagged
|
||||||
@@ -127,138 +128,16 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
|
|||||||
?? vm.SmartCollection?.Name
|
?? vm.SmartCollection?.Name
|
||||||
?? vm.MediaItem?.Name;
|
?? vm.MediaItem?.Name;
|
||||||
|
|
||||||
selectedId.ShouldBe(SelectedId, $"{collectionType} lost its selected id");
|
selectedId.ShouldBe(SelectionSeedData.SelectedId, $"{collectionType} lost its selected id");
|
||||||
selectedName.ShouldBe(ExpectedName(collectionType), $"{collectionType} projected the wrong name");
|
selectedName.ShouldBe(
|
||||||
|
SelectionSeedData.ExpectedName(collectionType),
|
||||||
|
$"{collectionType} projected the wrong name");
|
||||||
}
|
}
|
||||||
|
|
||||||
private async Task SeedSelection(CollectionType collectionType)
|
private async Task SeedSelection(CollectionType collectionType)
|
||||||
{
|
{
|
||||||
await using TvContext context = Db.CreateContext();
|
await using TvContext context = Db.CreateContext();
|
||||||
|
await SelectionSeedData.SeedSelection(context, collectionType);
|
||||||
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 = ["Song artist"] }]
|
|
||||||
});
|
|
||||||
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)
|
private async Task SeedRerunCollection(int id, CollectionType collectionType)
|
||||||
@@ -274,21 +153,12 @@ public class RerunCollectionQueryHandlerTests : MediaCollectionHandlerTestBase
|
|||||||
RerunPlaybackOrder = PlaybackOrder.Chronological
|
RerunPlaybackOrder = PlaybackOrder.Chronological
|
||||||
};
|
};
|
||||||
|
|
||||||
switch (collectionType)
|
SelectionSeedData.ApplySelection(
|
||||||
{
|
collectionType,
|
||||||
case CollectionType.Collection:
|
v => rerunCollection.CollectionId = v,
|
||||||
rerunCollection.CollectionId = SelectedId;
|
v => rerunCollection.MultiCollectionId = v,
|
||||||
break;
|
v => rerunCollection.SmartCollectionId = v,
|
||||||
case CollectionType.MultiCollection:
|
v => rerunCollection.MediaItemId = v);
|
||||||
rerunCollection.MultiCollectionId = SelectedId;
|
|
||||||
break;
|
|
||||||
case CollectionType.SmartCollection:
|
|
||||||
rerunCollection.SmartCollectionId = SelectedId;
|
|
||||||
break;
|
|
||||||
default:
|
|
||||||
rerunCollection.MediaItemId = SelectedId;
|
|
||||||
break;
|
|
||||||
}
|
|
||||||
|
|
||||||
context.RerunCollections.Add(rerunCollection);
|
context.RerunCollections.Add(rerunCollection);
|
||||||
await context.SaveChangesAsync();
|
await context.SaveChangesAsync();
|
||||||
|
|||||||
@@ -0,0 +1,207 @@
|
|||||||
|
using ErsatzTV.Controllers.Api.Requests;
|
||||||
|
using ErsatzTV.Core.Domain;
|
||||||
|
using ErsatzTV.Infrastructure.Data;
|
||||||
|
using NUnit.Framework;
|
||||||
|
|
||||||
|
namespace ErsatzTV.Tests.Support;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// One selection-type matrix, shared by every fixture that exercises a tagged-union selection
|
||||||
|
/// (rerun collections and playlist items). Both consumers of
|
||||||
|
/// <c>MediaCollections.Mapper.ProjectMediaItemToViewModel</c> are proved against the SAME data, so
|
||||||
|
/// widening the shared switch cannot be discharged for the second consumer by inspection alone —
|
||||||
|
/// which is the method that produced #671 in the first place.
|
||||||
|
/// </summary>
|
||||||
|
internal static class SelectionSeedData
|
||||||
|
{
|
||||||
|
public const int SelectedId = 42;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Derived from production rather than hand-listed, so a newly-supported type joins the matrix
|
||||||
|
/// automatically and trips the <c>default:</c> arms below until someone teaches them about it.
|
||||||
|
/// </summary>
|
||||||
|
public static IEnumerable<CollectionType> SupportedSelectionTypes =>
|
||||||
|
Enum.GetValues<CollectionType>().Where(RerunCollectionRequestMapping.IsSupportedSelectionType);
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The exact projected name per type. Pinning the whole string — rather than merely asserting
|
||||||
|
/// "not a placeholder" — is what makes a missing NESTED include leg visible: dropping
|
||||||
|
/// Episode → Season → Show still yields the placeholder-free "s??e04 - Selected episode", and
|
||||||
|
/// dropping MusicVideo → Artist still yields "Selected music video". Both would sail past a
|
||||||
|
/// looser assertion while having lost real data.
|
||||||
|
/// </summary>
|
||||||
|
public static string ExpectedName(CollectionType collectionType) =>
|
||||||
|
collectionType switch
|
||||||
|
{
|
||||||
|
CollectionType.Collection => "Selected collection",
|
||||||
|
CollectionType.MultiCollection => "Selected multi collection",
|
||||||
|
CollectionType.SmartCollection => "Selected smart collection",
|
||||||
|
CollectionType.TelevisionShow => "Selected show (2020)",
|
||||||
|
CollectionType.TelevisionSeason => "Parent show (2020) - Season 3",
|
||||||
|
CollectionType.Artist => "Selected artist",
|
||||||
|
CollectionType.Movie => "Selected movie (2019)",
|
||||||
|
CollectionType.Episode => "Episode's show - s02e04 - Selected episode",
|
||||||
|
CollectionType.MusicVideo => "Video's artist - Selected music video",
|
||||||
|
CollectionType.OtherVideo => "Selected other video",
|
||||||
|
CollectionType.Song => "Song artist - Selected song",
|
||||||
|
CollectionType.Image => "Selected image",
|
||||||
|
CollectionType.RemoteStream => "Selected remote stream",
|
||||||
|
_ => throw new AssertionException($"No expected name pinned for {collectionType}")
|
||||||
|
};
|
||||||
|
|
||||||
|
public static async Task SeedSelection(TvContext context, CollectionType collectionType)
|
||||||
|
{
|
||||||
|
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 = ["Song artist"] }]
|
||||||
|
});
|
||||||
|
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 selection type but this suite does not know how " +
|
||||||
|
"to seed it — teach SeedSelection about it rather than narrowing the matrix.");
|
||||||
|
}
|
||||||
|
|
||||||
|
await context.SaveChangesAsync();
|
||||||
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Assigns the one foreign key the tagged union uses for this type. Shared so the rerun and
|
||||||
|
/// playlist fixtures cannot disagree about which slot a type occupies.
|
||||||
|
/// </summary>
|
||||||
|
public static void ApplySelection(
|
||||||
|
CollectionType collectionType,
|
||||||
|
Action<int> setCollectionId,
|
||||||
|
Action<int> setMultiCollectionId,
|
||||||
|
Action<int> setSmartCollectionId,
|
||||||
|
Action<int> setMediaItemId)
|
||||||
|
{
|
||||||
|
switch (collectionType)
|
||||||
|
{
|
||||||
|
case CollectionType.Collection:
|
||||||
|
setCollectionId(SelectedId);
|
||||||
|
break;
|
||||||
|
case CollectionType.MultiCollection:
|
||||||
|
setMultiCollectionId(SelectedId);
|
||||||
|
break;
|
||||||
|
case CollectionType.SmartCollection:
|
||||||
|
setSmartCollectionId(SelectedId);
|
||||||
|
break;
|
||||||
|
default:
|
||||||
|
setMediaItemId(SelectedId);
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -174,6 +174,13 @@ whose include chains differ, so every metadata navigation is read through `Optio
|
|||||||
and degrades to the `"???"` placeholder rather than throwing. A bare `x.Season.Show.ShowMetadata`
|
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.
|
inside a projection is a latent 500 on some other caller's GET.
|
||||||
|
|
||||||
|
**And it is not only navigations.** `SongMetadata.Artists` is a nullable EF *primitive collection*
|
||||||
|
(a JSON array in one column), which `FallbackMetadataProvider` leaves unassigned for a song whose
|
||||||
|
tags failed to read — and `string.Join` throws `ArgumentNullException` on a null sequence, not a
|
||||||
|
`NullReferenceException`. Adding an include is therefore not automatically safe: it can promote a
|
||||||
|
latent throw on a previously-unloaded member into a live 500 that fails the whole page. When you
|
||||||
|
widen an include chain, audit what the newly-reachable projection dereferences.
|
||||||
|
|
||||||
## 3. Error mapping
|
## 3. Error mapping
|
||||||
|
|
||||||
Central helper: `ErsatzTV/Extensions/ApiResults.cs`. Use these extension methods instead of
|
Central helper: `ErsatzTV/Extensions/ApiResults.cs`. Use these extension methods instead of
|
||||||
|
|||||||
@@ -46,7 +46,24 @@ was never broken in production; adding the shared `RemoteStream` arm did oblige
|
|||||||
matching `RemoteStreamMetadata` include, or that name alone would have degraded to `"???"`.
|
matching `RemoteStreamMetadata` include, or that name alone would have degraded to `"???"`.
|
||||||
|
|
||||||
**Widening a shared switch incurs a debt in every caller loading for it.** Adding an arm is not
|
**Widening a shared switch incurs a debt in every caller loading for it.** Adding an arm is not
|
||||||
free: each consumer must be re-checked against the navigations the new arm dereferences.
|
free: each consumer must be re-checked against the navigations the new arm dereferences — and
|
||||||
|
discharged by a TEST, not by inspection, since inspection is the method that produced this bug.
|
||||||
|
`GetPlaylistItemsHandler` had no handler-level test at all (its controller tests stub the mediator),
|
||||||
|
so it gained the same 13-type matrix via the shared `SelectionSeedData`.
|
||||||
|
|
||||||
|
## Adding an include can CREATE a 500
|
||||||
|
|
||||||
|
Eager-loading is not automatically safe. `SongMetadata.Artists` is a nullable EF **primitive
|
||||||
|
collection** — a JSON array in one column, not a navigation — that `FallbackMetadataProvider` leaves
|
||||||
|
unassigned whenever a song's tags fail to read, and `string.Join` throws `ArgumentNullException` (not
|
||||||
|
`NullReferenceException`) on a null sequence. The rerun list previously did not load `SongMetadata`
|
||||||
|
at all, so the throw was unreachable there; adding the include promoted it to a live 500 that would
|
||||||
|
have failed the entire page. The guard is `Optional(sm.Artists).Flatten()`, and the empty case is
|
||||||
|
filtered too so an artist-less song is not prefixed with a bare `" - "`.
|
||||||
|
|
||||||
|
The general rule: when you widen an include chain, audit what the newly-reachable projection
|
||||||
|
dereferences. A "null navigation" audit is not enough — nullable primitive collections throw a
|
||||||
|
different exception type and will not turn up in a grep for `NullReferenceException`.
|
||||||
|
|
||||||
## Verification worth repeating
|
## Verification worth repeating
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user