fix(491): wire the unique-violation classifier in the scanner; make folder lookup case-exact
Review of 491f5099 found the fix inert in the only process that runs it, plus a MySQL collation defect in the lookup. B1 — TvContext.IsUniqueConstraintViolation was assigned only in ErsatzTV/ Startup.cs, but ErsatzTV.Scanner is a separate executable and every production caller of GetOrAddFolder/SetEtag lives there. The classifier kept its '_ => false' default, so the catch never ran and the DbUpdateException failed the whole scan - worse than the duplicate row it replaced. Wire both provider branches in ErsatzTV.Scanner/Program.cs, and add ProviderStaticsWiringTests (architecture) asserting the scanner assigns every TvContext static the host assigns, with IsSqlite documented as the one exemption. H1 — GetFolder's 'Path == folder' is case-insensitive on MySQL while PathHash is case-sensitive, and FirstOrDefault was unordered: a scan of '/x/foo' could resolve the '/x/Foo' row and stamp the wrong hash onto it (verified on MySQL 8.4: the WHERE matches both, LIMIT 1 returns the wrong one). Treat the SQL equality as a narrowing filter, order by Id, and settle identity ordinally. Route the heal through EF and drop a classified violation, so an opportunistic maintenance write can never abort a scan. Also: run the dedupe migration test with foreign keys ON (matching prod), clear the keeper's etag, null out a self-parent, and document the cleanup's limits (NULL paths excluded, Down does not restore deleted rows, CI's fresh-DB apply covers none of the data mutation). Refs #488 #308 fix #491
This commit is contained in:
@@ -0,0 +1,104 @@
|
||||
using System.Text.RegularExpressions;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Architecture.Tests;
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#491: <c>TvContext</c> carries settable provider statics (<c>LastInsertedRowId</c>,
|
||||
/// <c>CaseInsensitiveCollation</c>, <c>IsUniqueConstraintViolation</c>, …) that Infrastructure code
|
||||
/// reads at runtime. There are TWO composition roots that execute that Infrastructure code —
|
||||
/// <c>ErsatzTV/Startup.cs</c> (the host) and <c>ErsatzTV.Scanner/Program.cs</c> (a separate
|
||||
/// executable launched per scan by <c>CallLibraryScannerHandler</c>) — and each wires the statics in
|
||||
/// its own copy of the provider branch.
|
||||
/// <para>
|
||||
/// The failure mode this guards is "a static nobody assigned": #491 added
|
||||
/// <c>IsUniqueConstraintViolation</c> to <c>Startup</c> only, so every production caller of
|
||||
/// <c>GetOrAddFolder</c> (all of which live in the scanner) silently kept the conservative
|
||||
/// <c>_ => false</c> default and the new catch was inert. Nothing about that is visible in a
|
||||
/// unit test, because every test harness wires the classifier itself.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Source-level rather than reflective on purpose: the wiring lives inside a host-builder
|
||||
/// lambda that cannot be invoked without standing up a real application, and the thing being
|
||||
/// asserted is precisely that a line of code exists in both files.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class ProviderStaticsWiringTests
|
||||
{
|
||||
/// <summary>
|
||||
/// Statics the host wires that the scanner deliberately does not. Add to this only with a reason:
|
||||
/// the default must be provably harmless in the scanner process.
|
||||
/// </summary>
|
||||
private static readonly Dictionary<string, string> ScannerExemptions = new()
|
||||
{
|
||||
// Only read by DbInitializer / DatabaseMigratorService, which run in the host exclusively; no
|
||||
// Infrastructure code on a scan path reads it. Pre-dates #491.
|
||||
["IsSqlite"] = "read only by DbInitializer + DatabaseMigratorService, both host-only"
|
||||
};
|
||||
|
||||
private static string HostSource => ReadRepoFile(Path.Combine("ErsatzTV", "Startup.cs"));
|
||||
|
||||
private static string ScannerSource => ReadRepoFile(Path.Combine("ErsatzTV.Scanner", "Program.cs"));
|
||||
|
||||
[Test]
|
||||
public void Scanner_should_wire_every_TvContext_provider_static_the_host_wires()
|
||||
{
|
||||
HashSet<string> host = AssignedStatics(HostSource);
|
||||
HashSet<string> scanner = AssignedStatics(ScannerSource);
|
||||
|
||||
// sanity: the parser found the wiring at all, so a rename can't turn this test into a no-op
|
||||
host.ShouldContain("LastInsertedRowId");
|
||||
host.ShouldContain("IsUniqueConstraintViolation");
|
||||
scanner.ShouldContain("LastInsertedRowId");
|
||||
|
||||
List<string> missing = host
|
||||
.Except(scanner)
|
||||
.Except(ScannerExemptions.Keys)
|
||||
.OrderBy(name => name, StringComparer.Ordinal)
|
||||
.ToList();
|
||||
|
||||
missing.ShouldBeEmpty(
|
||||
"ErsatzTV.Scanner/Program.cs does not assign TvContext static(s) that ErsatzTV/Startup.cs "
|
||||
+ $"assigns: {string.Join(", ", missing)}. The scanner is a separate process, so an unassigned "
|
||||
+ "static keeps its default in every library scan. Wire it in BOTH provider branches, or add "
|
||||
+ "it to ScannerExemptions with a reason if the default is provably harmless there.");
|
||||
}
|
||||
|
||||
[Test]
|
||||
public void Both_hosts_should_wire_the_unique_constraint_classifier_for_both_providers()
|
||||
{
|
||||
// The specific #491 regression, asserted directly rather than via set arithmetic: the classifier
|
||||
// must be pointed at a real provider implementation on BOTH branches of BOTH composition roots.
|
||||
foreach ((string name, string source) in new[] { ("host", HostSource), ("scanner", ScannerSource) })
|
||||
{
|
||||
source.ShouldContain(
|
||||
"TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation",
|
||||
customMessage: $"{name} does not wire the Sqlite unique-constraint classifier");
|
||||
source.ShouldContain(
|
||||
"TvContext.IsUniqueConstraintViolation = MySqlErrorClassifier.IsUniqueConstraintViolation",
|
||||
customMessage: $"{name} does not wire the MySql unique-constraint classifier");
|
||||
}
|
||||
}
|
||||
|
||||
private static HashSet<string> AssignedStatics(string source) =>
|
||||
Regex.Matches(source, @"\bTvContext\.(?<name>[A-Za-z_][A-Za-z0-9_]*)\s*=[^=]")
|
||||
.Select(m => m.Groups["name"].Value)
|
||||
.ToHashSet(StringComparer.Ordinal);
|
||||
|
||||
private static string ReadRepoFile(string relativePath)
|
||||
{
|
||||
var directory = new DirectoryInfo(AppContext.BaseDirectory);
|
||||
while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "ErsatzTV.sln")))
|
||||
{
|
||||
directory = directory.Parent;
|
||||
}
|
||||
|
||||
directory.ShouldNotBeNull("could not locate the repository root (no ErsatzTV.sln above the test binary)");
|
||||
|
||||
string path = Path.Combine(directory!.FullName, relativePath);
|
||||
File.Exists(path).ShouldBeTrue($"expected source file not found: {path}");
|
||||
return File.ReadAllText(path);
|
||||
}
|
||||
}
|
||||
+11
@@ -59,6 +59,17 @@ namespace ErsatzTV.Infrastructure.MySql.Migrations
|
||||
WHERE ParentId IN (SELECT LoserId FROM `__LibraryFolderDedupe`)
|
||||
""");
|
||||
|
||||
// A folder parented on its OWN duplicate would become its own parent above. Unreachable from
|
||||
// any code path today, but this is a tree the scanner walks, so remove the cycle class rather
|
||||
// than reason about it.
|
||||
migrationBuilder.Sql("UPDATE LibraryFolder SET ParentId = NULL WHERE ParentId = Id");
|
||||
|
||||
// Clear the survivor's etag. Which duplicate the scanner was actually writing to was
|
||||
// arbitrary, so MIN(Id)'s etag may describe a stale view of the folder and would suppress the
|
||||
// next rescan. A null etag costs exactly one rescan and cannot be wrong.
|
||||
migrationBuilder.Sql(
|
||||
"UPDATE LibraryFolder SET Etag = NULL WHERE Id IN (SELECT KeeperId FROM `__LibraryFolderDedupe`)");
|
||||
|
||||
// ImageFolderDuration is 1:1 with LibraryFolder (unique index on LibraryFolderId), so the
|
||||
// duplicates' rows cannot all be repointed. Keep the keeper's own setting when it has one;
|
||||
// otherwise promote exactly one loser's (lowest Id) and drop the rest.
|
||||
|
||||
+11
@@ -57,6 +57,17 @@ namespace ErsatzTV.Infrastructure.Sqlite.Migrations
|
||||
WHERE ParentId IN (SELECT LoserId FROM __LibraryFolderDedupe)
|
||||
""");
|
||||
|
||||
// A folder parented on its OWN duplicate would become its own parent above. Unreachable from
|
||||
// any code path today, but this is a tree the scanner walks, so remove the cycle class rather
|
||||
// than reason about it.
|
||||
migrationBuilder.Sql("UPDATE LibraryFolder SET ParentId = NULL WHERE ParentId = Id");
|
||||
|
||||
// Clear the survivor's etag. Which duplicate the scanner was actually writing to was
|
||||
// arbitrary, so MIN(Id)'s etag may describe a stale view of the folder and would suppress the
|
||||
// next rescan. A null etag costs exactly one rescan and cannot be wrong.
|
||||
migrationBuilder.Sql(
|
||||
"UPDATE LibraryFolder SET Etag = NULL WHERE Id IN (SELECT KeeperId FROM __LibraryFolderDedupe)");
|
||||
|
||||
// ImageFolderDuration is 1:1 with LibraryFolder (unique index on LibraryFolderId), so the
|
||||
// duplicates' rows cannot all be repointed. Keep the keeper's own setting when it has one;
|
||||
// otherwise promote exactly one loser's (lowest Id) and drop the rest.
|
||||
|
||||
@@ -225,13 +225,31 @@ public class LibraryRepository(IFileSystem fileSystem, IDbContextFactory<TvConte
|
||||
}
|
||||
else if (string.IsNullOrEmpty(knownFolder.PathHash))
|
||||
{
|
||||
// heal a row created before the PathHash column existed, so it participates in the unique
|
||||
// Heal a row created before the PathHash column existed, so it participates in the unique
|
||||
// index from here on (a null hash is distinct from every other value, so it does not).
|
||||
knownFolder.PathHash = PathUtils.GetPathHash(folder);
|
||||
|
||||
await dbContext.Connection.ExecuteAsync(
|
||||
"UPDATE LibraryFolder SET PathHash = @PathHash WHERE Id = @Id",
|
||||
new { knownFolder.PathHash, knownFolder.Id });
|
||||
//
|
||||
// This is opportunistic maintenance on a hot scan path, so it must never be able to abort a
|
||||
// scan. It goes through EF rather than a raw Dapper UPDATE precisely so a collision surfaces
|
||||
// as a classifiable DbUpdateException instead of a bare provider exception, and a lost heal
|
||||
// is simply left for the next scan. Reachable only if some other row already owns
|
||||
// (LibraryPathId, hash) — a legacy duplicate the migration's dedupe could not see (e.g. one
|
||||
// with a NULL Path, which `NULL = NULL` excludes from its grouping).
|
||||
string pathHash = PathUtils.GetPathHash(folder);
|
||||
try
|
||||
{
|
||||
LibraryFolder tracked = await dbContext.LibraryFolders
|
||||
.FirstOrDefaultAsync(f => f.Id == knownFolder.Id && f.PathHash == null);
|
||||
if (tracked is not null)
|
||||
{
|
||||
tracked.PathHash = pathHash;
|
||||
await dbContext.SaveChangesAsync();
|
||||
knownFolder.PathHash = pathHash;
|
||||
}
|
||||
}
|
||||
catch (DbUpdateException ex) when (TvContext.IsUniqueConstraintViolation(ex))
|
||||
{
|
||||
// another row already owns this hash — leave this one unhealed rather than fail the scan
|
||||
}
|
||||
}
|
||||
|
||||
// update parent folder if not present
|
||||
@@ -268,11 +286,34 @@ public class LibraryRepository(IFileSystem fileSystem, IDbContextFactory<TvConte
|
||||
new { Path = normalizedLibraryPath, libraryPath.Id });
|
||||
}
|
||||
|
||||
private static Task<LibraryFolder> GetFolder(TvContext dbContext, int libraryPathId, string folder) =>
|
||||
dbContext.LibraryFolders
|
||||
/// <summary>
|
||||
/// Resolve a folder by its exact path within a library path.
|
||||
/// <para>
|
||||
/// The SQL equality is only a *narrowing* filter, not the identity test: on MySQL, `Path` is a
|
||||
/// `longtext` under the case-INsensitive server default (the same collation the codebase names
|
||||
/// in <see cref="TvContext.CaseInsensitiveCollation" />), so `Path = @folder` also matches
|
||||
/// sibling folders differing only in case — which are legal on a case-sensitive filesystem and
|
||||
/// which the #491 migration deliberately preserves. Identity is settled in memory with an
|
||||
/// ORDINAL comparison, matching <c>PathUtils.GetPathHash</c>, which hashes the exact bytes.
|
||||
/// Without this the case-insensitive lookup and the case-sensitive hash disagree, and the
|
||||
/// PathHash heal below could stamp one sibling's hash onto the other's row.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Ordered by Id so the result is deterministic: an unordered <c>FirstOrDefault</c> may return a
|
||||
/// different candidate run to run as the query plan changes (adding the composite index alone
|
||||
/// can flip it), which would make the heal non-idempotent.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
private static async Task<LibraryFolder> GetFolder(TvContext dbContext, int libraryPathId, string folder)
|
||||
{
|
||||
List<LibraryFolder> candidates = await dbContext.LibraryFolders
|
||||
.AsNoTracking()
|
||||
.Filter(f => f.LibraryPathId == libraryPathId && f.Path == folder)
|
||||
.FirstOrDefaultAsync();
|
||||
.OrderBy(f => f.Id)
|
||||
.ToListAsync();
|
||||
|
||||
return candidates.Find(f => string.Equals(f.Path, folder, StringComparison.Ordinal));
|
||||
}
|
||||
|
||||
private static LibraryFolder CreateNewFolder(LibraryPath libraryPath, Option<int> maybeParentFolder, string folder)
|
||||
{
|
||||
|
||||
@@ -24,6 +24,7 @@ using ErsatzTV.Infrastructure.Emby;
|
||||
using ErsatzTV.Infrastructure.Images;
|
||||
using ErsatzTV.Infrastructure.Jellyfin;
|
||||
using ErsatzTV.Infrastructure.Metadata;
|
||||
using ErsatzTV.Infrastructure.MySql.Data;
|
||||
using ErsatzTV.Infrastructure.Plex;
|
||||
using ErsatzTV.Infrastructure.Runtime;
|
||||
using ErsatzTV.Infrastructure.Search;
|
||||
@@ -152,10 +153,15 @@ public class Program
|
||||
}
|
||||
});
|
||||
|
||||
// Keep this block in sync with ErsatzTV/Startup.cs — the scanner is a SEPARATE process
|
||||
// (launched by CallLibraryScannerHandler), so any TvContext provider static the host wires
|
||||
// has to be wired here too or it silently keeps its default in every scan.
|
||||
// ProviderStaticsWiringTests enforces that parity.
|
||||
if (databaseProvider == Provider.Sqlite.Name)
|
||||
{
|
||||
TvContext.LastInsertedRowId = "last_insert_rowid()";
|
||||
TvContext.CaseInsensitiveCollation = "NOCASE";
|
||||
TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation;
|
||||
|
||||
SqlMapper.AddTypeHandler(new DateTimeOffsetHandler());
|
||||
SqlMapper.AddTypeHandler(new GuidHandler());
|
||||
@@ -166,6 +172,7 @@ public class Program
|
||||
{
|
||||
TvContext.LastInsertedRowId = "last_insert_id()";
|
||||
TvContext.CaseInsensitiveCollation = "utf8mb4_general_ci";
|
||||
TvContext.IsUniqueConstraintViolation = MySqlErrorClassifier.IsUniqueConstraintViolation;
|
||||
}
|
||||
|
||||
services.AddHttpClient();
|
||||
|
||||
@@ -23,7 +23,17 @@ public class LibraryFolderDedupeMigrationTests
|
||||
private const string PreviousMigration = "Add_Channel_Origin";
|
||||
|
||||
private string _databasePath = null!;
|
||||
private DbContextOptions<TvContext> _options = null!;
|
||||
|
||||
// Seeding writes deliberately partial object graphs (a LibraryPath with no Library, a MediaFile with
|
||||
// no MediaVersion), so it needs foreign keys OFF — as every other harness in this suite does.
|
||||
private DbContextOptions<TvContext> _seedOptions = null!;
|
||||
|
||||
// The migration itself runs with foreign keys ON, matching production (`Startup.cs` builds the SQLite
|
||||
// connection string with `foreign keys=true`). This matters: the single most dangerous statement in
|
||||
// the #491 migration is `DELETE FROM LibraryFolder` against two Restrict foreign keys
|
||||
// (MediaFile.LibraryFolderId, LibraryFolder.ParentId). With enforcement off, a wrong repoint order
|
||||
// would still pass; with it on, the delete fails loudly.
|
||||
private DbContextOptions<TvContext> _migrateOptions = null!;
|
||||
|
||||
[SetUp]
|
||||
public void SetUp()
|
||||
@@ -32,12 +42,16 @@ public class LibraryFolderDedupeMigrationTests
|
||||
TvContext.IsUniqueConstraintViolation = SqliteErrorClassifier.IsUniqueConstraintViolation;
|
||||
|
||||
_databasePath = Path.Combine(Path.GetTempPath(), $"etv491-{Guid.NewGuid():N}.sqlite3");
|
||||
_options = new DbContextOptionsBuilder<TvContext>()
|
||||
_seedOptions = BuildOptions(foreignKeys: false);
|
||||
_migrateOptions = BuildOptions(foreignKeys: true);
|
||||
}
|
||||
|
||||
private DbContextOptions<TvContext> BuildOptions(bool foreignKeys) =>
|
||||
new DbContextOptionsBuilder<TvContext>()
|
||||
.UseSqlite(
|
||||
$"Data Source={_databasePath};Foreign Keys=False",
|
||||
$"Data Source={_databasePath};Foreign Keys={foreignKeys}",
|
||||
o => o.MigrationsAssembly("ErsatzTV.Infrastructure.Sqlite"))
|
||||
.Options;
|
||||
}
|
||||
|
||||
[TearDown]
|
||||
public void TearDown()
|
||||
@@ -55,7 +69,7 @@ public class LibraryFolderDedupeMigrationTests
|
||||
[Test]
|
||||
public async Task Migration_Collapses_Duplicate_Folders_And_Repoints_Their_Dependents()
|
||||
{
|
||||
await using (TvContext context = CreateContext())
|
||||
await using (TvContext context = SeedContext())
|
||||
{
|
||||
await context.Database.MigrateAsync(PreviousMigration);
|
||||
|
||||
@@ -70,7 +84,13 @@ public class LibraryFolderDedupeMigrationTests
|
||||
(2, 1, '/data/music/artist1', NULL, 'etag-dupe-a'),
|
||||
(3, 1, '/data/music/artist1', NULL, 'etag-dupe-b'),
|
||||
(4, 1, '/data/music/artist2', NULL, NULL),
|
||||
(5, 1, '/data/music/artist1/album', 3, NULL)
|
||||
(5, 1, '/data/music/artist1/album', 3, NULL),
|
||||
-- a case-differing sibling, legal on a case-sensitive filesystem: must SURVIVE
|
||||
(6, 1, '/data/music/ARTIST2', NULL, NULL),
|
||||
-- duplicate 3 is parented on duplicate 2: repointing both would make the survivor
|
||||
-- its own parent, the cycle the ParentId null-out guards
|
||||
(7, 1, '/data/music/artist3', 8, NULL),
|
||||
(8, 1, '/data/music/artist3', NULL, NULL)
|
||||
""");
|
||||
|
||||
// a media file on each duplicate, and an image-folder-duration on a duplicate only
|
||||
@@ -89,12 +109,12 @@ public class LibraryFolderDedupeMigrationTests
|
||||
""");
|
||||
}
|
||||
|
||||
await using (TvContext context = CreateContext())
|
||||
await using (TvContext context = MigrateContext())
|
||||
{
|
||||
await context.Database.MigrateAsync();
|
||||
}
|
||||
|
||||
await using (TvContext context = CreateContext())
|
||||
await using (TvContext context = MigrateContext())
|
||||
{
|
||||
// the duplicates are gone; the lowest id survives
|
||||
List<int> folderIds =
|
||||
@@ -102,8 +122,25 @@ public class LibraryFolderDedupeMigrationTests
|
||||
"SELECT Id FROM LibraryFolder WHERE Path = '/data/music/artist1'")).ToList();
|
||||
folderIds.ShouldBe([1]);
|
||||
|
||||
// the untouched folder and the child are still there
|
||||
(await context.Connection.ExecuteScalarAsync<int>("SELECT COUNT(*) FROM LibraryFolder")).ShouldBe(3);
|
||||
// survivors: keeper 1, artist2 (4), the child (5), the case-differing sibling (6), artist3's
|
||||
// keeper (7). Losers 2, 3 and 8 are gone.
|
||||
List<int> survivors =
|
||||
(await context.Connection.QueryAsync<int>(
|
||||
"SELECT Id FROM LibraryFolder ORDER BY Id")).ToList();
|
||||
survivors.ShouldBe([1, 4, 5, 6, 7]);
|
||||
|
||||
// a folder differing only in case is NOT a duplicate — it must survive untouched
|
||||
(await context.Connection.ExecuteScalarAsync<int>(
|
||||
"SELECT COUNT(*) FROM LibraryFolder WHERE Path = '/data/music/ARTIST2'")).ShouldBe(1);
|
||||
|
||||
// the keeper's etag is cleared: which duplicate the scanner was writing to was arbitrary, so
|
||||
// MIN(Id)'s etag could suppress the rescan that repairs the collapsed folder
|
||||
(await context.Connection.ExecuteScalarAsync<string>(
|
||||
"SELECT Etag FROM LibraryFolder WHERE Id = 1")).ShouldBeNull();
|
||||
|
||||
// the folder parented on its own duplicate did not become its own parent
|
||||
(await context.Connection.ExecuteScalarAsync<int?>(
|
||||
"SELECT ParentId FROM LibraryFolder WHERE Id = 7")).ShouldBeNull();
|
||||
|
||||
// every media file follows the keeper — nothing orphaned, nothing deleted
|
||||
List<int> mediaFolderIds =
|
||||
@@ -138,7 +175,7 @@ public class LibraryFolderDedupeMigrationTests
|
||||
public async Task Migration_Leaves_A_Database_Without_Duplicates_Alone()
|
||||
{
|
||||
// positive control: the cleanup must not touch rows that were already unique
|
||||
await using (TvContext context = CreateContext())
|
||||
await using (TvContext context = SeedContext())
|
||||
{
|
||||
await context.Database.MigrateAsync(PreviousMigration);
|
||||
await context.Connection.ExecuteAsync(
|
||||
@@ -153,12 +190,12 @@ public class LibraryFolderDedupeMigrationTests
|
||||
"INSERT INTO ImageFolderDuration (Id, LibraryFolderId, DurationSeconds) VALUES (1, 2, 30.0)");
|
||||
}
|
||||
|
||||
await using (TvContext context = CreateContext())
|
||||
await using (TvContext context = MigrateContext())
|
||||
{
|
||||
await context.Database.MigrateAsync();
|
||||
}
|
||||
|
||||
await using (TvContext context = CreateContext())
|
||||
await using (TvContext context = MigrateContext())
|
||||
{
|
||||
(await context.Connection.ExecuteScalarAsync<int>("SELECT COUNT(*) FROM LibraryFolder")).ShouldBe(2);
|
||||
(await context.Connection.ExecuteScalarAsync<int>("SELECT ParentId FROM LibraryFolder WHERE Id = 2"))
|
||||
@@ -172,9 +209,13 @@ public class LibraryFolderDedupeMigrationTests
|
||||
}
|
||||
}
|
||||
|
||||
private TvContext CreateContext() =>
|
||||
private TvContext SeedContext() => Create(_seedOptions);
|
||||
|
||||
private TvContext MigrateContext() => Create(_migrateOptions);
|
||||
|
||||
private static TvContext Create(DbContextOptions<TvContext> options) =>
|
||||
new(
|
||||
_options,
|
||||
options,
|
||||
NullLoggerFactory.Instance,
|
||||
new SlowQueryInterceptor(NullLogger<SlowQueryInterceptor>.Instance));
|
||||
}
|
||||
|
||||
@@ -154,6 +154,114 @@ public class LibraryRepositoryTests
|
||||
(await context.LibraryFolders.CountAsync(f => f.LibraryPathId == libraryPathId)).ShouldBe(1);
|
||||
}
|
||||
|
||||
// ersatztv#491 / H1: when the lookup matches more than one row (legacy duplicates that predate the
|
||||
// unique index, which is exactly the state the migration cleans up), it must resolve deterministically
|
||||
// to the lowest Id. An unordered FirstOrDefault can return a different row as the plan changes — and
|
||||
// adding the composite index alone can flip it — which would make the PathHash heal non-idempotent:
|
||||
// each scan would heal a different row and the second would collide on (LibraryPathId, PathHash).
|
||||
[Test]
|
||||
public async Task GetOrAddFolder_Should_Resolve_Legacy_Duplicates_Deterministically()
|
||||
{
|
||||
int libraryPathId = await SeedLibraryPath("/data/music");
|
||||
var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
|
||||
|
||||
int firstId;
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
var a = new LibraryFolder { LibraryPathId = libraryPathId, Path = "/data/music/artist1" };
|
||||
var b = new LibraryFolder { LibraryPathId = libraryPathId, Path = "/data/music/artist1" };
|
||||
await seed.LibraryFolders.AddRangeAsync(a, b);
|
||||
await seed.SaveChangesAsync();
|
||||
firstId = Math.Min(a.Id, b.Id);
|
||||
}
|
||||
|
||||
// repeated calls must agree, and must agree with MIN(Id) — the same row the migration keeps
|
||||
LibraryFolder first = await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
|
||||
LibraryFolder second = await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
|
||||
|
||||
first.Id.ShouldBe(firstId);
|
||||
second.Id.ShouldBe(firstId);
|
||||
|
||||
await using TvContext context = _db.CreateContext();
|
||||
|
||||
// exactly one of the two got the hash — the heal is idempotent, not alternating
|
||||
(await context.LibraryFolders.CountAsync(
|
||||
f => f.LibraryPathId == libraryPathId && f.PathHash != null)).ShouldBe(1);
|
||||
LibraryFolder healed = await context.LibraryFolders.SingleAsync(f => f.Id == firstId);
|
||||
healed.PathHash.ShouldBe(PathUtils.GetPathHash("/data/music/artist1"));
|
||||
}
|
||||
|
||||
// ersatztv#491 / H1: the lookup narrows with a SQL equality that is case-INsensitive on MySQL, while
|
||||
// PathHash is a case-SENSITIVE hash. Identity must be settled ordinally, or a scan of "/x/foo" can
|
||||
// resolve the "/x/Foo" row and stamp the wrong hash onto it. Reproduced provider-independently by
|
||||
// making the two rows exist and asserting the exact row is chosen and healed.
|
||||
[Test]
|
||||
public async Task GetOrAddFolder_Should_Not_Resolve_A_Folder_Differing_Only_In_Case()
|
||||
{
|
||||
int libraryPathId = await SeedLibraryPath("/data/music");
|
||||
var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
|
||||
|
||||
int upperId;
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
var upper = new LibraryFolder { LibraryPathId = libraryPathId, Path = "/data/music/Foo" };
|
||||
var lower = new LibraryFolder { LibraryPathId = libraryPathId, Path = "/data/music/foo" };
|
||||
await seed.LibraryFolders.AddRangeAsync(upper, lower);
|
||||
await seed.SaveChangesAsync();
|
||||
upperId = upper.Id;
|
||||
}
|
||||
|
||||
LibraryFolder result = await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/foo");
|
||||
|
||||
result.Path.ShouldBe("/data/music/foo");
|
||||
result.Id.ShouldNotBe(upperId);
|
||||
result.PathHash.ShouldBe(PathUtils.GetPathHash("/data/music/foo"));
|
||||
|
||||
await using TvContext context = _db.CreateContext();
|
||||
|
||||
// the case-differing sibling must be untouched — no foreign hash stamped onto it
|
||||
LibraryFolder upperPersisted = await context.LibraryFolders.SingleAsync(f => f.Id == upperId);
|
||||
upperPersisted.Path.ShouldBe("/data/music/Foo");
|
||||
upperPersisted.PathHash.ShouldBeNull();
|
||||
|
||||
// and no duplicate was inserted for either spelling
|
||||
(await context.LibraryFolders.CountAsync(f => f.LibraryPathId == libraryPathId)).ShouldBe(2);
|
||||
}
|
||||
|
||||
// The heal is opportunistic maintenance on a hot scan path: if some other row already owns
|
||||
// (LibraryPathId, hash) — a legacy duplicate the migration's grouping could not see — it must leave
|
||||
// the row unhealed rather than abort the scan.
|
||||
[Test]
|
||||
public async Task GetOrAddFolder_Should_Not_Fail_The_Scan_When_The_PathHash_Heal_Collides()
|
||||
{
|
||||
int libraryPathId = await SeedLibraryPath("/data/music");
|
||||
var libraryPath = new LibraryPath { Id = libraryPathId, Path = "/data/music", LibraryFolders = null };
|
||||
string hash = PathUtils.GetPathHash("/data/music/artist1");
|
||||
|
||||
int legacyId;
|
||||
await using (TvContext seed = _db.CreateContext())
|
||||
{
|
||||
// a legacy row with a null hash, plus a squatter that already owns the hash it would heal to
|
||||
var legacy = new LibraryFolder { LibraryPathId = libraryPathId, Path = "/data/music/artist1" };
|
||||
var squatter = new LibraryFolder
|
||||
{
|
||||
LibraryPathId = libraryPathId, Path = "/data/music/squatter", PathHash = hash
|
||||
};
|
||||
await seed.LibraryFolders.AddRangeAsync(legacy, squatter);
|
||||
await seed.SaveChangesAsync();
|
||||
legacyId = legacy.Id;
|
||||
}
|
||||
|
||||
// must not throw — the scan continues and simply returns the folder
|
||||
LibraryFolder result = await _repository.GetOrAddFolder(libraryPath, Option<int>.None, "/data/music/artist1");
|
||||
|
||||
result.Id.ShouldBe(legacyId);
|
||||
|
||||
await using TvContext context = _db.CreateContext();
|
||||
LibraryFolder persisted = await context.LibraryFolders.SingleAsync(f => f.Id == legacyId);
|
||||
persisted.PathHash.ShouldBeNull(); // heal declined, not half-applied
|
||||
}
|
||||
|
||||
private async Task<int> SeedLibraryPath(string path)
|
||||
{
|
||||
await using TvContext context = _db.CreateContext();
|
||||
|
||||
Reference in New Issue
Block a user