Skip to content

Commit

Permalink
Refactor to validate all images up front
Browse files Browse the repository at this point in the history
  • Loading branch information
@1337joe
1337joe committed Nov 1, 2021
1 parent 0fbd8d8 commit a97b48b
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 54 deletions.
19 changes: 2 additions & 17 deletions MediaBrowser.Controller/Entities/BaseItem.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2495,11 +2495,11 @@ public IEnumerable<ItemImageInfo> GetImages(ImageType imageType)
}

/// <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)
{
Expand All @@ -2512,7 +2512,6 @@ public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
.ToList();

var newImageList = new List<FileSystemMetadata>();
var imageAdded = false;
var imageUpdated = false;

foreach (var newImage in images)
Expand All @@ -2528,7 +2527,6 @@ public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
if (existing == null)
{
newImageList.Add(newImage);
imageAdded = true;
}
else
{
Expand All @@ -2549,19 +2547,6 @@ public bool AddImages(ImageType imageType, List<FileSystemMetadata> images)
}
}

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();
Expand Down
29 changes: 3 additions & 26 deletions MediaBrowser.Providers/Manager/ItemImageProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ private void PruneImages(BaseItem item, List<ItemImageInfo> images)
/// <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++)
{
Expand Down Expand Up @@ -431,31 +431,16 @@ public bool MergeImages(BaseItem item, IReadOnlyList<LocalImageInfo> images)
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))
{
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;
Expand All @@ -480,14 +465,6 @@ private bool UpdateMultiImages(BaseItem item, IReadOnlyList<LocalImageInfo> imag
{
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)
Expand Down
31 changes: 20 additions & 11 deletions tests/Jellyfin.Providers.Tests/Manager/ItemImageProviderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ public void MergeImages_PopulatedItemWithGoodPathsAndSameNewImages_NoChange(Imag

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);
Expand Down Expand Up @@ -213,7 +213,7 @@ public void MergeImages_PopulatedItemWithGoodPathsAndSameNewImagesWithNewTimesta

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);
Expand Down Expand Up @@ -363,7 +363,7 @@ public async void RefreshImages_PopulatedItemPopulatedProviderDynamicFullRefresh
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));
Expand Down Expand Up @@ -401,7 +401,7 @@ public async void RefreshImages_PopulatedItemPopulatedProviderRemote_NoChange(Im
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));
Expand Down Expand Up @@ -459,7 +459,7 @@ public async void RefreshImages_EmptyNonStubItemPopulatedProviderRemote_Download
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));
Expand Down Expand Up @@ -496,7 +496,7 @@ public async void RefreshImages_EmptyItemPopulatedProviderRemoteExtras_LimitsIma
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));
Expand Down Expand Up @@ -543,7 +543,7 @@ public async void RefreshImages_PopulatedItemPopulatedProviderRemoteFullRefresh_
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));
Expand Down Expand Up @@ -573,19 +573,28 @@ public async void RefreshImages_PopulatedItemEmptyProviderRemoteFullRefresh_Does
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)
Expand Down

0 comments on commit a97b48b

Please sign in to comment.