fix(668): review round 3 -- MySQL does NOT over-match; correct the claim everywhere
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 13s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 12s
PR Gates / Docs update reminder (pull_request) Successful in 17s
review-verdict/h10 Awaiting review verdict for dda98ef
PR Gates / decisions lifecycle (pull_request) Successful in 25s
Review verdict / Set review-verdict status (pull_request) Successful in 13s
PR Gates / Script tests (pytest) (pull_request) Successful in 55s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m32s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m24s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m18s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m5s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 13s
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 12s
PR Gates / Docs update reminder (pull_request) Successful in 17s
review-verdict/h10 Awaiting review verdict for dda98ef
PR Gates / decisions lifecycle (pull_request) Successful in 25s
Review verdict / Set review-verdict status (pull_request) Successful in 13s
PR Gates / Script tests (pytest) (pull_request) Successful in 55s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 1m32s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 16m24s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m18s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m5s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Review BLOCKED on a false CI claim I copied from the sibling fixture ("CI sets
ETV_REQUIRE_MYSQL_TESTS=1"). Nothing sets it; the doc now says plainly that CI
does not arm this lane and points at ersatztv#627. That was the blocker.
Chasing the reviewer's second finding then overturned something bigger. It
predicted that seeding an unaccented "Edith" would make the in-memory ordinal
filter load-bearing on MySQL, since utf8mb4_0900_ai_ci treats é as e. Mutation
test says otherwise: with the filter deleted the MySQL test stays GREEN.
Measured against a live 8.4 to find out why:
LOWER(Name) LIKE 'é%' (literal) -> Édith AND Edith
LOWER(Name) LIKE @v (ai_ci variable) -> Édith AND Edith
LOWER(Name) LIKE @v COLLATE _bin -> Édith only
the EF query, executed -> Édith only
The driver binds the pattern with a BINARY collation, so the executed
comparison is accent-SENSITIVE and MySQL does not over-match at all. MySQL's
correctness rests on its Unicode-aware LOWER(), not on the collation.
My earlier probe used a LITERAL pattern -- a different query from the one the
code runs -- and I wrote its result into the handler comment, the decision
record and the PR body. All three now say what actually happens, and the record
carries the lesson: measure the query the CODE runs, not one you type.
The "Edith" row stays as a near-miss control, with a docstring that says what it
does and does not prove rather than the over-match story it was added for.
Also moved EnsureCreatedAsync out of [SetUp]: NUnit skips [TearDown] when
[SetUp] throws, so a mid-create failure would strand the database.
Decisions-Edit: yes
This commit is contained in:
@@ -29,7 +29,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `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-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). The FINAL filter, dedup and ordering applied to the response are ORDINAL (`OrdinalIgnoreCase` / `StringComparer.Ordinal`), never current-culture, because `UseRequestLocalization` makes the culture caller-controlled — scoped to the in-memory stages on purpose: a field sourced by a plain EF query is filtered and truncated by the DATABASE collation first (SQLite's `LOWER()` is ASCII-only), which ordinal semantics downstream cannot undo (ersatztv#668). 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, and its rows are read by a keyset page carrying **NO RESIDUAL predicate** — `SELECT Id, <col> AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch`, no `LIKE`, no `LOWER`, not even `IS NOT NULL`. The cursor is itself a predicate, but a SEEKABLE one on the ordering key: it positions the scan and never discards a row. A RESIDUAL predicate discards rows the engine already produced, and `LIMIT` truncates only the survivors — so with one present it bounds the OUTPUT rather than the row count. All selectivity is in memory. The guarantee is scoped: **at most 20,000 LOGICAL rows returned/materialized and at most 10 round trips (11 for `artist`)** — NOT bounded physical work and NOT bounded bytes, because MySQL traverses deleted-but-unpurged index records and the `TEXT`/`longtext` payload width is unrestricted. The walk pages 2,000 rows at a time, stopping on the first of enough distinct matches, a short page, or the ceiling. | 2026-07-26 | [link](records/api/search-field-values-sources.md) |
|
||||
| `api.search-field-values-unicode-fold` | The EF-sourced facet fields (`genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`'s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite's `LOWER()` folds ASCII only (`lower('Édith')` is `'Édith'` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware and its ci collation makes the predicate OVER-match, which the existing ordinal filter discards. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT <col> AS Value FROM <table> WHERE [<discriminator> AND] etv_upper(<col>) LIKE @Pattern ESCAPE '\' ORDER BY <col> LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today's EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record's "Known limitation inherited, not introduced" clause; everything else it settles still holds. | 2026-07-27 | [link](records/api/search-field-values-unicode-fold.md) |
|
||||
| `api.search-field-values-unicode-fold` | The EF-sourced facet fields (`genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`'s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite's `LOWER()` folds ASCII only (`lower('Édith')` is `'Édith'` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware, so `LOWER('Édith')` really is `'édith'` and the existing predicate reaches the row unaided. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT <col> AS Value FROM <table> WHERE [<discriminator> AND] etv_upper(<col>) LIKE @Pattern ESCAPE '\' ORDER BY <col> LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today's EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record's "Known limitation inherited, not introduced" clause; everything else it settles still holds. | 2026-07-27 | [link](records/api/search-field-values-unicode-fold.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) |
|
||||
|
||||
@@ -5,7 +5,7 @@ status: active
|
||||
since: '2026-07-27'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'The EF-sourced facet fields (`genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`''s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite''s `LOWER()` folds ASCII only (`lower(''Édith'')` is `''Édith''` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware and its ci collation makes the predicate OVER-match, which the existing ordinal filter discards. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT <col> AS Value FROM <table> WHERE [<discriminator> AND] etv_upper(<col>) LIKE @Pattern ESCAPE ''\'' ORDER BY <col> LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today''s EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record''s "Known limitation inherited, not introduced" clause; everything else it settles still holds.'
|
||||
rule: 'The EF-sourced facet fields (`genre`, `studio`, `director`, `writer`, `actor`, `tag`, `network`, `collection`, `video_codec`, `album`, and `artist`''s entity half) reach stored values whose prefix carries an uppercase non-ASCII character, on BOTH providers, with no row budget and no accepted loss. The defect was SQLite-only and ONE-SIDED: SQLite''s `LOWER()` folds ASCII only (`lower(''Édith'')` is `''Édith''` unchanged), so the predicate UNDER-matched, which no later stage can repair. MySQL was already correct — its `LOWER()` is Unicode-aware, so `LOWER(''Édith'')` really is `''édith''` and the existing predicate reaches the row unaided. The fix is a SECOND, ADDITIVE query taken only when `isSqlite && q contains a non-ASCII character`: raw Dapper SQL `SELECT DISTINCT <col> AS Value FROM <table> WHERE [<discriminator> AND] etv_upper(<col>) LIKE @Pattern ESCAPE ''\'' ORDER BY <col> LIMIT @Limit`, where `etv_upper` is a `SqliteConnection.CreateFunction` scalar implementing `ToUpperInvariant`. Every other case — all-ASCII `q`, and MySQL for all `q` — runs today''s EF query BYTE-IDENTICALLY. Keeping selectivity in SQL here is NOT the refuted family from `api.search-field-values-sources`: those four attempts bounded a walk around a predicate that could not be made correct over JSON escape text, whereas this is a correct fold on a plain column in an ordinary `LIMIT`ed query. It narrows that record''s "Known limitation inherited, not introduced" clause; everything else it settles still holds.'
|
||||
signals: 'accented facet values missing, Édith not suggested, SQLite LOWER is ASCII only, etv_upper, CreateFunction custom scalar, ToUpperInvariant fold, OrdinalIgnoreCase is not invariant-upper, U+017F long s upper-folds to S, U+212A Kelvin sign, utf8mb4_0900_ai_ci accent insensitive, MySQL LOWER is unicode aware, over-match harmless under-match not, ESCAPE clause raw SQL LIKE wildcards, EF null semantics ExternalTypeId, RegisterUnicodeCaseFunctions provider static, non-sargable LOWER LIKE full table scan · paths: `ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs`, `ErsatzTV.Infrastructure.Sqlite/Data/SqliteUnicodeFunctions.cs`, `ErsatzTV.Infrastructure/Data/TvContext.cs`, `ErsatzTV/Startup.cs`, `ErsatzTV.Scanner/Program.cs` · issues: #668, #578, #434, #669'
|
||||
mechanics: '`GetSearchFieldValuesHandler` (`ContainsNonAscii`, `IsSqlite`, `EscapeLikePrefix`, `UnicodeFoldSql`, `GetUnicodeFoldSources`, `GetUnicodeFoldedValues`, `UpperFunction`); `SqliteUnicodeFunctions.Register`; `TvContext.RegisterUnicodeCaseFunctions`; `GetSearchFieldValuesHandlerTests.Unicode_Fold_Agrees_With_The_Ordinal_Filter`; `SearchFieldValuesQueryShapeTests.Unicode_Fold_Function_Name_Matches_The_Registration`; `ProviderStaticsWiringTests`'
|
||||
---
|
||||
@@ -17,24 +17,25 @@ Narrows `api.search-field-values-sources` (#578), which deferred this gap; the r
|
||||
The handler lowercases `q` with `ToLowerInvariant` **before** SQL, so both casings produce one pattern.
|
||||
A stored **lowercase** accented value was therefore always reachable from either casing; only one whose
|
||||
prefix carries an **uppercase** non-ASCII character was lost. ersatztv#668's body claimed `q=É` failed
|
||||
against a stored `édith`; that is false, and a test pins the passing case beside the fixed one.
|
||||
against a stored `édith`; false, and a test pins the passing case beside the fixed one.
|
||||
|
||||
## MySQL was never broken, for a reason worth recording
|
||||
|
||||
Verified on a throwaway MySQL 8.4 (server-default `utf8mb4_0900_ai_ci`): `LOWER('Édith')` returns
|
||||
`édith`, and `LOWER(name) LIKE 'é%'` matches `Édith`, `édith` AND `Edith`. Over-match is free, because
|
||||
the ordinal filter discards the extras. **This is configuration-incidental, not designed**: it rests on
|
||||
the server default for columns `TvContext` does not name in its `UseCollation` list (which excludes
|
||||
`Genre`, `Studio`, `Director`, `Writer`, `Actor`, `Tag`, `MusicVideoArtist`). Any ci collation is safe;
|
||||
`_bin` would need re-checking.
|
||||
Verified on a live MySQL 8.4: `LOWER('Édith')` is `édith`, so the existing predicate reaches the row.
|
||||
**Measure the query the CODE runs, not one you type.** With a LITERAL pattern `LOWER(name) LIKE 'é%'`
|
||||
also matches `Edith` (the column is accent-insensitive `utf8mb4_0900_ai_ci`), and an earlier revision of
|
||||
this record concluded from exactly that probe that MySQL over-matches and the ordinal filter corrects it.
|
||||
It does not: through EF the driver binds the pattern with a BINARY collation, so the executed comparison
|
||||
is accent-SENSITIVE and returns `Édith` alone. MySQL's correctness rests on Unicode-aware `LOWER()`, not
|
||||
on the collation.
|
||||
|
||||
## Why a fold, and not the #578 walk
|
||||
|
||||
Reusing #578's shape — drop SQL selectivity, keyset-walk, filter in memory — answers the wrong question.
|
||||
That walk is bounded-best-effort at 20,000 rows; `Genre` and `Actor` carry one row per media item, so a
|
||||
large library exceeds the budget and `Édith` stays unreachable — the bug restated. #578 accepts that
|
||||
contract for `SongMetadata.Artists` because server-side projection is **impossible** on both providers;
|
||||
these are plain columns, where it is merely inconvenient.
|
||||
That walk is best-effort at 20,000 rows; `Genre` and `Actor` carry one row per media item, so a large
|
||||
library exceeds the budget and `Édith` stays unreachable — the bug restated. #578 accepts that contract
|
||||
for `SongMetadata.Artists` because server-side projection is **impossible** there; these are plain
|
||||
columns, where it is merely inconvenient.
|
||||
|
||||
The cost objection to a managed per-row fold is weak: `LOWER(v) LIKE` is non-sargable and **no index on
|
||||
any of these `Name` columns exists** (every index is on the foreign key), so this swaps a native per-row
|
||||
@@ -51,8 +52,7 @@ is measurable: `char.ToUpperInvariant('ſ')` (U+017F) is `'S'`, yet
|
||||
it — the harmless direction. An earlier draft justified the fold by claiming the opposite;
|
||||
`Fold_LongS_IsNotOrdinalEqualToS` pins the truth.
|
||||
|
||||
That same fact makes the all-ASCII fast path sound: no non-ASCII codepoint is `OrdinalIgnoreCase`-equal
|
||||
to printable ASCII (#578's sweep found 0), so an ASCII query only ever ordinal-matches an ASCII prefix.
|
||||
That same fact makes the all-ASCII fast path sound: no non-ASCII codepoint is `OrdinalIgnoreCase`-equal to printable ASCII (#578's sweep found 0), so an ASCII query only ever ordinal-matches an ASCII prefix.
|
||||
|
||||
## Three traps, each guarded by a test and explained at its call site
|
||||
|
||||
@@ -60,13 +60,13 @@ Raw SQL gets none of EF's LIKE escaping (`EscapeLikePrefix`, backslash first, ex
|
||||
Discriminators must mirror EF's NULL semantics — `t.ExternalTypeId != X` INCLUDES a NULL-typed row,
|
||||
where plain SQL `<>` drops it. Registration is per-connection and lives at the call site, not in a
|
||||
`DbConnectionInterceptor`: Dapper opens a closed connection itself and a direct ADO open raises no EF
|
||||
interceptor, so that seam would miss exactly this query. The function name is duplicated across the
|
||||
Application/provider boundary, so a rename on one side fails only at runtime; a test pins them equal.
|
||||
interceptor, so that seam would miss exactly this query.
|
||||
|
||||
## Residuals, stated rather than glossed
|
||||
|
||||
**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.
|
||||
count (never a wrong value). Unreachable on MySQL through the binary-bound pattern above; the SQLite fold
|
||||
has 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 rejected (it perturbs the pinned `"apple"`/`"Zulu"` examples). **Ordering stays
|
||||
best-effort** per #578.
|
||||
|
||||
Reference in New Issue
Block a user