Apply review suggestions

3 files changed, 90 insertions, 70 deletions

diff --git a/Emby.Server.Implementations/Library/Search/SearchManager.cs b/Emby.Server.Implementations/Library/Search/SearchManager.cs
index af916ec9a7..39fff42d9b 100644
--- a/Emby.Server.Implementations/Library/Search/SearchManager.cs
+++ b/Emby.Server.Implementations/Library/Search/SearchManager.cs
@@ -85,19 +85,20 @@ public class SearchManager : ISearchManager
         var searchTerm = query.SearchTerm.Trim().RemoveDiacritics();
 
         var results = await CollectFromProvidersAsync(_externalProviders, query, searchTerm, cancellationToken).ConfigureAwait(false);
+        var fromExternal = results.Count > 0;
         if (results.Count == 0 && _internalProviders.Length > 0)
         {
             _logger.LogDebug("No results from external providers, falling back to internal providers");
             results = await CollectFromProvidersAsync(_internalProviders, query, searchTerm, cancellationToken).ConfigureAwait(false);
         }
 
-        // External providers don't know about user permissions, so they may return IDs from
-        // hidden libraries or items the user is otherwise blocked from. Filter the candidate
-        // set to only items this user can access (top-parent libraries, parental rating,
-        // blocked/allowed tags, owned-item rules) before returning. The Items controller's
-        // second roundtrip via folder.GetItems applies most of these again, but it does not
-        // restrict by TopParentIds when ItemIds is set, leaving a gap that this closes.
-        if (results.Count > 0 && query.UserId.HasValue && !query.UserId.Value.IsEmpty())
+        // Internal providers apply user-access filtering inline in their queries. External
+        // providers don't know about user permissions, so they may return IDs from hidden
+        // libraries or items the user is otherwise blocked from. Run the post-filter only
+        // when results came from externals to close that gap. The Items controller's second
+        // roundtrip via folder.GetItems applies most of these again, but it does not restrict
+        // by TopParentIds when ItemIds is set.
+        if (fromExternal && results.Count > 0 && query.UserId.HasValue && !query.UserId.Value.IsEmpty())
         {
             var user = _userManager.GetUserById(query.UserId.Value);
             if (user is not null)
@@ -120,31 +121,28 @@ public class SearchManager : ISearchManager
         var accessFilter = new InternalItemsQuery(user);
         _libraryManager.ConfigureUserAccess(accessFilter, user);
 
-        var candidateIds = new Guid[candidates.Count];
-        for (var i = 0; i < candidates.Count; i++)
-        {
-            candidateIds[i] = candidates[i].ItemId;
-        }
+        Guid[] candidateIds = [.. candidates.Select(c => c.ItemId)];
 
         var dbContext = await _dbProvider.CreateDbContextAsync(cancellationToken).ConfigureAwait(false);
         await using (dbContext.ConfigureAwait(false))
         {
             var baseQuery = dbContext.BaseItems
                 .AsNoTracking()
-                .Where(e => candidateIds.Contains(e.Id));
+                .WhereOneOrMany(candidateIds, e => e.Id);
 
             baseQuery = _queryHelpers.ApplyAccessFiltering(dbContext, baseQuery, accessFilter);
 
+            var allowedCount = await baseQuery.CountAsync(cancellationToken).ConfigureAwait(false);
+            if (allowedCount == candidates.Count)
+            {
+                return candidates;
+            }
+
             var allowedIds = await baseQuery
                 .Select(e => e.Id)
                 .ToHashSetAsync(cancellationToken)
                 .ConfigureAwait(false);
 
-            if (allowedIds.Count == candidates.Count)
-            {
-                return candidates;
-            }
-
             var filtered = candidates.Where(c => allowedIds.Contains(c.ItemId)).ToList();
             if (filtered.Count < candidates.Count)
             {
@@ -253,17 +251,24 @@ public class SearchManager : ISearchManager
         string searchTerm,
         CancellationToken cancellationToken)
     {
-        var bestScores = new Dictionary<Guid, float>();
         var requestedLimit = providerQuery.Limit ?? 100;
+        var applicable = providers.Where(p => p.CanSearch(providerQuery)).ToArray();
+        if (applicable.Length == 0)
+        {
+            return [];
+        }
+
+        var perProvider = await Task.WhenAll(
+            applicable.Select(p => CollectFromProviderAsync(p, providerQuery, searchTerm, requestedLimit, cancellationToken)))
+            .ConfigureAwait(false);
 
-        foreach (var provider in providers.Where(p => p.CanSearch(providerQuery)))
+        var bestScores = new Dictionary<Guid, float>();
+        foreach (var providerResults in perProvider)
         {
-            if (bestScores.Count >= requestedLimit || cancellationToken.IsCancellationRequested)
+            foreach (var result in providerResults)
             {
-                break;
+                UpdateBestScore(bestScores, result);
             }
-
-            await CollectFromProviderAsync(provider, providerQuery, searchTerm, bestScores, requestedLimit, cancellationToken).ConfigureAwait(false);
         }
 
         return bestScores
@@ -273,66 +278,50 @@ public class SearchManager : ISearchManager
             .ToList();
     }
 
-    private async Task CollectFromProviderAsync(
+    private async Task<IReadOnlyList<SearchResult>> CollectFromProviderAsync(
         ISearchProvider provider,
         SearchProviderQuery providerQuery,
         string searchTerm,
-        Dictionary<Guid, float> bestScores,
         int requestedLimit,
         CancellationToken cancellationToken)
     {
         try
         {
-            var count = provider is IExternalSearchProvider externalProvider
-                ? await CollectFromExternalProviderAsync(externalProvider, providerQuery, bestScores, requestedLimit, cancellationToken).ConfigureAwait(false)
-                : await CollectFromInternalProviderAsync(provider, providerQuery, bestScores, cancellationToken).ConfigureAwait(false);
+            var results = provider is IExternalSearchProvider externalProvider
+                ? await CollectFromExternalProviderAsync(externalProvider, providerQuery, requestedLimit, cancellationToken).ConfigureAwait(false)
+                : await provider.SearchAsync(providerQuery, cancellationToken).ConfigureAwait(false);
 
             _logger.LogDebug(
                 "Provider {Provider} returned {Count} candidates for search term '{SearchTerm}'",
                 provider.Name,
-                count,
+                results.Count,
                 searchTerm);
+            return results;
         }
         catch (Exception ex)
         {
             _logger.LogWarning(ex, "Search provider {Provider} failed for term '{SearchTerm}'", provider.Name, searchTerm);
+            return [];
         }
     }
 
-    private static async Task<int> CollectFromExternalProviderAsync(
+    private static async Task<IReadOnlyList<SearchResult>> CollectFromExternalProviderAsync(
         IExternalSearchProvider provider,
         SearchProviderQuery providerQuery,
-        Dictionary<Guid, float> bestScores,
         int requestedLimit,
         CancellationToken cancellationToken)
     {
-        var count = 0;
+        var results = new List<SearchResult>();
         await foreach (var result in provider.SearchAsync(providerQuery, cancellationToken).ConfigureAwait(false))
         {
-            UpdateBestScore(bestScores, result);
-            count++;
-            if (bestScores.Count >= requestedLimit)
+            results.Add(result);
+            if (results.Count >= requestedLimit)
             {
                 break;
             }
         }
 
-        return count;
-    }
-
-    private static async Task<int> CollectFromInternalProviderAsync(
-        ISearchProvider provider,
-        SearchProviderQuery providerQuery,
-        Dictionary<Guid, float> bestScores,
-        CancellationToken cancellationToken)
-    {
-        var candidates = await provider.SearchAsync(providerQuery, cancellationToken).ConfigureAwait(false);
-        foreach (var result in candidates)
-        {
-            UpdateBestScore(bestScores, result);
-        }
-
-        return candidates.Count;
+        return results;
     }
 
     private static void UpdateBestScore(Dictionary<Guid, float> bestScores, SearchResult result)
diff --git a/Emby.Server.Implementations/Library/Search/SqlSearchProvider.cs b/Emby.Server.Implementations/Library/Search/SqlSearchProvider.cs
index 53c1cbbb79..bc766f1c8c 100644
--- a/Emby.Server.Implementations/Library/Search/SqlSearchProvider.cs
+++ b/Emby.Server.Implementations/Library/Search/SqlSearchProvider.cs
@@ -10,6 +10,7 @@ using Jellyfin.Data.Enums;
 using Jellyfin.Database.Implementations;
 using Jellyfin.Database.Implementations.Entities;
 using Jellyfin.Extensions;
+using MediaBrowser.Controller.Entities;
 using MediaBrowser.Controller.Library;
 using MediaBrowser.Controller.Persistence;
 using MediaBrowser.Model.Configuration;
@@ -32,16 +33,30 @@ public class SqlSearchProvider : IInternalSearchProvider
 
     private readonly IDbContextFactory<JellyfinDbContext> _dbProvider;
     private readonly IItemTypeLookup _itemTypeLookup;
+    private readonly ILibraryManager _libraryManager;
+    private readonly IUserManager _userManager;
+    private readonly IItemQueryHelpers _queryHelpers;
 
     /// <summary>
     /// Initializes a new instance of the <see cref="SqlSearchProvider"/> class.
     /// </summary>
     /// <param name="dbProvider">The database context factory.</param>
     /// <param name="itemTypeLookup">The item type lookup.</param>
-    public SqlSearchProvider(IDbContextFactory<JellyfinDbContext> dbProvider, IItemTypeLookup itemTypeLookup)
+    /// <param name="libraryManager">The library manager.</param>
+    /// <param name="userManager">The user manager.</param>
+    /// <param name="queryHelpers">The shared item query helpers.</param>
+    public SqlSearchProvider(
+        IDbContextFactory<JellyfinDbContext> dbProvider,
+        IItemTypeLookup itemTypeLookup,
+        ILibraryManager libraryManager,
+        IUserManager userManager,
+        IItemQueryHelpers queryHelpers)
     {
         _dbProvider = dbProvider;
         _itemTypeLookup = itemTypeLookup;
+        _libraryManager = libraryManager;
+        _userManager = userManager;
+        _queryHelpers = queryHelpers;
     }
 
     /// <inheritdoc/>
@@ -99,6 +114,7 @@ public class SqlSearchProvider : IInternalSearchProvider
             dbQuery = ApplyTypeFilter(dbQuery, query.IncludeItemTypes, query.ExcludeItemTypes);
             dbQuery = ApplyMediaTypeFilter(dbQuery, query.MediaTypes);
             dbQuery = ApplyParentFilter(dbQuery, query.ParentId);
+            dbQuery = ApplyUserAccessFilter(dbContext, dbQuery, query.UserId);
 
             // Compute the score in SQL: the ternary translates to a CASE WHEN. CleanName is
             // the pre-normalized (lowercase, diacritic-stripped) form, so we score against it
@@ -116,20 +132,13 @@ public class SqlSearchProvider : IInternalSearchProvider
                     : ContainsMatchScore
             });
 
-            var top = await scored
+            return await scored
                 .OrderByDescending(x => x.Score)
                 .ThenBy(x => x.Id)
                 .Take(limit)
-                .ToListAsync(cancellationToken)
+                .Select(x => new SearchResult(x.Id, x.Score))
+                .ToArrayAsync(cancellationToken)
                 .ConfigureAwait(false);
-
-            var results = new List<SearchResult>(top.Count);
-            foreach (var row in top)
-            {
-                results.Add(new SearchResult(row.Id, row.Score));
-            }
-
-            return results;
         }
     }
 
