Backport pull request #17873 from jellyfin/release-12.z

Delete the full ownership closure when deleting items

Original-merge: 705b69e93d

Merged-by: crobibero <cody@robibe.ro>

Backported-by: Cody Robibero <cody@robibe.ro>

Jellyfin.Server.Implementations/Item/ItemPersistenceService.cs

@@ -69,16 +69,24 @@ public class ItemPersistenceService : IItemPersistenceService
         // Use WhereOneOrMany instead of a raw HashSet.Contains so large id sets are bound as a
         // single parameter (json_each) rather than one SQL variable per id, which would otherwise
         // overflow SQLite's variable limit when deleting many items at once (e.g. migrations).
-        var ownerIds = descendantIds.ToArray();
-        var extraIds = context.BaseItems
-            .Where(e => e.OwnerId.HasValue)
-            .WhereOneOrMany(ownerIds, e => e.OwnerId!.Value)
-            .Select(e => e.Id)
-            .ToArray();
-
-        foreach (var extraId in extraIds)
+        var frontier = descendantIds.ToArray();
+        while (frontier.Length > 0)
         {
-            descendantIds.Add(extraId);
+            var ownedIds = context.BaseItems
+                .Where(e => e.OwnerId.HasValue)
+                .WhereOneOrMany(frontier, e => e.OwnerId!.Value)
+                .Select(e => e.Id)
+                .ToArray();
+
+            var childIds = context.BaseItems
+                .Where(e => e.ParentId.HasValue)
+                .WhereOneOrMany(frontier, e => e.ParentId!.Value)
+                .Select(e => e.Id)
+                .ToArray();
+
+            // Only ids that were not already known become the next frontier, so ownership cycles
+            // terminate instead of looping forever.
+            frontier = [.. ownedIds.Concat(childIds).Where(e => descendantIds.Add(e))];
         }
 
         var relatedItems = descendantIds.ToArray();

tests/Jellyfin.Server.Implementations.Tests/Item/ItemPersistenceDeleteItemTests.cs

@@ -0,0 +1,103 @@
+using System;
+using System.Linq;
+using Jellyfin.Database.Implementations.Entities;
+using Jellyfin.Server.Implementations.Item;
+using MediaBrowser.Controller;
+using Microsoft.Extensions.Logging.Abstractions;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Item;
+
+/// <summary>
+/// DeleteItem has to hand SQLite one statement that already contains everything the foreign keys
+/// on BaseItems require, because FK_BaseItems_BaseItems_OwnerId is NO ACTION: anything left behind
+/// pointing at a deleted row fails the whole delete with SQLite error 19.
+/// </summary>
+public sealed class ItemPersistenceDeleteItemTests : SqliteDbTestFixture
+{
+    private static readonly Guid _owner = Guid.Parse("aaaaaaaa-0000-0000-0000-000000000001");
+    private static readonly Guid _extra = Guid.Parse("eeeeeeee-0000-0000-0000-000000000001");
+    private static readonly Guid _extraOfExtra = Guid.Parse("eeeeeeee-0000-0000-0000-000000000002");
+    private static readonly Guid _child = Guid.Parse("cccccccc-0000-0000-0000-000000000001");
+    private static readonly Guid _extraOfChild = Guid.Parse("eeeeeeee-0000-0000-0000-000000000003");
+
+    private readonly ItemPersistenceService _service;
+
+    public ItemPersistenceDeleteItemTests()
+    {
+        _service = new ItemPersistenceService(
+            CreateDbContextFactory(),
+            new Mock<IServerApplicationHost>().Object,
+            NullLogger<ItemPersistenceService>.Instance);
+    }
+
+    [Fact]
+    public void DeleteItem_OwnerIdChain_DeletesWholeChain()
+    {
+        // An extra that owns an extra of its own. Real libraries carry these in bulk, and a single
+        // expansion pass over OwnerId leaves the second level behind.
+        Seed(
+            (_owner, null, null),
+            (_extra, _owner, null),
+            (_extraOfExtra, _extra, null));
+
+        _service.DeleteItem([_owner]);
+
+        using var context = CreateDbContext();
+        Assert.Empty(context.BaseItems.Where(e => e.Id.Equals(_owner) || e.Id.Equals(_extra) || e.Id.Equals(_extraOfExtra)));
+    }
+
+    [Fact]
+    public void DeleteItem_ExtraOwnedByCascadedChild_DeletesExtraToo()
+    {
+        // The child goes away through FK_BaseItems_BaseItems_ParentId's ON DELETE CASCADE whether or
+        // not it is listed, so an extra owned by that child has to be listed with it.
+        Seed(
+            (_owner, null, null),
+            (_child, null, _owner),
+            (_extraOfChild, _child, null));
+
+        _service.DeleteItem([_owner]);
+
+        using var context = CreateDbContext();
+        Assert.Empty(context.BaseItems.Where(e => e.Id.Equals(_owner) || e.Id.Equals(_child) || e.Id.Equals(_extraOfChild)));
+    }
+
+    [Fact]
+    public void DeleteItem_OwnershipCycle_Terminates()
+    {
+        // A malformed pair that owns each other must not spin the closure loop forever.
+        Seed((_owner, null, null), (_extra, _owner, null));
+
+        using (var context = CreateDbContext())
+        {
+            context.BaseItems.Single(e => e.Id.Equals(_owner)).OwnerId = _extra;
+            context.SaveChanges();
+        }
+
+        _service.DeleteItem([_owner]);
+
+        using var assertContext = CreateDbContext();
+        Assert.Empty(assertContext.BaseItems.Where(e => e.Id.Equals(_owner) || e.Id.Equals(_extra)));
+    }
+
+    private void Seed(params (Guid Id, Guid? OwnerId, Guid? ParentId)[] items)
+    {
+        using var context = CreateDbContext();
+
+        // Owners before the rows referencing them: the seed itself is foreign key checked.
+        foreach (var (id, ownerId, parentId) in items)
+        {
+            context.BaseItems.Add(new BaseItemEntity
+            {
+                Id = id,
+                Type = "MediaBrowser.Controller.Entities.Video",
+                OwnerId = ownerId,
+                ParentId = parentId
+            });
+
+            context.SaveChanges();
+        }
+    }
+}