Review of 1b78dc9e found the pre-filter's correctness claim was false, and the claim was in the decision record as well as the code. F1 (high). The pattern JSON-encoded the whole query prefix on the reasoning that the stored text escapes non-ASCII, so encoding the prefix the same way would line up. It does not: SQL LOWER() lowercases the *escape text* (`É` -> `é`); it cannot case-fold the codepoint that escape denotes. So `q=é` built `%"é%`, the stored `Édith Piaf` never matched, and the row was discarded before the in-memory filter could accept it. Every accented artist — Beyoncé, Björk, Sigur Rós, Édith Piaf — was silently unsuggestable, which in a music library is the common case. The invariant that was missing, now stated in the code: the SQL pre-filter is an OPTIMIZATION. It may over-match; it must never under-match. Correctness lives in the in-memory filter. So the pattern now narrows only on the leading run of characters the JSON writer stores verbatim and stops at the first character it cannot prove — `q=Beyoncé` still narrows on `beyonc`, `q=é` narrows on nothing and leans on the row cap. Soundness rests on two facts now asserted by exhaustive computation rather than argued: no non-ASCII codepoint in U+0080..U+10FFFF OrdinalIgnoreCase-equals a printable ASCII character (false for InvariantCultureIgnoreCase, which folds ~190 — the choice of Ordinal is load-bearing), and the exact set of ASCII the encoder escapes. F1b. `UseRequestLocalization` honours Accept-Language, so the culture was caller-controlled and `ToLower()` plus the default linguistic `StartsWith(string)` let a header change the answer. Comparison is now OrdinalIgnoreCase and ordering StringComparer.Ordinal throughout — including the shared FilterSortTake that state/video_dynamic_range/content_rating also use. Sets unchanged, order now ordinal rather than culture-dependent. F2. The merge comment asserted an exactness the code does not have: sources truncate by their own ordering (DB collation / primary key), not the merge's, so a dropped value can outrank a survivor. Comment and record now say best-effort, exact only below the truncation points. F3/F4. The cap now rides `ORDER BY Id` rather than the JSON column: MySQL sorts TEXT by only max_sort_length bytes, so the old ordering was not deterministic there, and sorting the whole matching set was avoidable work. What the cap still does NOT bound is the scan — a leading-wildcard LIKE cannot seek an index — so that cost is now documented as accepted, with a normalized `SongArtist` table named as the follow-up candidate rather than left implicit. Every clause above is covered by a test verified to FAIL when that clause is mutated (old pattern builder: 5 red; culture chain: 3 red; cap=3 / cap=limit / ORDER BY json / no cap: red each). F5. Converted to a proper supersession. The old record did not merely hold a stale fact — it recorded song/music-video credits as an "intentionally-uncovered gap" and album_artist as unsupported, and this reverses that call, which `docs.decision-lifecycle` says is never a line-edit. `api.search-field-values` is archived with its original prose restored, and `api.search-field-values-sources` replaces it carrying the whole endpoint contract.
111 lines
4.6 KiB
C#
111 lines
4.6 KiB
C#
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");
|
|
}
|
|
}
|
|
|
|
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));
|
|
}
|