Project the lowered person credit values once when updating people

2 files changed, 222 insertions, 20 deletions

diff --git a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
index ef96952f67..05c8bffd66 100644
--- a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
+++ b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
@@ -117,13 +117,17 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
             person.Role = person.Role?.Trim() ?? string.Empty;
         }
 
+        // Project the values every comparison below needs once, so neither the case folding nor the
+        // enum formatting is repeated per candidate.
+        var credits = people.Select(e => (Person: e, LoweredName: e.Name.ToLowerInvariant(), PersonType: e.Type.ToString(), LoweredRole: e.Role.ToLowerInvariant()));
+
         // multiple metadata providers can provide the _same_ credit; dedupe case-insensitively.
         // The role is part of the key because one person can hold several credits of the same type
         // on an item, e.g. a Writer credited for both the Novel and the Screenplay.
-        people = people.DistinctBy(e => e.Name.ToLowerInvariant() + "-" + e.Type + "-" + e.Role.ToLowerInvariant()).ToArray();
+        var distinctCredits = credits.DistinctBy(e => (e.LoweredName, e.PersonType, e.LoweredRole)).ToArray();
 
-        var distinctPersons = people.DistinctBy(e => e.Name.ToLowerInvariant() + "-" + e.Type).ToArray();
-        var personKeys = distinctPersons.Select(e => e.Name.ToLowerInvariant() + "-" + e.Type).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();
         using var transaction = context.Database.BeginTransaction();
@@ -136,24 +140,44 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
             .Select(f => f.item)
             .ToArray();
 
+        var existingPersonKeys = existingPersons.Select(e => (e.Name.ToLowerInvariant(), e.PersonType ?? string.Empty)).ToHashSet();
+
         var toAdd = distinctPersons
-            .Where(e => !existingPersons.Any(f => string.Equals(f.Name, e.Name, StringComparison.OrdinalIgnoreCase) && f.PersonType == e.Type.ToString()))
-            .Select(Map)
+            .Where(e => !existingPersonKeys.Contains((e.LoweredName, e.PersonType)))
+            .Select(e => Map(e.Person))
             .ToArray();
         context.Peoples.AddRange(toAdd);
         context.SaveChanges();
 
-        var personsEntities = toAdd.Concat(existingPersons).ToArray();
+        // The Peoples table can hold case-only duplicates, so keep the first match per key just as
+        // the previous First() lookup did.
+        var personsEntities = new Dictionary<(string LoweredName, string PersonType), People>();
+        foreach (var entity in toAdd.Concat(existingPersons))
+        {
+            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)
+        {
+            existingMapsByCredit.TryAdd((map.People.Name.ToLowerInvariant(), map.People.PersonType ?? string.Empty, map.Role?.ToLowerInvariant() ?? string.Empty), map);
+        }
 
         var listOrder = 0;
 
