feat(578): artist typeahead covers music-video and song credits; album_artist stops 404ing #676
@@ -1,3 +1,6 @@
|
||||
using System.Text;
|
||||
using System.Text.Json;
|
||||
using Dapper;
|
||||
using ErsatzTV.Core.Api.Search;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
@@ -11,6 +14,62 @@ public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> dbContextF
|
||||
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)
|
||||
@@ -24,17 +83,22 @@ public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> 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<MediaItemState>(), qLower, limit));
|
||||
FilterSortTake(Enum.GetNames<MediaItemState>(), 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);
|
||||
@@ -42,34 +106,55 @@ public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> dbContextF
|
||||
if (request.Name == "content_rating")
|
||||
{
|
||||
return new SearchFieldValuesResponseModel(
|
||||
await GetContentRatingValues(dbContext, qLower, limit, cancellationToken));
|
||||
await GetContentRatingValues(dbContext, query, limit, cancellationToken));
|
||||
}
|
||||
|
||||
IQueryable<string> source = GetSource(dbContext, request.Name);
|
||||
if (source is null)
|
||||
string listColumn = GetSongListValuedColumn(request.Name);
|
||||
if (source is null && listColumn is null)
|
||||
{
|
||||
return Option<SearchFieldValuesResponseModel>.None;
|
||||
}
|
||||
|
||||
List<string> values = await source
|
||||
.Where(v => v != null && v.ToLower().StartsWith(qLower))
|
||||
.Distinct()
|
||||
.OrderBy(v => v)
|
||||
.Take(limit)
|
||||
.ToListAsync(cancellationToken);
|
||||
var values = new List<string>();
|
||||
|
||||
return new SearchFieldValuesResponseModel(values);
|
||||
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));
|
||||
}
|
||||
|
||||
private static IQueryable<string> GetSource(TvContext dbContext, string name) => name switch
|
||||
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),
|
||||
// entity artists only; free-text music-video/song artist credits are not included (known limitation)
|
||||
"artist" => dbContext.ArtistMetadata.Select(m => m.Title),
|
||||
// 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),
|
||||
@@ -87,9 +172,139 @@ public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> dbContextF
|
||||
_ => 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 qLower,
|
||||
string query,
|
||||
int limit,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
@@ -108,13 +323,22 @@ public class GetSearchFieldValuesHandler(IDbContextFactory<TvContext> dbContextF
|
||||
.Where(cr => !string.IsNullOrEmpty(cr))
|
||||
.Distinct();
|
||||
|
||||
return FilterSortTake(split, qLower, limit);
|
||||
return FilterSortTake(split, query, limit);
|
||||
}
|
||||
|
||||
private static List<string> FilterSortTake(IEnumerable<string> values, string qLower, int 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.ToLower().StartsWith(qLower))
|
||||
.OrderBy(v => v)
|
||||
.Where(v => v.StartsWith(query, StringComparison.OrdinalIgnoreCase))
|
||||
.OrderBy(v => v, StringComparer.Ordinal)
|
||||
.Take(limit)
|
||||
.ToList();
|
||||
}
|
||||
|
||||
@@ -1,9 +1,11 @@
|
||||
using System.Globalization;
|
||||
using ErsatzTV.Application.Search.Queries;
|
||||
using ErsatzTV.Core.Api.Search;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using LanguageExt;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
@@ -176,6 +178,461 @@ public class GetSearchFieldValuesHandlerTests
|
||||
networkResult.IfSome(r => r.Values.ShouldBe(new List<string> { "HBO" }));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Artist_Merges_Entity_Artists_Music_Video_Credits_And_Song_Credits()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.ArtistMetadata.Add(Artist("Alpha Entity"));
|
||||
|
||||
// negative control: an entity artist that must NOT match the "al" prefix
|
||||
context.ArtistMetadata.Add(Artist("Zeta Entity"));
|
||||
|
||||
context.MusicVideoMetadata.AddRange(
|
||||
MusicVideo("MV One", "Alpha Credit", "Alpha Shared"),
|
||||
// "Alpha Shared" appears in two rows, so DISTINCT has something to collapse
|
||||
MusicVideo("MV Two", "Alpha Shared"),
|
||||
MusicVideo("MV Three", "Zeta Credit"));
|
||||
|
||||
context.SongMetadata.AddRange(
|
||||
Song("Song One", ["Alpha Song", "Zeta Song"]),
|
||||
Song("Song Two", ["Alpha Song"]),
|
||||
Song("Song Three", ["Zeta Only"]));
|
||||
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("artist", "al", 50),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBe(
|
||||
new List<string> { "Alpha Credit", "Alpha Entity", "Alpha Shared", "Alpha Song" }));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Artist_Returns_Every_Source_For_Empty_Query()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.ArtistMetadata.Add(Artist("Entity"));
|
||||
context.MusicVideoMetadata.Add(MusicVideo("MV", "Credit"));
|
||||
context.SongMetadata.Add(Song("Song", ["SongArtist"]));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("artist", string.Empty, 50),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBe(new List<string> { "Credit", "Entity", "SongArtist" }));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task Album_Artist_Returns_Song_Album_Artists_Instead_Of_NotFound()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.SongMetadata.AddRange(
|
||||
Song("One", ["Performer"], ["Alpha Album Artist", "Beta Album Artist"]),
|
||||
// repeated across rows so DISTINCT is exercised
|
||||
Song("Two", ["Performer"], ["Alpha Album Artist"]),
|
||||
// negative control: a row whose album artists are absent entirely
|
||||
Song("Three", ["Performer"], null));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("album_artist", string.Empty, 50),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBe(new List<string> { "Alpha Album Artist", "Beta Album Artist" }));
|
||||
|
||||
// the performers on the same rows must not leak into album_artist
|
||||
result.IfSome(r => r.Values.ShouldNotContain("Performer"));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task List_Valued_Fields_Match_Whole_Elements_Not_Substrings_And_Ignore_Neighbours()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.SongMetadata.AddRange(
|
||||
// "Neighbour" arrives on the same row as "Radiohead" -- rows are read whole -- and must be
|
||||
// dropped by the in-memory exact prefix filter.
|
||||
Song("One", ["Radiohead", "Neighbour"]),
|
||||
// "The Radio Dept." contains "radio" but does not start with it
|
||||
Song("Two", ["The Radio Dept."]),
|
||||
Song("Three", ["Radio Birdman"]));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("artist", "radio", 50),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBe(new List<string> { "Radio Birdman", "Radiohead" }));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task List_Valued_Fields_Match_Literally_Including_Json_Escaped_And_Wildcard_Characters()
|
||||
{
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.SongMetadata.AddRange(
|
||||
// non-ASCII: stored on disk JSON-escaped as \u00E9, and must survive the round trip
|
||||
Song("One", ["Beyoncé"]),
|
||||
// an embedded quote is stored as \u0022
|
||||
Song("Two", ["\"Weird Al\" Yankovic"]),
|
||||
// SQL wildcards must be ordinary characters here, matched literally
|
||||
Song("Three", ["50% Off"]),
|
||||
Song("Four", ["50 Cent"]));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "beyoncé", 50), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { "Beyoncé" }));
|
||||
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "\"weird", 50), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { "\"Weird Al\" Yankovic" }));
|
||||
|
||||
// "50%" must not behave as the wildcard "50<anything>" — "50 Cent" must not come back
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "50%", 50), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { "50% Off" }));
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Seeds <paramref name="fillerRows" /> non-matching songs through raw SQL — 20k rows via the change
|
||||
/// tracker is minutes, this is milliseconds.
|
||||
/// </summary>
|
||||
private static Task SeedFiller(TvContext context, int fillerRows) =>
|
||||
context.Database.ExecuteSqlRawAsync(
|
||||
$"""
|
||||
WITH RECURSIVE seq(n) AS (SELECT 1 UNION ALL SELECT n + 1 FROM seq WHERE n < {fillerRows})
|
||||
INSERT INTO SongMetadata (SongId, MetadataKind, Title, Artists, DateAdded, DateUpdated)
|
||||
SELECT 0, 0, 'Filler ' || n, '["zzz-filler"]', '2026-01-01', '2026-01-01' FROM seq
|
||||
""");
|
||||
|
||||
[Test]
|
||||
public async Task List_Valued_Walk_Reads_At_Most_20000_Rows()
|
||||
{
|
||||
// Pinned in both directions so the ceiling itself is nailed down: a match in row 20000 is read, the same
|
||||
// match in row 20001 is not. The query has no RESIDUAL predicate -- only the cursor -- so "rows read" is
|
||||
// what LIMIT returns. That bounds LOGICAL rows, not physical work: the engine may still traverse more
|
||||
// index records than it returns (MySQL purge lag), and row width is unbounded.
|
||||
const string needle = "\u00E9clair-the-needle";
|
||||
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
await SeedFiller(context, 19999);
|
||||
context.SongMetadata.Add(Song("Needle", [needle]));
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
(await context.SongMetadata.CountAsync()).ShouldBe(20000);
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "\u00E9", 50), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { needle }, "row 20000 is inside the ceiling"));
|
||||
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
SongMetadata existing = await context.SongMetadata.SingleAsync(m => m.Title == "Needle");
|
||||
context.SongMetadata.Remove(existing);
|
||||
await SeedFiller(context, 1);
|
||||
await context.SaveChangesAsync();
|
||||
context.SongMetadata.Add(Song("Needle", [needle]));
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
(await context.SongMetadata.CountAsync()).ShouldBe(20001);
|
||||
}
|
||||
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "\u00E9", 50), CancellationToken.None))
|
||||
.IfSome(
|
||||
r => r.Values.ShouldBeEmpty(
|
||||
"row 20001 is past the ceiling; this false negative is the documented bounded-best-effort "
|
||||
+ "contract, deliberately pinned rather than papered over"));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task List_Valued_Walk_Reads_Live_Rows_Regardless_Of_Id_Density()
|
||||
{
|
||||
// THE round-4 killer. That revision bounded the Id KEYSPACE, and keyspace is not rows: with 20,000
|
||||
// historical rows deleted and one live song at Id 20001, the walk spent its whole allowance on empty
|
||||
// ranges and returned [] for a table containing exactly one row. Capacity degraded linearly with
|
||||
// deletion ratio, and no ratio was safe -- one placed gap hid the next match.
|
||||
//
|
||||
// Paging by row position rather than Id value makes density irrelevant: LIMIT @Batch returns @Batch
|
||||
// ROWS, wherever they sit in the keyspace.
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
await SeedFiller(context, 20000);
|
||||
await context.Database.ExecuteSqlRawAsync("DELETE FROM SongMetadata");
|
||||
context.SongMetadata.Add(Song("Survivor", ["Queen"]));
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
// one live row, sitting past the old keyspace allowance
|
||||
(await context.SongMetadata.CountAsync()).ShouldBe(1);
|
||||
(await context.SongMetadata.Select(m => m.Id).SingleAsync()).ShouldBeGreaterThan(20000);
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "que", 50), CancellationToken.None))
|
||||
.IfSome(
|
||||
r => r.Values.ShouldBe(
|
||||
new List<string> { "Queen" },
|
||||
"a one-row table must be fully readable no matter where its Id sits"));
|
||||
|
||||
// and a leading gap must not hide a later match either
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.SongMetadata.Add(Song("Second", ["Queens of the Stone Age"]));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", "que", 50), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { "Queen", "Queens of the Stone Age" }));
|
||||
}
|
||||
|
||||
[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 Matches_NonAscii_Values_In_Any_Casing(string query, string stored)
|
||||
{
|
||||
// Accented artists are the common case in a music library, so non-ASCII matching is pinned end to
|
||||
// end, in both casings of the query.
|
||||
//
|
||||
// Historical note, because it is why this suite exists: revision 1b78dc9e narrowed rows in SQL
|
||||
// with a LIKE built by JSON-encoding the query, which cannot work -- non-ASCII is stored escaped
|
||||
// (\u00C9) and SQL LOWER() folds the escape TEXT, not the codepoint it denotes. THREE of these nine
|
||||
// cases fail against that revision (the ones where query and stored casing differ, so \u00e9 and
|
||||
// \u00C9 diverge); the other six pass it, because when the casings agree the escape texts line up.
|
||||
// The SQL now has no residual predicate at all -- matching happens in memory, where a string is just
|
||||
// a string -- so these cases pin current behaviour rather than guard that revision.
|
||||
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<SearchFieldValuesResponseModel> result = await handler.Handle(
|
||||
new GetSearchFieldValues("artist", query, 50),
|
||||
CancellationToken.None);
|
||||
|
||||
result.IsSome.ShouldBeTrue();
|
||||
result.IfSome(r => r.Values.ShouldBe(new List<string> { 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<string> { "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<SearchFieldValuesResponseModel> 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<string> { "Apple", "Zulu", "apple", "\u00C9clair" };
|
||||
|
||||
CultureInfo original = CultureInfo.CurrentCulture;
|
||||
try
|
||||
{
|
||||
foreach (string culture in new[] { "en-US", "sv-SE" })
|
||||
{
|
||||
CultureInfo.CurrentCulture = new CultureInfo(culture);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> 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, 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.
|
||||
//
|
||||
// "Zulu" vs "apple" is the pair that actually diverges: ordinal puts every ASCII uppercase letter before
|
||||
// every lowercase one, so ordinal ranks "Zulu" first, while a case-insensitive database ordering ranks
|
||||
// "apple" first. (An earlier version used "Zulu"/"Éclair", where BOTH orderings pick "Zulu" — it could
|
||||
// not have told the two apart, and the divergence it claimed to show did not exist.)
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.ArtistMetadata.Add(Artist("Zulu"));
|
||||
context.ArtistMetadata.Add(Artist("apple"));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
// with room for both, the ordinal merge ranks "Zulu" first
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", string.Empty, 50), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { "Zulu", "apple" }));
|
||||
|
||||
// with limit=1 the database picks the survivor by ITS ordering, and the merge only ever sees that one
|
||||
(await handler.Handle(new GetSearchFieldValues("artist", string.Empty, 1), CancellationToken.None))
|
||||
.IfSome(r => r.Values.ShouldBe(new List<string> { "apple" }));
|
||||
}
|
||||
|
||||
[Test]
|
||||
public async Task A_Match_Behind_Many_NonMatching_Rows_Is_Still_Found()
|
||||
{
|
||||
// Fails a883e5f0, which capped rows at a fixed 1000 AFTER a deliberately over-matching SQL pre-filter:
|
||||
// the 1001st row -- the only exact match -- was discarded before the in-memory filter ever saw it and
|
||||
// the endpoint returned []. The pre-filter is gone, and the property it broke now holds for any match
|
||||
// within the read ceiling: preceding non-matching rows do not hide it. Past the ceiling it is still
|
||||
// lost by design -- see List_Valued_Walk_Reads_At_Most_20000_Rows, which pins that boundary.
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
for (var i = 0; i < 1000; i++)
|
||||
{
|
||||
context.SongMetadata.Add(Song($"Filler {i}", ["zzz-filler"], ["zzz-filler-album"]));
|
||||
}
|
||||
|
||||
context.SongMetadata.Add(Song("Needle", ["\u00E9clair"], ["\u00E9clair"]));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
var handler = new GetSearchFieldValuesHandler(_db.Factory);
|
||||
|
||||
Option<SearchFieldValuesResponseModel> albumArtist = await handler.Handle(
|
||||
new GetSearchFieldValues("album_artist", "\u00E9", 50),
|
||||
CancellationToken.None);
|
||||
|
||||
albumArtist.IsSome.ShouldBeTrue();
|
||||
albumArtist.IfSome(r => r.Values.ShouldBe(new List<string> { "\u00E9clair" }));
|
||||
|
||||
// same starvation shape on the merged `artist` field
|
||||
Option<SearchFieldValuesResponseModel> artist = await handler.Handle(
|
||||
new GetSearchFieldValues("artist", "\u00E9", 50),
|
||||
CancellationToken.None);
|
||||
|
||||
artist.IsSome.ShouldBeTrue();
|
||||
artist.IfSome(r => r.Values.ShouldBe(new List<string> { "\u00E9clair" }));
|
||||
|
||||
// ... and for a prefix beginning with a character that JSON escapes on disk. That used to collapse the
|
||||
// SQL pattern to the bare anchor; there is no prefix predicate at all now, so it is simply an ordinary
|
||||
// prefix -- kept because it is the input shape that broke the old scheme.
|
||||
await using (TvContext context = _db.CreateContext())
|
||||
{
|
||||
context.SongMetadata.Add(Song("Ampersand", ["&Me"]));
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
Option<SearchFieldValuesResponseModel> escapedPrefix = await handler.Handle(
|
||||
new GetSearchFieldValues("artist", "&M", 50),
|
||||
CancellationToken.None);
|
||||
|
||||
escapedPrefix.IsSome.ShouldBeTrue();
|
||||
escapedPrefix.IfSome(r => r.Values.ShouldBe(new List<string> { "&Me" }));
|
||||
}
|
||||
|
||||
private static ArtistMetadata Artist(string title) => new()
|
||||
{
|
||||
MetadataKind = MetadataKind.External,
|
||||
DateAdded = DateTime.UtcNow,
|
||||
DateUpdated = DateTime.UtcNow,
|
||||
Title = title
|
||||
};
|
||||
|
||||
private static MusicVideoMetadata MusicVideo(string title, params string[] artists) => new()
|
||||
{
|
||||
MetadataKind = MetadataKind.External,
|
||||
DateAdded = DateTime.UtcNow,
|
||||
DateUpdated = DateTime.UtcNow,
|
||||
Title = title,
|
||||
Artists = artists.Map(a => new MusicVideoArtist { Name = a }).ToList()
|
||||
};
|
||||
|
||||
private static SongMetadata Song(string title, IList<string> artists, IList<string> albumArtists = null) => new()
|
||||
{
|
||||
MetadataKind = MetadataKind.External,
|
||||
DateAdded = DateTime.UtcNow,
|
||||
DateUpdated = DateTime.UtcNow,
|
||||
Title = title,
|
||||
Artists = artists,
|
||||
AlbumArtists = albumArtists
|
||||
};
|
||||
|
||||
[Test]
|
||||
public async Task Dedupes_Repeated_Values()
|
||||
{
|
||||
|
||||
@@ -0,0 +1,134 @@
|
||||
using ErsatzTV.Application.Search.Queries;
|
||||
using ErsatzTV.Infrastructure;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Application.Search;
|
||||
|
||||
/// <summary>
|
||||
/// Provider-shape guards for the <c>artist</c> / <c>album_artist</c> facet-value sources (#578).
|
||||
/// <para>
|
||||
/// <see cref="GetSearchFieldValuesHandlerTests" /> runs against in-memory SQLite, so it structurally
|
||||
/// cannot see a MySQL translation or collation difference. These tests build the same LINQ against the
|
||||
/// Pomelo MySQL provider and assert the generated SQL — <c>ToQueryString</c> compiles the query without
|
||||
/// touching a server, so no MySQL instance is needed.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
[NonParallelizable]
|
||||
public class SearchFieldValuesQueryShapeTests
|
||||
{
|
||||
private bool _wasSqlite;
|
||||
|
||||
[SetUp]
|
||||
public void SetUp() => _wasSqlite = TvContext.IsSqlite;
|
||||
|
||||
[TearDown]
|
||||
public void TearDown() => TvContext.IsSqlite = _wasSqlite;
|
||||
|
||||
[Test]
|
||||
public void Artist_Entity_Union_Translates_On_Both_Providers_With_Lower_And_A_Row_Limit()
|
||||
{
|
||||
foreach ((string provider, Func<TvContext> create) in Providers())
|
||||
{
|
||||
using TvContext context = create();
|
||||
|
||||
// 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)
|
||||
.Take(50)
|
||||
.ToQueryString();
|
||||
|
||||
// case-insensitivity comes from LOWER() on the column, not from the provider's LIKE collation
|
||||
sql.ShouldContain("LOWER(", Case.Insensitive, $"{provider}: {sql}");
|
||||
sql.ShouldContain("LIKE", Case.Insensitive, $"{provider}: {sql}");
|
||||
sql.ShouldContain("MusicVideoArtist", Case.Insensitive, $"{provider}: {sql}");
|
||||
// the whole thing is one bounded server-side query, never a client-side scan
|
||||
sql.ShouldContain("LIMIT", Case.Insensitive, $"{provider}: {sql}");
|
||||
}
|
||||
}
|
||||
|
||||
[Test]
|
||||
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
|
||||
// upgrade ever makes this translate, this test fails and the raw-SQL path can be retired.
|
||||
foreach ((string provider, Func<TvContext> create) in Providers())
|
||||
{
|
||||
using TvContext context = create();
|
||||
|
||||
Should.Throw<InvalidOperationException>(
|
||||
() => context.SongMetadata.SelectMany(m => m.Artists).Distinct().Take(50).ToQueryString(),
|
||||
$"{provider} unexpectedly translated a primitive-collection projection");
|
||||
|
||||
Should.Throw<InvalidOperationException>(
|
||||
() => context.SongMetadata.SelectMany(m => m.AlbumArtists).Distinct().Take(50).ToQueryString(),
|
||||
$"{provider} unexpectedly translated a primitive-collection projection");
|
||||
}
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void List_Valued_Page_Query_Has_No_Predicate_Beyond_The_Keyset_Cursor()
|
||||
{
|
||||
// This is the whole basis of the row bound, so it is asserted rather than assumed. LIMIT truncates what
|
||||
// survives a RESIDUAL predicate — one that discards rows the engine already produced — so with such a
|
||||
// predicate present it bounds the output rather than the row count, and the engine may produce and
|
||||
// discard arbitrarily many rows first. That is how four successive revisions scanned past their own
|
||||
// bound. The cursor `Id > @AfterId` is NOT such a predicate: it is a seek on the ordering key, which
|
||||
// positions the scan without discarding anything, so LIMIT n yields n logical rows.
|
||||
//
|
||||
// What this test can and cannot do: it pins the SQL STRING. It cannot pin an execution plan, MVCC
|
||||
// visibility work or payload I/O -- physical work is NOT bounded (see the record: MySQL traverses
|
||||
// deleted-but-unpurged index records, and TEXT payloads spill to overflow pages).
|
||||
string sql = GetSearchFieldValuesHandler.ListValuedSql("Artists");
|
||||
|
||||
sql.ShouldBe(
|
||||
"SELECT Id, Artists AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch");
|
||||
|
||||
// named explicitly so a future "optimization" that reintroduces server-side selectivity fails here
|
||||
sql.ShouldNotContain("LIKE");
|
||||
sql.ShouldNotContain("LOWER");
|
||||
sql.ShouldNotContain("IS NOT NULL");
|
||||
}
|
||||
|
||||
private static IEnumerable<(string Provider, Func<TvContext> Create)> Providers() =>
|
||||
[
|
||||
("sqlite", Sqlite),
|
||||
("mysql", MySql)
|
||||
];
|
||||
|
||||
private static TvContext Sqlite()
|
||||
{
|
||||
TvContext.IsSqlite = true;
|
||||
var builder = new DbContextOptionsBuilder<TvContext>();
|
||||
builder.UseSqlite("Data Source=:memory:");
|
||||
return Create(builder.Options);
|
||||
}
|
||||
|
||||
private static TvContext MySql()
|
||||
{
|
||||
TvContext.IsSqlite = false;
|
||||
var builder = new DbContextOptionsBuilder<TvContext>();
|
||||
builder.UseMySql(
|
||||
"Server=localhost;Database=ersatztv_query_shape;User=root;Password=ersatztv;",
|
||||
new MySqlServerVersion(new Version(8, 0, 36)));
|
||||
return Create(builder.Options);
|
||||
}
|
||||
|
||||
private static TvContext Create(DbContextOptions<TvContext> options) =>
|
||||
new(
|
||||
options,
|
||||
NullLoggerFactory.Instance,
|
||||
new SlowQueryInterceptor(NullLogger<SlowQueryInterceptor>.Instance));
|
||||
}
|
||||
@@ -205,7 +205,11 @@ 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.")]
|
||||
"source. The final filter, dedup and ordering applied to the response are ordinal and not " +
|
||||
"culture-dependent; note that fields sourced by a plain database query are additionally " +
|
||||
"pre-filtered by the database collation first, which on SQLite is ASCII-only. The list-valued " +
|
||||
"music fields (artist, album_artist) are bounded best-effort: the server reads a bounded number " +
|
||||
"of song rows per request, so a library larger than that bound may yield a subset of the matches.")]
|
||||
[EndpointGroupName("general")]
|
||||
[ProducesResponseType(typeof(SearchFieldValuesResponseModel), StatusCodes.Status200OK)]
|
||||
[ProducesResponseType(typeof(ProblemDetails), StatusCodes.Status404NotFound)]
|
||||
|
||||
@@ -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.",
|
||||
"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 final filter, dedup and ordering applied to the response are ordinal and not culture-dependent; note that fields sourced by a plain database query are additionally pre-filtered by the database collation first, which on SQLite is ASCII-only. The list-valued music fields (artist, album_artist) are bounded best-effort: the server reads a bounded number of song rows per request, so a library larger than that bound may yield a subset of the matches.",
|
||||
"operationId": "GetSearchFieldValues",
|
||||
"parameters": [
|
||||
{
|
||||
|
||||
+43
-1
@@ -444,7 +444,7 @@ standard credential (catalog-read tier — no `[RequiresAuthentication]`):
|
||||
Query params: `q` (optional prefix filter, case-insensitive, default empty) and `limit` (optional,
|
||||
clamped `1..50`, default 50). `{name}` is allow-listed to `SearchFieldCatalog` fields with
|
||||
`type: "text"` AND a distinct-value source in the database — an unknown field, a non-text field (e.g.
|
||||
an enum), or a text field without a source (`title`, `show_title`, `album_artist`) 404s rather than
|
||||
an enum), or a text field without a source (`title`, `show_title`) 404s rather than
|
||||
returning an empty list, since enum fields already ship their values inline on
|
||||
`GET /api/v1/search/fields` and never need this endpoint. Returns `SearchFieldValuesResponseModel`
|
||||
(`{ values: string[] }`), sourced from a per-field distinct-values DB query (`IDbContextFactory<TvContext>`),
|
||||
@@ -452,6 +452,48 @@ not the Lucene term dictionary — analyzed text fields store lowercased word to
|
||||
No server-side caching. Powers the visual rule builder's value-input combobox for text fields; see
|
||||
`docs/decisions.md` 2026-07-23 (#434) and `spa-conventions.md` §12.
|
||||
|
||||
**Bounded best-effort for list-valued fields (#578)**: `artist` and `album_artist` are backed (wholly
|
||||
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. Those rows are read by a keyset page whose only condition is the
|
||||
**cursor** — no residual predicate that could discard a row — and filtered in memory, bounded at 20,000
|
||||
logical rows per request; so on a larger library the
|
||||
response may be a bounded subset of the matches — bounded in LOGICAL ROWS, which is not the same as
|
||||
bounded work or bytes. Say so in the `[EndpointDescription]` of any endpoint that adopts this shape.
|
||||
|
||||
Three rules generalize beyond this endpoint.
|
||||
|
||||
1. **`LIMIT` bounds the OUTPUT, not the row count, whenever a RESIDUAL predicate is present.** The
|
||||
distinction is not "predicate vs none" — a keyset cursor is a predicate. It is that a *seekable
|
||||
predicate on the ordering key* positions the scan and never discards a row, while a *residual*
|
||||
predicate (`LIKE`, `LOWER`, `IS NOT NULL`) throws away rows the engine already produced, so `LIMIT`
|
||||
truncates the survivors and says nothing about how many were produced — a query matching nothing
|
||||
must examine every eligible row before it can return an empty page. To bound rows, drop the residual
|
||||
predicate, page by row position over the primary key, and filter in memory. This endpoint got it
|
||||
wrong four times: bounding the result, then candidates returned, then `Id` keyspace width (keyspace
|
||||
is not rows — one live row at `Id` 20001 behind 20,000 deleted ones reads nothing), before arriving
|
||||
at "cursor only".
|
||||
**And scope the resulting claim to LOGICAL ROWS.** It is not bounded physical work: MySQL still
|
||||
traverses deleted-but-unpurged index records, so deletion history keeps affecting cost, and an
|
||||
unrestricted `TEXT` column spills to overflow pages so a row count implies no byte or page-read
|
||||
count. A SQL-string assertion pins none of that — not a plan, not visibility work, not I/O.
|
||||
2. **A SQL pre-filter under an in-memory exact filter may over-match but must never under-match — and
|
||||
that licence is void the moment the candidate set is truncated.** Widening the predicate then starves
|
||||
the budget with rows that cannot match. If you find yourself proving a superset property to keep a
|
||||
pre-filter honest, consider deleting the pre-filter instead: here it removed a JSON-escaping bug
|
||||
class, an exhaustive Unicode sweep and an `ESCAPE` portability workaround along with it.
|
||||
3. **Prefix matching, dedup and ordering must be ordinal, not current-culture** (`OrdinalIgnoreCase`,
|
||||
`StringComparer.Ordinal`): `UseRequestLocalization` honours `Accept-Language`, so `ToLower()` and the
|
||||
default linguistic `StartsWith(string)` let a caller change the result by changing a header. **Scope
|
||||
the claim to the stage that actually holds it** — a value set that a database `LOWER`/`DISTINCT`/
|
||||
`ORDER BY`/`LIMIT` already filtered and truncated is not ordinal no matter what runs after it, and
|
||||
saying otherwise in an `[EndpointDescription]` publishes a false contract (ersatztv#668).
|
||||
|
||||
Full rationale, the measured transfer cost, the four-attempts table and the rejected
|
||||
normalized-side-table alternative (ersatztv#669): `api.search-field-values-sources` (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`
|
||||
precedent; `pageNum` 0-based, clamped `0..2_000_000` so `pageNum * pageSize` can't overflow `int` to a 500)
|
||||
|
||||
+2
-1
@@ -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,3 +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 — Facet-value typeahead restated: every artist source covered; the JSON-column source is paged by row position with no residual SQL predicate (#578) — [`api.search-field-values-sources`](decisions/records/api/search-field-values-sources.md)
|
||||
|
||||
@@ -28,7 +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-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-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) |
|
||||
|
||||
+4
-4
@@ -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
|
||||
@@ -0,0 +1,207 @@
|
||||
---
|
||||
key: api.search-field-values-sources
|
||||
title: '2026-07-26 — Facet-value typeahead, restated: every artist-bearing source is covered, and the JSON-column source is paged by ROW POSITION with no RESIDUAL SQL predicate (#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). 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.'
|
||||
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, LIMIT bounds output not work, seekable cursor vs residual predicate, cursor is a predicate too, MySQL purge lag traverses deleted index records, TEXT overflow pages, logical rows not physical work, keyspace is not rows, page by row position, density-independent paging, deleted rows leave Id gaps, ListValuedBatchRows, ListValuedMaxRowsRead, bounded best-effort facet values, OrdinalIgnoreCase vs InvariantCultureIgnoreCase folding, Accept-Language tr-TR dotless i, content_rating split, text field allow-list · paths: `ErsatzTV.Application/Search/Queries/GetSearchFieldValuesHandler.cs`, `ErsatzTV/Controllers/Api/SearchController.cs`, `ErsatzTV.Tests/Application/Search/GetSearchFieldValuesHandlerTests.cs`, `ErsatzTV.Tests/Application/Search/SearchFieldValuesQueryShapeTests.cs`, `web/src/api/search.ts` · issues: #578, #434, #176, #668, #669'
|
||||
mechanics: '`GetSearchFieldValuesHandler` (`GetSource`, `GetSongListValuedColumn`, `GetSongListValuedValues`, `ListValuedSql`, `ParseElements`, `FilterSortTake`); `SearchFieldValuesQueryShapeTests.List_Valued_Page_Query_Has_No_Predicate_Beyond_The_Keyset_Cursor`; 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<string>` 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 SQL predicate is gone, and that is the point
|
||||
|
||||
Three revisions tried to narrow the rows in SQL before filtering them in memory. All three were
|
||||
wrong, in three different ways, and the fourth was wrong too — the history is worth more than the
|
||||
code, so it is written out below under "four wrong quantities". The conclusion is short: **there is
|
||||
no `WHERE` clause beyond the keyset cursor.** No `LIKE`, no `LOWER`, not even `IS NOT NULL`.
|
||||
|
||||
That deletes an entire family of bugs along with the predicate. Gone with it: the JSON-escape
|
||||
reasoning (`Édith` is stored `\u00C9dith`, and SQL `LOWER()` folds the escape *text* rather than the
|
||||
codepoint it denotes, so a `q=é` pattern of `\u00e9` never matched `\u00C9`); the "narrow only on the
|
||||
leading verbatim-ASCII run" rule and the exhaustive Unicode sweep that proved it sound; the
|
||||
`ESCAPE '/'` portability workaround; and the whole may-over-match-never-under-match invariant, which
|
||||
turned out to be conditional on something that was not true. In memory a string is just a string:
|
||||
`element.StartsWith(query, StringComparison.OrdinalIgnoreCase)`.
|
||||
|
||||
Worth keeping one number from that history, because it is the reason the first bug survived review:
|
||||
the JSON-encoded pattern failed on **three of nine** pinned cases, not all nine — those where the
|
||||
query's casing differed from the stored casing, so the two escape texts diverged. When the casings
|
||||
agreed it worked. A bug that fires on some inputs and not others reads as "works" during a spot
|
||||
check.
|
||||
|
||||
## 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 the in-memory stages, including the shared `FilterSortTake` that
|
||||
`state`, `video_dynamic_range` and `content_rating` also use. That is a deliberate change to shared
|
||||
behaviour, and **not a cosmetic one**: ordering happens before `Take(limit)`, so changing the
|
||||
comparer can change *which* values survive, not merely their order. With `"Zulu"` and `"apple"`, an
|
||||
empty `q` and `limit=1`, linguistic ordering yields `"apple"` and ordinal yields `"Zulu"`. An earlier
|
||||
version of this record claimed the response sets were unchanged; that was false.
|
||||
|
||||
**Scope this claim carefully — it is not endpoint-wide.** A field sourced by a plain EF query runs
|
||||
the database's `LOWER`, `DISTINCT`, `ORDER BY` and `LIMIT` *before* any ordinal code executes, so the
|
||||
database has already decided which values survive. Store a genre `"Éclair"` on SQLite and ask for
|
||||
`genre?q=é`: SQLite's ASCII-only `LOWER()` drops it before the ordinal in-memory filter ever runs, and
|
||||
a case-insensitive collation's `DISTINCT` can likewise collapse values ordinal dedup would have kept.
|
||||
The endpoint description and this record's `rule:` therefore say "the final filter, dedup and
|
||||
ordering", not "matching is ordinal". Closing that gap means filtering those tables client-side and is
|
||||
tracked separately as **ersatztv#668** — do not describe it as fixed here.
|
||||
|
||||
## 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, the list source by primary key — and
|
||||
neither is the ordinal ordering the merge applies. The pair that actually demonstrates it is `"Zulu"`
|
||||
and `"apple"`: ordinal puts every ASCII uppercase letter before every lowercase one, so the merge
|
||||
ranks `"Zulu"` first, while the case-insensitive database ordering ranks `"apple"` first — at
|
||||
`limit=1` the response is `["apple"]`, not the ordinally-first `"Zulu"`. 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. (An earlier version of this record used `"Zulu"`/`"Éclair"` as the
|
||||
example, where both orderings pick `"Zulu"` — it demonstrated nothing.)
|
||||
|
||||
## What the bound bounds — four attempts, four wrong quantities
|
||||
|
||||
Read this before "optimizing" the query. Every one of these looked obviously correct when written,
|
||||
and each was caught only by someone constructing the adversarial case rather than reading the code.
|
||||
|
||||
| # | Bounded | Why it wasn't a bound |
|
||||
|---|---|---|
|
||||
| 1–2 | the **result** — fixed `LIMIT 1000` on pre-filtered rows | the pre-filter was deliberately allowed to over-match, so a widened pattern (any non-ASCII or JSON-escaped prefix collapses it to `%"%`) filled the budget with rows that could not match. 1,000 `"zzz"` songs, `"éclair"` at row 1,001, `q=é` → `[]` |
|
||||
| 3 | **candidates returned** — keyset paging + `LIMIT` | a query matching nothing must evaluate every eligible row before it can return an empty page, so the first empty page ended the walk having counted **zero** against the ceiling. Rows returned bounded, rows inspected unbounded |
|
||||
| 4 | **keyspace width** — closed `Id` range per page | keyspace is not rows. Delete 20,000 historical rows, put one song at `Id` 20001, `q=que` → `[]`. **One row in the table, zero rows inspected.** Capacity fell linearly with deletion ratio and no ratio was safe: one placed gap hides the next match |
|
||||
| 5 | **logical rows returned** — keyset page by row position, cursor only, **no residual predicate** | — (physical work still unbounded; see below) |
|
||||
|
||||
The through-line: **`LIMIT` truncates what survives a RESIDUAL predicate.** The distinction is not
|
||||
"predicate vs no predicate" — attempt 5's query still has `Id > @AfterId`. It is:
|
||||
|
||||
- a **seekable predicate on the ordering key** (the cursor) positions the scan and never discards a
|
||||
row, so `LIMIT n` yields `n` rows;
|
||||
- a **residual predicate** (`LIKE`, `LOWER`, `IS NOT NULL`) throws away rows the engine already
|
||||
produced, so `LIMIT` bounds the survivors and says nothing about how many were produced.
|
||||
|
||||
Attempts 3 and 4 both kept selectivity in SQL and tried to add accounting around it. Attempt 5 drops
|
||||
the residual predicate and keeps only the cursor, so the accounting becomes trivial:
|
||||
|
||||
```sql
|
||||
SELECT Id, Artists AS Payload FROM SongMetadata WHERE Id > @AfterId ORDER BY Id LIMIT @Batch
|
||||
```
|
||||
|
||||
`ListValuedBatchRows = 2000`, `ListValuedMaxRowsRead = 20000`. The walk stops on the first of: enough
|
||||
distinct matches for `limit`, a short page (with no residual predicate that can only mean exhaustion
|
||||
— it can never mean "this stretch matched nothing", which is exactly why the residual predicate had
|
||||
to go), or the ceiling. Round trips: **at most 10** for `album_artist`, **at most 11** for `artist`,
|
||||
which also runs one EF query for its entity/music-video half.
|
||||
`SearchFieldValuesQueryShapeTests` pins the SQL string exactly and asserts the absence of `LIKE`,
|
||||
`LOWER` and `IS NOT NULL`, so a reviewer reintroducing "just a cheap filter" fails a test instead of
|
||||
silently unbounding the walk.
|
||||
|
||||
### Exactly what is bounded — and what is NOT
|
||||
|
||||
State this precisely, because an earlier version of this record claimed more and the overclaim is
|
||||
more dangerous than the code ever was. What holds:
|
||||
|
||||
- **at most `ListValuedMaxRowsRead` logical rows returned and materialized per request**, and
|
||||
- **at most 10 (or 11) round trips.**
|
||||
|
||||
That is the whole guarantee. It is what makes the walk terminate and what caps the number of rows and
|
||||
round trips. **Explicitly retracted**, having been asserted here in earlier revisions:
|
||||
|
||||
- ~~"`LIMIT n` reads exactly `n` index entries"~~ — **false on MySQL.** Deleted clustered-index
|
||||
records survive until purge runs, and a range scan still traverses them. Hold an old InnoDB
|
||||
snapshot open, delete a million early `SongMetadata` rows, and query from a newer snapshot with
|
||||
purge blocked: returning 2,000 *visible* rows can touch far more index records. **Deletion history
|
||||
therefore still affects physical work** — the very thing attempt 4's failure was supposed to have
|
||||
made irrelevant. Attempt 5 fixes the *logical* dependence on `Id` distribution; it does not make
|
||||
physical work independent of deletion history.
|
||||
- ~~bounded physical work / bounded I/O~~ — row width is unbounded. `Artists`/`AlbumArtists` are
|
||||
unrestricted `TEXT`/`longtext`, and both SQLite and InnoDB spill large payloads to overflow pages,
|
||||
so a row count implies neither a byte count nor a page-read count.
|
||||
- ~~"caps what this process holds in memory"~~ — the same overclaim one level down, and it survived
|
||||
the first retraction. A row count bounds neither bytes buffered nor set size: payload width is
|
||||
unrestricted, and one JSON array can contain arbitrarily many strings, every one of which may enter
|
||||
the in-memory distinct set.
|
||||
|
||||
Nor can the query-shape test carry more than it does: it pins the SQL **string**. It cannot pin an
|
||||
execution plan, MVCC visibility work, or payload I/O — and on MySQL, using the index to satisfy
|
||||
`ORDER BY` is an optimizer choice, not a SQL semantic.
|
||||
|
||||
### The cost, measured
|
||||
|
||||
No server-side narrowing means rows are transferred that will be discarded. **This is ONE data point
|
||||
on ONE library, not a general figure** — see the row-width caveat above: these numbers hold for a
|
||||
library whose artist credits average ~20 bytes of JSON, and a library with long credit lists would
|
||||
transfer proportionally more for the same row count. Measured on a seeded 20,000-song library
|
||||
(in-memory SQLite, so the wall times are a floor, not a production figure):
|
||||
|
||||
| case | rows read | round trips | payload | wall |
|
||||
|---|---|---|---|---|
|
||||
| worst case — no match, full walk | 20,000 | 10 | **391.9 KiB** (avg 20.1 B/row) | 119 ms SQL / ~40 ms warm end-to-end |
|
||||
| empty `q` (fills `limit` on page 1) | 2,000 | 1 | ~39 KiB | ~60 ms |
|
||||
| dense prefix (`rad`) | 2,000 | 1 | ~39 KiB | ~38 ms |
|
||||
| non-ASCII prefix (`beyoncé`) | 2,000 | 1 | ~39 KiB | ~39 ms |
|
||||
|
||||
Judged acceptable **for this shape of library**: the worst case is a debounced typeahead keystroke
|
||||
that matches nothing, at ~392 KiB and tens of milliseconds against a local SQLite file. Dense queries
|
||||
— including the empty `q` the combobox opens with — stop on the first page. Re-measure rather than
|
||||
extrapolate if credit lists are long or the provider is MySQL over a network. **If it ever becomes
|
||||
unacceptable, do not reintroduce selectivity;** that is the trap this record exists to document. Go
|
||||
to #669.
|
||||
|
||||
### Accepted losses
|
||||
|
||||
A match past row 20,000 is not found — 20,000 filler rows then `"éclair"` at 20,001 returns `[]`, and
|
||||
a test pins exactly that rather than pretending otherwise. That is the documented bounded-best-effort
|
||||
contract, and unlike attempts 1–4 it now depends only on row count, not on prefix shape, deletion
|
||||
history or `Id` distribution.
|
||||
|
||||
**Follow-up: a normalized `SongArtist` join table** (the shape `MusicVideoArtist` already has) makes
|
||||
the predicate seekable, so there is nothing left to bound and nothing to transfer. Cost: 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. Tracked as **ersatztv#669**; 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. Tracked as **ersatztv#668**. Noted here so the next reader does not mistake the
|
||||
list-valued fix for a global one, and so the scoped ordinal wording above is not "tidied" into a
|
||||
broader claim.
|
||||
@@ -619,12 +619,16 @@ 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.
|
||||
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
|
||||
value.
|
||||
- **Single-child-group normalization** (#438) — `normalizeGroup` (`validation.ts`) coerces a group's
|
||||
`match` to `all` whenever it has fewer than two children, recursively. A one-child `any` group is
|
||||
semantically identical to `all` but doesn't round-trip through `compile`→`parse` (the compiled Lucene
|
||||
|
||||
@@ -11,6 +11,15 @@ REPO_ROOT="$(pwd)"
|
||||
# dotnet-getdocument against ErsatzTV.dll + ErsatzTV.deps.json, which don't exist in a clean
|
||||
# tree (e.g. the CI api-docs job, which only restores). Without the build the target fails with
|
||||
# "The specified deps.json … does not exist" (exit 129). Build first, then generate.
|
||||
#
|
||||
# LOCAL-DEV SHARP EDGE: if the project is ALREADY built and nothing changed, MSBuild skips the
|
||||
# document-generation work but still runs RenameOpenApiFiles (AfterTargets), whose Move then fails
|
||||
# with MSB3680 "ErsatzTV.json does not exist" — because nothing produced it. The script correctly
|
||||
# exits non-zero, but a caller that pipes this (`./scripts/update-openapi.sh | tail`) sees the
|
||||
# PIPELINE's status, i.e. tail's 0, and reads a no-op as success — leaving stale artifacts to fail
|
||||
# the blocking api-docs CI job. Before verifying artifacts are current, `touch` a file the project
|
||||
# compiles (or check this script's own exit status, unpiped). CI is unaffected: it restores into a
|
||||
# clean tree, so the generation never skips.
|
||||
(cd ErsatzTV && dotnet build && dotnet build -t:GenerateOpenApiDocuments) || exit
|
||||
|
||||
cd "$REPO_ROOT" || exit
|
||||
|
||||
Reference in New Issue
Block a user