Refactor to validate all images up front

3 files changed, 27 insertions, 56 deletions

diff --git a/MediaBrowser.Controller/Entities/BaseItem.cs b/MediaBrowser.Controller/Entities/BaseItem.cs
index 838a9f2f8..7dd8e310e 100644
--- a/MediaBrowser.Controller/Entities/BaseItem.cs
+++ b/MediaBrowser.Controller/Entities/BaseItem.cs
@@ -2495,11 +2495,11 @@ namespace MediaBrowser.Controller.Entities
         }
 
         /// <summary>
-        /// Adds the images.
+        /// Adds the images, updating metadata if they already are part of this item.
         /// </summary>
         /// <param name="imageType">Type of the image.</param>
         /// <param name="images">The images.</param>
-        /// <returns><c>true</c> if XXXX, <c>false</c> otherwise.</returns>
+        /// <returns><c>true</c> if images were added or updated, <c>false</c> otherwise.</returns>
         /// <exception cref="ArgumentException">Cannot call AddImages with chapter images.</exception>
         public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
         {
@@ -2512,7 +2512,6 @@ namespace MediaBrowser.Controller.Entities
                 .ToList();
 
             var newImageList = new List<FileSystemMetadata>();
-            var imageAdded = false;
             var imageUpdated = false;
 
             foreach (var newImage in images)
@@ -2528,7 +2527,6 @@ namespace MediaBrowser.Controller.Entities
                 if (existing == null)
                 {
                     newImageList.Add(newImage);
-                    imageAdded = true;
                 }
                 else
                 {
@@ -2549,19 +2547,6 @@ namespace MediaBrowser.Controller.Entities
                 }
             }
 
-            if (imageAdded || images.Count != existingImages.Count)
-            {
-                var newImagePaths = images.Select(i => i.FullName).ToList();
-
-                var deleted = existingImages
-                    .FindAll(i => i.IsLocalFile && !newImagePaths.Contains(i.Path.AsSpan(), StringComparison.OrdinalIgnoreCase) && !File.Exists(i.Path));
-
-                if (deleted.Count > 0)
-                {
-                    ImageInfos = ImageInfos.Except(deleted).ToArray();
-                }
-            }
-
             if (newImageList.Count > 0)
             {
                 ImageInfos = ImageInfos.Concat(newImageList.Select(i => GetImageInfo(i, imageType))).ToArray();
diff --git a/MediaBrowser.Providers/Manager/ItemImageProvider.cs b/MediaBrowser.Providers/Manager/ItemImageProvider.cs
index c80407bcb..f60fce11b 100644
--- a/MediaBrowser.Providers/Manager/ItemImageProvider.cs
+++ b/MediaBrowser.Providers/Manager/ItemImageProvider.cs
@@ -395,7 +395,7 @@ namespace MediaBrowser.Providers.Manager
         /// <returns><c>true</c> if changes were made to the item; otherwise <c>false</c>.</returns>
         public bool MergeImages(BaseItem item, IReadOnlyList<LocalImageInfo> images)
         {
-            var changed = false;
+            var changed = item.ValidateImages(new DirectoryService(_fileSystem));
 
             for (var i = 0; i < _singularImages.Length; i++)
             {
@@ -431,18 +431,6 @@ namespace MediaBrowser.Providers.Manager
                         currentImage.DateModified = newDateModified;
                     }
                 }
-                else
-                {
-                    var existing = item.GetImageInfo(type, 0);
-                    if (existing != null)
-                    {
-                        if (existing.IsLocalFile && !File.Exists(existing.Path))
-                        {
-                            item.RemoveImage(existing);
-                            changed = true;
-                        }
-                    }
-                }
             }
 
             if (UpdateMultiImages(item, images, ImageType.Backdrop))
@@ -450,12 +438,9 @@ namespace MediaBrowser.Providers.Manager
                 changed = true;
             }
 
