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

Avoid full people scans and writes for unchanged credits

Original-merge: 63c81a9975c339a0757142cb69a318b50b85d216

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

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

3 files changed, 192 insertions, 10 deletions

diff --git a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
index b1acb277be..fcddc09ad9 100644
--- a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
+++ b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
@@ -127,18 +127,61 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
         var distinctCredits = credits.DistinctBy(e => (e.LoweredName, e.PersonType, e.LoweredRole)).ToArray();
 
         var distinctPersons = distinctCredits.DistinctBy(e => (e.LoweredName, e.PersonType)).ToArray();
-        var personKeys = distinctPersons.Select(e => e.LoweredName + "-" + e.PersonType).ToArray();
 
         using var context = _dbProvider.CreateDbContext();
+        var existingMaps = context.PeopleBaseItemMap
+            .AsNoTracking()
+            .Include(e => e.People)
+            .Where(e => e.ItemId == itemId)
+            .ToList();
+
+        // Most library scans refresh unchanged local metadata. Avoid opening a write
+        // transaction when the item's people mappings, order and roles are unchanged.
+        var incomingCredits = distinctCredits
+            .Select((credit, index) => new
+            {
+                Key = (credit.LoweredName, credit.PersonType, credit.LoweredRole),
+                Role = credit.Person.Role,
+                ListOrder = index,
+                SortOrder = credit.Person.SortOrder
+            })
+            .ToDictionary(e => e.Key);
+        var mappingsAreUnchanged = existingMaps.Count == incomingCredits.Count
+            && existingMaps.All(map =>
+                incomingCredits.TryGetValue(
+                    (map.People.Name.ToLowerInvariant(), map.People.PersonType ?? string.Empty, map.Role?.ToLowerInvariant() ?? string.Empty),
+                    out var incoming)
+                && map.ListOrder == incoming.ListOrder
+                && map.SortOrder == incoming.SortOrder
+                && string.Equals(map.Role ?? string.Empty, incoming.Role, StringComparison.OrdinalIgnoreCase));
+
+        if (mappingsAreUnchanged)
+        {
+            return;
+        }
+
         using var transaction = context.Database.BeginTransaction();
-        var existingPersons = context.Peoples.Select(e => new
+        // The fast-path snapshot was read before acquiring the write transaction. Reload
+        // tracked mappings inside it so a concurrent refresh cannot leave stale credits.
+        existingMaps = context.PeopleBaseItemMap
+            .Include(e => e.People)
+            .Where(e => e.ItemId == itemId)
+            .ToList();
+
+        // Query each person type separately so SQLite can use IX_Peoples_NameLower.
+        // Combining the two fields into `lower(Name) || '-' || PersonType` forces a full
+        // scan of Peoples for every media item, which is prohibitive during a large import.
+        var existingPersons = new List<People>();
+        foreach (var personTypeGroup in distinctPersons.GroupBy(e => e.PersonType, StringComparer.Ordinal))
         {
-            item = e,
-            SelectionKey = e.Name.ToLower() + "-" + e.PersonType
-        })
-            .Where(p => personKeys.Contains(p.SelectionKey))
-            .Select(f => f.item)
-            .ToArray();
+            var names = personTypeGroup
+                .Select(e => e.LoweredName)
+                .ToArray();
+
+            existingPersons.AddRange(context.Peoples
+                .Where(e => e.PersonType == personTypeGroup.Key && names.Contains(e.Name.ToLower()))
+                .ToArray());
+        }
 
         var existingPersonKeys = existingPersons.Select(e => (e.Name.ToLowerInvariant(), e.PersonType ?? string.Empty)).ToHashSet();
 
@@ -157,7 +200,6 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
             personsEntities.TryAdd((entity.Name.ToLowerInvariant(), entity.PersonType ?? string.Empty), entity);
         }
 
