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
This commit is contained in:
@@ -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<long>(dataSource, $"""SELECT count(*) FROM "BaseItems" WHERE "Id" = '{PlaceholderItemId}'""", cancellationToken));
|
||||
Assert.Equal(new Guid(PlaceholderItemId), await ScalarAsync<Guid>(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<Guid>();
|
||||
var libraryManager = new Mock<ILibraryManager>();
|
||||
libraryManager
|
||||
.Setup(manager => manager.DeleteItemsUnsafeFast(It.IsAny<IReadOnlyCollection<BaseItem>>(), It.IsAny<bool>()))
|
||||
.Callback<IReadOnlyCollection<BaseItem>, bool>((items, _) => deletedItemIds.AddRange(items.Select(item => item.Id)));
|
||||
|
||||
await new CleanupOrphanedExtras(
|
||||
Mock.Of<IStartupLogger<CleanupOrphanedExtras>>(),
|
||||
new SingleContextFactory(dataSource),
|
||||
libraryManager.Object).PerformAsync(cancellationToken);
|
||||
|
||||
Assert.Contains(new Guid(DanglingOwnerItemId), deletedItemIds);
|
||||
Assert.DoesNotContain(new Guid(PlaceholderOwnedItemId), deletedItemIds);
|
||||
Assert.Equal(1L, await ScalarAsync<long>(dataSource, $"""SELECT count(*) FROM "BaseItems" WHERE "Id" = '{PlaceholderOwnedItemId}'""", cancellationToken));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
@@ -190,6 +283,18 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Creates the schema straight from the current model, without going through any migration.
|
||||
/// </summary>
|
||||
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<IReadOnlyList<string>> AppliedMigrationsAsync(NpgsqlDataSource dataSource, CancellationToken cancellationToken)
|
||||
{
|
||||
var context = CreateContext(dataSource);
|
||||
@@ -271,6 +376,41 @@ public sealed class PostgreSqlUpgradeTests : IAsyncLifetime
|
||||
""",
|
||||
cancellationToken);
|
||||
|
||||
private static Task<IReadOnlyList<string>> 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);
|
||||
|
||||
/// <summary>
|
||||
/// Adds one more item to a legacy database, with the owner and version ids still held as text.
|
||||
/// </summary>
|
||||
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}'";
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
|
||||
Reference in New Issue
Block a user