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

Stop rebuilding the inherited-tag set per row, and count only the people /Persons can return

Original-merge: 63b5a050a9dc943854bae01856f30f1a5a089245

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

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

7 files changed, 299 insertions, 4 deletions

diff --git a/Emby.Server.Implementations/Library/LibraryManager.cs b/Emby.Server.Implementations/Library/LibraryManager.cs
index 6f3df084c0..2d62fdab48 100644
--- a/Emby.Server.Implementations/Library/LibraryManager.cs
+++ b/Emby.Server.Implementations/Library/LibraryManager.cs
@@ -3657,6 +3657,11 @@ namespace Emby.Server.Implementations.Library
 
         public QueryResult<BaseItem> GetPeopleItems(InternalPeopleQuery query)
         {
+            ArgumentNullException.ThrowIfNull(query);
+
+            // This hands back by-name items, so the people without one are not ours to report.
+            query.MustHaveItem = true;
+
             var queryResult = _peopleRepository.GetPeople(query);
             var baseItems = queryResult.Items.Select(i =>
                 {
diff --git a/Jellyfin.Server.Implementations/Item/BaseItemRepository.QueryBuilding.cs b/Jellyfin.Server.Implementations/Item/BaseItemRepository.QueryBuilding.cs
index 1a4c9da41c..4cd02bcd8d 100644
--- a/Jellyfin.Server.Implementations/Item/BaseItemRepository.QueryBuilding.cs
+++ b/Jellyfin.Server.Implementations/Item/BaseItemRepository.QueryBuilding.cs
@@ -612,11 +612,12 @@ public sealed partial class BaseItemRepository
             var blockedTagItemIds = context.ItemValuesMap
                 .Where(f => f.ItemValue.Type == ItemValueType.Tags && excludedTags.Contains(f.ItemValue.CleanValue))
                 .Select(f => f.ItemId);
+            var blockedByAncestor = ItemsBelowTaggedAncestor(context, blockedTagItemIds);
 
             baseQuery = baseQuery.Where(e =>
                 !blockedTagItemIds.Contains(e.Id)
                 && !(e.SeriesId.HasValue && blockedTagItemIds.Contains(e.SeriesId.Value))
-                && !e.Parents!.Any(p => blockedTagItemIds.Contains(p.ParentItemId))
+                && !blockedByAncestor.Contains(e.Id)
                 && !(e.TopParentId.HasValue && blockedTagItemIds.Contains(e.TopParentId.Value)));
         }
 
@@ -629,10 +630,12 @@ public sealed partial class BaseItemRepository
                 .Where(f => f.ItemValue.Type == ItemValueType.Tags && includeTags.Contains(f.ItemValue.CleanValue))
                 .Select(f => f.ItemId);
 
+            var allowedByAncestor = ItemsBelowTaggedAncestor(context, allowedTagItemIds);
+
             baseQuery = baseQuery.Where(e =>
                 allowedTagItemIds.Contains(e.Id)
                 || (e.SeriesId.HasValue && allowedTagItemIds.Contains(e.SeriesId.Value))
-                || e.Parents!.Any(p => allowedTagItemIds.Contains(p.ParentItemId))
+                || allowedByAncestor.Contains(e.Id)
                 || (e.TopParentId.HasValue && allowedTagItemIds.Contains(e.TopParentId.Value))
 
                 // People don't carry the tags of the media they appear in and would never match
@@ -643,6 +646,17 @@ public sealed partial class BaseItemRepository
     }
 
     /// <summary>
+    /// Reads back the items that carry one of the tagged items as an ancestor.
+    /// </summary>
+    /// <param name="context">The database context.</param>
+    /// <param name="taggedItemIds">The ids of the items carrying the tag.</param>
+    /// <returns>The ids of the items below one of them.</returns>
+    private static IQueryable<Guid> ItemsBelowTaggedAncestor(JellyfinDbContext context, IQueryable<Guid> taggedItemIds)
+        => context.AncestorIds
+            .Where(a => taggedItemIds.Contains(a.ParentItemId))
+            .Select(a => a.ItemId);
+
+    /// <summary>
     /// Builds a filter expression for max parental rating that handles both rated items
     /// and unrated BoxSets/Playlists (which check linked children's ratings).
     /// </summary>
diff --git a/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs b/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs
index a7110485b4..e66f18bc6e 100644
--- a/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs
+++ b/Jellyfin.Server.Implementations/Item/BaseItemRepository.TranslateQuery.cs
@@ -1132,11 +1132,12 @@ public sealed partial class BaseItemRepository
             var blockedTagItemIds = context.ItemValuesMap
                 .Where(f => f.ItemValue.Type == ItemValueType.Tags && excludedTags.Contains(f.ItemValue.CleanValue))
                 .Select(f => f.ItemId);
+            var blockedByAncestor = ItemsBelowTaggedAncestor(context, blockedTagItemIds);
 
             baseQuery = baseQuery.Where(e =>
                 !blockedTagItemIds.Contains(e.Id)
                 && !(e.SeriesId.HasValue && blockedTagItemIds.Contains(e.SeriesId.Value))
-                && !e.Parents!.Any(p => blockedTagItemIds.Contains(p.ParentItemId))
+                && !blockedByAncestor.Contains(e.Id)
                 && !(e.TopParentId.HasValue && blockedTagItemIds.Contains(e.TopParentId.Value)));
         }
 
@@ -1148,11 +1149,12 @@ public sealed partial class BaseItemRepository
             var allowedTagItemIds = context.ItemValuesMap
                 .Where(f => f.ItemValue.Type == ItemValueType.Tags && includeTags.Contains(f.ItemValue.CleanValue))
                 .Select(f => f.ItemId);
+            var allowedByAncestor = ItemsBelowTaggedAncestor(context, allowedTagItemIds);
 
             baseQuery = baseQuery.Where(e =>
                 allowedTagItemIds.Contains(e.Id)
                 || (e.SeriesId.HasValue && allowedTagItemIds.Contains(e.SeriesId.Value))
-                || e.Parents!.Any(p => allowedTagItemIds.Contains(p.ParentItemId))
+                || allowedByAncestor.Contains(e.Id)
                 || (e.TopParentId.HasValue && allowedTagItemIds.Contains(e.TopParentId.Value))
 
                 // People don't carry the tags of the media they appear in and would never match
diff --git a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
index fcddc09ad9..bc79699486 100644
--- a/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
+++ b/Jellyfin.Server.Implementations/Item/PeopleRepository.cs
@@ -423,6 +423,13 @@ public class PeopleRepository(IDbContextFactory<JellyfinDbContext> dbProvider, I
                 .Any(m => m.PeopleId == e.Id && accessibleItems.Any(i => i.Id == m.ItemId)));
         }
 
+        if (filter.MustHaveItem)
+        {
+            // A credit with no by-name item behind it cannot be handed back as one.
+            var personType = itemTypeLookup.BaseItemKindNames[BaseItemKind.Person];
+            query = query.Where(e => context.BaseItems.Any(b => b.Type == personType && b.Name == e.Name));
+        }
+
         if (!filter.ItemId.IsEmpty())
         {
             var itemId = filter.ItemId;
diff --git a/MediaBrowser.Controller/Entities/InternalPeopleQuery.cs b/MediaBrowser.Controller/Entities/InternalPeopleQuery.cs
index 8d2a959f4d..50f749367f 100644
--- a/MediaBrowser.Controller/Entities/InternalPeopleQuery.cs
+++ b/MediaBrowser.Controller/Entities/InternalPeopleQuery.cs
@@ -65,5 +65,12 @@ namespace MediaBrowser.Controller.Entities
         /// people must satisfy through at least one of the items they are credited on.
         /// </summary>
         public InternalItemsQuery AccessFilter { get; set; }
+
+        /// <summary>
+        /// Gets or sets a value indicating whether to keep only people that have a by-name item behind
+        /// them. Set by callers that hand back items rather than credits, so the count they report
+        /// describes the same people they can return.
+        /// </summary>
+        public bool MustHaveItem { get; set; }
     }
 }
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/BaseItemRepositoryInheritedTagTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/BaseItemRepositoryInheritedTagTests.cs
new file mode 100644
index 0000000000..d995293923
--- /dev/null
+++ b/tests/Jellyfin.Server.Implementations.Tests/Item/BaseItemRepositoryInheritedTagTests.cs
@@ -0,0 +1,175 @@
+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.Extensions;
+using Jellyfin.Server.Implementations.Item;
+using MediaBrowser.Controller.Entities;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Item;
+
+/// <summary>
+/// Covers the inherited-tag filters a user's blocked and allowed tags turn into. A tag reaches an item
+/// four ways - on the item, on its series, on an ancestor and on its library - and each of them has to
+/// keep answering the same after the ancestor check moved off a per-row correlated subquery.
+/// </summary>
+public sealed class BaseItemRepositoryInheritedTagTests : SqliteDbTestFixture
+{
+    private const string FolderType = "MediaBrowser.Controller.Entities.Folder";
+    private const string SeriesType = "MediaBrowser.Controller.Entities.TV.Series";
+    private const string SeasonType = "MediaBrowser.Controller.Entities.TV.Season";
+    private const string EpisodeType = "MediaBrowser.Controller.Entities.TV.Episode";
+
+    private const string Tag = "Adult";
+
+    private readonly BaseItemRepository _repository;
+
+    private readonly Guid _library = Guid.NewGuid();
+    private readonly Guid _otherLibrary = Guid.NewGuid();
+
+    // Tagged on the item itself.
+    private readonly Guid _taggedSeries = Guid.NewGuid();
+
+    // Inherits the tag from the series it belongs to, without an ancestor row for it.
+    private readonly Guid _episodeOfTaggedSeries = Guid.NewGuid();
+
+    // Inherits the tag from a season in the middle of its ancestor chain.
+    private readonly Guid _taggedSeason = Guid.NewGuid();
+    private readonly Guid _episodeUnderTaggedSeason = Guid.NewGuid();
+
+    // Inherits the tag from the library above it.
+    private readonly Guid _taggedLibrarySeries = Guid.NewGuid();
+
+    // Carries the tag nowhere, the control the assertions are read against.
+    private readonly Guid _untaggedSeries = Guid.NewGuid();
+    private readonly Guid _untaggedEpisode = Guid.NewGuid();
+
+    public BaseItemRepositoryInheritedTagTests()
+    {
+        using (var ctx = CreateDbContext())
+        {
+            Seed(ctx);
+        }
+
+        _repository = CreateBaseItemRepository(new ItemTypeLookup());
+    }
+
+    [Fact]
+    public void ExcludeInheritedTags_DropsEveryItemTheTagReaches()
+    {
+        var ids = _repository.GetItemIdsList(new InternalItemsQuery { ExcludeInheritedTags = [Tag] }).ToHashSet();
+
+        // The blocked library carries the tag itself, so it goes with everything under it.
+        Assert.Equal(
+            new[] { _library, _untaggedSeries, _untaggedEpisode }.Order(),
+            ids.Order());
+    }
+
+    [Fact]
+    public void IncludeInheritedTags_KeepsExactlyTheItemsTheTagReaches()
+    {
+        var ids = _repository.GetItemIdsList(new InternalItemsQuery { IncludeInheritedTags = [Tag] }).ToHashSet();
+
+        Assert.Equal(
+            new[] { _otherLibrary, _taggedSeries, _episodeOfTaggedSeries, _taggedSeason, _episodeUnderTaggedSeason, _taggedLibrarySeries }.Order(),
+            ids.Order());
+    }
+
+    [Fact]
+    public void ExcludeInheritedTags_DropsAnItemReachedOnlyThroughAnAncestor()
+    {
+        var ids = _repository.GetItemIdsList(new InternalItemsQuery
+        {
+            IncludeItemTypes = [BaseItemKind.Episode],
+            ExcludeInheritedTags = [Tag]
+        });
+
+        Assert.Equal([_untaggedEpisode], ids);
+    }
+
+    [Fact]
+    public void ExcludeInheritedTags_WithAnUnusedTag_KeepsEverything()
+    {
+        var ids = _repository.GetItemIdsList(new InternalItemsQuery { ExcludeInheritedTags = ["Unused"] });
+
+        Assert.Equal(9, ids.Count);
+    }
+
+    private void Seed(JellyfinDbContext context)
+    {
+        AddItem(context, _library, FolderType, "Shows", true);
+        AddItem(context, _otherLibrary, FolderType, "Blocked library", true);
+        AddItem(context, _taggedSeries, SeriesType, "Tagged series", true);
+        AddItem(context, _episodeOfTaggedSeries, EpisodeType, "Episode of tagged series", false, _taggedSeries);
+        AddItem(context, _taggedSeason, SeasonType, "Tagged season", true);
+        AddItem(context, _episodeUnderTaggedSeason, EpisodeType, "Episode under tagged season", false);
+        AddItem(context, _taggedLibrarySeries, SeriesType, "Series in blocked library", true);
+        AddItem(context, _untaggedSeries, SeriesType, "Untagged series", true);
+        AddItem(context, _untaggedEpisode, EpisodeType, "Untagged episode", false, _untaggedSeries);
+
+        // AncestorIds is a closure: production writes one row per ancestor, not just the parent. The
+        // episode of the tagged series deliberately has none, so the series branch is what has to catch it.
+        AddAncestors(context, _taggedSeries, _library);
+        AddAncestors(context, _taggedSeason, _library);
+        AddAncestors(context, _episodeUnderTaggedSeason, _taggedSeason, _library);
+        AddAncestors(context, _taggedLibrarySeries, _otherLibrary);
+        AddAncestors(context, _untaggedSeries, _library);
+        AddAncestors(context, _untaggedEpisode, _untaggedSeries, _library);
+
+        Tagged(context, _taggedSeries, _taggedSeason, _otherLibrary);
+
+        context.SaveChanges();
+    }
+
+    private void AddItem(JellyfinDbContext context, Guid id, string type, string name, bool isFolder, Guid? seriesId = null)
+    {
+        context.BaseItems.Add(new BaseItemEntity
+        {
+            Id = id,
+            Type = type,
+            Name = name,
+            IsFolder = isFolder,
+            SeriesId = seriesId
+        });
+    }
+
+    private void AddAncestors(JellyfinDbContext context, Guid itemId, params Guid[] ancestorIds)
+    {
+        foreach (var ancestorId in ancestorIds)
+        {
+            context.AncestorIds.Add(new AncestorId
+            {
+                ItemId = itemId,
+                ParentItemId = ancestorId,
+                Item = null!,
+                ParentItem = null!
+            });
+        }
+    }
+
+    private void Tagged(JellyfinDbContext context, params Guid[] itemIds)
+    {
+        var itemValue = new ItemValue
+        {
+            ItemValueId = Guid.NewGuid(),
+            Type = ItemValueType.Tags,
+            Value = Tag,
+            CleanValue = Tag.GetCleanValue()
+        };
+
+        context.ItemValues.Add(itemValue);
+        foreach (var itemId in itemIds)
+        {
+            context.ItemValuesMap.Add(new ItemValueMap
+            {
+                ItemId = itemId,
+                ItemValueId = itemValue.ItemValueId,
+                Item = null!,
+                ItemValue = null!
+            });
+        }
+    }
+}
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryMustHaveItemTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryMustHaveItemTests.cs
new file mode 100644
index 0000000000..f143ce1e20
--- /dev/null
+++ b/tests/Jellyfin.Server.Implementations.Tests/Item/PeopleRepositoryMustHaveItemTests.cs
@@ -0,0 +1,85 @@
+using System;
+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 Moq;
+using Xunit;
+
+namespace Jellyfin.Server.Implementations.Tests.Item;
+
+/// <summary>
+/// Covers <see cref="InternalPeopleQuery.MustHaveItem"/>. A caller that hands back by-name items can
+/// only return the people that have one, so the count it reports has to leave out the rest rather than
+/// describe a larger set than it can page through.
+/// </summary>
+public sealed class PeopleRepositoryMustHaveItemTests : SqliteDbTestFixture
+{
+    private readonly PeopleRepository _people;
+    private readonly Guid _movie = Guid.NewGuid();
+
+    public PeopleRepositoryMustHaveItemTests()
+    {
+        var lookup = new ItemTypeLookup();
+        using (var context = CreateDbContext())
+        {
+            context.BaseItems.Add(new BaseItemEntity
+            {
+                Id = _movie,
+                Name = "Movie",
+                Type = lookup.BaseItemKindNames[BaseItemKind.Movie]
+            });
+
+            // Only one of the two credits has a by-name item behind it.
+            context.BaseItems.Add(new BaseItemEntity
+            {
+                Id = Guid.NewGuid(),
+                Name = "With Item",
+                Type = lookup.BaseItemKindNames[BaseItemKind.Person]
+            });
+
+            context.SaveChanges();
+        }
+
+        _people = new PeopleRepository(CreateDbContextFactory(), lookup, Mock.Of<IItemQueryHelpers>());
+        _people.UpdatePeople(_movie, [
+            new PersonInfo { Name = "With Item", Type = PersonKind.Actor },
+            new PersonInfo { Name = "Without Item", Type = PersonKind.Actor }
+        ]);
+    }
+
+    [Fact]
+    public void WithoutMustHaveItem_ReturnsAndCountsEveryCredit()
+    {
+        var result = _people.GetPeople(new InternalPeopleQuery());
+
+        Assert.Equal(2, result.TotalRecordCount);
+        Assert.Equal(2, result.Items.Count);
+    }
+
+    [Fact]
+    public void MustHaveItem_DropsTheCreditWithoutAnItem()
+    {
+        var result = _people.GetPeople(new InternalPeopleQuery { MustHaveItem = true });
+
+        Assert.Equal("With Item", Assert.Single(result.Items).Name);
+    }
+
+    [Fact]
+    public void MustHaveItem_CountsOnlyWhatItCanReturn()
+    {
+        var result = _people.GetPeople(new InternalPeopleQuery { MustHaveItem = true });
+
+        Assert.Equal(1, result.TotalRecordCount);
+    }
+
+    [Fact]
+    public void MustHaveItem_CountAgreesWithThePageWhenLimited()
+    {
+        var result = _people.GetPeople(new InternalPeopleQuery { MustHaveItem = true, Limit = 10 });
+
+        Assert.Equal(result.Items.Count, result.TotalRecordCount);
+    }
+}