-            if (item is IHasScreenshots)
+            if (item is IHasScreenshots && UpdateMultiImages(item, images, ImageType.Screenshot))
             {
-                if (UpdateMultiImages(item, images, ImageType.Screenshot))
-                {
-                    changed = true;
-                }
+                changed = true;
             }
 
             return changed;
@@ -480,14 +465,6 @@ namespace MediaBrowser.Providers.Manager
         {
             var changed = false;
 
-            var deletedImages = item.GetImages(type).Where(i => i.IsLocalFile && !File.Exists(i.Path)).ToList();
-
-            if (deletedImages.Count > 0)
-            {
-                item.RemoveImages(deletedImages);
-                changed = true;
-            }
-
             var newImageFileInfos = images
                 .Where(i => i.Type == type)
                 .Select(i => i.FileInfo)
diff --git a/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
index 253bcb7ca..b5efd8f01 100644
--- a/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
+++ b/tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
@@ -183,7 +183,7 @@ namespace Jellyfin.Providers.Tests.Manager
 
             var images = GetImages(imageType, imageCount, true);
 
-            var itemImageProvider = GetItemImageProvider(null, fileSystem.Object);
+            var itemImageProvider = GetItemImageProvider(null, fileSystem);
             var changed = itemImageProvider.MergeImages(item, images);
 
             Assert.False(changed);
@@ -213,7 +213,7 @@ namespace Jellyfin.Providers.Tests.Manager
 
             var images = GetImages(imageType, imageCount, true);
 
-            var itemImageProvider = GetItemImageProvider(null, fileSystem.Object);
+            var itemImageProvider = GetItemImageProvider(null, fileSystem);
             var changed = itemImageProvider.MergeImages(item, images);
 
             Assert.True(changed);
@@ -363,7 +363,7 @@ namespace Jellyfin.Providers.Tests.Manager
                 ReplaceAllImages = true
             };
 
-            var itemImageProvider = GetItemImageProvider(null, Mock.Of<IFileSystem>());
+            var itemImageProvider = GetItemImageProvider(null, new Mock<IFileSystem>());
             var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { dynamicProvider.Object }, refreshOptions, CancellationToken.None);
 
             Assert.True(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
@@ -401,7 +401,7 @@ namespace Jellyfin.Providers.Tests.Manager
             var providerManager = new Mock<IProviderManager>(MockBehavior.Strict);
             providerManager.Setup(pm => pm.GetAvailableRemoteImages(It.IsAny<BaseItem>(), It.IsAny<RemoteImageQuery>(), It.IsAny<CancellationToken>()))
                 .ReturnsAsync(remoteInfo);
-            var itemImageProvider = GetItemImageProvider(providerManager.Object, Mock.Of<IFileSystem>());
+            var itemImageProvider = GetItemImageProvider(providerManager.Object, null);
             var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { remoteProvider.Object }, refreshOptions, CancellationToken.None);
 
             Assert.False(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
@@ -459,7 +459,7 @@ namespace Jellyfin.Providers.Tests.Manager
             var fileSystem = new Mock<IFileSystem>();
             fileSystem.Setup(fs => fs.GetFileInfo(It.IsAny<string>()))
                 .Returns(new FileSystemMetadata { Length = 1 });
-            var itemImageProvider = GetItemImageProvider(providerManager.Object, fileSystem.Object);
+            var itemImageProvider = GetItemImageProvider(providerManager.Object, fileSystem);
             var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { remoteProvider.Object }, refreshOptions, CancellationToken.None);
 
             Assert.True(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
@@ -496,7 +496,7 @@ namespace Jellyfin.Providers.Tests.Manager
             var providerManager = new Mock<IProviderManager>(MockBehavior.Strict);
             providerManager.Setup(pm => pm.GetAvailableRemoteImages(It.IsAny<BaseItem>(), It.IsAny<RemoteImageQuery>(), It.IsAny<CancellationToken>()))
                 .ReturnsAsync(remoteInfo);
-            var itemImageProvider = GetItemImageProvider(providerManager.Object, Mock.Of<IFileSystem>());
+            var itemImageProvider = GetItemImageProvider(providerManager.Object, null);
             var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { remoteProvider.Object }, refreshOptions, CancellationToken.None);
 
             Assert.True(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
@@ -505,7 +505,7 @@ namespace Jellyfin.Providers.Tests.Manager
             // images from the provider manager are sorted by preference (earlier images are higher priority) so we can verify that low url numbers are chosen
             foreach (var image in actualImages)
             {
-                var index = int.Parse(Regex.Match(image.Path, @"\d+").Value, NumberStyles.Integer, CultureInfo.InvariantCulture);
+                var index = int.Parse(Regex.Match(image.Path, @"[0-9]+").Value, NumberStyles.Integer, CultureInfo.InvariantCulture);
                 Assert.True(index < imageCount);
             }
         }
