From 65af2bcbd7f45d417ce8308da42a35fe4d9d0cac Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sat, 12 Sep 2026 16:21:42 +1000 Subject: [PATCH] fix(db): harden the PostgreSQL upgrade against odd legacy data - accept only the two id shapes the uuid cast parses, so misplaced hyphens null out instead of aborting the migration - clear an OwnerId that already is the detached placeholder, matching the SQLite chain, so CleanupOrphanedExtras cannot delete the item - recreate the placeholder item before the repoint if it is missing - compare the migrated schema against the model, constraints included --- .../20260306000000_UpgradeToServer12Schema.cs | 24 ++- .../Migrations/PostgreSqlUpgradeTests.cs | 154 +++++++++++++++++- 2 files changed, 167 insertions(+), 11 deletions(-) diff --git a/src/Jellyfin.Database/Jellyfin.Database.Providers.PostgreSQL/Migrations/20260306000000_UpgradeToServer12Schema.cs b/src/Jellyfin.Database/Jellyfin.Database.Providers.PostgreSQL/Migrations/20260306000000_UpgradeToServer12Schema.cs index e042c10368..0c76bdf4d8 100644 --- a/src/Jellyfin.Database/Jellyfin.Database.Providers.PostgreSQL/Migrations/20260306000000_UpgradeToServer12Schema.cs +++ b/src/Jellyfin.Database/Jellyfin.Database.Providers.PostgreSQL/Migrations/20260306000000_UpgradeToServer12Schema.cs @@ -119,24 +119,28 @@ namespace Jellyfin.Database.Providers.PostgreSQL.Migrations defaultValue: false); // PostgreSQL refuses an implicit text to uuid conversion, so both columns are converted with an explicit - // USING cast. Anything that is not 32 hexadecimal digits, and the all-zero placeholder, becomes null. + // USING cast. Only the two shapes the cast accepts survive: 32 hexadecimal digits, or the hyphenated + // 8-4-4-4-12 form. A digit count alone is not enough, because misplaced hyphens keep the count and still + // fail the cast, which would abort the whole migration. migrationBuilder.Sql( """ UPDATE "BaseItems" SET "PrimaryVersionId" = NULL WHERE "PrimaryVersionId" IS NOT NULL - AND (replace("PrimaryVersionId", '-', '') !~ '^[0-9a-fA-F]{32}$' + AND ("PrimaryVersionId" !~ '^([0-9a-fA-F]{32}|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})$' OR replace("PrimaryVersionId", '-', '') = '00000000000000000000000000000000'); ALTER TABLE "BaseItems" ALTER COLUMN "PrimaryVersionId" TYPE uuid USING "PrimaryVersionId"::uuid; """); + // The detached-item placeholder is cleared as well, matching the SQLite chain: leaving it in place would + // make CleanupOrphanedExtras delete an item that has a real owner relation only by coincidence. migrationBuilder.Sql( """ UPDATE "BaseItems" SET "OwnerId" = NULL WHERE "OwnerId" IS NOT NULL - AND (replace("OwnerId", '-', '') !~ '^[0-9a-fA-F]{32}$' - OR replace("OwnerId", '-', '') = '00000000000000000000000000000000'); + AND ("OwnerId" !~ '^([0-9a-fA-F]{32}|[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})$' + OR replace("OwnerId", '-', '') IN ('00000000000000000000000000000000', '00000000000000000000000000000001')); ALTER TABLE "BaseItems" ALTER COLUMN "OwnerId" TYPE uuid USING "OwnerId"::uuid; @@ -302,6 +306,18 @@ namespace Jellyfin.Database.Providers.PostgreSQL.Migrations table: "LinkedChildren", columns: new[] { "ParentId", "ChildType" }); + // The baseline seeds the placeholder, but the repoint below and the foreign key both depend on it, so it is + // recreated if anything removed it rather than letting the migration abort. + migrationBuilder.Sql( + """ + INSERT INTO "BaseItems" ("Id", "Type", "Name", "IsMovie", "IsLocked", "IsSeries", "IsRepeat", + "IsInMixedFolder", "IsFolder", "IsVirtualItem") + VALUES ('00000000-0000-0000-0000-000000000001', 'PLACEHOLDER', + 'This is a placeholder item for UserData that has been detached from its original item', + false, false, false, false, false, false, false) + ON CONFLICT ("Id") DO NOTHING; + """); + // Owners that no longer exist are repointed at the detached-item placeholder so the new self referencing // foreign key holds. The CleanupOrphanedExtras routine removes the items afterwards. migrationBuilder.Sql( diff --git a/tests/Jellyfin.Server.Tests/Migrations/PostgreSqlUpgradeTests.cs b/tests/Jellyfin.Server.Tests/Migrations/PostgreSqlUpgradeTests.cs index 436ebc363f..9f7ac1481a 100644 --- a/tests/Jellyfin.Server.Tests/Migrations/PostgreSqlUpgradeTests.cs +++ b/tests/Jellyfin.Server.Tests/Migrations/PostgreSqlUpgradeTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Globalization; +using System.Linq; using System.Threading; using System.Threading.Tasks; using DotNet.Testcontainers.Builders; @@ -10,10 +11,14 @@ using Jellyfin.Database.Implementations.Entities; using Jellyfin.Database.Implementations.Locking; using Jellyfin.Database.Providers.PostgreSQL; using Jellyfin.Server.Migrations.Routines; +using Jellyfin.Server.ServerSetupApp; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Library; using Microsoft.EntityFrameworkCore; using Microsoft.EntityFrameworkCore.Infrastructure; using Microsoft.EntityFrameworkCore.Migrations; using Microsoft.Extensions.Logging.Abstractions; +using Moq; using Npgsql; using Testcontainers.PostgreSql; using Xunit; @@ -36,9 +41,15 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime private const string OwnedItemId = "22222222-2222-2222-2222-222222222222"; private const string UnparseableOwnerItemId = "33333333-3333-3333-3333-333333333333"; private const string DanglingOwnerItemId = "44444444-4444-4444-4444-444444444444"; + private const string MalformedOwnerItemId = "55555555-5555-5555-5555-555555555555"; + private const string PlaceholderOwnedItemId = "66666666-6666-6666-6666-666666666666"; private const string AliceId = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"; private const string BobId = "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"; + // Thirty-two hexadecimal digits with hyphens in the wrong places: a digit count alone accepts it, the uuid cast + // does not. + private const string MalformedGuid = "1111111-11111-1111-1111-111111111111"; + private readonly PostgreSqlContainer _container; public PostgreSqlUpgradeTests() @@ -134,12 +145,18 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime } [Fact] - public async Task LegacyDatabaseAndFreshInstall_EndUpWithTheSameSchema() + public async Task LegacyDatabaseAndFreshInstall_EndUpWithTheModelSchema() { var cancellationToken = TestContext.Current.CancellationToken; + var modelConnectionString = await CreateDatabaseAsync("schema_model", cancellationToken); var freshConnectionString = await CreateDatabaseAsync("schema_fresh", cancellationToken); var upgradedConnectionString = await CreateDatabaseAsync("schema_upgraded", cancellationToken); + // The model built straight from the context is the reference: comparing the two migrated databases against + // each other only proves they drifted together. + await using var modelDataSource = new NpgsqlDataSourceBuilder(modelConnectionString).Build(); + await CreateFromModelAsync(modelDataSource, cancellationToken); + await using var freshDataSource = new NpgsqlDataSourceBuilder(freshConnectionString).Build(); await MigrateAsync(freshDataSource, null, cancellationToken); @@ -147,12 +164,88 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime await CreateLegacyDatabaseAsync(upgradedDataSource, cancellationToken); await RunUpgradeInStartupOrderAsync(upgradedDataSource, cancellationToken); - Assert.Equal( - await DescribeColumnsAsync(freshDataSource, cancellationToken), - await DescribeColumnsAsync(upgradedDataSource, cancellationToken)); - Assert.Equal( - await DescribeIndexesAsync(freshDataSource, cancellationToken), - await DescribeIndexesAsync(upgradedDataSource, cancellationToken)); + var modelColumns = await DescribeColumnsAsync(modelDataSource, cancellationToken); + var modelIndexes = await DescribeIndexesAsync(modelDataSource, cancellationToken); + var modelConstraints = await DescribeConstraintsAsync(modelDataSource, cancellationToken); + + Assert.Equal(modelColumns, await DescribeColumnsAsync(freshDataSource, cancellationToken)); + Assert.Equal(modelIndexes, await DescribeIndexesAsync(freshDataSource, cancellationToken)); + Assert.Equal(modelConstraints, await DescribeConstraintsAsync(freshDataSource, cancellationToken)); + + Assert.Equal(modelColumns, await DescribeColumnsAsync(upgradedDataSource, cancellationToken)); + Assert.Equal(modelIndexes, await DescribeIndexesAsync(upgradedDataSource, cancellationToken)); + Assert.Equal(modelConstraints, await DescribeConstraintsAsync(upgradedDataSource, cancellationToken)); + } + + [Fact] + public async Task LegacyDatabase_MalformedGuidIds_AreCleared() + { + var cancellationToken = TestContext.Current.CancellationToken; + var connectionString = await CreateDatabaseAsync("malformed_guid", cancellationToken); + + await using var dataSource = new NpgsqlDataSourceBuilder(connectionString).Build(); + await CreateLegacyDatabaseAsync(dataSource, cancellationToken); + await InsertLegacyItemAsync(dataSource, MalformedOwnerItemId, "Malformed", MalformedGuid, MalformedGuid, cancellationToken); + + await RunUpgradeInStartupOrderAsync(dataSource, cancellationToken); + + Assert.Null(await NullableScalarAsync(dataSource, $"""SELECT "OwnerId" FROM "BaseItems" WHERE "Id" = '{MalformedOwnerItemId}'""", cancellationToken)); + Assert.Null(await NullableScalarAsync(dataSource, $"""SELECT "PrimaryVersionId" FROM "BaseItems" WHERE "Id" = '{MalformedOwnerItemId}'""", cancellationToken)); + await AssertServer12SchemaAsync(dataSource, cancellationToken); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task LegacyDatabase_UpgradesWhetherOrNotThePlaceholderItemExists(bool placeholderExists) + { + var cancellationToken = TestContext.Current.CancellationToken; + var connectionString = await CreateDatabaseAsync(placeholderExists ? "placeholder_present" : "placeholder_missing", cancellationToken); + + await using var dataSource = new NpgsqlDataSourceBuilder(connectionString).Build(); + await CreateLegacyDatabaseAsync(dataSource, cancellationToken); + if (!placeholderExists) + { + await ExecuteAsync(dataSource, $"""DELETE FROM "BaseItems" WHERE "Id" = '{PlaceholderItemId}'""", cancellationToken); + } + + await RunUpgradeInStartupOrderAsync(dataSource, cancellationToken); + + // The dangling owner is repointed at the placeholder, so the foreign key only holds if the placeholder is there. + Assert.Equal(1L, await ScalarAsync(dataSource, $"""SELECT count(*) FROM "BaseItems" WHERE "Id" = '{PlaceholderItemId}'""", cancellationToken)); + Assert.Equal(new Guid(PlaceholderItemId), await ScalarAsync(dataSource, $"""SELECT "OwnerId" FROM "BaseItems" WHERE "Id" = '{DanglingOwnerItemId}'""", cancellationToken)); + await AssertServer12SchemaAsync(dataSource, cancellationToken); + } + + [Fact] + public async Task LegacyDatabase_ItemOwnedByThePlaceholder_SurvivesCleanupOrphanedExtras() + { + var cancellationToken = TestContext.Current.CancellationToken; + var connectionString = await CreateDatabaseAsync("placeholder_owner", cancellationToken); + + await using var dataSource = new NpgsqlDataSourceBuilder(connectionString).Build(); + await CreateLegacyDatabaseAsync(dataSource, cancellationToken); + await InsertLegacyItemAsync(dataSource, PlaceholderOwnedItemId, "Owned by the placeholder", PlaceholderItemId, null, cancellationToken); + + await RunUpgradeInStartupOrderAsync(dataSource, cancellationToken); + + // The SQLite chain clears an owner that already is the placeholder, so the item is not mistaken for an orphan. + Assert.Null(await NullableScalarAsync(dataSource, $"""SELECT "OwnerId" FROM "BaseItems" WHERE "Id" = '{PlaceholderOwnedItemId}'""", cancellationToken)); + + var deletedItemIds = new List(); + var libraryManager = new Mock(); + libraryManager + .Setup(manager => manager.DeleteItemsUnsafeFast(It.IsAny>(), It.IsAny())) + .Callback, bool>((items, _) => deletedItemIds.AddRange(items.Select(item => item.Id))); + + await new CleanupOrphanedExtras( + Mock.Of>(), + new SingleContextFactory(dataSource), + libraryManager.Object).PerformAsync(cancellationToken); + + Assert.Contains(new Guid(DanglingOwnerItemId), deletedItemIds); + Assert.DoesNotContain(new Guid(PlaceholderOwnedItemId), deletedItemIds); + Assert.Equal(1L, await ScalarAsync(dataSource, $"""SELECT count(*) FROM "BaseItems" WHERE "Id" = '{PlaceholderOwnedItemId}'""", cancellationToken)); } [Fact] @@ -190,6 +283,18 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime } } + /// + /// Creates the schema straight from the current model, without going through any migration. + /// + private static async Task CreateFromModelAsync(NpgsqlDataSource dataSource, CancellationToken cancellationToken) + { + var context = CreateContext(dataSource); + await using (context.ConfigureAwait(false)) + { + await context.Database.EnsureCreatedAsync(cancellationToken).ConfigureAwait(false); + } + } + private static async Task> AppliedMigrationsAsync(NpgsqlDataSource dataSource, CancellationToken cancellationToken) { var context = CreateContext(dataSource); @@ -271,6 +376,41 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime """, cancellationToken); + private static Task> DescribeConstraintsAsync(NpgsqlDataSource dataSource, CancellationToken cancellationToken) + => QueryLinesAsync( + dataSource, + """ + SELECT relation.relname, constraint_.conname, pg_get_constraintdef(constraint_.oid) + FROM pg_constraint AS constraint_ + JOIN pg_class AS relation ON relation.oid = constraint_.conrelid + JOIN pg_namespace AS namespace_ ON namespace_.oid = relation.relnamespace + WHERE namespace_.nspname = 'public' AND relation.relname <> '__EFMigrationsHistory' + ORDER BY relation.relname, constraint_.conname + """, + cancellationToken); + + /// + /// Adds one more item to a legacy database, with the owner and version ids still held as text. + /// + private static Task InsertLegacyItemAsync( + NpgsqlDataSource dataSource, + string itemId, + string name, + string? ownerId, + string? primaryVersionId, + CancellationToken cancellationToken) + => ExecuteAsync( + dataSource, + $""" + INSERT INTO "BaseItems" ("Id", "Type", "IsMovie", "IsLocked", "IsSeries", "IsRepeat", "IsInMixedFolder", + "IsFolder", "IsVirtualItem", "Name", "Path", "OwnerId", "PrimaryVersionId") + VALUES ('{itemId}', 'MediaBrowser.Controller.Entities.Video', false, false, false, false, false, false, false, + '{name}', '/media/{itemId}.mkv', {Literal(ownerId)}, {Literal(primaryVersionId)}); + """, + cancellationToken); + + private static string Literal(string? value) => value is null ? "NULL" : $"'{value}'"; + /// /// Builds a database that looks like one written by the previous build: only the baseline migration applied, and /// data that only the old column types could hold.