Additional fixes

Co-Authored-By: Cody Robibero <cody@robibe.ro>

tests/Jellyfin.Model.Tests/Entities/ProviderIdsExtensionsTests.cs

@@ -229,6 +229,66 @@ namespace Jellyfin.Model.Tests.Entities
             Assert.Equal("11", provider.GetProviderId(MetadataProvider.Tmdb));
         }
 
+        [Theory]
+        [InlineData(nameof(MetadataProvider.Imdb), " tt0113375 ")]
+        [InlineData(" Imdb", ExampleImdbId)]
+        public void TrySetProviderId_SurroundingWhitespace_Trimmed(string name, string value)
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+
+            Assert.True(provider.TrySetProviderId(name, value));
+            Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb));
+        }
+
+        [Fact]
+        public void SetProviderIds_ReplacesAll()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+            provider.ProviderIds[MetadataProvider.Tvdb.ToString()] = "12345";
+
+            provider.SetProviderIds(new Dictionary<string, string>
+            {
+                [MetadataProvider.Imdb.ToString()] = ExampleImdbId
+            });
+
+            Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb));
+            Assert.False(provider.HasProviderId(MetadataProvider.Tvdb));
+        }
+
+        [Fact]
+        public void SetProviderIds_ForeignId_Dropped()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+
+            provider.SetProviderIds(new Dictionary<string, string>
+            {
+                [MetadataProvider.Tmdb.ToString()] = "nm0000123",
+                [MetadataProvider.Imdb.ToString()] = ExampleImdbId,
+                [MetadataProvider.Tvdb.ToString()] = string.Empty
+            });
+
+            Assert.False(provider.HasProviderId(MetadataProvider.Tmdb));
+            Assert.False(provider.HasProviderId(MetadataProvider.Tvdb));
+            Assert.Equal(ExampleImdbId, provider.GetProviderId(MetadataProvider.Imdb));
+        }
+
+        [Fact]
+        public void SetProviderIds_Null_Clears()
+        {
+            var provider = new ProviderIdsExtensionsTestsObject();
+            provider.ProviderIds[MetadataProvider.Imdb.ToString()] = ExampleImdbId;
+
+            provider.SetProviderIds(null);
+
+            Assert.Empty(provider.ProviderIds);
+        }
+
+        [Fact]
+        public void SetProviderIds_NullInstance_ThrowsArgumentNullException()
+        {
+            Assert.Throws<ArgumentNullException>(() => ProviderIdsExtensions.SetProviderIds(null!, new Dictionary<string, string>()));
+        }
+
         [Fact]
         public void RemoveProviderId_Null_Remove()
         {

tests/Jellyfin.Providers.Tests/Manager/MetadataServiceRefreshTests.cs

@@ -1,5 +1,6 @@
 using System;
 using System.Collections.Generic;
+using System.Net.Http;
 using System.Threading;
 using System.Threading.Tasks;
 using Jellyfin.Data.Enums;
@@ -22,9 +23,13 @@ namespace Jellyfin.Providers.Tests.Manager
     public class MetadataServiceRefreshTests
     {
         [Theory]
-        [InlineData(false, "existing overview")]
-        [InlineData(true, null)]
-        public async Task RefreshWithProviders_ReplaceAllMetadata_KeepsExistingDataOnProviderFailure(bool allProvidersSucceed, string? expectedOverview)
+        // RemoveOldMetadata is only ever set by an explicit user action - a refresh with "replace all
+        // metadata", or Identify. A provider failing must not silently downgrade that to a merge: the
+        // providers that did answer supplied the replacement, and the old values are the wrong match
+        // the user asked to get rid of.
+        [InlineData(false)]
+        [InlineData(true)]
+        public async Task RefreshWithProviders_ReplaceAllMetadata_ErasesOldDataWhenAProviderAnswers(bool allProvidersSucceed)
         {
             var item = new Movie
             {
@@ -63,7 +68,51 @@ namespace Jellyfin.Providers.Tests.Manager
 
             Assert.Equal(allProvidersSucceed ? 0 : 1, result.Failures);
             Assert.Equal("new tagline", item.Tagline);
-            Assert.Equal(expectedOverview, item.Overview);
+            Assert.Null(item.Overview);
+        }
+
+        [Fact]
+        public async Task RefreshWithProviders_ReplaceAllMetadata_KeepsExistingDataWhenEveryRemoteProviderFails()
+        {
+            var item = new Movie
+            {
+                Name = "Test Movie",
+                Overview = "existing overview"
+            };
+
+            // Something has to contribute for the merge to run at all, otherwise the item is never touched
+            // and the case is moot. The local provider is the replacement the remote ones did not deliver.
+            var local = new Mock<ILocalMetadataProvider<Movie>>(MockBehavior.Loose);
+            local.Setup(p => p.Name).Returns("Local");
+            local.Setup(p => p.GetMetadata(It.IsAny<ItemInfo>(), It.IsAny<IDirectoryService>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(new MetadataResult<Movie>
+                {
+                    HasMetadata = true,
+                    Item = new Movie { Name = "Test Movie", Tagline = "new tagline" }
+                });
+
+            var remote = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            remote.Setup(p => p.Name).Returns("Failing");
+            remote.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.FromException<MetadataResult<Movie>>(new HttpRequestException("unreachable")));
+
+            var service = new TestMetadataService();
+            var result = await service.RefreshWithProvidersInternal(
+                new MetadataResult<Movie> { Item = item },
+                new MovieInfo { Name = item.Name },
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = true,
+                    RemoveOldMetadata = true
+                },
+                [local.Object, remote.Object]).ConfigureAwait(true);
+
+            Assert.Equal(1, result.Failures);
+            Assert.Equal("new tagline", item.Tagline);
+
+            // No remote provider answered, so erasing the overview would lose it for good.
+            Assert.Equal("existing overview", item.Overview);
         }
 
         [Fact]
@@ -97,6 +146,49 @@ namespace Jellyfin.Providers.Tests.Manager
             Assert.Equal("tt0113375", item.GetProviderId(MetadataProvider.Imdb));
         }
 
+        [Fact]
+        public async Task RefreshWithProviders_ForeignProviderId_ReplacedInLookupInfo()
+        {
+            var item = new Movie { Name = "Test Movie" };
+            var lookupInfo = new MovieInfo { Name = item.Name };
+            lookupInfo.ProviderIds[MetadataProvider.Tmdb.ToString()] = "nm0000123";
+
+            var answering = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            answering.Setup(p => p.Name).Returns("Answering");
+            answering.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync(() =>
+                {
+                    var found = new Movie { Name = "Test Movie" };
+                    found.ProviderIds[MetadataProvider.Tmdb.ToString()] = "12345";
+                    return new MetadataResult<Movie> { HasMetadata = true, Item = found };
+                });
+
+            string? tmdbIdSeenBySecondProvider = null;
+            var following = new Mock<IRemoteMetadataProvider<Movie, MovieInfo>>(MockBehavior.Loose);
+            following.Setup(p => p.Name).Returns("Following");
+            following.Setup(p => p.GetMetadata(It.IsAny<MovieInfo>(), It.IsAny<CancellationToken>()))
+                .ReturnsAsync((MovieInfo info, CancellationToken _) =>
+                {
+                    tmdbIdSeenBySecondProvider = info.GetProviderId(MetadataProvider.Tmdb);
+                    return new MetadataResult<Movie> { HasMetadata = false };
+                });
+
+            var service = new TestMetadataService();
+            await service.RefreshWithProvidersInternal(
+                new MetadataResult<Movie> { Item = item },
+                lookupInfo,
+                new MetadataRefreshOptions(Mock.Of<IDirectoryService>())
+                {
+                    MetadataRefreshMode = MetadataRefreshMode.FullRefresh,
+                    ReplaceAllMetadata = true
+                },
+                [answering.Object, following.Object]).ConfigureAwait(true);
+
+            // The stored id cannot be a TMDb one, so the provider that still has to run must get the id
+            // that was just found instead of failing on the same bad one.
+            Assert.Equal("12345", tmdbIdSeenBySecondProvider);
+        }
+
         [Theory]
         [InlineData(true)]
         [InlineData(false)]