@@ -184,6 +193,27 @@ public class SqlSearchProvider : IInternalSearchProvider
         return query.Where(e => e.ParentId == pid || e.Parents!.Any(p => p.ParentItemId == pid));
     }
 
+    private IQueryable<BaseItemEntity> ApplyUserAccessFilter(
+        JellyfinDbContext dbContext,
+        IQueryable<BaseItemEntity> query,
+        Guid? userId)
+    {
+        if (!userId.HasValue || userId.Value.IsEmpty())
+        {
+            return query;
+        }
+
+        var user = _userManager.GetUserById(userId.Value);
+        if (user is null)
+        {
+            return query;
+        }
+
+        var accessFilter = new InternalItemsQuery(user);
+        _libraryManager.ConfigureUserAccess(accessFilter, user);
+        return _queryHelpers.ApplyAccessFiltering(dbContext, query, accessFilter);
+    }
+
     private List<string> MapKindsToTypeNames(BaseItemKind[] kinds)
     {
         var list = new List<string>(kinds.Length);
diff --git a/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs b/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs
index e366bdb095..1af7460540 100644
--- a/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs
+++ b/src/Jellyfin.Database/Jellyfin.Database.Implementations/JellyfinQueryHelperExtensions.cs
@@ -224,13 +224,14 @@ public static class JellyfinQueryHelperExtensions
 
         var containsMethodInfo = _containsQueryCache.GetOrAdd(typeof(TProperty), static (key) => _containsMethodGenericCache.MakeGenericMethod(key));
 
-        return Expression.Lambda<Func<TEntity, bool>>(
-            Expression.Call(
-                null,
-                containsMethodInfo,
-                Expression.Call(null, _efParameterInstruction.MakeGenericMethod(oneOf.GetType()), Expression.Constant(oneOf)),
-                property.Body),
-            parameter);
+        // Threshold picked from microbenchmarks on SQLite: inline IN(const,...) beats a
+        // parameterized array lookup by ~5-10% up to ~32 elements.
+        if (oneOf.Count <= 32)
+        {
+            return Expression.Lambda<Func<TEntity, bool>>(Expression.Call(null, containsMethodInfo, Expression.Constant(oneOf), property.Body), parameter);
+        }
+
+        return Expression.Lambda<Func<TEntity, bool>>(Expression.Call(null, containsMethodInfo, Expression.Call(null, _efParameterInstruction.MakeGenericMethod(oneOf.GetType()), Expression.Constant(oneOf)), property.Body), parameter);
     }
 
     internal static class ParameterReplacer