fix(668): review round 1 -- make two guards actually guard
PR Gates / Docs update reminder (pull_request) Successful in 16s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 17s
review-verdict/h10 Awaiting review verdict for ac67c9e
PR Gates / decisions lifecycle (pull_request) Failing after 30s
PR Gates / Script tests (pytest) (pull_request) Successful in 43s
Review verdict / Set review-verdict status (pull_request) Successful in 43s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m31s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m32s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m57s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 15m44s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m38s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / Docs update reminder (pull_request) Successful in 16s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 17s
review-verdict/h10 Awaiting review verdict for ac67c9e
PR Gates / decisions lifecycle (pull_request) Failing after 30s
PR Gates / Script tests (pytest) (pull_request) Successful in 43s
Review verdict / Set review-verdict status (pull_request) Successful in 43s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m31s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m32s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 8m57s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 15m44s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 19m38s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Independent review found both new guard tests weaker than they read. 1. A false docstring. It claimed the SQL stage "genuinely returns 'ſweet' for q='S'". It does not: 'S' is ASCII, so ContainsNonAscii is false and the fold branch is SKIPPED. Those three negative cases exercise the ASCII fast path, which is worth pinning but is not what the comment said -- and the consequence was that NO test drove a row through the fold for the ordinal filter to discard, i.e. the harmless over-match direction the whole design rests on was untested. Comment corrected and Unicode_Fold_Over_Match_Is_Discarded_By_The_Ordinal_Filter added (stored "Sword", q="ſ" -> fold runs, SQL pattern S%, SQLite returns the row, filter drops it, response empty). 2. Unicode_Fold_Escapes_Like_Wildcards could not fail if the %/_ escaping it names were deleted -- the in-memory filter masks the over-match, so the counts stay right. The escaping's real role is preventing LIMIT crowding, so Unicode_Fold_Escaping_Prevents_Limit_Crowding pins that instead. Verified by mutation: with the %/_ replaces removed the new test fails while the original two still pass. Also: the crowding residual in the decision record was attributed to MySQL alone; the SQLite fold shares it in principle, so "no accepted loss" is narrowed to mean no unreachable VALUE rather than a guaranteed count. And a comment says why the provider check is derived per-context instead of reading TvContext.IsSqlite (that static is scoped host-only by ProviderStaticsWiringTests, and reading it here would falsify the exemption). Refs #668 Decisions-Edit: yes
This commit is contained in:
@@ -213,6 +213,9 @@ public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> dbContextF
|
||||
return false;
|
||||
}
|
||||
|
||||
// Derived per-context rather than read from the TvContext.IsSqlite static on purpose: that static is
|
||||
// scoped by ProviderStaticsWiringTests to host-only readers, and reading it here would falsify its
|
||||
// scanner exemption. Do not "simplify" this to IsSqlite.
|
||||
private static bool IsSqlite(TvContext dbContext) =>
|
||||
(dbContext.Database.ProviderName ?? string.Empty).Contains("Sqlite", StringComparison.OrdinalIgnoreCase);
|
||||
|
||||
|
||||
@@ -714,10 +714,11 @@ public class GetSearchFieldValuesHandlerTests
|
||||
/// alone would say, so a fold that starts dropping rows (or one that stops filtering the extras out)
|
||||
/// fails here.
|
||||
/// <para>
|
||||
/// The negative cases are the load-bearing ones. <c>char.ToUpperInvariant('ſ')</c> IS <c>'S'</c>,
|
||||
/// so an upper-folding SQL stage genuinely returns "ſweet" for q="S" — and the expected result is
|
||||
/// still empty, because <c>"ſweet".StartsWith("S", OrdinalIgnoreCase)</c> is false. Same for
|
||||
/// U+212A KELVIN SIGN. That is the over-match being correctly discarded.
|
||||
/// The negative cases here have ASCII queries, so they exercise the FAST PATH (the fold is
|
||||
/// skipped entirely) and pin that it is exact: <c>"ſweet".StartsWith("S", OrdinalIgnoreCase)</c>
|
||||
/// is false even though <c>char.ToUpperInvariant('ſ')</c> IS <c>'S'</c>. The over-match the fold
|
||||
/// itself produces is a different path and is covered by
|
||||
/// <see cref="Unicode_Fold_Over_Match_Is_Discarded_By_The_Ordinal_Filter" />.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestCase("Édith", "é", true, TestName = "Fold_UppercaseAccent_LowercaseQuery")]
|
||||
@@ -751,6 +752,62 @@ public class GetSearchFieldValuesHandlerTests
|
||||
result.IfSome(r => r.Values.ShouldBe(expected ? new List<string> { stored } : []));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#668. Drives a row THROUGH the fold that the ordinal filter must then discard — the
|
||||
/// harmless over-match direction the whole design rests on, which the ASCII-query negative cases
|
||||
/// above cannot reach. q="ſ" is non-ASCII so the fold runs; <c>ToUpperInvariant('ſ')</c> is 'S', so
|
||||
/// the SQL pattern is <c>S%</c> and SQLite genuinely returns "Sword" — and the response must still
|
||||
/// be empty, because <c>"Sword".StartsWith("ſ", OrdinalIgnoreCase)</c> is false.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public async Task Unicode_Fold_Over_Match_Is_Discarded_By_The_Ordinal_Filter()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.Set<Genre>().Add(new Genre { Name = "Sword" });
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
"Sword".StartsWith("\u017F", StringComparison.OrdinalIgnoreCase).ShouldBeFalse();
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("genre", "\u017F", 50),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBeEmpty());
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#668. The escaping's load-bearing role is NOT filtering — the in-memory ordinal filter
|
||||
/// already drops an over-match, which is why a plain count assertion stays green even with the
|
||||
/// escaping removed. It is preventing LIMIT CROWDING: an unescaped <c>_</c> also matches the space,
|
||||
/// binary ORDER BY ranks "100 Édith" first, LIMIT 1 returns only that, the filter discards it, and
|
||||
/// the genuine "100_Édith" is never returned at all. This case fails if the escaping is removed.
|
||||
/// </summary>
|
||||
[Test]
|
||||
public async Task Unicode_Fold_Escaping_Prevents_Limit_Crowding()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.Set<Genre>().AddRange(
|
||||
new Genre { Name = "100 \u00C9dith" },
|
||||
new Genre { Name = "100_\u00C9dith" });
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("genre", "100_\u00C9", 1),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBe(new List<string> { "100_\u00C9dith" }));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#668. The non-ASCII branch is raw SQL, so it gets none of the LIKE-wildcard escaping EF
|
||||
/// does for <c>StartsWith</c>. An unescaped <c>%</c> or <c>_</c> in the query would match anything.
|
||||
|
||||
@@ -65,8 +65,8 @@ Application/provider boundary, so a rename on one side fails only at runtime; a
|
||||
|
||||
## Residuals, stated rather than glossed
|
||||
|
||||
**MySQL crowding**: with a ci collation the SQL `Take(limit)` can fill with accent-insensitive matches
|
||||
the ordinal filter then discards, under-DELIVERING the count (never a wrong value). Pre-existing, not
|
||||
gated by #668; an over-fetch was considered and rejected (it perturbs the pinned `"apple"`/`"Zulu"`
|
||||
ordering examples). **Ordering stays best-effort** per #578; `content_rating`, `state` and
|
||||
`video_dynamic_range` bypass the EF source entirely.
|
||||
**Crowding**: a SQL `LIMIT` can fill with rows the ordinal filter then discards, under-DELIVERING the
|
||||
count (never a wrong value). On MySQL via its ci collation; the SQLite fold shares it in principle, when
|
||||
limit-many stored values are upper-equal but ordinal-unequal to the prefix (a `ſ`/`K`/`İ` class). So
|
||||
"no accepted loss" means no unreachable VALUE, not a guaranteed count. An over-fetch was considered and
|
||||
rejected (it perturbs the pinned `"apple"`/`"Zulu"` examples). **Ordering stays best-effort** per #578.
|
||||
|
||||
Reference in New Issue
Block a user