diff --git a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs index da7899685..78e1e710a 100644 --- a/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs +++ b/ErsatzTV.Application/Graphics/Queries/GetAllGraphicsElementsForApiHandler.cs @@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory db .Select(e => new { Vm = ProjectToViewModel(e), - BuiltIn = e.Path == GraphicsElementDefaults.OnNowNextSeededPath + BuiltIn = GraphicsElementDefaults.IsOnNowNext(e.Path) }) .OrderBy(x => x.Vm.Name == x.Vm.FileName) .ThenBy(x => x.Vm.Name) diff --git a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs index aefa0977d..7b32e85c1 100644 --- a/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs +++ b/ErsatzTV.Core/Graphics/GraphicsElementDefaults.cs @@ -5,19 +5,36 @@ namespace ErsatzTV.Core.Graphics; public static class GraphicsElementDefaults { // Built-in "On Now / Next" text element filename -- a component of OnNowNextSeededPath below, - // never itself an identity check (a filename-only comparison is case-sensitive-by-accident and - // folder-agnostic; see OnNowNextSeededPath, #568). + // never itself an identity check (#568: a filename-only comparison is folder-agnostic). public const string OnNowNextFileName = "on-now-next.yml"; - // Display name only. Never use it for identity -- that is OnNowNextSeededPath below (#67 / #74 / #568). + // Display name only. Never use it for identity -- that is IsOnNowNext below (#67 / #74 / #568). public const string OnNowNextName = "On Now / Next"; - // The full path the seeder writes the built-in template to. A `builtIn` discriminator must - // match THIS, not `Path.GetFileName(...) == OnNowNextFileName` -- a filename-only comparison is - // case-sensitive-by-accident and folder-agnostic: a user element named exactly `on-now-next.yml` - // in any of the other four template folders (image/motion/subtitle/script) would also report - // `builtIn:true` (#568). `Kind == Text` alone does not close this either, since a second text - // template could share the filename in principle. + // The full path the seeder writes the built-in template to (GraphicsElementSeeder.SeedOnNowNext + // builds `target` the same way). A `builtIn` discriminator must match THIS, not + // `Path.GetFileName(...) == OnNowNextFileName` -- a filename-only comparison is folder-agnostic: + // a user element named exactly `on-now-next.yml` in any of the other four template folders + // (image/motion/subtitle/script) would also report `builtIn:true` (#568). `Kind == Text` alone + // does not close this either, since a second text template could share the filename in principle. public static string OnNowNextSeededPath => Path.Combine(FileSystemLayout.GraphicsElementsTextTemplatesFolder, OnNowNextFileName); + + /// + /// The one identity test for the built-in On Now / Next element. Ordinal on purpose, and so + /// case-sensitive on purpose: the built-in element is the exact file the seeder wrote, at + /// the exact path it wrote it to. + /// + /// + /// Callers compare in memory rather than in a Where clause, because in SQL the answer + /// would be the PROVIDER's to give: GraphicsElement.Path takes no explicit collation + /// (TvContext.OnModelCreating pins one only on the listed name/title columns), so + /// SQLite compares it case-sensitively while MySQL uses the server default, which is + /// normally case-INsensitive. Evaluating one discriminator site in SQL and the other in + /// memory would let the two disagree on MySQL alone. The SQLite test suite cannot tell the + /// two apart -- BINARY collation and an ordinal comparison agree on every input -- so this + /// is held by keeping the comparison out of SQL, not by a test. + /// + public static bool IsOnNowNext(string path) => + string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal); } diff --git a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs index 8fcd8022c..338f0be84 100644 --- a/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs +++ b/ErsatzTV.Infrastructure/Streaming/Graphics/GraphicsElementSeeder.cs @@ -253,16 +253,27 @@ public static class GraphicsElementSeeder /// The Kind filter stays as belt-and-braces since `OnNowNextSeededPath` is itself a Text-folder /// path. /// + /// + /// The path comparison is in memory rather + /// than a Where clause: in SQL the match would be the provider's collation to decide, and + /// this site would then disagree with the API's `builtIn` (which compares in memory) on MySQL. + /// The Kind filter stays in SQL because it is an enum, not a string. + /// public static async Task> GetBuiltInElementId( TvContext context, CancellationToken cancellationToken) { - List matches = await context.GraphicsElements + List<(int Id, string Path)> candidates = await context.GraphicsElements .Where(e => e.Kind == GraphicsElementKind.Text) - .Where(e => e.Path == GraphicsElementDefaults.OnNowNextSeededPath) - .Select(e => e.Id) + .Select(e => new { e.Id, e.Path }) + .ToListAsync(cancellationToken) + .Map(rows => rows.Select(r => (r.Id, r.Path)).ToList()); + + List matches = candidates + .Where(c => GraphicsElementDefaults.IsOnNowNext(c.Path)) + .Select(c => c.Id) .OrderBy(id => id) - .ToListAsync(cancellationToken); + .ToList(); // Lowest id wins if two rows somehow share the seeded path, so the choice is stable across // restarts rather than dependent on query order. diff --git a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs index 1d1500efb..ff0a8c540 100644 --- a/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Graphics/GraphicsElementHandlerTests.cs @@ -101,6 +101,31 @@ public class GraphicsElementHandlerTests result[0].BuiltIn.ShouldBeTrue(); } + // #568: identity is GraphicsElementDefaults.IsOnNowNext, an ORDINAL comparison, so a row whose + // path differs from the seeded one only in case is a different element. Reddens if that + // comparison is loosened to OrdinalIgnoreCase. It does NOT pin provider independence: under + // SQLite's BINARY collation a `Where(e => e.Path == ...)` in SQL answers identically, which is + // why the comparison is kept in memory rather than pinned here (see IsOnNowNext's remarks). + [Test] + public async Task GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn() + { + await SeedElement(1, CaseVariantOfSeededPath(), GraphicsElementKind.Text, string.Empty); + + var handler = new GetAllGraphicsElementsForApiHandler(_db.Factory); + + List result = + await handler.Handle(new GetAllGraphicsElementsForApi(), CancellationToken.None); + + result.Count.ShouldBe(1); + result[0].BuiltIn.ShouldBeFalse(); + } + + // The seeded path with only the FILENAME's case changed -- same folder, same spelling. + private static string CaseVariantOfSeededPath() => + System.IO.Path.Combine( + System.IO.Path.GetDirectoryName(GraphicsElementDefaults.OnNowNextSeededPath)!, + GraphicsElementDefaults.OnNowNextFileName.ToUpperInvariant()); + private async Task SeedElement(int id, string path, GraphicsElementKind kind, string name) { await using TvContext context = _db.CreateContext(); diff --git a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs index 5ed293f28..abf0e88f5 100644 --- a/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs +++ b/ErsatzTV.Tests/Infrastructure/GraphicsElementDefaultAttachTests.cs @@ -214,6 +214,32 @@ public class GraphicsElementDefaultAttachTests (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); } + // #568: GetBuiltInElementId resolves through GraphicsElementDefaults.IsOnNowNext, an ORDINAL + // comparison, so a Text row in the seeded folder whose filename differs only in case is a + // different element. Reddens if that comparison is loosened to OrdinalIgnoreCase. It does NOT + // pin provider independence -- under SQLite's BINARY collation an equivalent SQL `Where` answers + // identically; that is held by keeping the comparison in memory (see IsOnNowNext's remarks). + [Test] + public async Task Ignores_A_Case_Variant_Of_The_Seeded_Path() + { + await using TvContext context = _db.CreateContext(); + await context.GraphicsElements.AddAsync( + new GraphicsElement + { + Path = System.IO.Path.Combine( + System.IO.Path.GetDirectoryName(GraphicsElementDefaults.OnNowNextSeededPath)!, + GraphicsElementDefaults.OnNowNextFileName.ToUpperInvariant()), + Kind = GraphicsElementKind.Text + }); + await context.SaveChangesAsync(); + + Channel channel = await SeedChannel(context, "1"); + + await GraphicsElementSeeder.AttachOnNowNextByDefault(context, CancellationToken.None); + + (await AttachedElementIds(context, channel.Id)).ShouldBeEmpty(); + } + // The marker is permanent, so writing it with nothing resolved would strand every channel. Stay // armed instead and pick the work up once the element exists. [Test] diff --git a/docs/api-conventions.md b/docs/api-conventions.md index 83b6126cf..5835e5766 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -943,10 +943,9 @@ every other field on this full-replace DTO. The identical shape existed on `PUT (`ReplaceDecoRequest.graphicsElementIds`/`watermarkIds`, also top-level fields, not the deep-FK-in-a- nested-list carve-out below) and is hardened the same way by `UpdateDecoHandler.Validate` (`GraphicsElementIdsMustExist`/`WatermarkIdsMustExist`, #568). `GraphicsElementResponseModel` exposes -a server-derived -`builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as `element.Path == -GraphicsElementDefaults.OnNowNextSeededPath` (the full seeded path, not the bare filename — a -filename-only match was case-sensitive-by-accident and folder-agnostic, #568) — never client-settable. +a server-derived `builtIn`, computed by `GetAllGraphicsElementsForApiHandler` as +`GraphicsElementDefaults.IsOnNowNext(element.Path)` — ordinal equality against the full seeded path, +not the bare filename, which was folder-agnostic (#568) — never client-settable. ## 9. Authentication — session-or-key posture (fail-closed) diff --git a/docs/decisions/records/graphics/channel-level-attachment.md b/docs/decisions/records/graphics/channel-level-attachment.md index 33bb41a10..6eb717e8c 100644 --- a/docs/decisions/records/graphics/channel-level-attachment.md +++ b/docs/decisions/records/graphics/channel-level-attachment.md @@ -40,11 +40,11 @@ four joins (composite key `{ChannelId, GraphicsElementId}`), added via a dual-pr preset). - The API needed a way for the SPA to find the built-in element without a fragile name-match — the direct #67 lesson (`WatermarkResponseModel.imageSource`). `GraphicsElementResponseModel` gained a - server-derived `BuiltIn` bool, computed by comparing the row's `Path` to - `GraphicsElementDefaults.OnNowNextFileName` rather than trusting the element's editable `Name`. - (#568 sharpened this from a bare-filename comparison to the full seeded path, - `GraphicsElementDefaults.OnNowNextSeededPath` — a filename-only match was case-sensitive-by-accident - and folder-agnostic; see `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`.) + server-derived `BuiltIn` bool, computed from the row's `Path` rather than the element's editable + `Name`. The test is `GraphicsElementDefaults.IsOnNowNext` — ordinal equality against the full + seeded path. #568 sharpened it from the original bare-filename comparison, which was + folder-agnostic: a user element named exactly `on-now-next.yml` in another template folder also + reported `builtIn:true`. See `graphics.on-now-next-on-by-default` and `docs/graphics-elements.md`. - The channel editor's Branding-tab "Show On Now / Next overlay" switch follows the exact pattern of the existing logo-bug toggle: on adds the built-in element's id to `graphicsElementIds`, off removes it; disabled (with an explanatory caption) when the channel is HLS-Direct. diff --git a/docs/decisions/records/graphics/on-now-next-on-by-default.md b/docs/decisions/records/graphics/on-now-next-on-by-default.md index d60d72d55..5279b03b5 100644 --- a/docs/decisions/records/graphics/on-now-next-on-by-default.md +++ b/docs/decisions/records/graphics/on-now-next-on-by-default.md @@ -60,10 +60,10 @@ is simply absent, indistinguishable from never having enabled it — so the one- re-attaches it. That is inherent to "enable it on all channels by default" rather than a defect. The never-re-attach guarantee therefore holds *from the marker onwards*, not across the upgrade boundary. -Identity is the element's **full seeded path** (`GraphicsElementDefaults.OnNowNextSeededPath`), never -the user-editable `Name` — the #67 lesson carried through #74, sharpened from a bare filename to the -full path by #568 (a filename-only match was case-sensitive-by-accident and folder-agnostic; see -`graphics.channel-level-attachment` and `docs/graphics-elements.md`). +Identity is the element's **full seeded path** — `GraphicsElementDefaults.IsOnNowNext`, ordinal +equality against `OnNowNextSeededPath` — never the user-editable `Name`: the #67 lesson carried +through #74, sharpened from a bare filename to the full path by #568 because a filename-only match +was folder-agnostic (see `graphics.channel-level-attachment` and `docs/graphics-elements.md`). **HLS Direct is excluded at both sites.** ErsatzTV is not transcoding there, so `GraphicsElementSelector` returns empty and the editor disables the toggle; an attachment would be inert while still reading as diff --git a/docs/graphics-elements.md b/docs/graphics-elements.md index 64aba0534..2cfbc641d 100644 --- a/docs/graphics-elements.md +++ b/docs/graphics-elements.md @@ -121,12 +121,20 @@ never overwrites an operator's file. Two rules govern it after that: - Updating the shipped default → `graphics.seeded-template-upgrade-by-fingerprint`. - It is attached to channels by default → `graphics.on-now-next-on-by-default`. -Identity is the **full seeded path** (`GraphicsElementDefaults.OnNowNextSeededPath`), never the editable -`Name` and never the bare filename (`OnNowNextFileName`) alone — a filename-only, folder-agnostic -comparison let a user element named exactly `on-now-next.yml` in a different template folder also -report `builtIn:true` (#568). `GraphicsElementResponseModel.BuiltIn` and -`GraphicsElementSeeder.GetBuiltInElementId` both compare against the full path server-side so the SPA -never name-matches. +Identity is the **full seeded path**, never the editable `Name` and never the bare filename +(`OnNowNextFileName`) alone — a filename-only, folder-agnostic comparison let a user element named +exactly `on-now-next.yml` in a different template folder also report `builtIn:true` (#568). Both +`GraphicsElementResponseModel.BuiltIn` and `GraphicsElementSeeder.GetBuiltInElementId` resolve it +through the one predicate, `GraphicsElementDefaults.IsOnNowNext`, so the SPA never name-matches. + +`IsOnNowNext` is **ordinal**, and every caller applies it **in memory** rather than in a `Where` +clause. That is not incidental: `GraphicsElement.Path` takes no explicit collation (`TvContext` +pins one only on the listed name/title columns), so a SQL `Path ==` comparison is case-sensitive +under SQLite and normally case-INsensitive under MySQL. Evaluating one of the two discriminator +sites in SQL and the other in memory is what would let them disagree, on MySQL only. The SQLite test +suite cannot distinguish the two — BINARY collation and an ordinal comparison agree on every input — +so this is held by keeping the comparison out of SQL, not by a test; what the case-variant tests in +the table below pin is the ordinal rule itself, against a loosening to `OrdinalIgnoreCase`. ## Tests