PR Gates / Script tests (pytest) (pull_request) Successful in 51s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 1m38s
Review verdict / Set review-verdict status (pull_request) Successful in 1m18s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 4m51s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 18m52s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 21m10s
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 25m44s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
review-verdict/h10 Review-verdict: MERGEABLE @ fc3ede0 (base: main)
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 7s
PR Gates / Docs update reminder (pull_request) Successful in 8s
PR Gates / decisions lifecycle (pull_request) Successful in 21s
Comment- and docs-only; verified no non-comment line changed in any .cs.
I reported last round that I had "classified every surviving hit". That was false, and the false
confidence is the expensive part: a confidently-stated "I checked everything" stops anyone else
checking. The retracted wording survived in nine places, two of them the record's title and rule: —
and the catalog copies rule: verbatim, so the generated entry point and the record disagreed
semantically while docs/decisions.md said the correct thing.
Root cause of the miss, because it will recur otherwise: I built the sweep term list from the
DELETED MECHANISM's vocabulary (LIKE, superset, keyspace, anchor, over-match) and never added the
RETRACTED CLAIM's own words. "no predicate", "bound on work", "index entries", "no gap" and
"holds in memory" were never grepped. After a retraction the subject list has to include the words
of the thing being retracted, not just the thing already deleted.
Second, worse: my first attempt at this round's sweep printed nothing for every term and I nearly
read that as "all clear". zsh does not word-split an unquoted $FILES, so grep received one giant
non-existent path — and the `|| echo "(none)"` never fired because the pipeline's exit status was
sed's. Same failure shape as the bug arc itself: a check reporting success while examining nothing.
Re-run with a proper array plus a control term ("SongMetadata" -> 42 hits) so an empty result is
distinguishable from a broken grep.
Fixed all nine, replacing "no predicate" with the seekable-cursor-vs-residual distinction already
written correctly elsewhere:
- handler: the "real bound on work" claim, the short-page rationale
- SearchFieldValuesQueryShapeTests: "ANY predicate" + "reads exactly n index entries", and added what
the test can and cannot pin (a SQL string, not a plan / visibility work / payload I/O)
- GetSearchFieldValuesHandlerTests: "no gap between what the engine looks at and what it hands back",
and the current-behaviour comment
- record title, rule:, attempt-5 table row; api-conventions
- regenerated docs/decisions/README.md so catalog and record agree again
Tenth item, the same overclaim one level down and it survived the first retraction: the row bound was
said to cap what the process holds in memory. It does not — payload width is unrestricted and one
JSON array can contain arbitrarily many strings, each of which may enter the in-memory distinct set.
It caps logical rows returned/materialized and round-trip count, nothing about bytes. Added as a
third struck-through bullet next to the other two retractions.
345 lines
17 KiB
C#
345 lines
17 KiB
C#
using System.Text;
|
||
using System.Text.Json;
|
||
using Dapper;
|
||
using ErsatzTV.Core.Api.Search;
|
||
using ErsatzTV.Core.Domain;
|
||
using ErsatzTV.Infrastructure.Data;
|
||
using Microsoft.EntityFrameworkCore;
|
||
|
||
namespace ErsatzTV.Application.Search.Queries;
|
||
|
||
public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> dbContextFactory)
|
||
: IRequestHandler<GetSearchFieldValues, Option<SearchFieldValuesResponseModel>>
|
||
{
|
||
private const int DefaultLimit = 50;
|
||
private const int MaxLimit = 50;
|
||
|
||
/// <summary>
|
||
/// Rows read per round trip when walking the list-valued (JSON-array) columns on
|
||
/// <c>SongMetadata</c>, and the ceiling on rows read per request.
|
||
/// <para>
|
||
/// These count ACTUAL ROWS, and arriving at that took four tries — each earlier attempt bounded a
|
||
/// quantity that sounded like rows and was not. A fixed <c>LIMIT</c> budget bounded the RESULT, and
|
||
/// the pre-filter (allowed to over-match) starved it with rows that could not match. Keyset paging
|
||
/// with a <c>LIMIT</c> bounded CANDIDATES RETURNED — but a query matching nothing must evaluate
|
||
/// every eligible row before it can return an empty page, so rows inspected stayed unbounded. A
|
||
/// closed <c>Id</c> range bounded KEYSPACE WIDTH — but keyspace is not rows: delete 20,000
|
||
/// historical rows, put one song at <c>Id</c> 20001, and the walk burns its whole allowance on empty
|
||
/// ranges and inspects nothing.
|
||
/// </para>
|
||
/// <para>
|
||
/// What makes this one hold is that <b>the query has no RESIDUAL predicate</b> — nothing that can
|
||
/// discard a row the engine already produced. The only condition is the cursor
|
||
/// <c>Id > @AfterId</c>, which is a seek on the <c>ORDER BY</c> key itself, not a filter. So the
|
||
/// page returns exactly <see cref="ListValuedBatchRows" /> rows whenever that many logical rows
|
||
/// remain, independent of how sparse the matches are or where the <c>Id</c> gaps fall.
|
||
/// </para>
|
||
/// <para>
|
||
/// <b>Be precise about what is bounded: LOGICAL ROWS RETURNED AND MATERIALIZED, and the number of
|
||
/// round trips. Not physical work, and not bytes.</b> Two things break the stronger reading, and an
|
||
/// earlier version of this comment asserted it anyway:
|
||
/// <list type="bullet">
|
||
/// <item>
|
||
/// MySQL purge lag. Deleted clustered-index records survive until purge runs, and a range
|
||
/// scan still traverses them, so returning 2,000 VISIBLE rows can touch far more index
|
||
/// records. Deletion history therefore still affects physical work — the very thing the
|
||
/// keyspace attempt was trying to make irrelevant.
|
||
/// </item>
|
||
/// <item>
|
||
/// Row width is unbounded. These columns are <c>TEXT</c>/<c>longtext</c>, which both SQLite
|
||
/// and InnoDB spill to overflow pages, so a row count implies neither a byte count nor a
|
||
/// page-read count.
|
||
/// </item>
|
||
/// </list>
|
||
/// The logical-row bound is still worth having — it is what makes the walk terminate and what caps
|
||
/// the number of rows and round trips — but do not restate it as bounded I/O, and do not restate it
|
||
/// as bounded MEMORY either: payload width is unrestricted and a single JSON array can hold
|
||
/// arbitrarily many strings, every one of which may enter the in-memory set.
|
||
/// </para>
|
||
/// <para>
|
||
/// The trade is real and deliberate: no server-side narrowing, so a query with few matches transfers
|
||
/// rows it will discard, up to <see cref="ListValuedMaxRowsRead" />. A query with enough matches
|
||
/// stops as soon as it has <c>limit</c> distinct ones, so the dense cases — including an empty
|
||
/// <c>q</c> — finish on the first page. See <c>api.search-field-values-sources</c> for the measured
|
||
/// cost and for why reintroducing a <c>LIKE</c> is not an option.
|
||
/// </para>
|
||
/// </summary>
|
||
internal const int ListValuedBatchRows = 2000;
|
||
|
||
/// <inheritdoc cref="ListValuedBatchRows" />
|
||
internal const int ListValuedMaxRowsRead = 20000;
|
||
|
||
|
||
public async Task<Option<SearchFieldValuesResponseModel>> Handle(
|
||
GetSearchFieldValues request,
|
||
CancellationToken cancellationToken)
|
||
{
|
||
SearchFieldResponseModel field = SearchFieldCatalog.Fields
|
||
.FirstOrDefault(f => f.Name == request.Name);
|
||
|
||
if (field is null || field.Type != "text")
|
||
{
|
||
return Option<SearchFieldValuesResponseModel>.None;
|
||
}
|
||
|
||
int limit = request.Limit <= 0 ? DefaultLimit : Math.Clamp(request.Limit, 1, MaxLimit);
|
||
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<MediaItemState>(), query, limit));
|
||
case "video_dynamic_range":
|
||
return new SearchFieldValuesResponseModel(
|
||
FilterSortTake(["hdr", "sdr"], query, limit));
|
||
}
|
||
|
||
await using TvContext dbContext = await dbContextFactory.CreateDbContextAsync(cancellationToken);
|
||
|
||
if (request.Name == "content_rating")
|
||
{
|
||
return new SearchFieldValuesResponseModel(
|
||
await GetContentRatingValues(dbContext, query, limit, cancellationToken));
|
||
}
|
||
|
||
IQueryable<string> source = GetSource(dbContext, request.Name);
|
||
string listColumn = GetSongListValuedColumn(request.Name);
|
||
if (source is null && listColumn is null)
|
||
{
|
||
return Option<SearchFieldValuesResponseModel>.None;
|
||
}
|
||
|
||
var values = new List<string>();
|
||
|
||
if (source is not null)
|
||
{
|
||
values.AddRange(
|
||
await source
|
||
.Where(v => v != null && v.ToLower().StartsWith(qLower))
|
||
.Distinct()
|
||
.OrderBy(v => v)
|
||
.Take(limit)
|
||
.ToListAsync(cancellationToken));
|
||
}
|
||
|
||
if (listColumn is not null)
|
||
{
|
||
values.AddRange(await GetSongListValuedValues(dbContext, listColumn, query, limit, cancellationToken));
|
||
}
|
||
|
||
// 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 "apple" and limit=1 the database keeps "apple" (its
|
||
// ordering is case-insensitive) while ordinal ranks "Zulu" first, so the merge never sees "Zulu".
|
||
// Below the truncation points (the normal typeahead case) the result is exact.
|
||
return new SearchFieldValuesResponseModel(FilterSortTake(values.Distinct(StringComparer.Ordinal), query, limit));
|
||
}
|
||
|
||
internal static IQueryable<string> GetSource(TvContext dbContext, string name) => name switch
|
||
{
|
||
"genre" or "show_genre" => dbContext.Set<Genre>().Select(g => g.Name),
|
||
"studio" => dbContext.Set<Studio>().Select(s => s.Name),
|
||
"director" => dbContext.Set<Director>().Select(d => d.Name),
|
||
"writer" => dbContext.Set<Writer>().Select(w => w.Name),
|
||
"actor" => dbContext.Actors.Select(a => a.Name),
|
||
// Mirrors what LuceneSearchIndex writes to the `artist` field: the music video's linked artist entity
|
||
// (ArtistMetadata.Title) plus its free-text credits (MusicVideoArtist rows). The third contributor —
|
||
// SongMetadata.Artists — is a JSON-array column and is handled by GetSongListValuedValues instead.
|
||
"artist" => dbContext.ArtistMetadata.Select(m => m.Title)
|
||
.Concat(dbContext.Set<MusicVideoArtist>().Select(a => a.Name)),
|
||
"tag" => dbContext.Set<Tag>()
|
||
.Where(t => t.ExternalTypeId != Tag.NfoCountryTypeId && t.ExternalTypeId != Tag.PlexNetworkTypeId)
|
||
.Select(t => t.Name),
|
||
"network" => dbContext.Set<Tag>()
|
||
.Where(t => t.ExternalTypeId == Tag.PlexNetworkTypeId)
|
||
.Select(t => t.Name),
|
||
"collection" => dbContext.Collections.Select(c => c.Name),
|
||
"video_codec" => dbContext.MediaStreams
|
||
.Where(s => s.MediaStreamKind == MediaStreamKind.Video && s.Codec != null)
|
||
.Select(s => s.Codec),
|
||
"album" => dbContext.MusicVideoMetadata
|
||
.Where(m => m.Album != null)
|
||
.Select(m => m.Album)
|
||
.Concat(dbContext.SongMetadata.Where(m => m.Album != null).Select(m => m.Album)),
|
||
_ => null
|
||
};
|
||
|
||
/// <summary>
|
||
/// Maps a field name onto the <c>SongMetadata</c> column that backs it as an <c>IList<string></c>.
|
||
/// The returned value is a compile-time constant from this switch — never caller input — so it is safe
|
||
/// to interpolate into the SQL in <see cref="ListValuedSql" />.
|
||
/// </summary>
|
||
private static string GetSongListValuedColumn(string name) => name switch
|
||
{
|
||
"artist" => "Artists",
|
||
"album_artist" => "AlbumArtists",
|
||
_ => null
|
||
};
|
||
|
||
/// <summary>
|
||
/// Reads whole values out of a <c>SongMetadata</c> <c>IList<string></c> column.
|
||
/// <para>
|
||
/// EF maps these as primitive collections: one JSON array per row in a single <c>TEXT</c>/
|
||
/// <c>longtext</c> column. Neither provider can project the elements server-side — SQLite needs
|
||
/// the SQL <c>APPLY</c> operator it doesn't have, and Pomelo MySQL doesn't implement primitive
|
||
/// collections at all — so there is no server-side <c>SELECT DISTINCT</c> over the elements.
|
||
/// </para>
|
||
/// <para>
|
||
/// So the rows are walked in primary-key order, keyset-paged by row position, and split +
|
||
/// exact-filtered in memory. All selectivity is in memory — the query's only condition is the
|
||
/// cursor, a seek on the ordering key that never discards a row, so its <c>LIMIT</c> bounds the
|
||
/// LOGICAL ROWS returned. See <see cref="ListValuedBatchRows" /> for the four revisions it took to
|
||
/// get that right, and for what that bound does and does not cover.
|
||
/// </para>
|
||
/// </summary>
|
||
private static async Task<List<string>> GetSongListValuedValues(
|
||
TvContext dbContext,
|
||
string column,
|
||
string query,
|
||
int limit,
|
||
CancellationToken cancellationToken)
|
||
{
|
||
string sql = ListValuedSql(column);
|
||
|
||
var distinct = new System.Collections.Generic.HashSet<string>(StringComparer.Ordinal);
|
||
var afterId = 0;
|
||
var read = 0;
|
||
|
||
while (read < ListValuedMaxRowsRead && distinct.Count < limit)
|
||
{
|
||
int batch = Math.Min(ListValuedBatchRows, ListValuedMaxRowsRead - read);
|
||
|
||
List<ListValuedRow> rows = (await dbContext.Connection.QueryAsync<ListValuedRow>(
|
||
new CommandDefinition(
|
||
sql,
|
||
new { AfterId = afterId, Batch = batch },
|
||
cancellationToken: cancellationToken))).AsList();
|
||
|
||
if (rows.Count == 0)
|
||
{
|
||
break;
|
||
}
|
||
|
||
read += rows.Count;
|
||
afterId = rows[^1].Id;
|
||
|
||
foreach (ListValuedRow row in rows)
|
||
{
|
||
foreach (string element in ParseElements(row.Payload))
|
||
{
|
||
if (element.StartsWith(query, StringComparison.OrdinalIgnoreCase))
|
||
{
|
||
distinct.Add(element);
|
||
}
|
||
}
|
||
}
|
||
|
||
if (rows.Count < batch)
|
||
{
|
||
// With no RESIDUAL predicate -- only the cursor, which selects a range rather than discarding
|
||
// rows from it -- a short page can only mean the table is exhausted. It can never mean "this
|
||
// stretch happened to match nothing", which is precisely why the residual predicate had to go.
|
||
// Advancing from the last returned Id is safe for the same reason: nothing was filtered out
|
||
// behind it, so no row can be skipped.
|
||
break;
|
||
}
|
||
}
|
||
|
||
return distinct.ToList();
|
||
}
|
||
|
||
private static IEnumerable<string> ParseElements(string payload)
|
||
{
|
||
if (string.IsNullOrWhiteSpace(payload))
|
||
{
|
||
return [];
|
||
}
|
||
|
||
try
|
||
{
|
||
return (JsonSerializer.Deserialize<string[]>(payload) ?? []).Where(e => !string.IsNullOrEmpty(e));
|
||
}
|
||
catch (JsonException)
|
||
{
|
||
return [];
|
||
}
|
||
}
|
||
|
||
/// <summary>
|
||
/// One keyset page of rows, by ROW POSITION rather than by <c>Id</c> value.
|
||
/// <para>
|
||
/// The only condition is the cursor — deliberately <b>no RESIDUAL predicate</b>: no <c>LIKE</c>, no
|
||
/// <c>LOWER</c>, not even <c>IS NOT NULL</c>. The distinction that matters is not "no predicate"
|
||
/// (the cursor is one); it is that <c>Id > @AfterId</c> is a <i>seekable predicate on the
|
||
/// ordering key</i>, which positions the scan and never discards a row, whereas a residual
|
||
/// predicate throws away rows the engine already produced. <c>LIMIT</c> only truncates what
|
||
/// survives a residual predicate, so with one present it bounds the output rather than the row
|
||
/// count — which is how every earlier revision scanned past its own bound. With none, <c>LIMIT n</c>
|
||
/// yields <c>n</c> logical rows. Null payloads are dropped in memory by
|
||
/// <see cref="ParseElements" />.
|
||
/// </para>
|
||
/// <para>
|
||
/// Note this pins the SQL string only. It cannot pin an execution plan, MVCC visibility work, or
|
||
/// payload I/O — and on MySQL, using the index to satisfy <c>ORDER BY</c> is an optimizer choice,
|
||
/// not a semantic guarantee.
|
||
/// </para>
|
||
/// </summary>
|
||
internal static string ListValuedSql(string column) =>
|
||
$"SELECT Id, {column} AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch";
|
||
|
||
private sealed class ListValuedRow
|
||
{
|
||
public int Id { get; init; }
|
||
|
||
public string Payload { get; init; }
|
||
}
|
||
|
||
private static async Task<List<string>> GetContentRatingValues(
|
||
TvContext dbContext,
|
||
string query,
|
||
int limit,
|
||
CancellationToken cancellationToken)
|
||
{
|
||
List<string> raw = await dbContext.MovieMetadata
|
||
.Where(m => m.ContentRating != null)
|
||
.Select(m => m.ContentRating)
|
||
.Concat(dbContext.ShowMetadata.Where(m => m.ContentRating != null).Select(m => m.ContentRating))
|
||
.Concat(dbContext.OtherVideoMetadata.Where(m => m.ContentRating != null).Select(m => m.ContentRating))
|
||
.Concat(dbContext.RemoteStreamMetadata.Where(m => m.ContentRating != null).Select(m => m.ContentRating))
|
||
.Distinct()
|
||
.ToListAsync(cancellationToken);
|
||
|
||
IEnumerable<string> split = raw
|
||
.SelectMany(cr => cr.Split('/'))
|
||
.Select(cr => cr.Trim())
|
||
.Where(cr => !string.IsNullOrEmpty(cr))
|
||
.Distinct();
|
||
|
||
return FilterSortTake(split, query, limit);
|
||
}
|
||
|
||
/// <summary>
|
||
/// The one in-memory filter/sort/take every field funnels through. Both the comparison and the ordering
|
||
/// are ORDINAL on purpose: <c>UseRequestLocalization</c> honours <c>Accept-Language</c>, so the current
|
||
/// culture is caller-controlled, and <c>ToLower()</c> plus the default (linguistic)
|
||
/// <c>StartsWith(string)</c> would make the result depend on it — under <c>tr-TR</c>, <c>q=I</c> lowers
|
||
/// to <c>ı</c> and stops matching <c>Istanbul</c>. Note this is the LAST stage only: a field sourced by
|
||
/// a plain EF query has already been filtered and truncated by the database collation before it gets
|
||
/// here, which ordinal semantics downstream cannot undo (ersatztv#668).
|
||
/// </summary>
|
||
private static List<string> FilterSortTake(IEnumerable<string> values, string query, int limit) =>
|
||
values
|
||
.Where(v => v.StartsWith(query, StringComparison.OrdinalIgnoreCase))
|
||
.OrderBy(v => v, StringComparer.Ordinal)
|
||
.Take(limit)
|
||
.ToList();
|
||
}
|