-        foreach (var person in people)
+        foreach (var credit in distinctCredits)
         {
-            var entityPerson = personsEntities.First(e => string.Equals(e.Name, person.Name, StringComparison.OrdinalIgnoreCase) && e.PersonType == person.Type.ToString());
-            var existingMap = existingMaps.FirstOrDefault(e => string.Equals(e.People.Name, person.Name, StringComparison.OrdinalIgnoreCase) && e.People.PersonType == person.Type.ToString() && e.Role == person.Role);
-            if (existingMap is null)
+            var entityPerson = personsEntities[(credit.LoweredName, credit.PersonType)];
+            if (existingMapsByCredit.TryGetValue((credit.LoweredName, credit.PersonType, credit.LoweredRole), out var existingMap))
+            {
+                // Update the order for existing mappings
+                existingMap.ListOrder = listOrder;
+                existingMap.SortOrder = credit.Person.SortOrder;
+                // person mapping already exists so remove from list
+                existingMaps.Remove(existingMap);
+            }
+            else
             {
                 context.PeopleBaseItemMap.Add(new PeopleBaseItemMap()
                 {
@@ -162,18 +186,10 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
                     People = null!,
                     PeopleId = entityPerson.Id,
                     ListOrder = listOrder,
-                    SortOrder = person.SortOrder,
-                    Role = person.Role
+                    SortOrder = credit.Person.SortOrder,
+                    Role = credit.Person.Role
                 });
             }
-            else
-            {
-                // Update the order for existing mappings
-                existingMap.ListOrder = listOrder;
-                existingMap.SortOrder = person.SortOrder;
-                // person mapping already exists so remove from list
-                existingMaps.Remove(existingMap);
-            }
 
             listOrder++;
         }
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs
new file mode 100644
index 0000000000..70d8e1f833
--- /dev/null
+++ b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryUpdatePeopleTests.cs
@@ -0,0 +1,186 @@
+using System;
+using System.Linq;
+using Emby.Server.Implementations.Data;
+using Jellyfin.Data.Enums;
+using Jellyfin.Database.Implementations;
+using Jellyfin.Database.Implementations.Entities;
+using Jellyfin.Database.Implementations.Locking;
+using Jellyfin.Database.Providers.Sqlite;
+using Jellyfin.Server.Implementations.Item;
+using MediaBrowser.Controller.Entities;
+using MediaBrowser.Controller.Persistence;
+using Microsoft.Data.Sqlite;
+using Microsoft.EntityFrameworkCore;
+using Microsoft.Extensions.Logging.Abstractions;
+using Moq;
+using Xunit;
+using BaseItemKind = Jellyfin.Data.Enums.BaseItemKind;
+
+namespace Jellyfin.Server.Implementations.Tests.Item;
+
+public sealed class PeopleRepositoryUpdatePeopleTests : IDisposable
+{
+    private static readonly Guid _itemId = Guid.Parse("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa");
+
+    private readonly SqliteConnection _connection;
+    private readonly DbContextOptions<JellyfinDbContext> _dbOptions;
+    private readonly PeopleRepository _repository;
+
+    public PeopleRepositoryUpdatePeopleTests()
+    {
+        _connection = new SqliteConnection("Data Source=:memory:");
+        _connection.Open();
+
+        _dbOptions = new DbContextOptionsBuilder<JellyfinDbContext>()
+            .UseSqlite(_connection)
+            .Options;
+
+        var itemTypeLookup = new ItemTypeLookup();
+
+        using (var ctx = CreateDbContext())
+        {
+            ctx.Database.EnsureCreated();
+            ctx.BaseItems.Add(new BaseItemEntity
+            {
+                Id = _itemId,
+                Type = itemTypeLookup.BaseItemKindNames[BaseItemKind.Movie],
+                Name = "Movie",
+                MediaType = "Video",
+                IsMovie = true,
+                IsFolder = false,
+                IsVirtualItem = false
+            });
+            ctx.SaveChanges();
+        }
+
+        var factory = new Mock<IDbContextFactory<JellyfinDbContext>>();
+        factory.Setup(f => f.CreateDbContext()).Returns(CreateDbContext);
+
+        _repository = new PeopleRepository(
+            factory.Object,
+            itemTypeLookup,
+            new Mock<IItemQueryHelpers>().Object);
+    }
+
+    public void Dispose()
+    {
+        _connection.Dispose();
+    }
+
+    [Fact]
+    public void UpdatePeople_SamePersonAndTypeWithDifferentRoles_KeepsEveryCredit()
+    {
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Writer, "Novel"),
+            CreatePerson("Person A", PersonKind.Writer, "Screenplay")
+        ]);
+
+        using var ctx = CreateDbContext();
+        Assert.Single(ctx.Peoples);
+        Assert.Equal(
+            ["Novel", "Screenplay"],
+            ctx.PeopleBaseItemMap.OrderBy(e => e.ListOrder).Select(e => e.Role ?? string.Empty).ToArray());
+    }
+
+    [Fact]
+    public void UpdatePeople_CreditsDifferingOnlyInCase_AreDeduped()
+    {
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Actor, "Hero"),
+            CreatePerson("person a", PersonKind.Actor, "hero")
+        ]);
+
+        using var ctx = CreateDbContext();
+        Assert.Single(ctx.Peoples);
+        var map = Assert.Single(ctx.PeopleBaseItemMap);
+        Assert.Equal("Hero", map.Role);
+    }
+
+    [Fact]
+    public void UpdatePeople_SamePersonAsDifferentTypes_CreatesOnePersonPerType()
+    {
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Actor, "Hero"),
+            CreatePerson("Person A", PersonKind.Director, string.Empty)
+        ]);
+
+        using var ctx = CreateDbContext();
+        Assert.Equal(2, ctx.Peoples.Count());
+        Assert.Equal(2, ctx.PeopleBaseItemMap.Count());
+    }
+
+    [Fact]
+    public void UpdatePeople_RepeatedUpdate_ReusesMappingsAndRefreshesOrder()
+    {
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Actor, "Hero"),
+            CreatePerson("Person B", PersonKind.Actor, "Sidekick")
+        ]);
+
+        Guid[] peopleIdsBefore;
+        using (var ctx = CreateDbContext())
+        {
+            peopleIdsBefore = ctx.Peoples.Select(e => e.Id).OrderBy(e => e).ToArray();
+        }
+
+        // Reversed order, so the list order of both mappings has to be rewritten.
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person B", PersonKind.Actor, "Sidekick"),
+            CreatePerson("Person A", PersonKind.Actor, "Hero")
+        ]);
+
+        using var after = CreateDbContext();
+        Assert.Equal(peopleIdsBefore, after.Peoples.Select(e => e.Id).OrderBy(e => e).ToArray());
+        Assert.Equal(
+            ["Sidekick", "Hero"],
+            after.PeopleBaseItemMap.OrderBy(e => e.ListOrder).Select(e => e.Role ?? string.Empty).ToArray());
+    }
+
+    [Fact]
+    public void UpdatePeople_CreditRemoved_DropsOnlyThatMapping()
+    {
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Writer, "Novel"),
+            CreatePerson("Person A", PersonKind.Writer, "Screenplay")
+        ]);
+
+        _repository.UpdatePeople(_itemId, [
+            CreatePerson("Person A", PersonKind.Writer, "Novel")
+        ]);
+
+        using var ctx = CreateDbContext();
+        var map = Assert.Single(ctx.PeopleBaseItemMap);
+        Assert.Equal("Novel", map.Role);
+    }
+
+    [Fact]
+    public void UpdatePeople_RoleCaseChanged_KeepsExistingMapping()
+    {
+        _repository.UpdatePeople(_itemId, [CreatePerson("Person A", PersonKind.Actor, "Hero")]);
+
+        _repository.UpdatePeople(_itemId, [CreatePerson("Person A", PersonKind.Actor, "HERO")]);
+
+        using var ctx = CreateDbContext();
+        var map = Assert.Single(ctx.PeopleBaseItemMap);
+        Assert.Equal("Hero", map.Role);
+    }
+
+    private static PersonInfo CreatePerson(string name, PersonKind type, string role)
+    {
+        return new PersonInfo
+        {
+            Name = name,
+            Type = type,
+            Role = role
+        };
+    }
+
+    private JellyfinDbContext CreateDbContext()
+    {
+        return new JellyfinDbContext(
+            _dbOptions,
+            NullLogger<JellyfinDbContext>.Instance,
+            new SqliteDatabaseProvider(null!, NullLogger<SqliteDatabaseProvider>.Instance),
+            new NoLockBehavior(NullLogger<NoLockBehavior>.Instance));
+    }
+}