@@ -543,14 +543,14 @@ namespace Jellyfin.Providers.Tests.Manager
             var providerManager = new Mock<IProviderManager>(MockBehavior.Strict);
             providerManager.Setup(pm => pm.GetAvailableRemoteImages(It.IsAny<BaseItem>(), It.IsAny<RemoteImageQuery>(), It.IsAny<CancellationToken>()))
                 .ReturnsAsync(remoteInfo);
-            var itemImageProvider = GetItemImageProvider(providerManager.Object, Mock.Of<IFileSystem>());
+            var itemImageProvider = GetItemImageProvider(providerManager.Object, new Mock<IFileSystem>());
             var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { remoteProvider.Object }, refreshOptions, CancellationToken.None);
 
             Assert.True(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
             Assert.Equal(imageCount, item.GetImages(imageType).Count());
             foreach (var image in item.GetImages(imageType))
             {
-                Assert.Matches(@"image url \d", image.Path);
+                Assert.Matches(@"image url [0-9]", image.Path);
             }
         }
 
@@ -573,19 +573,28 @@ namespace Jellyfin.Providers.Tests.Manager
                 ReplaceAllImages = true
             };
 
-            var itemImageProvider = GetItemImageProvider(Mock.Of<IProviderManager>(), Mock.Of<IFileSystem>());
+            var itemImageProvider = GetItemImageProvider(Mock.Of<IProviderManager>(), null);
             var result = await itemImageProvider.RefreshImages(item, libraryOptions, new List<IImageProvider> { remoteProvider.Object }, refreshOptions, CancellationToken.None);
 
             Assert.False(result.UpdateType.HasFlag(ItemUpdateType.ImageUpdate));
             Assert.Equal(imageCount, item.GetImages(imageType).Count());
         }
 
-        private static ItemImageProvider GetItemImageProvider(IProviderManager? providerManager, IFileSystem? fileSystem)
+        private static ItemImageProvider GetItemImageProvider(IProviderManager? providerManager, Mock<IFileSystem>? mockFileSystem)
         {
             // strict to ensure this isn't accidentally used where a prepared mock is intended
             providerManager ??= Mock.Of<IProviderManager>(MockBehavior.Strict);
-            fileSystem ??= Mock.Of<IFileSystem>(MockBehavior.Strict);
-            return new ItemImageProvider(new NullLogger<ItemImageProvider>(), providerManager, fileSystem);
+
+            // BaseItem.ValidateImages depends on the directory service being able to list directory contents, give it the expected valid file paths
+            mockFileSystem ??= new Mock<IFileSystem>(MockBehavior.Strict);
+            mockFileSystem.Setup(fs => fs.GetFilePaths(It.IsAny<string>(), It.IsAny<bool>()))
+                .Returns(new[]
+                {
+                    string.Format(CultureInfo.InvariantCulture, TestDataImagePath, 0),
+                    string.Format(CultureInfo.InvariantCulture, TestDataImagePath, 1)
+                });
+
+            return new ItemImageProvider(new NullLogger<ItemImageProvider>(), providerManager, mockFileSystem.Object);
         }
 
         private static BaseItem GetItemWithImages(ImageType type, int count, bool validPaths)