-        var existingMaps = context.PeopleBaseItemMap.Include(e => e.People).Where(e => e.ItemId == itemId).ToList();
         var existingMapsByCredit = new Dictionary<(string LoweredName, string PersonType, string LoweredRole), PeopleBaseItemMap>();
         foreach (var map in existingMaps)
         {
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleUpdateQueryTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleUpdateQueryTests.cs
new file mode 100644
index 0000000000..b925f98197
--- /dev/null
+++ b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleUpdateQueryTests.cs
@@ -0,0 +1,138 @@
+using System;
+using System.Collections.Generic;
+using System.Data.Common;
+using System.Linq;
+using Emby.Server.Implementations.Data;
+using Jellyfin.Data.Enums;
+using Jellyfin.Database.Implementations.Entities;
+using Jellyfin.Server.Implementations.Item;
+using MediaBrowser.Controller.Entities;
+using MediaBrowser.Controller.Persistence;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Diagnostics;
+using Microsoft.EntityFrameworkCore.Migrations;
+using Microsoft.EntityFrameworkCore.Migrations.Operations;
+using Moq;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Item;
+
+public sealed class PeopleUpdateQueryTests : SqliteDbTestFixture
+{
+    private readonly CommandRecorder _recorder;
+    private readonly Guid _itemId = Guid.NewGuid();
+    private readonly PeopleRepository _people;
+
+    public PeopleUpdateQueryTests()
+        : this(new CommandRecorder())
+    {
+    }
+
+    private PeopleUpdateQueryTests(CommandRecorder recorder)
+        : base(recorder)
+    {
+        _recorder = recorder;
+        using var context = CreateDbContext();
+        context.BaseItems.Add(new BaseItemEntity
+        {
+            Id = _itemId,
+            Name = "Movie",
+            Type = new ItemTypeLookup().BaseItemKindNames[BaseItemKind.Movie]
+        });
+        context.SaveChanges();
+        _people = new PeopleRepository(CreateDbContextFactory(), new ItemTypeLookup(), Mock.Of<IItemQueryHelpers>());
+    }
+
+    [Theory]
+    [InlineData("Hero")]
+    [InlineData("HERO")]
+    public void UnchangedCredits_DoNotWriteOrLookUpAllPeople(string role)
+    {
+        _people.UpdatePeople(_itemId, [new PersonInfo { Name = "Actor", Type = PersonKind.Actor, Role = "Hero" }]);
+        _recorder.Commands.Clear();
+        _people.UpdatePeople(_itemId, [new PersonInfo { Name = "actor", Type = PersonKind.Actor, Role = role }]);
+        Assert.Single(_recorder.Commands);
+        Assert.StartsWith("SELECT", _recorder.Commands[0].Sql, StringComparison.Ordinal);
+        using var context = CreateDbContext();
+        Assert.Equal("Hero", Assert.Single(context.PeopleBaseItemMap).Role);
+    }
+
+    [Fact]
+    public void SortOrderChange_IsPersisted()
+    {
+        _people.UpdatePeople(_itemId, [new PersonInfo { Name = "Actor", Type = PersonKind.Actor, SortOrder = 1 }]);
+        _people.UpdatePeople(_itemId, [new PersonInfo { Name = "Actor", Type = PersonKind.Actor, SortOrder = 2 }]);
+        using var context = CreateDbContext();
+        Assert.Equal(2, Assert.Single(context.PeopleBaseItemMap).SortOrder);
+    }
+
+    [Fact]
+    public void UpdatePeople_GeneratedSqlUsesPeopleNameIndex()
+    {
+        ApplyMigration(new Jellyfin.Server.Implementations.Migrations.AddPeopleNameLowerIndex());
+        _recorder.Commands.Clear();
+        _people.UpdatePeople(_itemId, [
+            new PersonInfo { Name = "Actor A", Type = PersonKind.Actor },
+            new PersonInfo { Name = "Actor B", Type = PersonKind.Actor }
+        ]);
+        var query = Assert.Single(_recorder.Commands, c => c.Sql.Contains("lower(\"p\".\"Name\")", StringComparison.Ordinal));
+        Assert.Contains(Explain(query), line => line.Contains("SEARCH p USING INDEX IX_Peoples_NameLower", StringComparison.Ordinal));
+    }
+
+    private void ApplyMigration(Migration migration)
+    {
+        using var context = CreateDbContext();
+        foreach (var operation in migration.UpOperations.Cast<SqlOperation>())
+        {
+            context.Database.ExecuteSqlRaw(operation.Sql);
+        }
+    }
+
+    private string[] Explain(RecordedCommand query)
+    {
+        using var context = CreateDbContext();
+        using var command = context.Database.GetDbConnection().CreateCommand();
+#pragma warning disable CA2100 // query.Sql is generated by EF Core; query values remain bound parameters.
+        command.CommandText = "EXPLAIN QUERY PLAN " + query.Sql;
+#pragma warning restore CA2100
+        foreach (var value in query.Parameters)
+        {
+            var parameter = command.CreateParameter();
+            parameter.ParameterName = value.Name;
+            parameter.Value = value.Value;
+            command.Parameters.Add(parameter);
+        }
+
+        using var reader = command.ExecuteReader();
+        var plan = new List<string>();
+        while (reader.Read())
+        {
+            plan.Add(reader.GetString(3));
+        }
+
+        return plan.ToArray();
+    }
+
+    private sealed record RecordedCommand(string Sql, (string Name, object? Value)[] Parameters);
+
+    private sealed class CommandRecorder : DbCommandInterceptor
+    {
+        public List<RecordedCommand> Commands { get; } = [];
+
+        public override InterceptionResult<DbDataReader> ReaderExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<DbDataReader> result)
+        {
+            Record(command);
+            return result;
+        }
+
+        public override InterceptionResult<int> NonQueryExecuting(DbCommand command, CommandEventData eventData, InterceptionResult<int> result)
+        {
+            Record(command);
+            return result;
+        }
+
+        private void Record(DbCommand command) => Commands.Add(new RecordedCommand(
+            command.CommandText,
+            command.Parameters.Cast<DbParameter>().Select(p => (p.ParameterName, p.Value)).ToArray()));
+    }
+}
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs
index cfc9c9496c..6da176b4f1 100644
--- a/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs
+++ b/tests/Jellyfin.Server.Implementations.Tests/Item/SqliteDbTestFixture.cs
@@ -11,6 +11,7 @@ using MediaBrowser.Controller.Configuration;
 using MediaBrowser.Model.Configuration;
 using Microsoft.Data.Sqlite;
 using Microsoft.EntityFrameworkCore;
+using Microsoft.EntityFrameworkCore.Diagnostics;
 using Microsoft.Extensions.Logging.Abstractions;
 using Moq;
 
@@ -26,7 +27,7 @@ public abstract class SqliteDbTestFixture : IDisposable
     private readonly SqliteConnection _connection;
     private readonly DbContextOptions<JellyfinDbContext> _dbOptions;
 
-    protected SqliteDbTestFixture()
+    protected SqliteDbTestFixture(params IInterceptor[] interceptors)
     {
         ApplicationPaths = new Mock<IApplicationPaths>().Object;
 
@@ -35,6 +36,7 @@ public abstract class SqliteDbTestFixture : IDisposable
 
         _dbOptions = new DbContextOptionsBuilder<JellyfinDbContext>()
             .UseSqlite(_connection)
+            .AddInterceptors(interceptors)
             .Options;
 
         using var context = CreateDbContext();