fix(668): review round 5 -- three prose nits, including an off-by-one I filed
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 14s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 21s
Review verdict / Set review-verdict status (pull_request) Successful in 15s
PR Gates / decisions lifecycle (pull_request) Successful in 26s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
PR Gates / Script tests (pytest) (pull_request) Successful in 53s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m8s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m0s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 14m33s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ f2d9c0d (base: main)
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 14s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 21s
Review verdict / Set review-verdict status (pull_request) Successful in 15s
PR Gates / decisions lifecycle (pull_request) Successful in 26s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 15s
PR Gates / Script tests (pytest) (pull_request) Successful in 53s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 17m8s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 22m0s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 14m33s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ f2d9c0d (base: main)
Final sweep confirmed the retracted MySQL over-match claim survives in no file
on the branch (only in two immutable commit messages, which stay -- rewriting
history would invalidate every sha-bound review verdict). Three nits remained.
- The fixture's class docstring said the on-MySQL claim "rests on the server's
collation", which is the one thing the decision record says it does NOT rest
on. It rests on Unicode-aware LOWER(); the executed comparison bypasses the
collation entirely. Reworded.
- The record's `rule:` enumerated the covered fields but omitted show_genre,
which GetSource and the fold both handle ("genre" or "show_genre"). Added.
- My own #688 write-up was wrong twice: the 60-line ceiling warning is
NON-blocking by design, and the calibration pytest reds at >=61, not >=60 --
main's p90 is 59, so a 60-line record makes p90 == ceiling and PASSES. The
bullet even contradicted itself, since the next sentence relies on 60 passing.
Corrected in the PR body and in the issue.
Decisions-Edit: yes
This commit is contained in:
@@ -24,8 +24,9 @@ namespace ErsatzTV.Tests.Integration;
|
||||
/// SQLite semantics in depth against in-memory SQLite, and
|
||||
/// <c>SearchFieldValuesQueryShapeTests</c> pins the generated SQL for both providers without a
|
||||
/// server. Neither can show that a REAL MySQL server returns the accented value — the fix's central
|
||||
/// claim is "on both providers", and on MySQL that rests on the server's collation rather than on
|
||||
/// any code this repo owns. That is precisely the kind of assumption worth executing.
|
||||
/// claim is "on both providers", and on MySQL that rests on the server's Unicode-aware
|
||||
/// <c>LOWER()</c> rather than on any code this repo owns — explicitly NOT on its collation, which
|
||||
/// the executed comparison bypasses. That is precisely the kind of assumption worth executing.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// MySQL needs a live server via <c>ETV_TEST_MYSQL_CONNECTION</c>. Without it the MySQL fixture
|
||||
|
||||
@@ -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, 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-field-values-unicode-fold` | The EF-sourced facet fields (`genre`, `show_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, 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.'
|
||||
rule: 'The EF-sourced facet fields (`genre`, `show_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`'
|
||||
---
|
||||
|
||||
Reference in New Issue
Block a user