From 5e82b61bab8c9461624fd2095fc9ccd11e33ce8d Mon Sep 17 00:00:00 2001 From: Shadowghost Date: Mon, 4 May 2026 23:40:07 +0200 Subject: [PATCH] Apply review suggestions --- .../Library/Search/SearchManager.cs | 97 ++++++++----------- .../Library/Search/SqlSearchProvider.cs | 52 +++++++--- .../JellyfinQueryHelperExtensions.cs | 15 +-- 3 files changed, 92 insertions(+), 72 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(); var requestedLimit = providerQuery.Limit ?? 100; - - foreach (var provider in providers.Where(p => p.CanSearch(providerQuery))) + var applicable = providers.Where(p => p.CanSearch(providerQuery)).ToArray(); + if (applicable.Length == 0) { - if (bestScores.Count >= requestedLimit || cancellationToken.IsCancellationRequested) - { - break; - } + return []; + } - await CollectFromProviderAsync(provider, providerQuery, searchTerm, bestScores, requestedLimit, cancellationToken).ConfigureAwait(false); + var perProvider = await Task.WhenAll( + applicable.Select(p => CollectFromProviderAsync(p, providerQuery, searchTerm, requestedLimit, cancellationToken))) + .ConfigureAwait(false); + + var bestScores = new Dictionary(); + foreach (var providerResults in perProvider) + { + foreach (var result in providerResults) + { + UpdateBestScore(bestScores, result); + } } return bestScores @@ -273,66 +278,50 @@ public class SearchManager : ISearchManager .ToList(); } - private async Task CollectFromProviderAsync( + private async Task> CollectFromProviderAsync( ISearchProvider provider, SearchProviderQuery providerQuery, string searchTerm, - Dictionary 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 CollectFromExternalProviderAsync( + private static async Task> CollectFromExternalProviderAsync( IExternalSearchProvider provider, SearchProviderQuery providerQuery, - Dictionary bestScores, int requestedLimit, CancellationToken cancellationToken) { - var count = 0; + var results = new List(); 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 CollectFromInternalProviderAsync( - ISearchProvider provider, - SearchProviderQuery providerQuery, - Dictionary 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 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 _dbProvider; private readonly IItemTypeLookup _itemTypeLookup; + private readonly ILibraryManager _libraryManager; + private readonly IUserManager _userManager; + private readonly IItemQueryHelpers _queryHelpers; /// /// Initializes a new instance of the class. /// /// The database context factory. /// The item type lookup. - public SqlSearchProvider(IDbContextFactory dbProvider, IItemTypeLookup itemTypeLookup) + /// The library manager. + /// The user manager. + /// The shared item query helpers. + public SqlSearchProvider( + IDbContextFactory dbProvider, + IItemTypeLookup itemTypeLookup, + ILibraryManager libraryManager, + IUserManager userManager, + IItemQueryHelpers queryHelpers) { _dbProvider = dbProvider; _itemTypeLookup = itemTypeLookup; + _libraryManager = libraryManager; + _userManager = userManager; + _queryHelpers = queryHelpers; } /// @@ -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(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 ApplyUserAccessFilter( + JellyfinDbContext dbContext, + IQueryable 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 MapKindsToTypeNames(BaseItemKind[] kinds) { var list = new List(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>( - 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>(Expression.Call(null, containsMethodInfo, Expression.Constant(oneOf), property.Body), parameter); + } + + return Expression.Lambda>(Expression.Call(null, containsMethodInfo, Expression.Call(null, _efParameterInstruction.MakeGenericMethod(oneOf.GetType()), Expression.Constant(oneOf)), property.Body), parameter); } internal static class ParameterReplacer