From 5cd3d7ebb7c8ce5b7cf68b0696543780215a6fff Mon Sep 17 00:00:00 2001 From: zerafachris Date: Fri, 17 Jul 2026 17:20:30 +0200 Subject: fix: don't throw ArgumentNullException on partial UpdateItem payloads (#17366) BaseItemDto.Genres, .Tags, and .ProviderIds are plain auto-properties with no default initializer, so they deserialize to null when a client omits them from a partial POST /Items/{itemId} body. The OpenAPI spec documents every BaseItemDto field as optional, but ItemUpdateController.UpdateItem fed these three properties straight into Distinct()/Select()/ToList() without a null check, so a request that (for example) only sets Tags throws ArgumentNullException("source") once it reaches the unguarded Genres line, before Tags is even processed. Guard all three assignments with the same "if (request.X is not null)" pattern already used for the neighboring Studios/Taglines/ProductionLocations fields in this method, so omitted fields are left unchanged instead of crashing the request. Adds ItemUpdateControllerTests covering the reported repro (only Tags supplied) and a companion case asserting existing Genres/ProviderIds are preserved when omitted from the payload. Signed-off-by: zerafachris --- Jellyfin.Api/Controllers/ItemUpdateController.cs | 37 ++++++--- .../Controllers/ItemUpdateControllerTests.cs | 87 ++++++++++++++++++++++ 2 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs diff --git a/Jellyfin.Api/Controllers/ItemUpdateController.cs b/Jellyfin.Api/Controllers/ItemUpdateController.cs index 70b09a4a29..b0ea1dd911 100644 --- a/Jellyfin.Api/Controllers/ItemUpdateController.cs +++ b/Jellyfin.Api/Controllers/ItemUpdateController.cs @@ -250,7 +250,11 @@ public class ItemUpdateController : BaseJellyfinApiController item.IndexNumber = request.IndexNumber; item.ParentIndexNumber = request.ParentIndexNumber; item.Overview = request.Overview; - item.Genres = request.Genres.Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); + + if (request.Genres is not null) + { + item.Genres = request.Genres.Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); + } if (item is Episode episode) { @@ -293,10 +297,20 @@ public class ItemUpdateController : BaseJellyfinApiController item.CustomRating = request.CustomRating; var currentTags = item.Tags; - var newTags = request.Tags.Select(t => t.Trim()).Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); - var removedTags = currentTags.Except(newTags).ToList(); - var addedTags = newTags.Except(currentTags).ToList(); - item.Tags = newTags; + List removedTags; + List addedTags; + if (request.Tags is not null) + { + var newTags = request.Tags.Select(t => t.Trim()).Distinct(StringComparer.OrdinalIgnoreCase).ToArray(); + removedTags = currentTags.Except(newTags).ToList(); + addedTags = newTags.Except(currentTags).ToList(); + item.Tags = newTags; + } + else + { + removedTags = []; + addedTags = []; + } if (item is Series rseries) { @@ -408,15 +422,18 @@ public class ItemUpdateController : BaseJellyfinApiController item.RunTimeTicks = request.RunTimeTicks; } - foreach (var pair in request.ProviderIds.ToList()) + if (request.ProviderIds is not null) { - if (string.IsNullOrEmpty(pair.Value)) + foreach (var pair in request.ProviderIds.ToList()) { - request.ProviderIds.Remove(pair.Key); + if (string.IsNullOrEmpty(pair.Value)) + { + request.ProviderIds.Remove(pair.Key); + } } - } - item.ProviderIds = request.ProviderIds; + item.ProviderIds = request.ProviderIds; + } if (item is Video video) { diff --git a/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs b/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs new file mode 100644 index 0000000000..fa167203dd --- /dev/null +++ b/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs @@ -0,0 +1,87 @@ +using System; +using System.Reflection; +using System.Threading.Tasks; +using Jellyfin.Api.Controllers; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Entities.Movies; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; +using MediaBrowser.Model.Dto; +using MediaBrowser.Model.Globalization; +using MediaBrowser.Model.IO; +using Moq; +using Xunit; + +namespace Jellyfin.Api.Tests.Controllers; + +public class ItemUpdateControllerTests +{ + private readonly ItemUpdateController _subject; + + public ItemUpdateControllerTests() + { + _subject = new ItemUpdateController( + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of()); + } + + [Fact] + public async Task UpdateItem_WhenOnlyTagsFieldSupplied_DoesNotThrowAndAppliesTags() + { + // Regression test for https://github.com/jellyfin/jellyfin/issues/17366 + // A partial update payload that only sets "Tags" leaves every other + // BaseItemDto collection property null (they have no default + // initializer). Genres and ProviderIds used to be fed straight into + // Distinct()/ToList() without a null check, so this call used to throw + // ArgumentNullException before the fix below was applied. + var movie = new Movie(); + var request = new BaseItemDto + { + Tags = new[] { "new-tag-1", "new-tag-2" } + }; + + await InvokeUpdateItem(request, movie); + + Assert.Equal(new[] { "new-tag-1", "new-tag-2" }, movie.Tags); + Assert.Empty(movie.Genres); + Assert.Empty(movie.ProviderIds); + } + + [Fact] + public async Task UpdateItem_WhenGenresAndProviderIdsOmitted_LeavesExistingValuesUnchanged() + { + var movie = new Movie + { + Genres = new[] { "Action" } + }; + movie.ProviderIds["Imdb"] = "tt1234567"; + + var request = new BaseItemDto + { + Tags = Array.Empty() + }; + + await InvokeUpdateItem(request, movie); + + Assert.Equal(new[] { "Action" }, movie.Genres); + Assert.Equal("tt1234567", movie.ProviderIds["Imdb"]); + } + + private Task InvokeUpdateItem(BaseItemDto request, BaseItem item) + { + var method = typeof(ItemUpdateController).GetMethod( + "UpdateItem", + BindingFlags.NonPublic | BindingFlags.Instance, + null, + new[] { typeof(BaseItemDto), typeof(BaseItem) }, + null); + + Assert.NotNull(method); + + return (Task)method!.Invoke(_subject, new object[] { request, item })!; + } +} -- cgit v1.2.3 From 53e58d8b1b61c44405322bf91a8aa230f02b8323 Mon Sep 17 00:00:00 2001 From: zerafachris Date: Tue, 21 Jul 2026 08:51:30 +0200 Subject: Make ItemUpdateController.UpdateItem internal instead of reflection Addresses review feedback from @Bond-009 on PR #17370: the test helper InvokeUpdateItem was invoking the private UpdateItem(BaseItemDto, BaseItem) method via reflection. Jellyfin.Api.csproj already grants InternalsVisibleTo("Jellyfin.Api.Tests"), so the method is changed to internal and the test now calls it directly, removing the GetMethod/Invoke boilerplate. Co-Authored-By: Claude Sonnet 5 --- Jellyfin.Api/Controllers/ItemUpdateController.cs | 2 +- .../Controllers/ItemUpdateControllerTests.cs | 12 +----------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/Jellyfin.Api/Controllers/ItemUpdateController.cs b/Jellyfin.Api/Controllers/ItemUpdateController.cs index b0ea1dd911..65f53a23ff 100644 --- a/Jellyfin.Api/Controllers/ItemUpdateController.cs +++ b/Jellyfin.Api/Controllers/ItemUpdateController.cs @@ -236,7 +236,7 @@ public class ItemUpdateController : BaseJellyfinApiController return NoContent(); } - private async Task UpdateItem(BaseItemDto request, BaseItem item) + internal async Task UpdateItem(BaseItemDto request, BaseItem item) { item.Name = request.Name; item.ForcedSortName = request.ForcedSortName; diff --git a/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs b/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs index fa167203dd..1a91efe4f2 100644 --- a/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs +++ b/tests/Jellyfin.Api.Tests/Controllers/ItemUpdateControllerTests.cs @@ -1,5 +1,4 @@ using System; -using System.Reflection; using System.Threading.Tasks; using Jellyfin.Api.Controllers; using MediaBrowser.Controller.Configuration; @@ -73,15 +72,6 @@ public class ItemUpdateControllerTests private Task InvokeUpdateItem(BaseItemDto request, BaseItem item) { - var method = typeof(ItemUpdateController).GetMethod( - "UpdateItem", - BindingFlags.NonPublic | BindingFlags.Instance, - null, - new[] { typeof(BaseItemDto), typeof(BaseItem) }, - null); - - Assert.NotNull(method); - - return (Task)method!.Invoke(_subject, new object[] { request, item })!; + return _subject.UpdateItem(request, item); } } -- cgit v1.2.3