fix(568): one ordinal identity predicate for the built-in element, applied in memory at both sites
The branch moved the `builtIn` discriminator from a bare filename to the full seeded path, but split how the two sites evaluate it: the API handler compares in memory (ordinal) while GetBuiltInElementId's new `.Where(e => e.Path == OnNowNextSeededPath)` compares in SQL. GraphicsElement.Path takes no explicit collation -- TvContext.OnModelCreating pins one only on the listed name/title columns -- so SQLite answers that case-sensitively and MySQL uses the server default, which is normally case-INsensitive. On MySQL the two discriminators could therefore disagree about the same row: AttachOnNowNextByDefault would resolve a case-variant user element as the built-in one while the API reported builtIn:false for it. Collapse both onto GraphicsElementDefaults.IsOnNowNext, ordinal, applied in memory. GetBuiltInElementId goes back to loading the Text candidates and filtering in memory (the shape it had before this branch), keeping only the `Kind` enum filter in SQL. The prose claimed more than the code did. "A filename-only comparison is case-sensitive-by-accident" appeared in four places as a defect the full-path fix removed; a full-path comparison is exactly as case-sensitive, so the clause said nothing and implied a fix that had not happened. Case sensitivity is now deliberate and stated as such -- the built-in element is the exact file the seeder wrote, at the exact path it wrote it to -- and the reason the comparison is kept out of SQL is recorded where the predicate lives. docs/decisions/records/graphics/channel-level-attachment.md said BuiltIn was "computed by comparing the row's `Path` to GraphicsElementDefaults. OnNowNextFileName", which was true of neither the pre-#568 rule (filename to filename) nor the current one; an active record resolved by key now states the current predicate in its own sentence rather than in a parenthetical. Two tests pin the ordinal rule against a loosening to OrdinalIgnoreCase, one per site. Measured: OrdinalIgnoreCase reddens exactly GetAllGraphicsElementsForApi_Should_Not_Mark_A_Case_Variant_Of_The_Seeded_Path_As_BuiltIn and Ignores_A_Case_Variant_Of_The_Seeded_Path, 2 failed / 77 passed of the 79 graphics tests. They do NOT pin provider independence -- under SQLite's BINARY collation an equivalent SQL comparison answers identically, so no test in this suite can distinguish the two. That is stated at each site rather than left for a reader to assume the tests cover it. Decisions-Edit: yes Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QqCpYFsKgnAnx6jVwrKiV
This commit is contained in:
Claude Fable 5.1
@@ -22,7 +22,7 @@ public class GetAllGraphicsElementsForApiHandler(IDbContextFactory<TvContext> 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)
|
||||
|
||||
@@ -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);
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Callers compare in memory rather than in a <c>Where</c> clause, because in SQL the answer
|
||||
/// would be the PROVIDER's to give: <c>GraphicsElement.Path</c> takes no explicit collation
|
||||
/// (<c>TvContext.OnModelCreating</c> 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.
|
||||
/// </remarks>
|
||||
public static bool IsOnNowNext(string path) =>
|
||||
string.Equals(path, OnNowNextSeededPath, StringComparison.Ordinal);
|
||||
}
|
||||
|
||||
@@ -253,16 +253,27 @@ public static class GraphicsElementSeeder
|
||||
/// The Kind filter stays as belt-and-braces since `OnNowNextSeededPath` is itself a Text-folder
|
||||
/// path.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The path comparison is <see cref="GraphicsElementDefaults.IsOnNowNext"/> in memory rather
|
||||
/// than a <c>Where</c> 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 <c>Kind</c> filter stays in SQL because it is an enum, not a string.
|
||||
/// </remarks>
|
||||
public static async Task<Option<int>> GetBuiltInElementId(
|
||||
TvContext context,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
List<int> 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<int> 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.
|
||||
|
||||
@@ -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<GraphicsElementResponseModel> 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();
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user