From 1641ca83057e891df5c681abced18c1aa4b50ea2 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 26 Jul 2026 23:52:05 +0200 Subject: [PATCH] fix(578): the LIKE prefilter under-matched every accented artist; make the superset provable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of 1b78dc9e found the pre-filter's correctness claim was false, and the claim was in the decision record as well as the code. F1 (high). The pattern JSON-encoded the whole query prefix on the reasoning that the stored text escapes non-ASCII, so encoding the prefix the same way would line up. It does not: SQL LOWER() lowercases the *escape text* (`É` -> `é`); it cannot case-fold the codepoint that escape denotes. So `q=é` built `%"é%`, the stored `Édith Piaf` never matched, and the row was discarded before the in-memory filter could accept it. Every accented artist — Beyoncé, Björk, Sigur Rós, Édith Piaf — was silently unsuggestable, which in a music library is the common case. The invariant that was missing, now stated in the code: the SQL pre-filter is an OPTIMIZATION. It may over-match; it must never under-match. Correctness lives in the in-memory filter. So the pattern now narrows only on the leading run of characters the JSON writer stores verbatim and stops at the first character it cannot prove — `q=Beyoncé` still narrows on `beyonc`, `q=é` narrows on nothing and leans on the row cap. Soundness rests on two facts now asserted by exhaustive computation rather than argued: no non-ASCII codepoint in U+0080..U+10FFFF OrdinalIgnoreCase-equals a printable ASCII character (false for InvariantCultureIgnoreCase, which folds ~190 — the choice of Ordinal is load-bearing), and the exact set of ASCII the encoder escapes. F1b. `UseRequestLocalization` honours Accept-Language, so the culture was caller-controlled and `ToLower()` plus the default linguistic `StartsWith(string)` let a header change the answer. Comparison is now OrdinalIgnoreCase and ordering StringComparer.Ordinal throughout — including the shared FilterSortTake that state/video_dynamic_range/content_rating also use. Sets unchanged, order now ordinal rather than culture-dependent. F2. The merge comment asserted an exactness the code does not have: sources truncate by their own ordering (DB collation / primary key), not the merge's, so a dropped value can outrank a survivor. Comment and record now say best-effort, exact only below the truncation points. F3/F4. The cap now rides `ORDER BY Id` rather than the JSON column: MySQL sorts TEXT by only max_sort_length bytes, so the old ordering was not deterministic there, and sorting the whole matching set was avoidable work. What the cap still does NOT bound is the scan — a leading-wildcard LIKE cannot seek an index — so that cost is now documented as accepted, with a normalized `SongArtist` table named as the follow-up candidate rather than left implicit. Every clause above is covered by a test verified to FAIL when that clause is mutated (old pattern builder: 5 red; culture chain: 3 red; cap=3 / cap=limit / ORDER BY json / no cap: red each). F5. Converted to a proper supersession. The old record did not merely hold a stale fact — it recorded song/music-video credits as an "intentionally-uncovered gap" and album_artist as unsupported, and this reverses that call, which `docs.decision-lifecycle` says is never a line-edit. `api.search-field-values` is archived with its original prose restored, and `api.search-field-values-sources` replaces it carrying the whole endpoint contract. --- .../Queries/GetSearchFieldValuesHandler.cs | 143 ++++++++---- .../GetSearchFieldValuesHandlerTests.cs | 216 ++++++++++++++++-- ...SearchFieldValuesPrefilterSupersetTests.cs | 122 ++++++++++ .../SearchFieldValuesQueryShapeTests.cs | 13 +- ErsatzTV/Controllers/Api/SearchController.cs | 5 +- ErsatzTV/wwwroot/openapi/v1.json | 2 +- docs/api-conventions.md | 17 +- docs/decisions.md | 4 +- docs/decisions/README.md | 3 +- .../api/search-field-values.md | 22 +- .../api/search-field-values-list-columns.md | 75 ------ .../api/search-field-values-sources.md | 116 ++++++++++ docs/spa-conventions.md | 4 +- 13 files changed, 569 insertions(+), 173 deletions(-) create mode 100644 ErsatzTV.Tests/Application/Search/SearchFieldValuesPrefilterSupersetTests.cs rename docs/decisions/{records => archive}/api/search-field-values.md (70%) delete mode 100644 docs/decisions/records/api/search-field-values-list-columns.md create mode 100644 docs/decisions/records/api/search-field-values-sources.md diff --git a/ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs b/ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs index 456934e37..ac8c6a1aa 100644 --- a/ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs +++ b/ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs @@ -18,11 +18,13 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF /// Row cap for the list-valued (JSON-array) columns on SongMetadata. The SQL pre-filter over the /// raw JSON is a deliberate superset (a matching row is fetched whole, including its non-matching /// elements), so the number of rows it can return has to be bounded independently of limit — - /// otherwise an empty q degenerates into materializing the entire table. Combined with the - /// ORDER BY in , the truncation point is deterministic rather than - /// whatever the storage engine happened to emit first. + /// otherwise an empty q degenerates into materializing the entire table. The cap is applied on + /// ORDER BY Id, which is a unique integer primary key on both providers: unlike ordering on the + /// JSON column itself, that makes the truncation point genuinely deterministic (MySQL sorts TEXT + /// using only the first max_sort_length bytes, so long JSON rows sharing a prefix would tie + /// arbitrarily) and lets the engine walk the primary key instead of sorting the whole matching set. /// - private const int ListValuedRowCap = 1000; + internal const int ListValuedRowCap = 1000; /// /// Escape character for the raw-JSON LIKE pre-filter. Deliberately NOT a backslash: the stored @@ -32,6 +34,14 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF /// private const char LikeEscapeChar = '/'; + /// + /// The ASCII characters the JSON writer escapes rather than emitting verbatim (& and friends + /// become &). A prefix character in this set cannot be matched literally against the stored + /// text, so it terminates the narrowing run in . + /// SearchFieldValuesPrefilterSupersetTests pins this set against the encoder itself. + /// + private const string JsonEscapedAscii = "\"&'+<>\\`"; + public async Task> Handle( GetSearchFieldValues request, CancellationToken cancellationToken) @@ -45,17 +55,22 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF } int limit = request.Limit <= 0 ? DefaultLimit : Math.Clamp(request.Limit, 1, MaxLimit); - string qLower = (request.Query ?? string.Empty).ToLower(); + string query = request.Query ?? string.Empty; + + // Invariant, not current-culture: UseRequestLocalization honours Accept-Language, so a caller can select + // tr-TR and turn `q=I` into `ı` — which then matches nothing a Turkish-dotless-i-free library contains. + // This feeds the EF-translated filter, which has no StringComparison overload EF can translate. + string qLower = query.ToLowerInvariant(); // in-memory special cases (no DB query needed) switch (request.Name) { case "state": return new SearchFieldValuesResponseModel( - FilterSortTake(Enum.GetNames(), qLower, limit)); + FilterSortTake(Enum.GetNames(), query, limit)); case "video_dynamic_range": return new SearchFieldValuesResponseModel( - FilterSortTake(["hdr", "sdr"], qLower, limit)); + FilterSortTake(["hdr", "sdr"], query, limit)); } await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken); @@ -63,7 +78,7 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF if (request.Name == "content_rating") { return new SearchFieldValuesResponseModel( - await GetContentRatingValues(dbContext, qLower, limit, cancellationToken)); + await GetContentRatingValues(dbContext, query, limit, cancellationToken)); } IQueryable source = GetSource(dbContext, request.Name); @@ -88,16 +103,18 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF if (listColumn is not null) { - values.AddRange( - await GetSongListValuedValues(dbContext, listColumn, request.Query, qLower, cancellationToken)); + values.AddRange(await GetSongListValuedValues(dbContext, listColumn, query, cancellationToken)); } - // Each contributing source already returned its own prefix-filtered, ascending first-`limit` values, so - // sorting the union and taking `limit` again yields exactly the first `limit` of the combined set. - return new SearchFieldValuesResponseModel(FilterSortTake(values.Distinct(), qLower, limit)); + // ORDERING IS BEST-EFFORT, NOT EXACT. Each source truncates using its own ordering — the EF source by the + // database collation (SQLite's NOCASE/BINARY is ASCII-only), the list source by primary key — and neither + // is the ordinal ordering applied here. So when a source actually truncates, a value it dropped may have + // outranked one that survived: with "Zulu" and "Éclair" and limit=1 the EF source keeps "Zulu" and the + // merge never sees "Éclair". Below the truncation points (the normal typeahead case) the result is exact. + return new SearchFieldValuesResponseModel(FilterSortTake(values.Distinct(StringComparer.Ordinal), query, limit)); } - private static IQueryable GetSource(TvContext dbContext, string name) => name switch + internal static IQueryable GetSource(TvContext dbContext, string name) => name switch { "genre" or "show_genre" => dbContext.Set().Select(g => g.Name), "studio" => dbContext.Set().Select(s => s.Name), @@ -148,16 +165,17 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF /// /// /// Instead: pre-filter on the raw JSON text in SQL (a superset — a row is matched, not an element, - /// so its other elements come along), cap the rows, then split and exact-filter in memory. When the - /// library has more matching rows than the suggestion list for these - /// fields is a deterministic prefix of the matches rather than the complete set. + /// so its other elements come along), cap the rows by primary key, then split and exact-filter in + /// memory. Correctness lives in the in-memory filter; the SQL pattern is only an optimization and is + /// allowed to over-match but never to under-match. When the library has more matching rows than + /// the suggestion list for these fields is the lowest-id prefix of + /// the matches rather than the complete set. /// /// private static async Task> GetSongListValuedValues( TvContext dbContext, string column, string query, - string qLower, CancellationToken cancellationToken) { IEnumerable rows = await dbContext.Connection.QueryAsync( @@ -186,7 +204,7 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF foreach (string element in elements ?? []) { - if (!string.IsNullOrEmpty(element) && element.ToLower().StartsWith(qLower)) + if (!string.IsNullOrEmpty(element) && element.StartsWith(query, StringComparison.OrdinalIgnoreCase)) { values.Add(element); } @@ -199,45 +217,66 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF /// /// The bounded pre-filter query. LOWER(...) LIKE ... ESCAPE, ORDER BY and LIMIT are /// all portable between SQLite and MySQL, and the identifiers are unquoted so neither provider's - /// quoting dialect is baked in. + /// quoting dialect is baked in. The cap rides ORDER BY Id — a unique integer key both providers + /// can walk — rather than the JSON column, so the truncation point is well-defined and no full sort of + /// the matching set is required. /// - private static string ListValuedSql(string column) => + internal static string ListValuedSql(string column) => $"SELECT {column} FROM SongMetadata WHERE {column} IS NOT NULL " + $"AND LOWER({column}) LIKE @Pattern ESCAPE '{LikeEscapeChar}' " + - $"ORDER BY {column} LIMIT @Cap"; + "ORDER BY Id LIMIT @Cap"; /// - /// Builds the raw-JSON LIKE pattern that matches a row containing an element starting with - /// . Anchoring on the opening quote (%"prefix%) keeps the superset - /// tight: it matches element starts, not substrings anywhere in the row. + /// Builds the raw-JSON LIKE pattern used to narrow the rows fetched for a list-valued column. /// - /// The prefix is JSON-encoded first, because that is how it is stored — EF writes the array with the - /// default JavaScriptEncoder, so Beyoncé lands on disk as Beyoncé and a - /// literal quote as ". Encoding also makes the pattern pure ASCII, which is why this - /// match means the same thing on both providers: SQLite's lower() is ASCII-only and MySQL's - /// is Unicode-aware, but on ASCII input they agree. Lowercasing the pattern here (rather than - /// relying on LIKE's own case rules) is what keeps the comparison correct under a - /// case-sensitive MySQL column collation as well as a case-insensitive one; under a - /// case-insensitive collation it simply widens the superset, which the in-memory exact filter - /// then narrows again. + /// This is an optimization with one hard obligation: it must never under-match. Every row + /// holding an element that starts with under + /// — the comparison + /// then applies — has to survive it. Over-matching is free; + /// the in-memory filter discards the extras. + /// + /// + /// So the pattern narrows on the leading run of ASCII characters the JSON writer stores + /// verbatim, and stops at the first character it cannot reason about — a non-ASCII character, or + /// one of . A query starting with such a character narrows to the bare + /// element-opening anchor %"% and leans entirely on the row cap and the in-memory filter. + /// + /// + /// Why that is sound, and why an earlier "JSON-encode the whole prefix" version was NOT: the stored + /// text escapes non-ASCII, so Édith is on disk as Édith. SQL LOWER() + /// lowercases the escape text, giving É — it cannot case-fold the codepoint that + /// escape denotes, so a q=é pattern of é never matched and every accented artist + /// was silently unsuggestable. Restricting the run to verbatim ASCII removes the escape from the + /// comparison entirely. And for a run character c, any element character that + /// OrdinalIgnoreCase-equals c is itself ASCII — no non-ASCII codepoint in all of Unicode + /// OrdinalIgnoreCase-equals a printable ASCII character, which + /// SearchFieldValuesPrefilterSupersetTests proves by exhaustive sweep rather than assertion. + /// (That is false for InvariantCultureIgnoreCase, which has 190 such codepoints — the choice + /// of Ordinal is load-bearing, not stylistic.) ASCII case-folding is exactly what both providers' + /// LOWER() implements, and lowercasing the pattern here rather than trusting LIKE's own + /// case rules keeps it correct under a case-sensitive MySQL collation as well as a case-insensitive + /// one. /// /// - private static string JsonElementPrefixPattern(string query) + internal static string JsonElementPrefixPattern(string query) { - string encoded = JsonSerializer.Serialize(query ?? string.Empty); - - // strip the quotes JsonSerializer wraps a string in - encoded = encoded.Substring(1, encoded.Length - 2); - var builder = new StringBuilder("%\""); - foreach (char c in encoded.ToLowerInvariant()) + + foreach (char c in query ?? string.Empty) { - if (c is '%' or '_' or LikeEscapeChar) + if (!char.IsAscii(c) || char.IsControl(c) || JsonEscapedAscii.Contains(c)) + { + // cannot prove this character's stored form, so stop narrowing here + break; + } + + char lower = char.ToLowerInvariant(c); + if (lower is '%' or '_' or LikeEscapeChar) { builder.Append(LikeEscapeChar); } - builder.Append(c); + builder.Append(lower); } return builder.Append('%').ToString(); @@ -245,7 +284,7 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF private static async Task> GetContentRatingValues( TvContext dbContext, - string qLower, + string query, int limit, CancellationToken cancellationToken) { @@ -264,13 +303,21 @@ public class GetSearchFieldValuesHandler(IDbContextFactory dbContextF .Where(cr => !string.IsNullOrEmpty(cr)) .Distinct(); - return FilterSortTake(split, qLower, limit); + return FilterSortTake(split, query, limit); } - private static List FilterSortTake(IEnumerable values, string qLower, int limit) => + /// + /// The one in-memory filter/sort/take every field funnels through. Both the comparison and the ordering + /// are ORDINAL on purpose: UseRequestLocalization honours Accept-Language, so the current + /// culture is caller-controlled, and ToLower() plus the default (linguistic) + /// StartsWith(string) would make the result depend on it — under tr-TR, q=I lowers + /// to ı and stops matching Istanbul. Ordinal is also what the SQL pre-filter's superset + /// guarantee is proved against; see . + /// + private static List FilterSortTake(IEnumerable values, string query, int limit) => values - .Where(v => v.ToLower().StartsWith(qLower)) - .OrderBy(v => v) + .Where(v => v.StartsWith(query, StringComparison.OrdinalIgnoreCase)) + .OrderBy(v => v, StringComparer.Ordinal) .Take(limit) .ToList(); } diff --git a/ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs b/ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs index 8b5045948..e83d2896e 100644 --- a/ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs +++ b/ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs @@ -1,3 +1,4 @@ +using System.Globalization; using ErsatzTV.Application.Search.Queries; using ErsatzTV.Core.Api.Search; using ErsatzTV.Core.Domain; @@ -315,50 +316,95 @@ public class GetSearchFieldValuesHandlerTests } [Test] - public async Task List_Valued_Source_Is_Bounded_By_A_Row_Cap() + public async Task List_Valued_Source_Row_Cap_Is_Exactly_1000_Rows() { - // The handler caps the raw-JSON pre-filter at 1000 rows, ordered by the raw column. Seed one row past - // the cap whose JSON sorts last but whose element would sort FIRST in the output: an unbounded - // implementation returns it, a capped one cannot. - const string overCap = "aaa-past-the-row-cap"; + // Pins the cap value in BOTH directions. Asserting only that an over-cap row is dropped would also pass + // with a cap of 3, or with the handler wrongly using `limit` as the row cap; asserting only that an + // under-cap row survives would pass with no cap at all. The marker row's element sorts first in the + // output, so an unbounded implementation cannot help but return it. + const string marker = "aaa-the-marker-row"; - await using (TvContext context = _db.CreateContext()) + // rows are capped by ascending Id, and Id is assigned in insertion order + async Task SeedFillerThenMarker(int fillerRows) { - for (var i = 0; i < 1000; i++) + await using TvContext context = _db.CreateContext(); + for (var i = 0; i < fillerRows; i++) { context.SongMetadata.Add(Song($"Song {i}", [$"b{i:D4}"])); } - context.SongMetadata.Add(Song("Over", ["zzz-sorts-last", overCap])); + context.SongMetadata.Add(Song("Marker", [marker])); await context.SaveChangesAsync(); } var handler = new GetSearchFieldValuesHandler(_db.Factory); - Option result = await handler.Handle( + // 999 filler rows -> the marker is row 1000 -> inside a cap of 1000 + await SeedFillerThenMarker(999); + + Option atCap = await handler.Handle( new GetSearchFieldValues("artist", string.Empty, 3), CancellationToken.None); - result.IsSome.ShouldBeTrue(); - result.IfSome(r => r.Values.ShouldBe(new List { "b0000", "b0001", "b0002" })); - result.IfSome(r => r.Values.ShouldNotContain(overCap)); + atCap.IsSome.ShouldBeTrue(); + atCap.IfSome( + r => r.Values.ShouldBe( + new List { marker, "b0000", "b0001" }, + "the 1000th row must still be read - a cap below 1000 would drop it")); - // positive control: the same row IS reachable once the pre-filter narrows the candidate set below the cap + // one more filler row pushes the marker to row 1001 -> outside a cap of 1000 + await using (TvContext context = _db.CreateContext()) + { + context.SongMetadata.Add(Song("Extra", ["b9999"])); + await context.SaveChangesAsync(); + } + + await using (TvContext context = _db.CreateContext()) + { + // move the marker to the end so it sits at Id 1002, past the cap + SongMetadata existing = await context.SongMetadata.SingleAsync(m => m.Title == "Marker"); + context.SongMetadata.Remove(existing); + await context.SaveChangesAsync(); + context.SongMetadata.Add(Song("Marker", [marker])); + await context.SaveChangesAsync(); + + (await context.SongMetadata.CountAsync()).ShouldBe(1001); + } + + Option pastCap = await handler.Handle( + new GetSearchFieldValues("artist", string.Empty, 3), + CancellationToken.None); + + pastCap.IsSome.ShouldBeTrue(); + pastCap.IfSome( + r => r.Values.ShouldNotContain( + marker, + "the 1001st row must not be read - an unbounded query would return it first")); + pastCap.IfSome(r => r.Values.ShouldBe(new List { "b0000", "b0001", "b0002" })); + + // positive control: the same row IS reachable once the pre-filter narrows the candidates below the cap, + // so the assertion above cannot be passing because the value is unreachable for an unrelated reason Option narrowed = await handler.Handle( new GetSearchFieldValues("artist", "aaa", 50), CancellationToken.None); narrowed.IsSome.ShouldBeTrue(); - narrowed.IfSome(r => r.Values.ShouldBe(new List { overCap })); + narrowed.IfSome(r => r.Values.ShouldBe(new List { marker })); } [Test] - public async Task List_Valued_Columns_Are_Stored_As_Ascii_Escaped_Json_Arrays() + public async Task Regression_Pin_List_Valued_Columns_Are_Stored_As_Ascii_Escaped_Json_Arrays() { - // Pins the on-disk encoding the LIKE pre-filter is built against. If an EF upgrade changes the - // JavaScriptEncoder (e.g. stops escaping non-ASCII), the pattern would silently stop matching — - // this fails loudly instead. It also documents why LOWER() is provider-independent here: the - // stored text is pure ASCII, so SQLite's ASCII-only lower() and MySQL's Unicode-aware LOWER() agree. + // REGRESSION PIN, not coverage of #578: this asserts pre-existing EF behaviour and passes against the + // code before this change. It is here because the pre-filter's correctness argument depends on the + // stored form, so an encoder change must fail loudly rather than silently under-match. + // + // Specifically it pins that non-ASCII IS escaped — which is exactly why SQL LOWER() canNOT be used to + // case-fold it. LOWER() lowercases the escape TEXT (\u00E9 -> \u00e9); it does not touch the codepoint + // the escape denotes, so the stored `É` never folds to `é`. An earlier version of this change + // claimed the opposite and JSON-encoded the whole query prefix, which silently made every accented + // artist unsuggestable. The pattern builder now narrows only on verbatim ASCII; see + // SearchFieldValuesPrefilterSupersetTests. await using (TvContext context = _db.CreateContext()) { context.SongMetadata.Add(Song("One", ["Beyoncé", "\"Q\""])); @@ -375,6 +421,138 @@ public class GetSearchFieldValuesHandlerTests } } + [Test] + [TestCase("é", "\u00C9dith Piaf")] + [TestCase("\u00C9", "\u00C9dith Piaf")] + [TestCase("\u00E9dith", "\u00C9dith Piaf")] + [TestCase("bj", "Bj\u00F6rk")] + [TestCase("bj\u00F6", "Bj\u00F6rk")] + [TestCase("BJ\u00D6RK", "Bj\u00F6rk")] + [TestCase("beyonc\u00E9", "Beyonc\u00E9")] + [TestCase("sigur r", "Sigur R\u00F3s")] + [TestCase("\u00D6", "\u00D6zdemir")] + public async Task Prefilter_Never_Under_Matches_A_NonAscii_Value(string query, string stored) + { + // The SQL pre-filter narrows on raw JSON, where non-ASCII is stored escaped (\u00C9). Any narrowing + // that includes an escaped character under-matches, because SQL LOWER() folds the escape TEXT and not + // the codepoint — every one of these cases returned EMPTY before the pattern builder was restricted to + // the leading verbatim-ASCII run. Accented artists are the common case in a music library, not an edge. + await using (TvContext context = _db.CreateContext()) + { + context.SongMetadata.AddRange( + Song("Hit", [stored]), + // negative control: a row that must never come back for any of these queries + Song("Other", ["Nothing Relevant"])); + await context.SaveChangesAsync(); + } + + var handler = new GetSearchFieldValuesHandler(_db.Factory); + + Option result = await handler.Handle( + new GetSearchFieldValues("artist", query, 50), + CancellationToken.None); + + result.IsSome.ShouldBeTrue(); + result.IfSome(r => r.Values.ShouldBe(new List { stored })); + } + + [Test] + public async Task Results_Do_Not_Depend_On_The_Request_Culture() + { + // UseRequestLocalization honours Accept-Language, so CurrentCulture is caller-controlled. Under tr-TR + // the old `q.ToLower()` turned "I" into "\u0131" and the default linguistic StartsWith(string) compounded + // it, so the same library answered differently per caller. The contract is ordinal: "I" matches + // "Istanbul" and does NOT match "\u0131pek", in every culture. + await using (TvContext context = _db.CreateContext()) + { + context.SongMetadata.Add(Song("One", ["Istanbul Orkestrasi", "\u0131pek"])); + context.ArtistMetadata.Add(Artist("Idil Biret")); + await context.SaveChangesAsync(); + } + + var handler = new GetSearchFieldValuesHandler(_db.Factory); + var expected = new List { "Idil Biret", "Istanbul Orkestrasi" }; + + CultureInfo original = CultureInfo.CurrentCulture; + try + { + foreach (string culture in new[] { "en-US", "tr-TR", "az-AZ", "lt-LT" }) + { + CultureInfo.CurrentCulture = new CultureInfo(culture); + + Option result = await handler.Handle( + new GetSearchFieldValues("artist", "I", 50), + CancellationToken.None); + + result.IsSome.ShouldBeTrue(); + result.IfSome(r => r.Values.ShouldBe(expected, $"culture {culture} changed the result")); + } + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Test] + public async Task Ordering_Is_Ordinal_And_Culture_Independent() + { + // The merge sorts ordinally rather than by culture, so the response order does not depend on the caller + // either. Ordinal puts all ASCII uppercase before ASCII lowercase, and non-ASCII last. + await using (TvContext context = _db.CreateContext()) + { + context.SongMetadata.Add(Song("One", ["Zulu", "apple", "\u00C9clair", "Apple"])); + await context.SaveChangesAsync(); + } + + var handler = new GetSearchFieldValuesHandler(_db.Factory); + var expected = new List { "Apple", "Zulu", "apple", "\u00C9clair" }; + + CultureInfo original = CultureInfo.CurrentCulture; + try + { + foreach (string culture in new[] { "en-US", "sv-SE" }) + { + CultureInfo.CurrentCulture = new CultureInfo(culture); + + Option result = await handler.Handle( + new GetSearchFieldValues("artist", string.Empty, 50), + CancellationToken.None); + + result.IfSome(r => r.Values.ShouldBe(expected, $"culture {culture} changed the order")); + } + } + finally + { + CultureInfo.CurrentCulture = original; + } + } + + [Test] + public async Task Ordering_Is_Best_Effort_When_A_Source_Truncates() + { + // Documents the acknowledged imprecision rather than claiming exactness the code does not have. The EF + // source truncates by the DATABASE collation (SQLite's is ASCII-only), which is not the ordinal ordering + // the merge then applies — so a value the database ranked outside its first `limit` never reaches the + // merge, even if the merge would have ranked it first. + await using (TvContext context = _db.CreateContext()) + { + context.ArtistMetadata.Add(Artist("Zulu")); + context.ArtistMetadata.Add(Artist("\u00C9clair")); + await context.SaveChangesAsync(); + } + + var handler = new GetSearchFieldValuesHandler(_db.Factory); + + // with room for both, the ordinal merge ranks "Zulu" (ASCII) before "\u00C9clair" (non-ASCII) + (await handler.Handle(new GetSearchFieldValues("artist", string.Empty, 50), CancellationToken.None)) + .IfSome(r => r.Values.ShouldBe(new List { "Zulu", "\u00C9clair" })); + + // with limit=1 the database picks the survivor by ITS collation, and the merge only sees that one + (await handler.Handle(new GetSearchFieldValues("artist", string.Empty, 1), CancellationToken.None)) + .IfSome(r => r.Values.Count.ShouldBe(1)); + } + private static ArtistMetadata Artist(string title) => new() { MetadataKind = MetadataKind.External, diff --git a/ErsatzTV.Tests/Application/Search/SearchFieldValuesPrefilterSupersetTests.cs b/ErsatzTV.Tests/Application/Search/SearchFieldValuesPrefilterSupersetTests.cs new file mode 100644 index 000000000..f02958c11 --- /dev/null +++ b/ErsatzTV.Tests/Application/Search/SearchFieldValuesPrefilterSupersetTests.cs @@ -0,0 +1,122 @@ +using System.Text.Json; +using ErsatzTV.Application.Search.Queries; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Application.Search; + +/// +/// Proof obligations for the raw-JSON LIKE pre-filter that narrows the list-valued facet-value +/// sources (#578). The pre-filter is an optimization layered under an in-memory exact filter, so it is +/// allowed to over-match but must NEVER under-match — an under-match is a silent false negative, invisible +/// to any test that only checks the values it does return. +/// +/// The end-to-end behavioural cases live in GetSearchFieldValuesHandlerTests. These are the two +/// facts the superset argument rests on, asserted directly rather than trusted. +/// +/// +[TestFixture] +public class SearchFieldValuesPrefilterSupersetTests +{ + /// + /// Printable ASCII, the alphabet + /// narrows over (minus whatever the JSON writer escapes, which the sibling test pins). + /// + private static IEnumerable PrintableAscii() + { + for (var c = ' '; c <= '~'; c++) + { + yield return c; + } + } + + [Test] + public void No_NonAscii_Codepoint_OrdinalIgnoreCase_Equals_A_Printable_Ascii_Character() + { + // THIS is why the narrowing run may be restricted to ASCII and still be a superset. The in-memory filter + // compares with OrdinalIgnoreCase, so for a run character `c` the pre-filter must catch every element + // character that OrdinalIgnoreCase-equals `c`. If any non-ASCII codepoint did, that element would be + // stored as a \uXXXX escape, the ASCII pattern would miss the row, and the value would be unsuggestable. + // + // Note this is NOT true of InvariantCultureIgnoreCase, which folds ~190 codepoints (U+00AA -> a, + // U+017F -> s, the modifier letters, ...) onto ASCII letters. The choice of Ordinal is load-bearing. + char[] ascii = PrintableAscii().ToArray(); + var offenders = new List(); + + for (var cp = 0x80; cp <= 0x10FFFF; cp++) + { + if (cp is >= 0xD800 and <= 0xDFFF) + { + continue; + } + + string s = char.ConvertFromUtf32(cp); + foreach (char a in ascii) + { + if (string.Equals(s, a.ToString(), StringComparison.OrdinalIgnoreCase)) + { + offenders.Add($"U+{cp:X4} == '{a}'"); + } + } + } + + offenders.ShouldBeEmpty(); + } + + [Test] + public void The_Json_Writer_Escapes_Exactly_The_Ascii_Characters_The_Pattern_Builder_Refuses_To_Narrow_On() + { + // The narrowing run may only contain characters stored VERBATIM. Anything the writer escapes is stored + // as \uXXXX and must terminate the run. If an encoder change adds a character to this set, the pattern + // builder's exclusion list goes stale and starts under-matching — so pin the set, not a sample. + var escaped = PrintableAscii() + .Where(c => JsonSerializer.Serialize(c.ToString()) != $"\"{c}\"") + .ToArray(); + + escaped.ShouldBe(['"', '&', '\'', '+', '<', '>', '\\', '`']); + + // and the pattern builder must stop at every one of them (empty narrowing run -> bare anchor) + foreach (char c in escaped) + { + GetSearchFieldValuesHandler.JsonElementPrefixPattern($"{c}abc") + .ShouldBe("%\"%", $"expected no narrowing for the JSON-escaped '{c}'"); + } + } + + [Test] + public void Pattern_Narrows_Only_On_The_Leading_Verbatim_Ascii_Run() + { + // plain ASCII: full narrowing, lowercased + GetSearchFieldValuesHandler.JsonElementPrefixPattern("Radio").ShouldBe("%\"radio%"); + + // non-ASCII terminates the run, keeping the ASCII head as the narrowing + GetSearchFieldValuesHandler.JsonElementPrefixPattern("Beyoncé").ShouldBe("%\"beyonc%"); + + // a leading non-ASCII character means no narrowing at all + GetSearchFieldValuesHandler.JsonElementPrefixPattern("édith").ShouldBe("%\"%"); + GetSearchFieldValuesHandler.JsonElementPrefixPattern("Ünen").ShouldBe("%\"%"); + + // empty query: the bare element-opening anchor + GetSearchFieldValuesHandler.JsonElementPrefixPattern(string.Empty).ShouldBe("%\"%"); + GetSearchFieldValuesHandler.JsonElementPrefixPattern(null).ShouldBe("%\"%"); + + // LIKE wildcards inside the run are escaped, not passed through + GetSearchFieldValuesHandler.JsonElementPrefixPattern("50%").ShouldBe("%\"50/%%"); + GetSearchFieldValuesHandler.JsonElementPrefixPattern("a_b").ShouldBe("%\"a/_b%"); + + // the escape character itself is escaped + GetSearchFieldValuesHandler.JsonElementPrefixPattern("AC/DC").ShouldBe("%\"ac//dc%"); + } + + [Test] + public void Row_Cap_Is_Applied_On_The_Primary_Key_Not_The_Json_Column() + { + // MySQL sorts TEXT/longtext using only max_sort_length bytes, so ordering on the JSON column would let + // rows sharing a long prefix tie arbitrarily and the surviving 1000 would be unspecified. Id is a unique + // integer key on both providers. + string sql = GetSearchFieldValuesHandler.ListValuedSql("Artists"); + + sql.ShouldContain("ORDER BY Id LIMIT @Cap"); + sql.ShouldNotContain("ORDER BY Artists"); + } +} diff --git a/ErsatzTV.Tests/Application/Search/SearchFieldValuesQueryShapeTests.cs b/ErsatzTV.Tests/Application/Search/SearchFieldValuesQueryShapeTests.cs index e3c44f535..d43e71d70 100644 --- a/ErsatzTV.Tests/Application/Search/SearchFieldValuesQueryShapeTests.cs +++ b/ErsatzTV.Tests/Application/Search/SearchFieldValuesQueryShapeTests.cs @@ -1,4 +1,4 @@ -using ErsatzTV.Core.Domain; +using ErsatzTV.Application.Search.Queries; using ErsatzTV.Infrastructure; using ErsatzTV.Infrastructure.Data; using Microsoft.EntityFrameworkCore; @@ -36,9 +36,9 @@ public class SearchFieldValuesQueryShapeTests { using TvContext context = create(); - // exactly the queryable half of GetSearchFieldValuesHandler.GetSource("artist") - string sql = context.ArtistMetadata.Select(m => m.Title) - .Concat(context.Set().Select(a => a.Name)) + // calls the handler's own source builder (internal, via InternalsVisibleTo) rather than rebuilding + // the LINQ here — a copy would keep passing after the handler's query changed underneath it + string sql = GetSearchFieldValuesHandler.GetSource(context, "artist") .Where(v => v != null && v.ToLower().StartsWith("a")) .Distinct() .OrderBy(v => v) @@ -55,8 +55,11 @@ public class SearchFieldValuesQueryShapeTests } [Test] - public void Song_List_Columns_Cannot_Be_Projected_Server_Side_On_Either_Provider() + public void Regression_Pin_Song_List_Columns_Cannot_Be_Projected_Server_Side_On_Either_Provider() { + // REGRESSION PIN, not coverage of #578: this asserts pre-existing EF/provider behaviour and passes + // against the code before this change. + // // Documents WHY the handler drops to raw SQL for SongMetadata.Artists / .AlbumArtists rather than // SelectMany-ing them: EF maps them as JSON primitive collections and neither provider can translate // the projection (SQLite needs APPLY; Pomelo has no primitive-collection support). If a provider diff --git a/ErsatzTV/Controllers/Api/SearchController.cs b/ErsatzTV/Controllers/Api/SearchController.cs index 78e306cb4..9f794c634 100644 --- a/ErsatzTV/Controllers/Api/SearchController.cs +++ b/ErsatzTV/Controllers/Api/SearchController.cs @@ -205,8 +205,9 @@ public class SearchController(IMediator mediator) : ControllerBase "Returns distinct whole values from the database for the given text field, filtered by an " + "optional case-insensitive prefix. Powers the visual rule builder's facet-value typeahead. " + "404 when the field is unknown, is not a text field, or is a text field with no distinct-value " + - "source. The list-valued music fields (artist, album_artist) are bounded best-effort: on a very " + - "large library they return a deterministic subset of the matches rather than every match.")] + "source. Matching, dedup and ordering are ordinal, not culture-dependent. The list-valued music " + + "fields (artist, album_artist) are bounded best-effort: on a very large library they return a " + + "bounded subset of the matches rather than every match.")] [EndpointGroupName("general")] [ProducesResponseType(typeof(SearchFieldValuesResponseModel), StatusCodes.Status200OK)] [ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)] diff --git a/ErsatzTV/wwwroot/openapi/v1.json b/ErsatzTV/wwwroot/openapi/v1.json index 9d15967d6..22395ee44 100644 --- a/ErsatzTV/wwwroot/openapi/v1.json +++ b/ErsatzTV/wwwroot/openapi/v1.json @@ -18063,7 +18063,7 @@ "Search" ], "summary": "List distinct database values for a text search field", - "description": "Returns distinct whole values from the database for the given text field, filtered by an optional case-insensitive prefix. Powers the visual rule builder's facet-value typeahead. 404 when the field is unknown, is not a text field, or is a text field with no distinct-value source. The list-valued music fields (artist, album_artist) are bounded best-effort: on a very large library they return a deterministic subset of the matches rather than every match.", + "description": "Returns distinct whole values from the database for the given text field, filtered by an optional case-insensitive prefix. Powers the visual rule builder's facet-value typeahead. 404 when the field is unknown, is not a text field, or is a text field with no distinct-value source. Matching, dedup and ordering are ordinal, not culture-dependent. The list-valued music fields (artist, album_artist) are bounded best-effort: on a very large library they return a bounded subset of the matches rather than every match.", "operationId": "GetSearchFieldValues", "parameters": [ { diff --git a/docs/api-conventions.md b/docs/api-conventions.md index fc2b28e11..06567872f 100644 --- a/docs/api-conventions.md +++ b/docs/api-conventions.md @@ -456,11 +456,18 @@ No server-side caching. Powers the visual rule builder's value-input combobox fo or partly) by `SongMetadata.Artists`/`AlbumArtists`, which EF maps as **primitive collections** — one JSON array per row in a single column, with no server-side projection on either provider. `album_artist` therefore no longer 404s, and `artist` now also covers free-text music-video (`MusicVideoArtist`) and -song credits, not only entity artists. For these fields the handler pre-filters on the raw JSON in SQL -and caps the rows it reads, so on a very large library the response is a **deterministic subset** of the -matches rather than every match — say so in the `[EndpointDescription]` of any endpoint that adopts this -shape, and never let an unbounded scan stand in for it. Full rationale, the rejected normalized-side-table -alternative and the dual-provider `LIKE`/`LOWER` portability rules: `api.search-field-values-list-columns`. +song credits, not only entity artists. For these fields the handler narrows on the raw JSON in SQL and +caps the rows it reads, so on a very large library the response is a bounded subset of the matches rather +than every match — say so in the `[EndpointDescription]` of any endpoint that adopts this shape. + +Two rules generalize beyond this endpoint. **A SQL pre-filter layered under an in-memory exact filter may +over-match but must NEVER under-match** — an under-match is a silent false negative that no assertion over +the returned values can see, so prove the superset (ours is proved by an exhaustive Unicode sweep, not by +argument) rather than eyeballing it. And **prefix matching, dedup and ordering on any `/api/*` read must be +ordinal, never current-culture**: `UseRequestLocalization` honours `Accept-Language`, so `ToLower()` and the +default linguistic `StartsWith(string)` let a caller change the result by changing a header. Full rationale, +the rejected normalized-side-table alternative, the accepted scan cost and the dual-provider `LIKE`/`LOWER` +rules: `api.search-field-values-sources` (which supersedes `api.search-field-values`). **Param + DTO expansion (#293, cap `search/all-items`)**: no new endpoint — `GET /api/v1/search/all-items` gained two **optional** query params (`pageSize` default 500, clamped 1–1000 via the §1 Logs `Math.Clamp` diff --git a/docs/decisions.md b/docs/decisions.md index 9ea033132..2b33c74b1 100644 --- a/docs/decisions.md +++ b/docs/decisions.md @@ -206,7 +206,7 @@ another doc or an old issue comment should land here and then follow the link. - 2026-07-22 — per-schedule clock-boundary padding is a synthetic content-less Pad over the existing per-episode machinery (#392) — [`sched.clock-padding-schedule-toggle`](decisions/records/sched/clock-padding-schedule-toggle.md) - 2026-07-23 — Channel health = a server-derived `health` object on the channel DTOs, built-timeline detection (#415) — [`api.channel-health-object`](decisions/records/api/channel-health-object.md) - 2026-07-23 — Channel origin is immutable creation-provenance, stamped at insert, not a health signal (#414) — [`channel.origin-marker`](decisions/records/channel/origin-marker.md) -- 2026-07-23 — Facet-value typeahead is a new endpoint, allow-listed to text fields, no caching (#434) — [`api.search-field-values`](decisions/records/api/search-field-values.md) +- 2026-07-23 — Facet-value typeahead is a new endpoint, allow-listed to text fields, no caching (#434) — [`api.search-field-values`](decisions/archive/api/search-field-values.md) (superseded by `api.search-field-values-sources`) - 2026-07-23 — Relative-date rule builder operators are a frontend-only mapping onto existing Lucene macros (#435) — [`rulebuilder.relative-date-macros`](decisions/records/rulebuilder/relative-date-macros.md) - 2026-07-25 — A media-server sweep also refuses when the api client silently dropped items whose projection threw; the ratio threshold is rejected (#484) — [`scan.projection-failure-sweep-guard`](decisions/records/scan/projection-failure-sweep-guard.md) - 2026-07-25 — LibraryFolder identity is enforced by a unique index on `(LibraryPathId, PathHash)`, not an in-process lock (#491) — [`scan.libraryfolder-unique-identity`](decisions/records/scan/libraryfolder-unique-identity.md) @@ -215,4 +215,4 @@ another doc or an old issue comment should land here and then follow the link. - 2026-07-25 — Rule-builder group nesting is bounded-arbitrary depth (`MAX_GROUP_DEPTH`), not one level (#436) — [`spa.rulebuilder-nesting`](decisions/records/spa/rulebuilder-nesting.md) - 2026-07-25 — The rationale-edit marker is a git trailer, not a substring anywhere in the commit range (#609) — [`ci.decisions-edit-trailer`](decisions/records/ci/decisions-edit-trailer.md) - 2026-07-25 — UI-E2E: headless Playwright flows in the existing `functional-e2e` job, browser baked into the CI image (#445) — [`ci.ui-e2e-harness`](decisions/records/ci/ui-e2e-harness.md) -- 2026-07-26 — List-valued (JSON primitive collection) facet fields get bounded best-effort values via a raw-JSON LIKE pre-filter, not a normalized side table (#578) — [`api.search-field-values-list-columns`](decisions/records/api/search-field-values-list-columns.md) +- 2026-07-26 — Facet-value typeahead restated: every artist source covered; list-valued columns via a superset LIKE pre-filter that may over-match but never under-match (#578) — [`api.search-field-values-sources`](decisions/records/api/search-field-values-sources.md) diff --git a/docs/decisions/README.md b/docs/decisions/README.md index ea26ec8c7..72bbd138b 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -28,8 +28,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `api.schedule-item-flat-dto` | Schedule-item GET/POST/PUT use a flat, non-polymorphic `ScheduleItemResponseModel` (every subtype field promoted to a nullable top-level member) instead of the polymorphic Application VM hierarchy, with mutation field names matching `ScheduleItemRequest` 1:1 for a lossless round-trip. | 2026-07-10 | [link](records/api/schedule-item-flat-dto.md) | | `api.scheduling-hardening` | Create/Replace handlers guard against null/whitespace `name` (`IsNullOrWhiteSpace`, not just `Length`) to prevent NRE-500s, template-item overlap validation compares by index (not record value-equality) to catch exact-duplicate items, and unreachable 404 `ProducesResponseType` attributes on create-only actions are trimmed. | 2026-07-13 | [link](records/api/scheduling-hardening.md) | | `api.search-allitems-paging` | `GET /api/v1/search/all-items` is paginated (capped page size, `Totals` field) to bound DoS exposure; the SPA add-all flow pages to completeness instead of relying on an unbounded response. | 2026-07-18 | [link](records/api/search-allitems-paging.md) | -| `api.search-field-values` | `GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for one of a narrow allow-list of catalog fields (not the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a typeahead suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source; case-insensitive prefix-filtered on `q`, `limit` clamped to `[1, 50]` (default 50). | 2026-07-23 | [link](records/api/search-field-values.md) | -| `api.search-field-values-list-columns` | A `text` search field whose values live in an EF **primitive collection** (one JSON array per row in a single column — `SongMetadata.Artists`, `SongMetadata.AlbumArtists`) IS served by `GET /api/v1/search/fields/{name}/values`, not 404'd, but as **bounded best-effort**: SQL pre-filters on the raw JSON with `LOWER(col) LIKE '%"%' ESCAPE '/'` — a deliberate superset, since it matches a row, not an element — capped at a fixed row cap (1000) with an `ORDER BY` on the raw column so the truncation point is deterministic; the rows are then split, exact-prefix-filtered, deduped, sorted and `limit`ed in memory. An empty `q` is supported (it degenerates to `%"%`, still capped), so these fields need no special-case client contract. The prefix is JSON-encoded before matching because that is how it is stored, which also makes the pattern pure ASCII and therefore makes `LOWER()` mean the same thing on SQLite (ASCII-only) and MySQL (Unicode-aware); the pattern is lowercased in C# rather than relying on `LIKE`'s own case rules, so the match is correct under a case-sensitive MySQL collation as well as a case-insensitive one. The escape character is `/`, never `\` — `ESCAPE '\'` is not a portable SQL literal. | 2026-07-26 | [link](records/api/search-field-values-list-columns.md) | +| `api.search-field-values-sources` | `GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for a narrow allow-list of catalog fields (never the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source (`title`, `show_title` only); `limit` clamped to `[1, 50]` (default 50). Prefix matching, dedup and ordering are ORDINAL (`OrdinalIgnoreCase` / `StringComparer.Ordinal`), never current-culture, because `UseRequestLocalization` makes the culture caller-controlled. A field whose values live in an EF **primitive collection** (one JSON array per row in a single column: `SongMetadata.Artists`, `SongMetadata.AlbumArtists`) is served, not 404'd, as bounded best-effort: SQL narrows on the raw JSON with a `LIKE` pattern built ONLY from the leading run of characters the JSON writer stores verbatim (printable ASCII minus `"&'+<>\`), stopping at the first character it cannot prove — this pre-filter is an optimization that MAY over-match and MUST NEVER under-match, with correctness living in the in-memory exact filter; the rows are capped at 1000 on `ORDER BY Id` (a unique integer key, not the JSON column) and then split, filtered, deduped, sorted and `limit`ed in memory. Ordering across merged sources is best-effort, exact only below the truncation points. | 2026-07-26 | [link](records/api/search-field-values-sources.md) | | `api.search-paging-cap` | Search stays capped at 100 items per media kind; an overflowing kind's "See all" reuses library-browse paging instead of adding new API surface. | 2026-07-11 | [link](records/api/search-paging-cap.md) | | `api.versioning-v1` | The entire `/api` surface is versioned to `/api/v1` uniformly (no unversioned corner); legacy unversioned callers are rewritten in-pipeline (not redirected) with Deprecation/Link/Sunset headers, and post-freeze `/api/v1` is additive-only — a breaking change requires `/api/v2`. | 2026-07-13 | [link](records/api/versioning-v1.md) | | `blazor.rollback-tag` | The commit immediately preceding the Blazor-removal merge is tagged `blazor-final` (not a `v*` tag, so it doesn't trigger a prod release build) as the documented rollback/restore path. | 2026-07-11 | [link](records/blazor/rollback-tag.md) | diff --git a/docs/decisions/records/api/search-field-values.md b/docs/decisions/archive/api/search-field-values.md similarity index 70% rename from docs/decisions/records/api/search-field-values.md rename to docs/decisions/archive/api/search-field-values.md index 68262f3ca..d1578a20f 100644 --- a/docs/decisions/records/api/search-field-values.md +++ b/docs/decisions/archive/api/search-field-values.md @@ -1,13 +1,13 @@ --- key: api.search-field-values title: 2026-07-23 — Facet-value typeahead is a new endpoint, allow-listed to text fields, no caching (#434) -status: active +status: superseded since: '2026-07-23' supersedes: none -superseded-by: none -rule: '`GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for one of a narrow allow-list of catalog fields (not the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a typeahead suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source; case-insensitive prefix-filtered on `q`, `limit` clamped to `[1, 50]` (default 50).' +superseded-by: api.search-field-values-sources@2026-07-26 +rule: '(superseded) `GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for one of a narrow allow-list of catalog fields (not the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a typeahead suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source; case-insensitive prefix-filtered on `q`, `limit` clamped to `[1, 50]` (default 50).' signals: 'facet-value typeahead, rule builder value combobox, distinct field values, GetSearchFieldValues, text field allow-list, DB-sourced distinct values, content_rating split · paths: `ErsatzTV/Controllers/Api/SearchController.cs`, `ErsatzTV.Application/Search/Queries/GetSearchFieldValues.cs`, `ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs`, `web/src/api/search.ts` · issues: #434, #176' -mechanics: '`SearchController.GetSearchFieldValues`; `GetSearchFieldValuesHandler`; api-conventions.md; spa-conventions.md §12' +mechanics: superseded by `api.search-field-values-sources` (ersatztv#578), which keeps this endpoint contract and reverses the "no distinct-value source" call for the list-valued music fields --- Enum fields (e.g. `type`, `content_rating` group) already ship their allowed values inline on @@ -23,9 +23,8 @@ an empty list for a field that will never have values — a 404 tells a caller " **DB-sourced, not the search index.** The handler injects `IDbContextFactory` and resolves an explicit per-field-name `IQueryable` (or, for a few special cases, an in-memory list) rather than querying `ISearchIndex`: `genre`/`show_genre` → `Set()`, `studio` → `Set()`, `director` → -`Set()`, `writer` → `Set()`, `actor` → `Actors`, `artist` → `ArtistMetadata.Title` -concatenated with `Set().Name` (plus `SongMetadata.Artists` via the list-valued path — -see `api.search-field-values-list-columns`, #578), `tag` → +`Set()`, `writer` → `Set()`, `actor` → `Actors`, `artist` → `ArtistMetadata.Title` (entity +artists only — free-text music-video/song artist credits are a known, intentionally-uncovered gap), `tag` → `Set()` excluding `Tag.NfoCountryTypeId`/`Tag.PlexNetworkTypeId` (reapplying the indexer's own exclusions so country/network strings don't leak in as tags), `network` → `Set()` filtered to `Tag.PlexNetworkTypeId`, `collection` → `Collections`, `video_codec` → `MediaStreams` filtered to @@ -37,11 +36,10 @@ of queried: `state` (the fixed 4-value `MediaItemState` enum) and `video_dynamic `MovieMetadata`/`ShowMetadata`/`OtherVideoMetadata`/`RemoteStreamMetadata`, so the handler pulls the distinct raw strings then `Split('/')`s, trims, and dedupes in memory before the same prefix-filter/sort/take — this matches what search actually matches on, rather than surfacing the compound string as one facet value. -**`title` and `show_title` are explicitly NOT supported** (404, free-text fallback): they are near-unique -free-text fields spanning ~9 metadata tables where a distinct list of every title isn't a useful facet. -`album_artist` was originally 404 here for a different reason — it backs onto `SongMetadata.AlbumArtists`, -which EF can't translate into a server-side distinct query — and since #578 it returns bounded best-effort -values instead; see `api.search-field-values-list-columns`. +**`title`, `show_title`, `album_artist` are explicitly NOT supported** (404, free-text fallback): `title`/ +`show_title` are near-unique free-text fields spanning ~9 metadata tables where a distinct list of every +title isn't a useful facet; `album_artist` backs onto `SongMetadata.AlbumArtists`, a value-converted +`IList` column EF can't translate into a server-side distinct query. **Why a thin query, not a cache.** No result cache, no debounce on the server side (the SPA combobox debounces the keystroke) — each per-field query is a bounded, indexed `Distinct`/`Take`; adding a cache diff --git a/docs/decisions/records/api/search-field-values-list-columns.md b/docs/decisions/records/api/search-field-values-list-columns.md deleted file mode 100644 index 6bfaa4353..000000000 --- a/docs/decisions/records/api/search-field-values-list-columns.md +++ /dev/null @@ -1,75 +0,0 @@ ---- -key: api.search-field-values-list-columns -title: '2026-07-26 — list-valued (JSON primitive collection) fields get bounded best-effort facet values via a raw-JSON LIKE pre-filter, not a normalized side table (#578)' -status: active -since: '2026-07-26' -supersedes: none -superseded-by: none -rule: 'A `text` search field whose values live in an EF **primitive collection** (one JSON array per row in a single column — `SongMetadata.Artists`, `SongMetadata.AlbumArtists`) IS served by `GET /api/v1/search/fields/{name}/values`, not 404''d, but as **bounded best-effort**: SQL pre-filters on the raw JSON with `LOWER(col) LIKE ''%"%'' ESCAPE ''/''` — a deliberate superset, since it matches a row, not an element — capped at a fixed row cap (1000) with an `ORDER BY` on the raw column so the truncation point is deterministic; the rows are then split, exact-prefix-filtered, deduped, sorted and `limit`ed in memory. An empty `q` is supported (it degenerates to `%"%`, still capped), so these fields need no special-case client contract. The prefix is JSON-encoded before matching because that is how it is stored, which also makes the pattern pure ASCII and therefore makes `LOWER()` mean the same thing on SQLite (ASCII-only) and MySQL (Unicode-aware); the pattern is lowercased in C# rather than relying on `LIKE`''s own case rules, so the match is correct under a case-sensitive MySQL collation as well as a case-insensitive one. The escape character is `/`, never `\` — `ESCAPE ''\''` is not a portable SQL literal.' -signals: 'artist typeahead free-text credits, album_artist 404, SongMetadata.Artists, SongMetadata.AlbumArtists, MusicVideoArtist suggestions, EF primitive collection, PrimitiveCollection JSON column, SelectMany requires APPLY on SQLite, Pomelo primitive collections not enabled, raw JSON LIKE pre-filter, ListValuedRowCap, bounded best-effort facet values, LIKE ESCAPE portability · paths: `ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs`, `ErsatzTV/Controllers/Api/SearchController.cs`, `ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs`, `ErsatzTV.Tests/Application/Search/SearchFieldValuesQueryShapeTests.cs` · issues: #578, #434' -mechanics: '`GetSearchFieldValuesHandler.GetSongListValuedColumn` / `.GetSongListValuedValues` / `.ListValuedSql` / `.JsonElementPrefixPattern`; `SearchFieldValuesQueryShapeTests` (dual-provider `ToQueryString` guard); `api.search-field-values` (the base endpoint contract this extends)' ---- - -Extends `api.search-field-values` rather than superseding it: that record's rule still reads true — -whole values, from the database, not the Lucene term dictionary, 404 for a field with no -distinct-value source. What changed is only **which** fields have a source. Its body's claim that -music-video/song artist credits are "a known, intentionally-uncovered gap" and that `album_artist` -has no server-side source was corrected in place. - -**The three `artist` sources are not the same problem.** `LuceneSearchIndex` writes the `artist` -field from three places, and only two of them are ordinary columns: - -- `ArtistMetadata.Title` — a plain column on a plain table. -- `MusicVideoArtist.Name` — also a real entity table (`MusicVideoMetadata.HasMany(m => m.Artists)`), - so the free-text music-video credits are directly `SELECT DISTINCT`-able. These two are simply - `Concat`ed into the existing server-side pipeline - (`.Where(v => v.ToLower().StartsWith(qLower)).Distinct().OrderBy(v => v).Take(limit)`), which EF - emits as one bounded `UNION ALL` + `LOWER(...) LIKE ... LIMIT` on both providers. -- `SongMetadata.Artists` (and, for `album_artist`, `SongMetadata.AlbumArtists`) — an - `IList` that EF 9 maps as a **primitive collection**: no `HasConversion` anywhere, one - JSON array per row in a single `TEXT`/`longtext` column. This one has no server-side projection at - all. Verified empirically against both providers: SQLite reports *"Translating this query requires - the SQL APPLY operation, which is not supported on SQLite"*, and Pomelo MySQL 9.0.0 reports - *"Primitive collections support has not been enabled"*. `SearchFieldValuesQueryShapeTests` pins - both failures, so if a provider upgrade ever makes the projection translate, the raw-SQL path can - be retired on a red test rather than on a hunch. - -**Why the LIKE superset and not a normalized `SongArtist` table.** A real join table (the shape -`MusicVideoArtist` already has) would make this exact and indexable forever, but it costs a -dual-provider schema migration plus a data backfill, changes to every scanner write path that -populates `SongMetadata.Artists`, changes to the Lucene indexer, and leaves two representations of -the same fact to drift apart — a large blast radius for a typeahead convenience. Rejected on cost. -The raw-JSON pre-filter is confined to one private method, needs no migration, and is a strictly -additive read path. Also rejected: querying the Lucene term dictionary, which is the thing -`api.search-field-values` exists to avoid (analyzed `TextField`s store lowercased word tokens). - -**The bound is the point, and it is tested behaviourally.** `ListValuedRowCap = 1000` bounds rows -fetched, not values returned, because the pre-filter matches rows whole. The `ORDER BY` on the raw -column is what makes the truncation deterministic — and therefore testable: -`List_Valued_Source_Is_Bounded_By_A_Row_Cap` seeds 1001 rows where the over-cap row's JSON sorts -*last* but its element would sort *first* in the output, so an unbounded implementation returns it -and a capped one cannot. A positive control in the same test narrows `q` until that row is back -under the cap and asserts it reappears, so the test cannot pass by the value being unreachable for -some unrelated reason. - -**`album_artist` 404 → 200 is additive.** Nothing consumes the 404 as a signal: the SPA's -`getSearchFieldValues` (`web/src/api/search.ts`) treats any non-200 as "no suggestions, fall back to -a free-text input", which is exactly what it will now do less often. Per `api.versioning-v1`, -widening which fields return values — and which values a field returns — adds capability without -removing any, so it needs no `/api/v2`. - -**Known imprecision, deliberately accepted.** On a library with more than 1000 rows matching the -pre-filter, the suggestions for `artist`/`album_artist` are a deterministic prefix of the matches -rather than every match. This is stated on the endpoint's `[EndpointDescription]` so it reaches the -OpenAPI document. The alternative — requiring a non-empty `q` for list-valued fields — was rejected: -it would make one group of fields behave differently from every other field for the same client -code, for a case the row cap already handles. - -**Dual-provider verification, and its limit.** The queryable half is pinned on both providers by -`SearchFieldValuesQueryShapeTests`, which compiles the query with `ToQueryString()` — no server -needed. The raw-SQL half cannot be executed against MySQL from the unit-test suite; it is defended -by construction instead (portable `LOWER`/`LIKE`/`ESCAPE`/`ORDER BY`/`LIMIT`, unquoted identifiers, -a non-backslash escape character, and an ASCII-only pattern so collation and `LOWER()` semantics -cannot diverge) plus `List_Valued_Columns_Are_Stored_As_Ascii_Escaped_Json_Arrays`, which pins the -on-disk JSON encoding the pattern is built against so an EF encoder change fails loudly instead of -silently returning nothing. diff --git a/docs/decisions/records/api/search-field-values-sources.md b/docs/decisions/records/api/search-field-values-sources.md new file mode 100644 index 000000000..f138aab11 --- /dev/null +++ b/docs/decisions/records/api/search-field-values-sources.md @@ -0,0 +1,116 @@ +--- +key: api.search-field-values-sources +title: '2026-07-26 — Facet-value typeahead, restated: every artist-bearing source is covered, list-valued columns via a superset LIKE pre-filter that may over-match but never under-match (#578)' +status: active +since: '2026-07-26' +supersedes: api.search-field-values@2026-07-23 +superseded-by: none +rule: '`GET /api/v1/search/fields/{name}/values?q=&limit=` returns distinct WHOLE values from the database for a narrow allow-list of catalog fields (never the Lucene term dictionary — analyzed `TextField`s store lowercased word tokens, e.g. "Science Fiction" → `science`/`fiction`, useless as a suggestion), 404 for an unknown field, a non-`text` field, or a `text` field with no distinct-value source (`title`, `show_title` only); `limit` clamped to `[1, 50]` (default 50). Prefix matching, dedup and ordering are ORDINAL (`OrdinalIgnoreCase` / `StringComparer.Ordinal`), never current-culture, because `UseRequestLocalization` makes the culture caller-controlled. A field whose values live in an EF **primitive collection** (one JSON array per row in a single column: `SongMetadata.Artists`, `SongMetadata.AlbumArtists`) is served, not 404''d, as bounded best-effort: SQL narrows on the raw JSON with a `LIKE` pattern built ONLY from the leading run of characters the JSON writer stores verbatim (printable ASCII minus `"&''+<>\`), stopping at the first character it cannot prove — this pre-filter is an optimization that MAY over-match and MUST NEVER under-match, with correctness living in the in-memory exact filter; the rows are capped at 1000 on `ORDER BY Id` (a unique integer key, not the JSON column) and then split, filtered, deduped, sorted and `limit`ed in memory. Ordering across merged sources is best-effort, exact only below the truncation points.' +signals: 'artist typeahead free-text credits, album_artist 404, artist suggestions missing music videos, SongMetadata.Artists, SongMetadata.AlbumArtists, MusicVideoArtist, EF primitive collection, PrimitiveCollection JSON column, SelectMany requires APPLY on SQLite, Pomelo primitive collections not enabled, raw JSON LIKE pre-filter, superset never under-match, accented artist unsuggestable, LOWER cannot fold a \u00XX escape, OrdinalIgnoreCase vs InvariantCultureIgnoreCase folding, Accept-Language tr-TR dotless i, ListValuedRowCap, ORDER BY Id not TEXT, max_sort_length, content_rating split, text field allow-list · paths: `ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs`, `ErsatzTV/Controllers/Api/SearchController.cs`, `ErsatzTV.Tests/Application/Search/SearchFieldValuesPrefilterSupersetTests.cs`, `ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs`, `web/src/api/search.ts` · issues: #578, #434, #176' +mechanics: '`GetSearchFieldValuesHandler` (`GetSource`, `GetSongListValuedColumn`, `GetSongListValuedValues`, `ListValuedSql`, `JsonElementPrefixPattern`, `FilterSortTake`); `SearchFieldValuesPrefilterSupersetTests` (the superset proof); api-conventions.md; spa-conventions.md §12' +--- + +Supersedes `api.search-field-values` (#434). That record did not merely carry a stale implementation +detail — it recorded a **call**: free-text music-video/song artist credits were "a known, +intentionally-uncovered gap" and `album_artist` was unsupported. #578 reverses that call, so this is +a supersession, not a line-edit. Everything #434 settled that still holds is restated here rather +than left in the archive: enum fields ship their values inline on `GET /api/v1/search/fields` and +need no lookup; text fields need a live one; the source is the database and never the search index; +`content_rating` splits its compound `"PG-13/TV-14"` strings in memory; there is no result cache. + +## The three `artist` sources are three different problems + +`LuceneSearchIndex` writes the `artist` field from three places, and only two are ordinary columns: + +- `ArtistMetadata.Title` — a plain column. Already worked. +- `MusicVideoArtist.Name` — also a real entity table (`MusicVideoMetadata.HasMany(m => m.Artists)`), + so the free-text music-video credits are directly `SELECT DISTINCT`-able. It just joins the + existing server-side pipeline as a `Concat`, emitted as one bounded `UNION ALL` + + `LOWER(...) LIKE ... LIMIT` on both providers. +- `SongMetadata.Artists` (and, for `album_artist`, `AlbumArtists`) — an `IList` EF 9 maps as + a **primitive collection**: no `HasConversion` anywhere, one JSON array per row in a single + `TEXT`/`longtext` column, with no server-side projection at all. Verified against both providers: + SQLite reports *"Translating this query requires the SQL APPLY operation, which is not supported on + SQLite"*, Pomelo MySQL 9.0.0 reports *"Primitive collections support has not been enabled"*. Both + failures are pinned by a test, so a provider upgrade that fixes them surfaces as a red rather than + leaving a workaround in place forever. + +## The pre-filter is an optimization; it may over-match, it must never under-match + +This is the load-bearing invariant, and the first implementation got it wrong in a way that no test +of the returned values could see. It JSON-encoded the whole query prefix, reasoning that since the +stored text escapes non-ASCII (`Édith` is on disk as `Édith`), encoding the prefix the same way +would line up. **It does not.** SQL `LOWER()` lowercases the *escape text* — `É` — it cannot +case-fold the codepoint that escape denotes. So `q=é` produced a pattern of `é` that never +matched, the row was discarded before the in-memory filter ever saw it, and **every accented artist +was silently unsuggestable** — in a music library, the common case, not an edge case. + +The fix is to narrow only on ground that can be proved: the leading run of characters the JSON +writer stores **verbatim**, stopping at the first non-ASCII or JSON-escaped character. A query +starting with such a character narrows to the bare element-opening anchor `%"%` and leans on the row +cap and the in-memory filter. `q=Beyoncé` still narrows on `beyonc`; `q=é` narrows on nothing. + +Soundness rests on two facts, both **asserted by exhaustive computation** in +`SearchFieldValuesPrefilterSupersetTests` rather than argued: + +1. **No non-ASCII codepoint in all of Unicode `OrdinalIgnoreCase`-equals a printable ASCII + character** (swept over `U+0080`–`U+10FFFF`). So for a run character `c`, every element character + that can match `c` is itself ASCII, hence stored verbatim, hence foldable by both providers' + `LOWER()`. This is *false* for `InvariantCultureIgnoreCase`, which folds ~190 codepoints + (`U+00AA`→`a`, `U+017F`→`s`, the modifier letters) onto ASCII letters — **the choice of Ordinal is + load-bearing, not stylistic**. +2. **The exact set of printable-ASCII characters the JSON writer escapes** is `"`, `&`, `'`, `+`, + `<`, `>`, `\`, `` ` ``, pinned against the encoder itself so an encoder change fails the build + instead of quietly shrinking the superset. + +## Ordinal everywhere, because the culture is caller-controlled + +`UseRequestLocalization` honours `Accept-Language`, so a caller can select `tr-TR` and turn `q=I` +into `ı`. The old chain used `ToLower()` plus the default *linguistic* `StartsWith(string)`, making +the same library answer differently per caller. Comparison is now `OrdinalIgnoreCase` and ordering +`StringComparer.Ordinal` throughout, including the shared `FilterSortTake` that `state`, +`video_dynamic_range` and `content_rating` also use. That is a deliberate change to shared behaviour: +response *sets* are unchanged, response *order* is now ordinal rather than culture-dependent. + +## Ordering is best-effort, and the code says so + +Merging sources does **not** yield the exact first `limit` of the union. Each source truncates using +its own ordering — the EF source by the database collation (SQLite's is ASCII-only), the list source +by primary key — and neither is the ordinal ordering the merge applies. With `"Zulu"` and `"Éclair"` +and `limit=1`, the EF source keeps `"Zulu"` and the merge never sees `"Éclair"`, which it would have +ranked first. Below the truncation points — the normal typeahead case — the result is exact. An +earlier comment claimed exactness the code does not have; do not restore it. + +## What the row cap does and does not bound + +`ListValuedRowCap = 1000` bounds **rows fetched**, not values returned, because the pre-filter +matches rows whole. The cap rides `ORDER BY Id` — a unique integer primary key both providers can +walk — rather than the JSON column: ordering on `TEXT` would not be deterministic on MySQL, which +sorts using only the first `max_sort_length` (default 1024) bytes, so long rows sharing a prefix +would tie arbitrarily; and it avoids sorting the whole matching set. + +**It does not bound database work, and that is a known cost.** `LOWER(col) LIKE '%…'` has a leading +wildcard, so no index can be sought and every `SongMetadata` row is scanned before the cap applies. +An empty `q` matches nearly every row. Every `artist` request now scans the song table's JSON column +in addition to its previous work, once per debounced keystroke; the endpoint is authenticated by +default but `Api:RequireKeyForReads=false` makes it anonymous. Accepted for now because the scan is +bounded in *memory* and the affected table is one of the smaller ones, and because the alternative is +the migration below. **Follow-up candidate: a normalized `SongArtist` join table** (the shape +`MusicVideoArtist` already has) would make this exact, indexable and seekable — at the cost of a +dual-provider schema migration plus data backfill, changes to every scanner write path populating +`SongMetadata.Artists`, changes to the Lucene indexer, and two representations of the same fact free +to drift. Rejected for #578 on blast radius, not on merit. + +## `album_artist` 404 → 200 is additive + +Nothing consumes the 404 as a signal: the SPA's `getSearchFieldValues` (`web/src/api/search.ts`) +treats any non-200 as "no suggestions, fall back to a free-text input", which it will now do less +often. Per `api.versioning-v1`, widening which fields return values adds capability without removing +any, so no `/api/v2`. + +## Known limitation inherited, not introduced + +The **EF-sourced** fields (`genre`, `studio`, `artist`'s entity half, …) still prefix-match through +SQL `LOWER()`, which on SQLite is ASCII-only — so `q=é` does not match a stored `Édith` for those +fields either. That predates #578 and is unchanged by it; fixing it would mean filtering those tables +client-side. Noted here so the next reader does not mistake the list-valued fix for a global one. diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md index 9a67c771a..827f70f0e 100644 --- a/docs/spa-conventions.md +++ b/docs/spa-conventions.md @@ -619,12 +619,12 @@ not wired here. for these two fields — the compiled query is the existing `released_inthelast:"7 day"`-style `CustomMultiFieldQueryParser` macro, so nothing downstream changes. `validation.ts`'s `ruleError` requires the value to parse as a positive integer before it's compiled. -- **Facet-value typeahead** (#434, `api.search-field-values`) — the value input for a `text` field +- **Facet-value typeahead** (#434/#578, `api.search-field-values-sources`) — the value input for a `text` field (not enum) is a combobox backed by `getSearchFieldValues` (`web/src/api/search.ts` → `GET /api/v1/search/fields/{name}/values?q=&limit=`), debounced on keystroke, prefix-matching the in-progress value against distinct terms already in the index. It always allows free-text entry as a fallback — a 404 (non-text field) or an empty result list (e.g. ElasticSearch backend) degrades to a - plain text input rather than blocking the rule. Since #578 (`api.search-field-values-list-columns`) + plain text input rather than blocking the rule. Since #578 (`api.search-field-values-sources`) `album_artist` returns values instead of 404ing, and `artist` covers free-text music-video/song credits as well as entity artists; for those two the server's list is **bounded best-effort** on a very large library, so the free-text fallback stays load-bearing — never treat an absent suggestion as an invalid