diff options
| author | Paolo Antinori <pantinor@redhat.com> | 2026-07-21 08:20:40 +0200 |
|---|---|---|
| committer | Paolo Antinori <pantinor@redhat.com> | 2026-07-22 07:43:58 +0200 |
| commit | 5d580abb08d9d23f54e74050fdaa8fcdbc21571f (patch) | |
| tree | e680320b74724f9369c4fde71ddaf22d7db53e88 /tests | |
| parent | bdf263d8677ee87078c680a355a91a8048b8308b (diff) | |
fix: avoid NRE when sorting by user-dependent keys without a user
A query sorted by a user-dependent key (PlayCount, IsFavoriteOrLiked,
DatePlayed, IsPlayed, IsUnplayed) but carrying no User caused a
NullReferenceException inside UserDataManager.GetUserData, surfacing as
"Failed to compare two elements in the array" (InvalidOperationException
wrapping the NRE from the LINQ sort) and 500-ing the /Items request.
Root cause: LibraryManager.GetComparer assigned comparer.User = user
without a null guard, so PlayCountComparer.GetValue called
UserDataManager.GetUserData(null, item), dereferencing user.Id.
Two-part fix:
- LibraryManager.GetComparer: when user is null and the sort key requires a
user (IUserBaseItemComparer), substitute the SortName comparer so the
result stays deterministic instead of 500-ing. SortName is the project's
canonical tiebreaker (ItemsController injects it for album-by-artist).
- UserDataManager.GetUserData: ArgumentNullException.ThrowIfNull(user) as
defense in depth (matches the existing guards on the SaveUserData
overloads in the same file). On master this overload was rewritten to use
ResolveUserDataRow, so the NRE dereferences user.Id rather than
user.InternalId as on the release branch — same bug, different line.
Also fixes DateLastMediaAddedComparer being statically mis-tagged as
IUserBaseItemComparer: its GetDate is static and never reads User, so it
does not need one. Without this, the SortName fallback above would wrongly
engage for DateLastContentAdded on anonymous queries (returning SortName
order instead of date order). Re-tagged to IBaseItemComparer and dropped the
unused User/UserManager/UserDataManager properties.
Tests:
- UserDataManagerTests.GetUserData_NullUser_ThrowsArgumentNullException:
reproduces the crash (NRE -> now ArgumentNullException). Added to master's
existing UserDataManagerTests.
- LibraryManagerSortTests.Sort_UserDependentKey_NullUser_FallsBackToSortNameWithoutThrowing:
Sort with a user-dependent key + null user no longer throws and returns
items ordered by the SortName fallback (direction preserved).
- LibraryManagerSortTests.Sort_DateLastContentAdded_NullUser_OrdersByDateNotSortName:
guards that DateLastContentAdded still sorts by date with no user (fixture
chosen so date-desc and SortName-desc disagree, so a revert is caught).
Full Jellyfin.Server.Implementations.Tests suite: 642 passed, 0 failed.
Fixes #17393
Diffstat (limited to 'tests')
| -rw-r--r-- | tests/Jellyfin.Server.Implementations.Tests/Library/LibraryManagerSortTests.cs | 101 | ||||
| -rw-r--r-- | tests/Jellyfin.Server.Implementations.Tests/Library/UserDataManagerTests.cs | 7 |
2 files changed, 108 insertions, 0 deletions
diff --git a/tests/Jellyfin.Server.Implementations.Tests/Library/LibraryManagerSortTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Library/LibraryManagerSortTests.cs new file mode 100644 index 0000000000..9cec9d6736 --- /dev/null +++ b/tests/Jellyfin.Server.Implementations.Tests/Library/LibraryManagerSortTests.cs @@ -0,0 +1,101 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using AutoFixture; +using AutoFixture.AutoMoq; +using Emby.Naming.Common; +using Emby.Server.Implementations.Library; +using Emby.Server.Implementations.Sorting; +using Jellyfin.Data.Enums; +using Jellyfin.Database.Implementations.Enums; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Entities.Audio; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Persistence; +using MediaBrowser.Controller.Resolvers; +using MediaBrowser.Controller.Sorting; +using MediaBrowser.Model.IO; +using Moq; +using Xunit; +using BaseItem = MediaBrowser.Controller.Entities.BaseItem; + +namespace Jellyfin.Server.Implementations.Tests.Library; + +public class LibraryManagerSortTests +{ + [Fact] + public void Sort_UserDependentKey_NullUser_ThrowsArgumentException() + { + var libraryManager = CreateLibraryManager( + new IBaseItemComparer[] { new PlayCountComparer(), new SortNameComparer() }); + + BaseItem[] items = + { + new Audio { Name = "Zulu", SortName = "Zulu", Id = Guid.NewGuid() }, + new Audio { Name = "Alpha", SortName = "Alpha", Id = Guid.NewGuid() }, + }; + + // A user-dependent sort key with no user is a caller contract violation — throw, + // don't silently fall back to a different key (review feedback on #17395). + Assert.Throws<ArgumentException>(() => libraryManager.Sort( + items, + user: null, + new[] { (ItemSortBy.PlayCount, SortOrder.Descending) }).ToArray()); + } + + [Fact] + public void Sort_DateLastContentAdded_NullUser_OrdersByDateNotSortName() + { + // DateLastMediaAddedComparer does not use User (its GetDate is static), so it must NOT be + // treated as a user-dependent comparer: with no user it should still sort by date, not fall + // back to SortName. + var libraryManager = CreateLibraryManager( + new IBaseItemComparer[] { new DateLastMediaAddedComparer(), new SortNameComparer() }); + + // Names are chosen so date-descending and SortName-descending DISAGREE: Alpha is newest + // (date-desc rank 1), but Zulu sorts last alphabetically (SortName-desc rank 1). If the + // comparer were still tagged IUserBaseItemComparer, the null-user SortName fallback would + // return [Zulu, Mike, Alpha] and this assertion would fail. + BaseItem[] items = + { + MakeFolder("Alpha", new DateTime(2026, 1, 1)), + MakeFolder("Mike", new DateTime(2025, 1, 1)), + MakeFolder("Zulu", new DateTime(2024, 1, 1)) + }; + + var sorted = libraryManager.Sort( + items, + user: null, + new[] { (ItemSortBy.DateLastContentAdded, SortOrder.Descending) }).ToArray(); + + // Descending by date => newest first: Alpha, Mike, Zulu. (SortName-desc would be Zulu, Mike, Alpha.) + Assert.Equal(new[] { "Alpha", "Mike", "Zulu" }, sorted.Select(i => i.Name)); + } + + private static Folder MakeFolder(string name, DateTime dateLastMediaAdded) + => new() { Name = name, Id = Guid.NewGuid(), DateLastMediaAdded = dateLastMediaAdded }; + + private static Emby.Server.Implementations.Library.LibraryManager CreateLibraryManager(IReadOnlyCollection<IBaseItemComparer> comparers) + { + var fixture = new Fixture().Customize(new AutoMoqCustomization()); + fixture.Register(() => new NamingOptions()); + var configMock = fixture.Freeze<Mock<IServerConfigurationManager>>(); + configMock.Setup(c => c.ApplicationPaths.ProgramDataPath).Returns("/data"); + // BaseItem.SortName/CreateSortName dereference this static; set it so SortName-fallback + // paths don't NRE in-process (mirrors AudioResolverTests in the sibling test project). + BaseItem.ConfigurationManager ??= configMock.Object; + var itemRepository = fixture.Freeze<Mock<IItemRepository>>(); + itemRepository.Setup(i => i.RetrieveItem(It.IsAny<Guid>())).Returns<BaseItem>(null); + var fileSystemMock = fixture.Freeze<Mock<IFileSystem>>(); + fileSystemMock.Setup(f => f.GetFileInfo(It.IsAny<string>())).Returns<string>(path => new FileSystemMetadata { FullName = path }); + + return fixture.Build<Emby.Server.Implementations.Library.LibraryManager>().Do(s => s.AddParts( + fixture.Create<IEnumerable<IResolverIgnoreRule>>(), + fixture.Create<IEnumerable<IItemResolver>>(), + fixture.Create<IEnumerable<IIntroProvider>>(), + comparers, + fixture.Create<IEnumerable<ILibraryPostScanTask>>())) + .Create(); + } +} diff --git a/tests/Jellyfin.Server.Implementations.Tests/Library/UserDataManagerTests.cs b/tests/Jellyfin.Server.Implementations.Tests/Library/UserDataManagerTests.cs index bd14ca008d..ba3127bc08 100644 --- a/tests/Jellyfin.Server.Implementations.Tests/Library/UserDataManagerTests.cs +++ b/tests/Jellyfin.Server.Implementations.Tests/Library/UserDataManagerTests.cs @@ -206,4 +206,11 @@ public sealed class UserDataManagerTests : IDisposable Assert.Equal(222, result[fossilItem.Id].PlaybackPositionTicks); Assert.Equal(333, result[retiredItem.Id].PlaybackPositionTicks); } + + [Fact] + public void GetUserData_NullUser_ThrowsArgumentNullException() + { + var item = CreateAudioBook(); + Assert.Throws<ArgumentNullException>(() => _userDataManager.GetUserData(null!, item)); + } } |
