diff --git a/ErsatzTV.Application/Playouts/Mapper.cs b/ErsatzTV.Application/Playouts/Mapper.cs index c869aaddb..876b8a11d 100644 --- a/ErsatzTV.Application/Playouts/Mapper.cs +++ b/ErsatzTV.Application/Playouts/Mapper.cs @@ -1,5 +1,6 @@ using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Application.Playouts; @@ -40,9 +41,15 @@ internal static class Mapper programScheduleAlternate.Id, programScheduleAlternate.Index, programScheduleAlternate.ProgramScheduleId, - programScheduleAlternate.DaysOfWeek, - programScheduleAlternate.DaysOfMonth, - programScheduleAlternate.MonthsOfYear, + // ersatztv#823: these three are NULLABLE columns and a legacy row can hold NULL. Substitute the + // SAME unrestricted defaults AlternateScheduleSelector.GetScheduleForDate reads, so the DTO the + // SPA renders agrees with what actually gets scheduled -- web/src/screens/playoutTemplateCalendar.ts + // `appliesToDate` is an exact port of that method, and it would otherwise both mispreview and + // throw (`[...template.daysOfMonth]` on a null is a TypeError). Never assigned back onto the + // entity (`media.nullable-primitive-collection-mutation`). + programScheduleAlternate.DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(), + programScheduleAlternate.DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(), + programScheduleAlternate.MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(), programScheduleAlternate.LimitToDateRange, programScheduleAlternate.StartMonth, programScheduleAlternate.StartDay, diff --git a/ErsatzTV.Application/Scheduling/Mapper.cs b/ErsatzTV.Application/Scheduling/Mapper.cs index 91e6356b5..4ae54728c 100644 --- a/ErsatzTV.Application/Scheduling/Mapper.cs +++ b/ErsatzTV.Application/Scheduling/Mapper.cs @@ -1,6 +1,7 @@ -using ErsatzTV.Application.Tree; +using ErsatzTV.Application.Tree; using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; namespace ErsatzTV.Application.Scheduling; @@ -190,9 +191,15 @@ internal static class Mapper ProjectToViewModel(playoutTemplate.Template), ProjectToViewModel(playoutTemplate.DecoTemplate), playoutTemplate.Index, - playoutTemplate.DaysOfWeek, - playoutTemplate.DaysOfMonth, - playoutTemplate.MonthsOfYear, + // ersatztv#823: these three are NULLABLE columns and a legacy row can hold NULL. Substitute the + // SAME unrestricted defaults AlternateScheduleSelector.GetScheduleForDate reads, so the DTO the + // SPA renders agrees with what actually gets scheduled -- web/src/screens/playoutTemplateCalendar.ts + // `appliesToDate` is an exact port of that method, and it would otherwise both mispreview and + // throw (`[...template.daysOfMonth]` on a null is a TypeError). Never assigned back onto the + // entity (`media.nullable-primitive-collection-mutation`). + playoutTemplate.DaysOfWeek ?? AlternateScheduleSelector.AllDaysOfWeek(), + playoutTemplate.DaysOfMonth ?? AlternateScheduleSelector.AllDaysOfMonth(), + playoutTemplate.MonthsOfYear ?? AlternateScheduleSelector.AllMonthsOfYear(), playoutTemplate.LimitToDateRange, playoutTemplate.StartMonth, playoutTemplate.StartDay, diff --git a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs index 34af2a3f9..4c584ee8b 100644 --- a/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs +++ b/ErsatzTV.Core.Tests/Scheduling/AlternateScheduleSelectorTests.cs @@ -1,3 +1,4 @@ +using ErsatzTV.Core.Domain; using ErsatzTV.Core.Domain.Scheduling; using ErsatzTV.Core.Scheduling; using NUnit.Framework; @@ -864,4 +865,241 @@ public static class AlternateScheduleSelectorTests result.IsNone.ShouldBeFalse(); } } + + /// + /// ersatztv#823. DaysOfWeek, DaysOfMonth and MonthsOfYear on + /// and are six + /// single-column primitive collections whose columns are nullable: true on both providers. + /// A NULL column materializes as CLR null — EF does not invoke the value converter for a + /// NULL at all — so unguarded, each .Contains in + /// throws + /// . These tests are RED without the read-site guard. + /// + /// A null reads as UNRESTRICTED (the All*() sets), not as empty. The deciding case is + /// SQLite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth, which adds the column + /// nullable: true with NO default: a row inserted before it had no day-of-month + /// restriction, so reading its NULL as empty would INVERT its meaning and silently stop the + /// template applying. That is the one NULL reachable without any code writing one. + /// + /// + /// Reachability itself is pinned by + /// ErsatzTV.Tests.Integration.SchedulingCollectionColumnNullTests against a real + /// TvContext; these tests pin what the selector does once the null is there. + /// + /// + [TestFixture] + public class GetScheduleForDate_NullCollections + { + private static readonly TimeSpan Offset = TimeSpan.FromHours(-5); + + // A Wednesday in March, so no All*() member is coincidentally excluded — and deliberately the + // 20th rather than the 6th. With a day <= 12 a CROSS-WIRED substitution survives the whole + // fixture: `DaysOfMonth ?? AllMonthsOfYear()` hands back 1..12, which still contains day 6, so + // every assertion here passes while the guard substitutes the wrong set. Day 20 is outside 1..12 + // and kills it. + private static readonly DateTimeOffset AnyDate = new(2024, 3, 20, 0, 0, 0, Offset); + + private static PlayoutTemplate Unrestricted() => + new() + { + DaysOfWeek = AlternateScheduleSelector.AllDaysOfWeek(), + DaysOfMonth = AlternateScheduleSelector.AllDaysOfMonth(), + MonthsOfYear = AlternateScheduleSelector.AllMonthsOfYear() + }; + + private static Option Select(params PlayoutTemplate[] templates) => + AlternateScheduleSelector.GetScheduleForDate(templates.ToList(), AnyDate); + + [Test] + public void Null_DaysOfWeek_Reads_As_Unrestricted() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = null!; + + Select(template).IsSome.ShouldBeTrue( + "a NULL DaysOfWeek means no weekday restriction was recorded, so the template still applies"); + } + + [Test] + public void Null_DaysOfMonth_Reads_As_Unrestricted() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfMonth = null!; + + Select(template).IsSome.ShouldBeTrue(); + } + + [Test] + public void Null_MonthsOfYear_Reads_As_Unrestricted() + { + PlayoutTemplate template = Unrestricted(); + template.MonthsOfYear = null!; + + Select(template).IsSome.ShouldBeTrue(); + } + + [Test] + public void All_Three_Null_On_ProgramScheduleAlternate_Reads_As_Unrestricted() + { + var alternate = new ProgramScheduleAlternate + { + DaysOfWeek = null!, + DaysOfMonth = null!, + MonthsOfYear = null! + }; + + AlternateScheduleSelector.GetScheduleForDate( + new List { alternate }, + AnyDate) + .IsSome.ShouldBeTrue(); + } + + /// + /// THE DISCRIMINATING CONTROL. Every test above sets a NULL and expects the item to be selected, + /// so all of them pass equally under "NULL means unrestricted" and under the much broader + /// "any NULL makes this item match unconditionally" — a refactor that short-circuits the whole + /// date check when any dimension is null keeps them green. Here the nulled dimension is paired + /// with a RESTRICTIVE non-null one that the date fails, so only the narrow reading passes. + /// + [Test] + public void A_Null_Dimension_Does_Not_Relax_The_Other_Dimensions() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = null!; + + // AnyDate is in MARCH; restrict to January only. + template.MonthsOfYear = [1]; + + Select(template).IsNone.ShouldBeTrue( + "a NULL DaysOfWeek relaxes ONLY the weekday dimension — the January restriction still " + + "excludes a March date"); + } + + /// + /// One arrangement is not enough: with only the DaysOfWeek case above, a PER-DIMENSION + /// mutant survives the whole fixture — e.g. if (item.MonthsOfYear is null) return item; + /// placed ahead of the checks is never reached by that test, because its MonthsOfYear is + /// non-null. So each of the three dimensions is nulled in turn against a restriction on a + /// DIFFERENT dimension. + /// + [Test] + public void A_Null_MonthsOfYear_Does_Not_Relax_The_Other_Dimensions() + { + PlayoutTemplate template = Unrestricted(); + template.MonthsOfYear = null!; + + // AnyDate is a WEDNESDAY; restrict to Monday only. + template.DaysOfWeek = [DayOfWeek.Monday]; + + Select(template).IsNone.ShouldBeTrue( + "a NULL MonthsOfYear relaxes ONLY the month dimension — the Monday restriction still " + + "excludes a Wednesday"); + } + + /// + /// The third of the per-dimension controls — see + /// for why one + /// arrangement is not enough. Here the nulled dimension is DaysOfMonth and the + /// restriction that must still bite is on MonthsOfYear. + /// + [Test] + public void A_Null_DaysOfMonth_Does_Not_Relax_The_Other_Dimensions() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfMonth = null!; + + // AnyDate is in MARCH; restrict to January only. + template.MonthsOfYear = [1]; + + Select(template).IsNone.ShouldBeTrue( + "a NULL DaysOfMonth relaxes ONLY the day-of-month dimension — the January restriction " + + "still excludes a March date"); + } + + /// + /// A null must not be confused with an explicitly EMPTY collection. Empty is a legal, reachable + /// state meaning "matches no day", and it keeps that meaning — which is exactly why a NULL + /// cannot be normalized to it. + /// + [Test] + public void An_Explicitly_Empty_Collection_Still_Matches_Nothing() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = []; + + Select(template).IsNone.ShouldBeTrue( + "an empty DaysOfWeek is a recorded restriction of NO days, unlike a NULL"); + } + + /// + /// The guard resolves PER ITEM: a null on the first item must not decide the second. Making the + /// nulled item genuinely non-matching is what measures that — with an unrestricted nulled item + /// at index 0 it simply wins on ordering and the second item is never evaluated, so the + /// invariant would go unmeasured while the test passed. + /// + [Test] + public void A_Null_On_One_Item_Does_Not_Decide_A_Later_Item() + { + PlayoutTemplate nulled = Unrestricted(); + nulled.DaysOfWeek = null!; + nulled.MonthsOfYear = [1]; // AnyDate is in March, so this item must NOT match + nulled.Index = 0; + + PlayoutTemplate second = Unrestricted(); + second.Index = 1; + + foreach (PlayoutTemplate selected in Select(nulled, second)) + { + selected.ShouldBeSameAs(second); + return; + } + + Assert.Fail("the loop stopped at the null-collection item instead of continuing to the next"); + } + + /// + /// ...and when the nulled item IS unrestricted it legitimately wins on ordering. Paired with the + /// test above so "index 0 wins" and "the loop continues past a non-matching null item" are + /// separately pinned. + /// + [Test] + public void An_Unrestricted_Null_Item_Wins_On_Index_Order() + { + PlayoutTemplate nulled = Unrestricted(); + nulled.DaysOfWeek = null!; + nulled.Index = 0; + + PlayoutTemplate second = Unrestricted(); + second.Index = 1; + + foreach (PlayoutTemplate selected in Select(nulled, second)) + { + selected.ShouldBeSameAs(nulled); + return; + } + + Assert.Fail("the null-collection item was skipped instead of read as unrestricted"); + } + + /// + /// The read-site guard must not be written BACK onto the item. These are single-column + /// primitive collections, so assigning the guard would flip a tracked entity to + /// Modified and the next SaveChanges would persist the substituted collection + /// over the NULL — the mechanism recorded as media.nullable-primitive-collection-mutation. + /// + [Test] + public void Guard_Must_Not_Be_Written_Back_Onto_The_Item() + { + PlayoutTemplate template = Unrestricted(); + template.DaysOfWeek = null!; + template.DaysOfMonth = null!; + template.MonthsOfYear = null!; + + Select(template); + + template.DaysOfWeek.ShouldBeNull(); + template.DaysOfMonth.ShouldBeNull(); + template.MonthsOfYear.ShouldBeNull(); + } + } } diff --git a/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs b/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs index c9ca6b162..672829d0b 100644 --- a/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs +++ b/ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs @@ -85,19 +85,46 @@ public static class AlternateScheduleSelector } } - bool daysOfWeek = item.DaysOfWeek.Contains(date.DayOfWeek); + // These three are NULLABLE single-column primitive collections, and a runtime null IS + // reachable (ersatztv#823, measured against a real TvContext on SQLite and MySQL 8.4): EF does + // NOT invoke the value converter for a NULL column, so it materializes as CLR null rather than + // through IntCollectionValueConverter's null-to-empty branch, which never runs on this path. + // Unguarded, each .Contains below throws NullReferenceException. + // + // A NULL reads as UNRESTRICTED -- the All*() sets -- NOT as empty. This is the whole semantic + // question and it is decided by the one NULL that is reachable WITHOUT any code writing one: + // Sqlite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth adds DaysOfMonth with + // `nullable: true` and NO defaultValue, so a PlayoutTemplate row inserted before it holds NULL + // and, by construction, had NO day-of-month restriction. Reading that as empty would INVERT + // the row's meaning and silently stop the template applying at all. All*() preserves it, and + // it is how "no restriction recorded" is already represented elsewhere in this domain + // (GetPlayoutAlternateSchedulesHandler, PreviewBlockPlayoutHandler). Note what does NOT decide + // it: the API request records normalize an omitted field with `?? []`, but that is a client + // omitting a field on a WRITE and says nothing about what a legacy database NULL meant. + // + // Guarded at the READ SITE, into locals, and NEVER assigned back onto `item`: the property IS + // the column value, so writing the guard back would flip a tracked entry to Modified and + // persist the substituted collection over the NULL + // (`media.nullable-primitive-collection-mutation`). The matching substitution happens at the + // entity->DTO boundary in the two Mapper.ProjectToViewModel overloads, so the SPA's + // appliesToDate -- an exact port of this method -- previews what this actually schedules. + ICollection itemDaysOfWeek = item.DaysOfWeek ?? AllDaysOfWeek(); + ICollection itemDaysOfMonth = item.DaysOfMonth ?? AllDaysOfMonth(); + ICollection itemMonthsOfYear = item.MonthsOfYear ?? AllMonthsOfYear(); + + bool daysOfWeek = itemDaysOfWeek.Contains(date.DayOfWeek); if (!daysOfWeek) { continue; } - bool daysOfMonth = item.DaysOfMonth.Contains(date.Day); + bool daysOfMonth = itemDaysOfMonth.Contains(date.Day); if (!daysOfMonth) { continue; } - bool monthOfYear = item.MonthsOfYear.Contains(date.Month); + bool monthOfYear = itemMonthsOfYear.Contains(date.Month); if (monthOfYear) { return item; diff --git a/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs new file mode 100644 index 000000000..3a4d613b3 --- /dev/null +++ b/ErsatzTV.Tests/Application/Scheduling/RecurrenceLimitsMapperNullTests.cs @@ -0,0 +1,151 @@ +using ErsatzTV.Application.Playouts; +using ErsatzTV.Application.Scheduling; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Core.Scheduling; +using NUnit.Framework; +using Shouldly; +using PlayoutsMapper = ErsatzTV.Application.Playouts.Mapper; +using SchedulingMapper = ErsatzTV.Application.Scheduling.Mapper; + +namespace ErsatzTV.Tests.Application.Scheduling; + +/// +/// ersatztv#823. Guarding alone would have left the OTHER read +/// of the same six columns unguarded — the entity→view-model mappers, which feed +/// PlayoutController's response models and therefore the SPA. +/// +/// Two things break without the guard, and neither is a C# exception, which is why the selector +/// tests cannot see them. web/src/screens/PlayoutScheduleEditors.tsx spreads the collection +/// (daysOfMonth: [...template.daysOfMonth]) and throws TypeError: not iterable on a +/// JSON null; and web/src/screens/playoutTemplateCalendar.ts's appliesToDate — +/// an exact TypeScript port of — +/// calls .includes on it. +/// +/// +/// So the mappers substitute the SAME unrestricted defaults the selector reads. That agreement is +/// the point: a DTO that said "empty" while the selector scheduled "unrestricted" would make the +/// preview calendar disagree with the playout it is previewing. +/// +/// +[TestFixture] +public class RecurrenceLimitsMapperNullTests +{ + private static Template MinimalTemplate() => + new() + { + Id = 7, + Name = "T", + TemplateGroupId = 1, + TemplateGroup = new TemplateGroup { Name = "G" }, + Items = [] + }; + + [Test] + public void ProgramScheduleAlternate_Null_Collections_Map_To_Unrestricted() + { + var alternate = new ProgramScheduleAlternate + { + Id = 1, + Index = 0, + ProgramScheduleId = 2, + DaysOfWeek = null!, + DaysOfMonth = null!, + MonthsOfYear = null! + }; + + PlayoutAlternateScheduleViewModel vm = PlayoutsMapper.ProjectToViewModel(alternate); + + vm.DaysOfWeek.ShouldBe(AlternateScheduleSelector.AllDaysOfWeek()); + vm.DaysOfMonth.ShouldBe(AlternateScheduleSelector.AllDaysOfMonth()); + vm.MonthsOfYear.ShouldBe(AlternateScheduleSelector.AllMonthsOfYear()); + + // Never assigned back: these are single-column primitive collections, so writing the guard onto a + // tracked entity would persist the substituted set over the NULL + // (media.nullable-primitive-collection-mutation). + alternate.DaysOfWeek.ShouldBeNull(); + alternate.DaysOfMonth.ShouldBeNull(); + alternate.MonthsOfYear.ShouldBeNull(); + } + + [Test] + public void PlayoutTemplate_Null_Collections_Map_To_Unrestricted() + { + var template = new PlayoutTemplate + { + Id = 1, + Index = 0, + Template = MinimalTemplate(), + DecoTemplate = null, + DaysOfWeek = null!, + DaysOfMonth = null!, + MonthsOfYear = null! + }; + + PlayoutTemplateViewModel vm = SchedulingMapper.ProjectToViewModel(template); + + vm.DaysOfWeek.ShouldBe(AlternateScheduleSelector.AllDaysOfWeek()); + vm.DaysOfMonth.ShouldBe(AlternateScheduleSelector.AllDaysOfMonth()); + vm.MonthsOfYear.ShouldBe(AlternateScheduleSelector.AllMonthsOfYear()); + + template.DaysOfWeek.ShouldBeNull(); + template.DaysOfMonth.ShouldBeNull(); + template.MonthsOfYear.ShouldBeNull(); + } + + /// + /// An explicitly EMPTY collection is a recorded restriction of no days and must survive the mapper + /// unchanged. Without this, a guard written as "empty or null becomes All*" would pass the two tests + /// above while silently rewriting real user data on the way out. + /// + /// There is one of these per MAPPER, not one in total. The two overloads are byte-identical + /// triples in different files, so a defensive edit to one alone is exactly the "one helper, two + /// callers" shape this repo has been bitten by: covering only the Playouts mapper would leave + /// the PlayoutTemplate one free to acquire an `empty-or-null` guard with the suite still green. + /// + /// + [Test] + public void An_Explicitly_Empty_Collection_Is_Not_Rewritten_By_The_PlayoutTemplate_Mapper() + { + var template = new PlayoutTemplate + { + Id = 1, + Index = 0, + Template = MinimalTemplate(), + DecoTemplate = null, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [] + }; + + PlayoutTemplateViewModel vm = SchedulingMapper.ProjectToViewModel(template); + + vm.DaysOfWeek.ShouldBeEmpty(); + vm.DaysOfMonth.ShouldBeEmpty(); + vm.MonthsOfYear.ShouldBeEmpty(); + } + + /// + /// The ProgramScheduleAlternate half of the same pair — see the PlayoutTemplate one above + /// for why there is one per MAPPER rather than one in total. + /// + [Test] + public void An_Explicitly_Empty_Collection_Is_Not_Rewritten() + { + var alternate = new ProgramScheduleAlternate + { + Id = 1, + Index = 0, + ProgramScheduleId = 2, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [] + }; + + PlayoutAlternateScheduleViewModel vm = PlayoutsMapper.ProjectToViewModel(alternate); + + vm.DaysOfWeek.ShouldBeEmpty(); + vm.DaysOfMonth.ShouldBeEmpty(); + vm.MonthsOfYear.ShouldBeEmpty(); + } +} diff --git a/ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs b/ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs new file mode 100644 index 000000000..1e82cb022 --- /dev/null +++ b/ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs @@ -0,0 +1,299 @@ +using System.Reflection; +using System.Text; +using Elastic.Clients.Elasticsearch; +using Elastic.Transport; +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Interfaces.Metadata; +using ErsatzTV.Core.Interfaces.Repositories; +using ErsatzTV.Core.Search; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.Extensions; +using ErsatzTV.Infrastructure.Search; +using ErsatzTV.Tests.Support; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; +using NSubstitute; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#824 — the gap ersatztv#701 named rather than papered over. +/// +/// ElasticSearchIndex.UpdateSong holds an INDEPENDENT copy of the logic +/// pins on LuceneSearchIndex. #701 removed +/// metadata.AlbumArtists ??= []; metadata.Artists ??= []; from both, but only Lucene gained +/// a regression test — so reintroducing the mutation in the Elastic copy ALONE left the whole +/// suite green. This fixture closes that: the assertions are the same three, driven through the +/// real ElasticSearchIndex against a real . +/// +/// +/// Why a stubbed transport rather than a live server. #824 listed "inject a non-network +/// ElasticsearchClient transport" as option 1 and it is what shipped: +/// Elastic.Transport.InMemoryRequestInvoker is public in the pinned Elastic.Transport, and +/// ElasticsearchClientSettings(NodePool, IRequestInvoker) accepts it. No server, no socket, +/// no new package. +/// +/// +/// The canned response body is load-bearing, not decoration. A bare +/// InMemoryRequestInvoker() answers with an EMPTY body, which the client cannot deserialize +/// into an IndexResponse. That throw lands in UpdateSong's catch, which logs a +/// warning and assigns metadata.Song = null — so the fixture would measure the ERROR path +/// while every "did not mutate" assertion below still passed, vacuously. The +/// is the belt to that brace: it fails the test if the +/// catch ran at all. +/// +/// +/// The client is injected into the private _client field rather than obtained normally, +/// because CreateClient reads the process-wide static ElasticSearchIndex.Uri and +/// would open a real socket. UpdateItems — unlike IndexExists and +/// Initialize — never runs _client ??= CreateClient(), so the injected instance is +/// the one used and an uninjected one would simply be null. +/// +/// +[TestFixture] +[NonParallelizable] +public class ElasticSongIndexerMetadataMutationTests +{ + private const string TestIndexName = "etv-824-test"; + + private string? _originalIndexName; + + /// + /// ElasticSearchIndex.IndexName is a process-wide static. Only Startup reads it today, + /// so leaving it set leaks nothing that currently runs — but a static this fixture writes and never + /// restores is a cross-test hazard waiting for the first test that does read it. + /// + [SetUp] + public void SetUp() => _originalIndexName = ElasticSearchIndex.IndexName; + + [TearDown] + public void TearDown() => ElasticSearchIndex.IndexName = _originalIndexName; + + /// + /// A well-formed IndexResponse. See the fixture docstring: an empty body diverts the run + /// into UpdateSong's catch and makes every assertion below vacuous. + /// + private const string IndexResponseBody = + """ + {"_index":"etv-824-test","_id":"1","_version":1,"result":"created", + "_shards":{"total":1,"successful":1,"failed":0},"_seq_no":0,"_primary_term":1} + """; + + [Test] + public async Task UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity() + { + await using var harness = await InMemoryTvContext.CreateAsync(); + + int metadataId; + int songId; + await using (TvContext context = harness.CreateContext()) + { + var library = new LocalLibrary { Name = "Music", MediaKind = LibraryMediaKind.Songs }; + context.Add(library); + await context.SaveChangesAsync(); + + var libraryPath = new LibraryPath { Path = "/music", LibraryId = library.Id }; + context.Add(libraryPath); + await context.SaveChangesAsync(); + + var song = new Song + { + LibraryPathId = libraryPath.Id, + MediaVersions = [], + SongMetadata = + [ + new SongMetadata + { + MetadataKind = MetadataKind.Fallback, + Title = "Untagged Track", + SortTitle = "untagged track", + DateAdded = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc), + + // The shape FallbackMetadataProvider.GetSongMetadata leaves behind: it never + // assigns either primitive collection, so both columns persist as NULL. + Artists = null!, + AlbumArtists = null!, + + Genres = [], + Tags = [], + Studios = [], + Actors = [], + Artwork = [], + Guids = [] + } + ] + }; + + context.Add(song); + await context.SaveChangesAsync(); + metadataId = song.SongMetadata[0].Id; + songId = song.Id; + } + + // The seed must actually have produced NULL columns, or every assertion below is vacuous. + (await ReadRawArtists(harness, metadataId)).ShouldBeNull(); + + await using (TvContext context = harness.CreateContext()) + { + // Deliberately TRACKED -- the indexer's own contract is what is being pinned, not the + // AsNoTracking() habit of today's two callers. See SongIndexerMetadataMutationTests. + Song tracked = await context.Songs + .IncludeForSearch() + .AsSplitQuery() + .SingleAsync(); + + SongMetadata metadata = tracked.SongMetadata[0]; + metadata.Artists.ShouldBeNull("EF must materialize the NULL column as null, not as an empty list"); + + var logger = new ThrowOnWarningLogger(); + var index = new ElasticSearchIndex( + new SearchQueryParser( + Substitute.For(), + Substitute.For>()), + logger); + + var invoker = new CapturingRequestInvoker( + new InMemoryRequestInvoker( + Encoding.UTF8.GetBytes(IndexResponseBody), + 200, + exception: null, + contentType: "application/json", + // The X-Elastic-Product header is REQUIRED, not cosmetic. The client runs a product + // check on its first response and throws UnsupportedProductException ("the server is + // not a supported distribution of Elasticsearch") without it -- which lands in + // UpdateSong's catch and makes the fixture measure the error path. Measured: this is + // exactly how this fixture first failed. + headers: ProductCheckHeaders())); + + var settings = new ElasticsearchClientSettings( + new SingleNodePool(new Uri("http://localhost:9200")), + invoker) + .DefaultIndex(TestIndexName); + + ElasticSearchIndex.IndexName = TestIndexName; + + typeof(ElasticSearchIndex) + .GetField("_client", BindingFlags.NonPublic | BindingFlags.Instance)! + .SetValue(index, new ElasticsearchClient(settings)); + + // A bare substitute returns null from GetAllLanguageCodes, which NPEs inside AddLanguages and + // would divert the run into UpdateSong's catch. + var languageCodeService = Substitute.For(); + languageCodeService.GetAllLanguageCodes(Arg.Any>()).Returns([]); + languageCodeService.GetAllLanguageCodes(Arg.Any()).Returns([]); + + await index.UpdateItems( + Substitute.For(), + Substitute.For(), + languageCodeService, + [tracked]); + + // Surfacing the exception rather than asserting ShouldBeNull: the catch is the fixture's + // most likely failure mode (see the canned-response note above), and "expected null but was + // " without the message sends the next reader hunting for a cause the fixture + // already had in its hand. + if (logger.Failure is not null) + { + Assert.Fail("UpdateSong threw and its catch ran, so this probe measured the error path " + + $"rather than the indexing path: {logger.Failure}"); + } + + // POSITIVE CONTROL, and it is not optional: every assertion below says something did NOT + // happen, so all of them hold vacuously if UpdateSong never ran. The Lucene fixture uses + // `writer.NumDocs == 1` for exactly this; the transport-level equivalent is that the indexer + // actually issued the index request for THIS song. Like NumDocs, it proves the song-indexing + // path ran -- it does NOT prove the artist reads specifically ran. + invoker.Requests.Count.ShouldBe( + 1, + "UpdateSong did not issue exactly one index request, so the assertions below would pass " + + $"without exercising the code under test. Captured: [{string.Join(", ", invoker.Requests)}]"); + + // The document id is compared as the LAST PATH SEGMENT, not with ShouldContain. A substring + // test is a false-pass vector here: the index name itself carries digits ("etv-824-test"), so + // ShouldContain("2") or ShouldContain("4") would be satisfied by the index name alone for a + // song whose id happened to be 2 or 4, and the assertion would stop discriminating without + // ever failing. + string path = invoker.Requests[0].Split(' ')[^1].Split('?')[0]; + path.Split('/')[^1].ShouldBe( + songId.ToString(), + $"the index request was not for the seeded song. Captured: {invoker.Requests[0]}"); + + // 1. The indexer left the entity alone. + metadata.Artists.ShouldBeNull(); + metadata.AlbumArtists.ShouldBeNull(); + + // 2. ...so EF has nothing to persist. This is the assertion that fails loudly the day the + // mutation returns, even if a later refactor stopped the value from being observable above. + context.Entry(metadata).State.ShouldBe(EntityState.Unchanged); + + // 3. And the save that a real caller would go on to make does not rewrite the column. + await context.SaveChangesAsync(); + } + + (await ReadRawArtists(harness, metadataId)).ShouldBeNull(); + } + + private static Dictionary> ProductCheckHeaders() => + new(StringComparer.OrdinalIgnoreCase) { ["x-elastic-product"] = ["Elasticsearch"] }; + + /// + /// Records every request the client actually issues, so the fixture can prove the code under test + /// ran. Delegates the answering to a real rather than + /// hand-building a response. + /// + private sealed class CapturingRequestInvoker(InMemoryRequestInvoker inner) : IRequestInvoker + { + public List Requests { get; } = []; + + public ResponseFactory ResponseFactory => inner.ResponseFactory; + + public TResponse Request( + Endpoint endpoint, + BoundConfiguration boundConfiguration, + PostData? postData) + where TResponse : TransportResponse, new() + { + Requests.Add($"{endpoint.Method} {endpoint.PathAndQuery}"); + return inner.Request(endpoint, boundConfiguration, postData); + } + + public Task RequestAsync( + Endpoint endpoint, + BoundConfiguration boundConfiguration, + PostData? postData, + CancellationToken cancellationToken) + where TResponse : TransportResponse, new() + { + Requests.Add($"{endpoint.Method} {endpoint.PathAndQuery}"); + return inner.RequestAsync(endpoint, boundConfiguration, postData, cancellationToken); + } + + // IRequestInvoker extends IDisposable, but InMemoryRequestInvoker holds no disposable state and + // exposes no Dispose of its own -- there is nothing to forward to. + public void Dispose() + { + } + } + + private static async Task ReadRawArtists(InMemoryTvContext harness, int metadataId) + { + await using TvContext context = harness.CreateContext(); + await using var command = context.Database.GetDbConnection().CreateCommand(); + command.CommandText = $"SELECT Artists FROM SongMetadata WHERE Id = {metadataId}"; + object? value = await command.ExecuteScalarAsync(); + + // ExecuteScalar returns CLR null both for "the column is NULL" and for "there is no such row", + // and the second is reachable: UpdateSong's catch assigns metadata.Song = null, which severs a + // required relationship and cascades the row to Deleted, so a SaveChanges on the error path + // DELETES it and a plain null check would pass for the wrong reason. + if (value is null) + { + Assert.Fail($"SongMetadata row {metadataId} no longer exists, so its Artists column cannot " + + "be read -- the probe measured a deleted row rather than a preserved NULL."); + } + + return value is DBNull ? null : value; + } +} diff --git a/ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs b/ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs new file mode 100644 index 000000000..7ed8bb851 --- /dev/null +++ b/ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs @@ -0,0 +1,388 @@ +using ErsatzTV.Core.Domain; +using ErsatzTV.Core.Domain.Scheduling; +using ErsatzTV.Infrastructure; +using ErsatzTV.Infrastructure.Data; +using ErsatzTV.Infrastructure.MySql.Data; +using ErsatzTV.Infrastructure.Sqlite.Data; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging.Abstractions; +using MySqlConnector; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#823 — the question ersatztv#701 split out rather than answered: can a runtime null reach +/// one of the SIX collection-valued scalar columns (DaysOfMonth, MonthsOfYear, +/// DaysOfWeek on and )? +/// +/// Measured, not reasoned about, because the reasoning available beforehand pointed the +/// wrong way. The two converters differ on their read side — +/// IntCollectionValueConverter maps null-or-blank to Array.Empty<int>() while +/// EnumCollectionJsonValueConverter would dereference the result of +/// JsonConvert.DeserializeObject — so the expectation was that the two behave differently +/// on a NULL row. They do not: EF does not invoke a value converter for a NULL column at +/// all, so all six materialize as CLR null and the int converter's null-to-empty +/// branch is dead on this path. That is the measurement this fixture pins. +/// +/// +/// It also pins the WRITE half: assigning null to one of the six and calling +/// SaveChanges SUCCEEDS and stores SQL NULL — the converter is skipped on the way out too. +/// State that precisely, because the overclaim is tempting: this is a property of the CODE, not a +/// live caller. NO caller supplies a null today — every production construction of +/// ReplacePlayoutAlternateSchedule / ReplacePlayoutTemplate goes through the HTTP +/// request records, which normalize with ?? []. What makes it a latent gun is that +/// ReplacePlayoutAlternateScheduleItemsHandler / ReplacePlayoutTemplateItemsHandler +/// assign the command value straight onto the entity, so nothing in the write path itself refuses. +/// +/// +/// The LEGACY route is narrower than "the columns are nullable", and conflating the two is the easy +/// error: all six are nullable: true on both providers, but five of the six were present at +/// CreateTable, so a NULL there still needs code to write one. EXACTLY ONE case is +/// code-path-free — SQLite's 20240113140741_Add_PlayoutTemplate_DaysOfMonth is an +/// AddColumn with nullable: true and NO defaultValue, so PlayoutTemplate +/// rows inserted before it hold NULL, and by construction those rows had no day-of-month +/// restriction. On MySQL PlayoutTemplate arrived whole in +/// 20240114034944_Add_BlockScheduling, so there is no code-path-free NULL for any of the six +/// there. This fixture manufactures its NULL with a raw UPDATE, which is a code path — it +/// measures MATERIALIZATION, and the legacy route above is established by reading the migrations. +/// +/// +/// What the selector then does with it is pinned by +/// ErsatzTV.Core.Tests.Scheduling.AlternateScheduleSelectorTests.GetScheduleForDate_NullCollections: +/// unguarded, .Contains throws . +/// +/// +/// Runs against BOTH providers from ONE fixture body, because the question is about provider +/// materialization and a SQLite-only answer would not have settled it. MySQL needs a live server, +/// supplied via ETV_TEST_MYSQL_CONNECTION; without it the MySQL fixture ignores — a +/// visible skip, never a silent pass. ETV_REQUIRE_MYSQL_TESTS turns that skip into a hard +/// failure for a runner that is supposed to have one. This mirrors +/// , which established the pattern. +/// +/// +/// Cost, stated. Each test migrates a FRESH database (per-test isolation by construction — +/// ersatztv#491 measured what a shared database and a wipe-that-must-succeed cost). Replaying every +/// migration is the expensive part and it is deliberate: a reachability fixture should stand on the +/// SHIPPED schema, not on one EnsureCreated builds from the current model. +/// +/// +[TestFixture(TestProvider.Sqlite)] +[TestFixture(TestProvider.MySql)] +[NonParallelizable] +public class SchedulingCollectionColumnNullTests(TestProvider provider) +{ + private const string MySqlConnectionVariable = "ETV_TEST_MYSQL_CONNECTION"; + private const string MySqlRequiredVariable = "ETV_REQUIRE_MYSQL_TESTS"; + + private string _databasePath = null!; + private string? _mySqlConnectionString; + private DbContextOptions _options = null!; + + private string _originalLastInsertedRowId = null!; + private string _originalCollation = null!; + private bool _originalIsSqlite; + private Func _originalUniqueViolation = null!; + + /// + /// TvContext's provider statics are process-wide, and the MySQL arm sets them to MySQL + /// values. Restoring them is NOT belt-and-braces: — the harness + /// most of this suite uses — resets only three of the five (IsSqlite, + /// IsUniqueConstraintViolation, RegisterUnicodeCaseFunctions) and leaves + /// LastInsertedRowId and CaseInsensitiveCollation alone. So without this, a MySQL arm + /// running before a SQLite test leaves last_insert_id() in place for a SQLite connection, + /// which is an order-dependent failure in a fixture that never mentions MySQL. + /// [NonParallelizable] serialises execution; it does not restore state. + /// + [SetUp] + public async Task SetUp() + { + _originalLastInsertedRowId = TvContext.LastInsertedRowId; + _originalCollation = TvContext.CaseInsensitiveCollation; + _originalIsSqlite = TvContext.IsSqlite; + _originalUniqueViolation = TvContext.IsUniqueConstraintViolation; + + if (provider is TestProvider.Sqlite) + { + TvContext.IsSqlite = true; + TvContext.LastInsertedRowId = "last_insert_rowid()"; + TvContext.CaseInsensitiveCollation = "NOCASE"; + TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation; + + _databasePath = Path.Combine(Path.GetTempPath(), $"etv823-{Guid.NewGuid():N}.sqlite3"); + _options = new DbContextOptionsBuilder() + .UseSqlite( + $"Data Source={_databasePath};Foreign Keys=False", + o => o.MigrationsAssembly("ErsatzTV.Infrastructure.Sqlite")) + .Options; + + await using TvContext sqlite = Create(_options); + await sqlite.Database.MigrateAsync(); + return; + } + + string? baseConnectionString = Environment.GetEnvironmentVariable(MySqlConnectionVariable); + if (string.IsNullOrWhiteSpace(baseConnectionString)) + { + string message = + $"{MySqlConnectionVariable} is not set, so the MySql half of the #823 measurement cannot " + + "run. Whether a NULL column materializes as CLR null is a provider question, so a " + + "SQLite-only answer does not settle it."; + + if (IsTrue(Environment.GetEnvironmentVariable(MySqlRequiredVariable))) + { + Assert.Fail($"{message} {MySqlRequiredVariable} is set, so this is a failure, not a skip."); + } + + Assert.Ignore($"{message} Set it to run this locally."); + } + + // A database of our own, and a FRESH one per test: isolation by construction, per #491's finding + // that a shared name trades isolation for a wipe that has to succeed. + _mySqlConnectionString = + new MySqlConnectionStringBuilder(baseConnectionString) { Database = $"etv823_{Guid.NewGuid():N}" } + .ConnectionString; + + TvContext.IsSqlite = false; + TvContext.LastInsertedRowId = "last_insert_id()"; + TvContext.CaseInsensitiveCollation = "utf8mb4_general_ci"; + TvContext.IsUniqueConstraintViolation = MySqlErrorClassifier.IsUniqueConstraintViolation; + + ServerVersion serverVersion = ServerVersion.AutoDetect(_mySqlConnectionString); + _options = new DbContextOptionsBuilder() + .UseMySql( + _mySqlConnectionString, + serverVersion, + o => o.MigrationsAssembly("ErsatzTV.Infrastructure.MySql")) + .Options; + + // NUnit does not run [TearDown] when [SetUp] throws, and by this point the database exists — the + // migration itself created it. Without this, a migration that fails part way strands a database + // and its connection pool on a SHARED server, once per attempt. + try + { + await using TvContext mysql = Create(_options); + await mysql.Database.MigrateAsync(); + } + catch + { + await DropMySqlDatabase(); + _mySqlConnectionString = null; + throw; + } + } + + private async Task DropMySqlDatabase() + { + try + { + await using (TvContext context = Create(_options)) + { + await context.Database.EnsureDeletedAsync(); + } + + await using var probe = new MySqlConnection(_mySqlConnectionString); + await MySqlConnection.ClearPoolAsync(probe); + } + catch (Exception ex) + { + // Never mask the original failure with a cleanup failure, and never fail a PASSING test in + // teardown because the server went away -- but say so, because a silent cleanup failure is how + // a leak becomes invisible. + await TestContext.Out.WriteLineAsync( + $"WARNING: could not drop the MySql test database {_mySqlConnectionString}: {ex.Message}"); + } + } + + [TearDown] + public async Task TearDown() + { + TvContext.LastInsertedRowId = _originalLastInsertedRowId; + TvContext.CaseInsensitiveCollation = _originalCollation; + TvContext.IsSqlite = _originalIsSqlite; + TvContext.IsUniqueConstraintViolation = _originalUniqueViolation; + + if (provider is TestProvider.Sqlite) + { + Microsoft.Data.Sqlite.SqliteConnection.ClearAllPools(); + foreach (string path in new[] { _databasePath, $"{_databasePath}-wal", $"{_databasePath}-shm" }) + { + if (File.Exists(path)) + { + File.Delete(path); + } + } + + return; + } + + if (_mySqlConnectionString is not null) + { + await DropMySqlDatabase(); + _mySqlConnectionString = null; + } + } + + /// + /// A row whose three columns are NULL materializes as CLR null on every one of them — including + /// the two IntCollectionValueConverter columns, whose converter would have produced + /// Array.Empty<int>() had it been invoked. + /// + [Test] + public async Task A_Null_Column_Materializes_As_Clr_Null_On_All_Six() + { + await using (TvContext context = Create(_options)) + { + await DisableForeignKeys(context); + + context.ProgramScheduleAlternates.Add(NewAlternate(playoutId: 1)); + context.PlayoutTemplates.Add(NewTemplate(playoutId: 1)); + await context.SaveChangesAsync(); + + // Out of band, so the row is byte-identical to a legacy one. This UPDATE is itself a code + // path and does NOT demonstrate the legacy route -- see the class docstring for which single + // (column, provider) case is genuinely code-path-free. What is being measured here is what EF + // MATERIALIZES from such a row, which is the same regardless of how the NULL got there. + await context.Database.ExecuteSqlRawAsync( + "UPDATE ProgramScheduleAlternate SET DaysOfWeek = NULL, DaysOfMonth = NULL, MonthsOfYear = NULL"); + await context.Database.ExecuteSqlRawAsync( + "UPDATE PlayoutTemplate SET DaysOfWeek = NULL, DaysOfMonth = NULL, MonthsOfYear = NULL"); + } + + // ANTI-VACUITY: a read that found no row would leave every ShouldBeNull below trivially true, + // so both reads are Single and would throw on an empty table. + await using (TvContext context = Create(_options)) + { + ProgramScheduleAlternate alternate = + await context.ProgramScheduleAlternates.AsNoTracking().SingleAsync(); + + alternate.DaysOfWeek.ShouldBeNull( + "EnumCollectionJsonValueConverter must not be invoked for a NULL column"); + alternate.DaysOfMonth.ShouldBeNull( + "IntCollectionValueConverter's null-to-empty branch must not run — EF skips the " + + "converter for a NULL column, so this is null rather than an empty array"); + alternate.MonthsOfYear.ShouldBeNull(); + + PlayoutTemplate template = await context.PlayoutTemplates.AsNoTracking().SingleAsync(); + + template.DaysOfWeek.ShouldBeNull(); + template.DaysOfMonth.ShouldBeNull(); + template.MonthsOfYear.ShouldBeNull(); + } + } + + /// + /// The write half. A caller that hands one of the six a null — which the two Replace* handlers do + /// verbatim from their command, and only the HTTP request records guard against — gets a stored + /// SQL NULL and no error at all. + /// + [Test] + public async Task Assigning_Null_Persists_Sql_Null_Rather_Than_Throwing() + { + await using (TvContext context = Create(_options)) + { + await DisableForeignKeys(context); + + ProgramScheduleAlternate alternate = NewAlternate(playoutId: 2); + alternate.DaysOfWeek = null!; + alternate.DaysOfMonth = null!; + alternate.MonthsOfYear = null!; + + context.ProgramScheduleAlternates.Add(alternate); + + // Not Should.NotThrow: the point is that this is the SHIPPED behaviour of the write path, so + // the assertion is that the round-trip below finds NULL, not merely that nothing blew up. + await context.SaveChangesAsync(); + } + + await using (TvContext context = Create(_options)) + { + ProgramScheduleAlternate reloaded = + await context.ProgramScheduleAlternates.AsNoTracking().SingleAsync(); + + reloaded.DaysOfWeek.ShouldBeNull(); + reloaded.DaysOfMonth.ShouldBeNull(); + reloaded.MonthsOfYear.ShouldBeNull(); + } + + // ...and it really is SQL NULL in the column, not an empty string the converter round-trips. + // ExecuteScalar hands back CLR null both for a NULL column and for NO SUCH ROW, so the row is + // counted first — otherwise a fixture that silently deleted its row would report the same thing. + await using (TvContext context = Create(_options)) + { + (await ScalarAsync(context, "SELECT COUNT(*) FROM ProgramScheduleAlternate")) + .ShouldNotBeNull(); + Convert.ToInt32(await ScalarAsync(context, "SELECT COUNT(*) FROM ProgramScheduleAlternate")) + .ShouldBe(1, "the row is gone, so a null read below would prove nothing"); + + (await ScalarAsync(context, "SELECT DaysOfWeek FROM ProgramScheduleAlternate")).ShouldBeNull(); + (await ScalarAsync(context, "SELECT DaysOfMonth FROM ProgramScheduleAlternate")).ShouldBeNull(); + (await ScalarAsync(context, "SELECT MonthsOfYear FROM ProgramScheduleAlternate")).ShouldBeNull(); + } + } + + private static ProgramScheduleAlternate NewAlternate(int playoutId) => + new() + { + PlayoutId = playoutId, + ProgramScheduleId = 1, + Index = 0, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [], + StartMonth = 1, + StartDay = 1, + EndMonth = 12, + EndDay = 31 + }; + + private static PlayoutTemplate NewTemplate(int playoutId) => + new() + { + PlayoutId = playoutId, + TemplateId = 1, + Index = 0, + DaysOfWeek = [], + DaysOfMonth = [], + MonthsOfYear = [], + StartMonth = 1, + StartDay = 1, + EndMonth = 12, + EndDay = 31 + }; + + /// + /// The rows here are deliberately partial graphs (a PlayoutId pointing at no Playout), so + /// foreign keys are off. SQLite takes it as a connection-string keyword; MySQL's + /// foreign_key_checks is a SESSION variable, so it is set on the context's own connection + /// and lives as long as that context does. + /// + private async Task DisableForeignKeys(TvContext context) + { + if (provider is TestProvider.MySql) + { + await context.Database.OpenConnectionAsync(); + await context.Database.ExecuteSqlRawAsync("SET SESSION foreign_key_checks = 0"); + } + } + + private static async Task ScalarAsync(TvContext context, string sql) + { + await context.Database.OpenConnectionAsync(); + await using System.Data.Common.DbCommand command = context.Database.GetDbConnection().CreateCommand(); + command.CommandText = sql; + object? value = await command.ExecuteScalarAsync(); + return value is DBNull ? null : value; + } + + private static bool IsTrue(string? value) => + !string.IsNullOrWhiteSpace(value) + && (value == "1" || value.Equals("true", StringComparison.OrdinalIgnoreCase)); + + private static TvContext Create(DbContextOptions options) => + new( + options, + NullLoggerFactory.Instance, + new SlowQueryInterceptor(NullLogger.Instance)); +} diff --git a/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs new file mode 100644 index 000000000..115b8ad4f --- /dev/null +++ b/ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs @@ -0,0 +1,129 @@ +using System.Reflection; +using ErsatzTV.Core.Interfaces.Search; +using ErsatzTV.Infrastructure.Search; +using NUnit.Framework; +using NUnit.Framework.Interfaces; +using Shouldly; + +namespace ErsatzTV.Tests.Integration; + +/// +/// ersatztv#824. The defect that produced #824 was not that ElasticSearchIndex was hard to +/// test — it was that NOTHING NOTICED it had no cover. #701 fixed two independent copies of the same +/// UpdateSong logic and pinned one; the suite stayed green, and the gap survived on a +/// hand-written list of what had been covered (namely, one entry). +/// +/// So the covered set is compared against a population DERIVED FROM THE ASSEMBLY rather than +/// restated: a third implementation reddens this test until it is +/// given a mutation fixture of its own. That is +/// testing.guard-derives-population-from-source applied to a test population instead of a +/// file population. +/// +/// +/// Scope, stated rather than implied. The derivation is over the assembly that declares +/// both indexers (ErsatzTV.Infrastructure). An implementation added in a DIFFERENT +/// assembly is outside what this sees — it is not covered and this test cannot say so. Both +/// implementations have lived here since the interface existed, so the narrower scope buys a +/// guard that cannot be defeated by an unrelated assembly load order; widening it to every +/// loaded assembly would be the false-confidence version of the same check. +/// +/// +[TestFixture] +public class SearchIndexMutationCoverageTests +{ + /// + /// The indexers whose UpdateSong is pinned against the ersatztv#701 mutation. Referenced by + /// TYPE, so renaming an indexer or deleting a fixture is a compile error rather than a silent + /// divergence. + /// + private static readonly Dictionary CoveredBy = new() + { + [typeof(LuceneSearchIndex)] = typeof(SongIndexerMetadataMutationTests), + [typeof(ElasticSearchIndex)] = typeof(ElasticSongIndexerMetadataMutationTests) + }; + + [Test] + public void Every_ISearchIndex_Implementation_Has_A_Metadata_Mutation_Fixture() + { + List implementations = typeof(LuceneSearchIndex).Assembly + .GetTypes() + .Where(t => t is { IsAbstract: false, IsInterface: false }) + .Where(t => typeof(ISearchIndex).IsAssignableFrom(t)) + .OrderBy(t => t.FullName, StringComparer.Ordinal) + .ToList(); + + // The set comparison below would ALREADY fail on an empty derivation, because the expected side + // is non-empty -- so this floor is not load-bearing for correctness and saying it is would be a + // false claim about a check. It is a DIAGNOSTIC: it separates "the reflection stopped finding + // types" (a moved type, a renamed interface) from "someone added an indexer", which the set + // comparison alone reports identically. + implementations.Count.ShouldBeGreaterThanOrEqualTo( + 2, + "the ISearchIndex population derivation found almost nothing -- this is the derivation " + + "breaking, not an indexer being added"); + + implementations.ShouldBe( + CoveredBy.Keys.OrderBy(t => t.FullName, StringComparer.Ordinal), + ignoreOrder: false, + "every ISearchIndex implementation needs an UpdateSong metadata-mutation fixture — see " + + "ersatztv#824, where a second copy of the same logic went uncovered and an Elastic-only " + + "reintroduction of `metadata.Artists ??= []` left the whole suite green"); + + // Comparing the KEYS alone would leave the mapping half-checked: a third indexer could be pointed + // at an EXISTING fixture, or at a fixture class holding no runnable test, and the set comparison + // above would still pass. Both are closed here. What NO static check can establish is that the + // named fixture actually DRIVES its indexer -- that is stated as a residual on this guard rather + // than implied away, and it is why the record claims a third implementation cannot be added + // WITHOUT NOTICE, not that it cannot be mis-covered. + CoveredBy.Values.Distinct().Count().ShouldBe( + CoveredBy.Count, + "two indexers are mapped to the SAME fixture, so one of them is not actually covered"); + + foreach ((Type indexer, Type fixture) in CoveredBy) + { + // Test-ness is decided by NUnit's INTERFACES, not by a hand-listed set of attribute types. + // A list has to be kept in step with NUnit and silently falsely-reddens whatever it omits -- + // an earlier revision listed TestAttribute alone (so a [TestCase]-only fixture failed), then + // three types (so a [Theory] one did). ITestBuilder/ISimpleTestBuilder is what NUnit itself + // dispatches on, so it cannot fall behind the vocabulary. + // + // DeclaredOnly is load-bearing: without it a fixture that merely SUBCLASSES another inherits + // its [Test] and satisfies this while driving the wrong indexer -- and Values.Distinct() + // cannot catch that, since the two Types differ. Static is included because NUnit runs a + // public static test method in a non-static fixture, and omitting the flag would falsely + // redden one. + const BindingFlags TestMethods = + BindingFlags.Public | BindingFlags.Instance | BindingFlags.Static | BindingFlags.DeclaredOnly; + + static bool NeverRuns(object[] attributes) => + attributes.Any(a => a is ExplicitAttribute or IgnoreAttribute); + + bool declaresARunnableTest = fixture + .GetMethods(TestMethods) + .Select(m => m.GetCustomAttributes(inherit: true)) + .Any(attrs => + attrs.Any(a => a is ITestBuilder or ISimpleTestBuilder) && !NeverRuns(attrs)); + + declaresARunnableTest.ShouldBeTrue( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but DECLARES no " + + "runnable test method of its own, so it proves nothing"); + + // The same "wired is not running" failure at fixture level, in its three forms: an abstract + // class NUnit will not instantiate, and [Explicit]/[Ignore]. The indexer population at the top + // of this method already filters IsAbstract; the fixture side needs the mirror of that. + // `IsAbstract` ALONE is wrong here and would have been a false red: a C# `static class` + // compiles to `abstract sealed`, and NUnit runs a test declared in one. What must be rejected + // is an abstract BASE (abstract and NOT sealed), which NUnit cannot instantiate. Deliberately + // no in-repo witness is cited: `AlternateScheduleSelectorTests` is a static class, but it + // merely NESTS its `[TestFixture]`es and declares no test of its own, so it would fail the + // sibling assertion above instead -- naming it here would have been a wrong example attached + // to a right rule. + (fixture.IsAbstract && !fixture.IsSealed).ShouldBeFalse( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is an abstract " + + "base class, so NUnit never runs it"); + NeverRuns(fixture.GetCustomAttributes(inherit: true)).ShouldBeFalse( + $"{fixture.Name} is named as the mutation fixture for {indexer.Name} but is [Explicit] or " + + "[Ignore]d, so it never runs and proves nothing"); + } + } +} diff --git a/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs b/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs index b22b43662..b409722d4 100644 --- a/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs +++ b/ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs @@ -186,28 +186,6 @@ public class SongIndexerMetadataMutationTests (await ReadRawArtists(harness, metadataId)).ShouldBeNull(); } - private sealed class ThrowOnWarningLogger : ILogger - { - public Exception? Failure { get; private set; } - - public IDisposable? BeginScope(TState state) where TState : notnull => null; - - public bool IsEnabled(LogLevel logLevel) => true; - - public void Log( - LogLevel logLevel, - EventId eventId, - TState state, - Exception? exception, - Func formatter) - { - if (logLevel >= LogLevel.Warning) - { - Failure ??= exception ?? new InvalidOperationException(formatter(state, exception)); - } - } - } - private static async Task ReadRawArtists(InMemoryTvContext harness, int metadataId) { await using TvContext context = harness.CreateContext(); diff --git a/ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs b/ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs new file mode 100644 index 000000000..a58e20867 --- /dev/null +++ b/ErsatzTV.Tests/Support/ThrowOnWarningLogger.cs @@ -0,0 +1,37 @@ +using Microsoft.Extensions.Logging; + +namespace ErsatzTV.Tests.Support; + +/// +/// Captures the first warning-or-worse a component logs, so a fixture can fail on it instead of +/// silently measuring an error path. +/// +/// Both search indexers wrap each Update* body in a catch that logs a warning and +/// assigns metadata.Song = null — which severs a required relationship and cascades the +/// metadata row to . A fixture +/// that let that catch run quietly would report the wrong cause for every assertion after it, and +/// on the first run of the ersatztv#701 probe it did exactly that (a bare +/// ILanguageCodeService substitute NPEs inside AddLanguages). +/// +/// +public sealed class ThrowOnWarningLogger : ILogger +{ + public Exception? Failure { get; private set; } + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (logLevel >= LogLevel.Warning) + { + Failure ??= exception ?? new InvalidOperationException(formatter(state, exception)); + } + } +} diff --git a/docs/README.md b/docs/README.md index 795608ce1..9b57cbcb9 100644 --- a/docs/README.md +++ b/docs/README.md @@ -39,6 +39,7 @@ doc below, or that changes which sections a task signal points to.** | Live local run / Playwright-MCP verification | `docs/e2e-local.md` + `scripts/e2e-local.sh` | | Adding/changing a UI-E2E browser flow | `docs/e2e-local.md` → "UI-E2E harness" + `scripts/e2e-ui.sh` | | What does a test suite cover | `docs/testing.md` | +| Writing a test whose behaviour is PROVIDER-SPECIFIC (collation, a value converter, data-migration DML) | `docs/testing.md` → "Provider-parity fixtures (opt-in MySQL)" — run one fixture body against both providers via `ETV_TEST_MYSQL_CONNECTION`; without it the MySQL arm `Assert.Ignore`s visibly, and CI does not currently run it (ersatztv#627) | | Legacy Blazor route lookup | `docs/blazor-route-parity.md` (historical #91 phase (b) inventory) | | "Why do we do X this way" / challenging a convention | **Catalog-first**: `docs/decisions/README.md` (active rows) → follow the row's link to `docs/decisions/records//.md` for full rationale. `docs/decisions/archive//` only for "what did the rule used to be." | diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 85dd76ade..2272d95bc 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -107,7 +107,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) | | `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, and EVERY tool (read and write) declares exactly its endpoint's query parameters, both asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields` and `Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. An omitted query parameter is UNREACHABLE, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) | | `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) | -| `media.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — `Optional(x).Flatten()` hoisted into a local — and NEVER writes the guard back onto the entity with `??= []`. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Only the `SongMetadata` pair is left NULL in practice, because `FallbackMetadataProvider` never assigns it. No site applies `??=` to any of the six (`grep -rn 'DaysOfMonth ??=\\|MonthsOfYear ??=\\|DaysOfWeek ??=' --include='*.cs' .` returns 0 at time of writing), so THIS defect has no instance there; whether a null can reach one of them at runtime is a SEPARATE question this record does not answer and does not assert — the API request records normalize with `?? []`, but `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity, so a non-API caller is UNVERIFIED (#823). A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today's callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun. | 2026-08-22 | [link](records/media/nullable-primitive-collection-mutation.md) | +| `media.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question's own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter's null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`'s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API's `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft's actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row's meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user's next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to 'matches no day', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from 'playout build throws' to 'the item applies', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout's default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, 'the operator selected all 31' and 'this is a pre-2024 legacy row' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as 'matches no day', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record's `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` 'allocates for nothing', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today's callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun. | 2026-08-22 | [link](records/media/nullable-primitive-collection-mutation.md) | | `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) | | `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) | | `media.source-mgmt-write-api` | Media-source management (local/Plex/Jellyfin/Emby) is a REST write API + SPA under `/app/libraries/*`, wrapping existing MediatR commands 1:1 with no new commands or DB migration; connection GETs never leak a stored `apiKey`, and each PUT-replace family's identity contract is documented per-family (not assumed uniform). | 2026-07-11 | [link](records/media/source-mgmt-write-api.md) | diff --git a/docs/decisions/records/media/nullable-primitive-collection-mutation.md b/docs/decisions/records/media/nullable-primitive-collection-mutation.md index 5989a0593..07b3fad23 100644 --- a/docs/decisions/records/media/nullable-primitive-collection-mutation.md +++ b/docs/decisions/records/media/nullable-primitive-collection-mutation.md @@ -5,9 +5,9 @@ status: active since: '2026-08-22' supersedes: none superseded-by: none -rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — `Optional(x).Flatten()` hoisted into a local — and NEVER writes the guard back onto the entity with `??= []`. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Only the `SongMetadata` pair is left NULL in practice, because `FallbackMetadataProvider` never assigns it. No site applies `??=` to any of the six (`grep -rn ''DaysOfMonth ??=\|MonthsOfYear ??=\|DaysOfWeek ??='' --include=''*.cs'' .` returns 0 at time of writing), so THIS defect has no instance there; whether a null can reach one of them at runtime is a SEPARATE question this record does not answer and does not assert — the API request records normalize with `?? []`, but `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity, so a non-API caller is UNVERIFIED (#823). A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today''s callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun.' -signals: 'Artists ??= [] on a tracked entity · nullable primitive collection not a navigation · JSON array in one column · value converter is the same hazard as a primitive collection · IntCollectionValueConverter EnumCollectionJsonValueConverter · derive the collection-column population from the model configuration · SaveChanges writes empty array over NULL · entity flips to Modified on a read guard · AsNoTracking today is not a safety argument · untagged song loses its NULL artists · foreach over null throws NRE while string.Join throws ArgumentNullException · sweep by FIELD not by file · Optional Flatten hoisted local · paths: `ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs`, `ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs`, `ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs`, `ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs` · issues: #701, #691, #671, #823, #824' -mechanics: 'Pinned by `SongIndexerMetadataMutationTests.UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity`, which drives the real `LuceneSearchIndex` against a real `TvContext` on SQLite with a deliberately TRACKED song. That fixture covers LUCENE ONLY — `ElasticSearchIndex` holds an independent copy of the same code and needs a stubbed transport, so an Elastic-only reintroduction stays green (#824). No repo-wide detector: the `??=` idiom is correct on navigations and appears 92 times across the app projects (`grep -rn ''??= '' --include=''*.cs'' ErsatzTV ErsatzTV.Core ErsatzTV.Application ErsatzTV.Infrastructure ErsatzTV.Scanner | grep -v ''/obj/\|/bin/'' | grep -c ''''`, 2026-08-22), so a grep for it would be noise — the eight-column population is small enough to sweep by field instead.' +rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — hoisted into a LOCAL — and NEVER writes the guard back onto the entity with `??= []`. The local is the load-bearing half and it is universal; the SUBSTITUTED VALUE is not, so read to the end before copying a form: `Optional(x).Flatten()` (i.e. EMPTY) is right for the `SongMetadata` pair and WRONG for the six scheduling columns, which substitute the `All*()` sets. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Of the eight, only the `SongMetadata` pair is left NULL by a live code path (`FallbackMetadataProvider` never assigns it); for the six the NULL is legacy-only and narrow, per the migration analysis below. No site applies `??=` to any of the six, so THIS defect has no instance there — but a runtime null IS REACHABLE and is now guarded (#823, MEASURED 2026-08-29 against a real `TvContext` on BOTH providers, SQLite and MySQL 8.4). The measurement overturned the question''s own framing: the two converters differ on their read side (`IntCollectionValueConverter` maps null-or-blank to `Array.Empty()`, `EnumCollectionJsonValueConverter` would dereference `JsonConvert.DeserializeObject`), so the expectation was that they behave differently on a NULL row. NEITHER RUNS: EF does not invoke a value converter for a NULL column at all, so all six materialize as CLR null and the int converter''s null-to-empty branch is DEAD on this path — do not reason from the converter bodies here. The WRITE path ACCEPTS a null: assigning one and calling `SaveChanges` SUCCEEDS and stores SQL NULL, because the COLUMN is nullable and the converter is skipped outbound too. Separately — this is how a null could REACH the entity, which is a different question from why the save succeeds — only the HTTP request records normalize with `?? []`, while `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity. State that precisely: NO caller supplies a null today — every production construction of the two commands goes through the request records — so this is a property of the CODE, not evidence of a live caller, and writing it down as "a non-API caller does this" would be the banned `AsNoTracking`-today argument pointed the other way. The LEGACY route is narrower than "the columns are nullable", and the difference matters: all six are `nullable: true` on both providers, but a nullable column does not produce a NULL row — five of the six were present at `CreateTable`, so a NULL there still needs code to write one. EXACTLY ONE case is code-path-free, and it is the one to go and check: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` is an `AddColumn` with `nullable: true` and NO `defaultValue`, so `PlayoutTemplate` rows inserted before it hold NULL. On MySQL `PlayoutTemplate` arrived whole in `20240114034944_Add_BlockScheduling`, so there is NO code-path-free NULL for any of the six there. Unguarded, `AlternateScheduleSelector`''s three `.Contains` calls throw `NullReferenceException`. The guard is three LOCALS in `GetScheduleForDate`, never assigned back onto the item. A null reads as UNRESTRICTED — the `All*()` sets — and the REJECTED alternative was EMPTY (the item matches nothing and the loop moves on). Both readings were written and one was shipped, so the rejection is recorded to stop it being re-adopted. What does NOT decide it: the API''s `?? []` must not be cited as if it did — that is a client omitting a field on a WRITE, not evidence about what a legacy database NULL meant, and citing it was the first draft''s actual error. What DOES decide it is the single (column, provider) case that is code-path-free: Sqlite''s `20240113140741_Add_PlayoutTemplate_DaysOfMonth` adds the column with no `defaultValue`, so a `PlayoutTemplate` row inserted before it holds NULL and, BY CONSTRUCTION, had no day-of-month restriction — it applied on every day of the month. Reading that NULL as empty INVERTS the row''s meaning and silently stops the template applying at all, which is strictly worse than the throw it replaces; `All*()` preserves it. Note there is no correct behaviour being preserved in the general case either, since the row THROWS today and fails the playout build outright — so the choice is between two silent repairs, and the one that keeps a legacy row doing what it did wins. The SAME substitution is applied at the entity→DTO boundary in both `Mapper.ProjectToViewModel` overloads, because the selector is not the only reader: the SPA spreads these collections (`daysOfWeek: [...template.daysOfWeek]`, `PlayoutScheduleEditors.tsx`) and `appliesToDate` — an exact port of `GetScheduleForDate` — calls `.includes` on them, so a JSON null is a `TypeError` and a DTO disagreeing with the selector would mispreview the calendar. That boundary ROUND-TRIPS, and it is the reason the empty reading was actively dangerous rather than merely debatable: the draft the SPA builds from the DTO is PUT back whole, so whatever the mapper substitutes is what a user''s next save PERSISTS over the NULL. With `All*()` that write is benign — it stores exactly what the selector already behaves as, making the implicit explicit. With `[]` it would have silently rewritten the row to ''matches no day'', which is a read guard turning into data corruption one save later. THREE RESIDUALS, stated rather than argued away. (1) The substitution is SILENT: a legacy NULL row changes from ''playout build throws'' to ''the item applies'', with nothing logged — the static, hot-loop selector has nowhere to log from. The LOUDNESS change is the real cost and it is worst for an all-three-NULL `ProgramScheduleAlternate`, which now matches unconditionally and shadows the playout''s default schedule for every date where it previously threw. That state is unreachable today (nothing writes it on either provider) and the reversal is justified by a case where the OTHER dimensions are non-NULL, so extending it to the all-NULL row is a choice over an unreachable state rather than a measured requirement. (2) The normalization is ONE-WAY and WHOLE-LIST: both PUT paths are full replaces, so editing any row in a playout persists `All*()` over EVERY NULL row in it, including rows the operator never opened — and once written, ''the operator selected all 31'' and ''this is a pre-2024 legacy row'' are no longer distinguishable, so revisiting the unrestricted reading later is a door an ordinary user action closes. (3) The WRITE side disagrees with the READ side about what ABSENCE means: the request records normalize an omitted `daysOfWeek` to `[]`, which reads as ''matches no day'', while a NULL column now reads as unrestricted — so a client omitting the field gets HTTP 200 and a row that silently never applies. That asymmetry predates this record and is tracked separately (#880); it is deliberately NOT resolved here, because a client omitting a field on a write is a different question from what a legacy NULL meant. For these six the guard form is `?? All*()` into a local, NOT this record''s `Optional(x).Flatten()`, and the deviation is SEMANTIC rather than stylistic: `Flatten()` yields EMPTY, which is the reading rejected above, so the idiom cannot be reused here whatever its ergonomics. Do not restate this as a performance argument — an earlier draft did, claiming `.ToList()` ''allocates for nothing'', which is both irrelevant to the choice and false about the shipped code, since `AllDaysOfMonth()`/`AllMonthsOfYear()` are themselves `Enumerable.Range(...).ToList()` on the null path. What carries over from the `Optional(x).Flatten()` form is the only part that was ever load-bearing: a LOCAL, never assigned back onto the entity. A grep for `IList` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today''s callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun.' +signals: 'Artists ??= [] on a tracked entity · nullable primitive collection not a navigation · JSON array in one column · value converter is the same hazard as a primitive collection · IntCollectionValueConverter EnumCollectionJsonValueConverter · derive the collection-column population from the model configuration · SaveChanges writes empty array over NULL · entity flips to Modified on a read guard · AsNoTracking today is not a safety argument · untagged song loses its NULL artists · foreach over null throws NRE while string.Join throws ArgumentNullException · sweep by FIELD not by file · Optional Flatten hoisted local for the SongMetadata pair · but the six SCHEDULING columns substitute the All*() sets, not empty · a legacy NULL DaysOfMonth means NO day-of-month restriction, not ''matches no day'' · reading it as empty INVERTS a pre-2024 PlayoutTemplate row · EF skips the value converter entirely for a NULL column, on both providers · AlternateScheduleSelector .Contains throws NRE on a null · guard the two Mapper.ProjectToViewModel overloads too or the SPA spreads a null and throws TypeError · appliesToDate is an exact port and must preview what is scheduled · the full-replace PUT persists the substituted set over the NULL on the next save · write side normalizes an omitted array to [] while the read side reads NULL as unrestricted (#880) · paths: `ErsatzTV.Core/Scheduling/AlternateScheduleSelector.cs`, `ErsatzTV.Application/Playouts/Mapper.cs`, `ErsatzTV.Application/Scheduling/Mapper.cs`, `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs`, `ErsatzTV.Tests/Integration/SchedulingCollectionColumnNullTests.cs`, `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs`, `ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs`, `ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs`, `ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs`, `ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs` · issues: #701, #691, #671, #823, #824' +mechanics: 'Pinned by `SongIndexerMetadataMutationTests.UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity`, which drives the real `LuceneSearchIndex` against a real `TvContext` on SQLite with a deliberately TRACKED song. That fixture covers LUCENE ONLY; `ElasticSearchIndex` holds an independent copy and is pinned separately by `ErsatzTV.Tests/Integration/ElasticSongIndexerMetadataMutationTests.cs` (#824), which drives the REAL indexer through an `Elastic.Transport.InMemoryRequestInvoker` injected into the private `_client` — no server, no socket, no new package. Two traps there are load-bearing and both were measured: the canned response must carry an `X-Elastic-Product: Elasticsearch` header or the client''s product check throws `UnsupportedProductException` INTO `UpdateSong`''s catch, and an empty response body fails to deserialize the same way — either turns the fixture into a green measurement of the ERROR path. Proof, executed: restoring the `??=` clause in `ElasticSearchIndex` ALONE reddens the Elastic fixture on `metadata.Artists should be null but was []` while the LUCENE fixture stays GREEN — the #824 hole demonstrated rather than described. A third implementation cannot be added WITHOUT NOTICE: `SearchIndexMutationCoverageTests` derives the `ISearchIndex` population from the declaring assembly and compares it to the covered set, with an anti-vacuity floor, rejects two indexers sharing one fixture, and rejects a named fixture that does not DECLARE a runnable test of its own. Test-ness is decided by NUnit''s `ITestBuilder`/`ISimpleTestBuilder` interfaces rather than a list of attribute types, because two successive hand-written lists each falsely reddened whatever they omitted (`[TestCase]`, then `[Theory]`); `DeclaredOnly` is what stops a fixture SUBCLASSING another from inheriting its `[Test]` and passing while driving the wrong indexer, which `Values.Distinct()` cannot see since the two types differ; and `abstract`, `[Explicit]` and `[Ignore]` are each rejected as the fixture-level form of ''wired is not running''. The claim stops there deliberately — NO static check can establish that the named fixture actually DRIVES its indexer, so this forces a human to look, it does not prove coverage. No repo-wide detector: the `??=` idiom is correct on navigations and appears 92 times across the app projects (`grep -rn ''??= '' --include=''*.cs'' ErsatzTV ErsatzTV.Core ErsatzTV.Application ErsatzTV.Infrastructure ErsatzTV.Scanner | grep -v ''/obj/\|/bin/'' | grep -c ''''`, 2026-08-22), so a grep for it would be noise — the eight-column population is small enough to sweep by field instead.' --- **Deriving this population from the domain classes gives the wrong answer.** "The `IList` @@ -64,5 +64,22 @@ is unreproducible by construction.) The exception type follows the read FORM, no of each. That is the same trap #691 hit from the other direction, and it is why the rule pairs the removal with the read-site guard rather than stating them separately. +**The six scheduling columns were a SEPARATE question, and it was answered later (#823).** The body above +is about `SongMetadata` and the `??=` write; everything in `rule:` about `ProgramScheduleAlternate` / +`PlayoutTemplate` was added when #823 measured the reachability this record originally declined to assert. +Two results are worth having in prose rather than only in the rule, because both contradicted the +reasoning available beforehand: + +- **The converters do not run.** The read-side difference between `IntCollectionValueConverter` + (null-or-blank → `Array.Empty()`) and `EnumCollectionJsonValueConverter` (would dereference) is + what the question was framed around, and it is IRRELEVANT: EF does not invoke a value converter for a + NULL column at all, on either provider. Measured, not reasoned about — the same step that changed the + answer in #701. +- **A NULL means UNRESTRICTED, not empty**, and the reading is not a matter of taste. Exactly one + (column, provider) case is reachable without code writing a NULL, and it decides the rest: a + `PlayoutTemplate` row predating Sqlite's `20240113140741_Add_PlayoutTemplate_DaysOfMonth` had no + day-of-month restriction at all, so reading its NULL as empty would stop the template applying — an + inversion, and silent. The `All*()` sets preserve it. + Related: `api.selection-projection-include-chain` (#671) records the read-site guard itself and the sweep-by-FIELD instruction; this record covers the write half it does not address. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 443b2dc8c..46344fe3f 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -64,7 +64,10 @@ with have sat in these gaps. Treat anything not named here as unassessed, not as the check proves each job was classified, never that the classification is CORRECT; and an `inline` assertion cannot be proven to work without running the job, so those rows carry `Proof: NONE` honestly instead of borrowing a neighbouring file's proof. -2. **C# and TypeScript guards** — five files, none with a row: +2. **C# and TypeScript guards** — six files, none with a row: + `ErsatzTV.Tests/Integration/SearchIndexMutationCoverageTests.cs` (ersatztv#824 — derives the + `ISearchIndex` population from the declaring assembly and asserts set equality against a + hand-written covered set), `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `web/src/api/pageSizeCallSites.guard.test.ts`, `web/src/api/completeRequest.guard.test.ts`, and the pair `web/vite-plugins/trackedSourceFiles.test.ts` + its `.realgit.test.ts` sibling, which together diff --git a/docs/testing.md b/docs/testing.md index ee1a6bfcb..0d93d1685 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -116,6 +116,33 @@ UI-E2E (needs a built solution + built SPA; boots and tears down its own instanc scripts/e2e-ui.sh # from the repo root, NOT web/ ``` +## Provider-parity fixtures (opt-in MySQL) + +Most of `ErsatzTV.Tests` runs on the in-memory SQLite harness described above. A few fixtures in +`ErsatzTV.Tests/Integration/` instead drive a **real, migrated** database, and run the SAME body against +**both** providers because the behaviour they pin is provider-specific: + +| Fixture | What is provider-specific about it | +| --- | --- | +| `LibraryFolderDedupeMigrationTests` | the #491 dedupe DML — two MySQL-only collation defects (case-insensitive grouping, then `PAD SPACE`) were unreachable from SQLite | +| `SchedulingCollectionColumnNullTests` | what a NULL column materializes as through a value converter (ersatztv#823) | +| `SearchFieldValuesProviderTests` | the search-field-values query shape, which differs per provider | + +The MySQL half needs a live server, supplied as `ETV_TEST_MYSQL_CONNECTION`. **Without it these +fixtures `Assert.Ignore` — a visible skip, never a silent pass**, so an ordinary local run needs no +MySQL. Setting `ETV_REQUIRE_MYSQL_TESTS=1` turns that skip into a hard failure, for a runner that is +supposed to have one. + +```bash +ETV_TEST_MYSQL_CONNECTION='Server=;Port=3306;Uid=root;Pwd=;DefaultCommandTimeout=300;' \ + dotnet test ErsatzTV.Tests --filter FullyQualifiedName~SchedulingCollectionColumnNullTests +``` + +Each test uses a database name it generates per run, so isolation does not depend on a wipe +succeeding, and drops it in teardown. **CI does not currently run any of these MySQL halves** — the +`migrations` job spins a `mysql:8.4` service but only applies migrations to a fresh EMPTY database, +so it executes no data rows; re-arming these fixtures there is tracked by ersatztv#627. + ## Per-PR verification gate Before opening a PR: build the solution, run `ErsatzTV.Tests` + `ErsatzTV.Core.Tests` (plus