From 2e2a594e19038bc2fcea5fdbeda9d37e8394fff7 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Tue, 30 Nov 2021 23:53:34 +0100 Subject: [PATCH 01/19] Move Get*Providers definitions to interface --- .../Providers/IProviderManager.cs | 18 ++++++++++++++++++ .../Manager/MetadataService.cs | 4 ++-- .../Manager/ProviderManager.cs | 15 ++------------- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/MediaBrowser.Controller/Providers/IProviderManager.cs b/MediaBrowser.Controller/Providers/IProviderManager.cs index 44bc4a50cb..32a7951f62 100644 --- a/MediaBrowser.Controller/Providers/IProviderManager.cs +++ b/MediaBrowser.Controller/Providers/IProviderManager.cs @@ -131,6 +131,24 @@ namespace MediaBrowser.Controller.Providers /// IEnumerable{ImageProviderInfo}. IEnumerable GetRemoteImageProviderInfo(BaseItem item); + /// + /// Gets the image providers for the provided item. + /// + /// The item. + /// The image refresh options. + /// The image providers for the item. + IEnumerable GetImageProviders(BaseItem item, ImageRefreshOptions refreshOptions); + + /// + /// Gets the metadata providers for the provided item. + /// + /// The item. + /// The library options. + /// The type of metadata provider. + /// The metadata providers. + IEnumerable> GetMetadataProviders(BaseItem item, LibraryOptions libraryOptions) + where T : BaseItem; + /// /// Gets all metadata plugins. /// diff --git a/MediaBrowser.Providers/Manager/MetadataService.cs b/MediaBrowser.Providers/Manager/MetadataService.cs index 0c52d26736..01e2a5db9f 100644 --- a/MediaBrowser.Providers/Manager/MetadataService.cs +++ b/MediaBrowser.Providers/Manager/MetadataService.cs @@ -94,7 +94,7 @@ namespace MediaBrowser.Providers.Manager var localImagesFailed = false; - var allImageProviders = ((ProviderManager)ProviderManager).GetImageProviders(item, refreshOptions).ToList(); + var allImageProviders = ProviderManager.GetImageProviders(item, refreshOptions).ToList(); if (refreshOptions.RemoveOldMetadata && refreshOptions.ReplaceAllImages) { @@ -522,7 +522,7 @@ namespace MediaBrowser.Providers.Manager protected IEnumerable GetProviders(BaseItem item, LibraryOptions libraryOptions, MetadataRefreshOptions options, bool isFirstRefresh, bool requiresRefresh) { // Get providers to refresh - var providers = ((ProviderManager)ProviderManager).GetMetadataProviders(item, libraryOptions).ToList(); + var providers = ProviderManager.GetMetadataProviders(item, libraryOptions).ToList(); var metadataRefreshMode = options.MetadataRefreshMode; diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 0c31d460ff..e644f0e74d 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -302,12 +302,7 @@ namespace MediaBrowser.Providers.Manager return GetRemoteImageProviders(item, true).Select(i => new ImageProviderInfo(i.Name, i.GetSupportedImages(item).ToArray())); } - /// - /// Gets the image providers for the provided item. - /// - /// The item. - /// The image refresh options. - /// The image providers for the item. + /// public IEnumerable GetImageProviders(BaseItem item, ImageRefreshOptions refreshOptions) { return GetImageProviders(item, _libraryManager.GetLibraryOptions(item), GetMetadataOptions(item), refreshOptions, false); @@ -342,13 +337,7 @@ namespace MediaBrowser.Providers.Manager .ThenBy(GetOrder); } - /// - /// Gets the metadata providers for the provided item. - /// - /// The item. - /// The library options. - /// The type of metadata provider. - /// The metadata providers. + /// public IEnumerable> GetMetadataProviders(BaseItem item, LibraryOptions libraryOptions) where T : BaseItem { From 4ace7f5c532b655449f7121b660a2cb9e66570be Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Tue, 30 Nov 2021 23:55:53 +0100 Subject: [PATCH 02/19] Fix unused var, log typo --- MediaBrowser.Providers/Manager/ProviderManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index e644f0e74d..1b73574774 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -354,7 +354,7 @@ namespace MediaBrowser.Providers.Manager return _metadataProviders.OfType>() .Where(i => CanRefresh(i, item, libraryOptions, includeDisabled, forceEnableInternetMetadata)) - .OrderBy(i => GetConfiguredOrder(item, i, libraryOptions, globalMetadataOptions)) + .OrderBy(i => GetConfiguredOrder(item, i, libraryOptions, currentOptions)) .ThenBy(GetDefaultOrder); } @@ -908,7 +908,7 @@ namespace MediaBrowser.Providers.Manager } catch (Exception ex) { - _logger.LogError(ex, "Error in {0}.Suports", i.GetType().Name); + _logger.LogError(ex, "Error in {0}.Supports", i.GetType().Name); return false; } }); From 785cc1bb6ed1cbd3d0c300b9842af6e15167e715 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sun, 5 Dec 2021 17:19:40 +0100 Subject: [PATCH 03/19] Implement sort test for ProviderManager.GetImageProviders --- .../Manager/ProviderManagerTests.cs | 177 ++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs new file mode 100644 index 0000000000..31b1913346 --- /dev/null +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -0,0 +1,177 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using MediaBrowser.Controller.Configuration; +using MediaBrowser.Controller.Entities; +using MediaBrowser.Controller.Entities.Movies; +using MediaBrowser.Controller.Library; +using MediaBrowser.Controller.Providers; +using MediaBrowser.Model.Configuration; +using MediaBrowser.Providers.Manager; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace Jellyfin.Providers.Tests.Manager +{ + public class ProviderManagerTests + { + private static TheoryData GetImageProvidersOrderData() + => new () + { + { 3, null, null, null, null, new[] { 0, 1, 2 } }, // no order options set + + // library options ordering + { 3, null, Array.Empty(), null, null, new[] { 0, 1, 2 } }, // no order provided + { 3, null, new[] { 1 }, null, null, new[] { 1, 0, 2 } }, // one item in order + { 3, null, new[] { 2, 1, 0 }, null, null, new[] { 2, 1, 0 } }, // full reverse order + + // server options ordering + { 3, null, null, Array.Empty(), null, new[] { 0, 1, 2 } }, // no order provided + { 3, null, null, new[] { 1 }, null, new[] { 1, 0, 2 } }, // one item in order + { 3, null, null, new[] { 2, 1, 0 }, null, new[] { 2, 1, 0 } }, // full reverse order + + // IHasOrder ordering + // TODO unintuitive - default if not IHasOrder is 0, not max + { 3, null, null, null, new int?[] { null, 0, null }, new[] { 0, 1, 2 } }, // one item with order 0, no change because default order value is 0 + { 3, null, null, null, new int?[] { null, 1, null }, new[] { 0, 2, 1 } }, // one item in order (goes to end, not beginning) + { 3, null, null, null, new int?[] { 2, 1, 0 }, new[] { 2, 1, 0 } }, // full reverse order + + // multiple orders set + // TODO should library fall through to server if both are set on different elements? + { 3, null, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // library order first, server order ignored + { 3, null, new[] { 1 }, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby + { 3, null, new[] { 2, 1, 0 }, new[] { 1, 2, 0 }, new int?[] { 2, 0, 1 }, new[] { 2, 1, 0 } }, // library order wins + + // ordering with ILocalImageProvider + // TODO what is the value of testing for ILocalImageProvider on the sort, should this be removed? Behavior is unintuitive + { 3, new[] { false, true, false }, new[] { 1, 0, 2 }, null, null, new[] { 0, 2, 1 } }, // ILocalImageProvider - sorts to end even when set first + { 3, new[] { false, true, false }, new[] { 1 }, null, null, new[] { 0, 1, 2 } }, // ILocalImageProvider - set order ignored when only value set + { 2, new[] { true, true }, new[] { 1, 0 }, null, null, new[] { 0, 1 } }, // ILocalImageProvider - set order ignored + { 2, new[] { true, true }, null, null, new int?[] { 1, 0 }, new[] { 1, 0 } }, // ILocalImageProvider - IHasOrder applies + }; + + [Theory] + [MemberData(nameof(GetImageProvidersOrderData))] + public void GetImageProviders_ProviderOrder_MatchesExpected(int providerCount, bool[]? localImageProvider, int[]? libraryOrder, int[]? serverOrder, int?[]? hasOrderOrder, int[] expectedOrder) + { + var item = new Movie(); + + var nameProvider = new Func(i => "Provider" + i); + + var providerList = new List(); + for (var i = 0; i < providerCount; i++) + { + var order = hasOrderOrder?[i]; + if (localImageProvider != null && localImageProvider[i]) + { + providerList.Add(MockIImageProvider(nameProvider(i), item, order)); + } + else + { + providerList.Add(MockIImageProvider(nameProvider(i), item, order)); + } + } + + var libraryOptions = new LibraryOptions(); + if (libraryOrder != null) + { + libraryOptions.TypeOptions = new[] + { + new TypeOptions + { + Type = item.GetType().Name, + ImageFetcherOrder = libraryOrder.Select(nameProvider).ToArray() + } + }; + } + + var serverConfiguration = new ServerConfiguration(); + if (serverOrder != null) + { + serverConfiguration.MetadataOptions = new[] + { + new MetadataOptions + { + ItemType = item.GetType().Name, + ImageFetcherOrder = serverOrder.Select(nameProvider).ToArray() + } + }; + } + + var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, libraryOptions: libraryOptions); + AddParts(providerManager, imageProviders: providerList); + + var refreshOptions = new ImageRefreshOptions(Mock.Of(MockBehavior.Strict)); + var actualProviders = providerManager.GetImageProviders(item, refreshOptions).ToList(); + + Assert.Equal(providerList.Count, actualProviders.Count); + for (var i = 0; i < providerList.Count; i++) + { + Assert.Equal(i, actualProviders.IndexOf(providerList[expectedOrder[i]])); + } + } + + private static IImageProvider MockIImageProvider(string name, BaseItem supportedType, int? order = null) + where T : class, IImageProvider + { + Mock? hasOrder = null; + if (order != null) + { + hasOrder = new Mock(MockBehavior.Strict); + hasOrder.Setup(i => i.Order) + .Returns((int)order); + } + + var provider = hasOrder == null + ? new Mock(MockBehavior.Strict) + : hasOrder.As(); + provider.Setup(p => p.Name) + .Returns(name); + provider.Setup(p => p.Supports(supportedType)) + .Returns(true); + return provider.Object; + } + + private static ProviderManager GetProviderManager(ServerConfiguration? serverConfiguration = null, LibraryOptions? libraryOptions = null) + { + var serverConfigurationManager = new Mock(MockBehavior.Strict); + serverConfigurationManager.Setup(i => i.Configuration) + .Returns(serverConfiguration ?? new ServerConfiguration()); + + var libraryManager = new Mock(MockBehavior.Strict); + libraryManager.Setup(i => i.GetLibraryOptions(It.IsAny())) + .Returns(libraryOptions ?? new LibraryOptions()); + + var providerManager = new ProviderManager( + null, + null, + serverConfigurationManager.Object, + null, + new NullLogger(), + null, + null, + libraryManager.Object, + null); + + return providerManager; + } + + private static void AddParts( + ProviderManager providerManager, + IEnumerable? imageProviders = null, + IEnumerable? metadataServices = null, + IEnumerable? metadataProviders = null, + IEnumerable? metadataSavers = null, + IEnumerable? externalIds = null) + { + imageProviders ??= Array.Empty(); + metadataServices ??= Array.Empty(); + metadataProviders ??= Array.Empty(); + metadataSavers ??= Array.Empty(); + externalIds ??= Array.Empty(); + + providerManager.AddParts(imageProviders, metadataServices, metadataProviders, metadataSavers, externalIds); + } + } +} From 8515e8fbd111278cad95430e50904d6721be3a63 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sun, 5 Dec 2021 21:33:31 +0100 Subject: [PATCH 04/19] Improve image provider sorting Remove irrelevant check for ILocalImageProvider Providers that are not IHasOrder default to middle, not beginning --- .../Manager/ProviderManager.cs | 60 ++++++++----------- .../Manager/ProviderManagerTests.cs | 47 +++++---------- 2 files changed, 40 insertions(+), 67 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 1b73574774..855c467208 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -310,31 +310,25 @@ namespace MediaBrowser.Providers.Manager private IEnumerable GetImageProviders(BaseItem item, LibraryOptions libraryOptions, MetadataOptions options, ImageRefreshOptions refreshOptions, bool includeDisabled) { - // Avoid implicitly captured closure - var currentOptions = options; - var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name); - var typeFetcherOrder = typeOptions?.ImageFetcherOrder; + var fetcherOrder = typeOptions?.ImageFetcherOrder ?? options.ImageFetcherOrder; return ImageProviders.Where(i => CanRefresh(i, item, libraryOptions, refreshOptions, includeDisabled)) - .OrderBy(i => - { - // See if there's a user-defined order - if (i is not ILocalImageProvider) - { - var fetcherOrder = typeFetcherOrder ?? currentOptions.ImageFetcherOrder; - var index = Array.IndexOf(fetcherOrder, i.Name); + .OrderBy(i => GetConfiguredOrder(fetcherOrder, i.Name)) + .ThenBy(GetOrder); + } - if (index != -1) - { - return index; - } - } + private static int GetConfiguredOrder(string[] order, string providerName) + { + var index = Array.IndexOf(order, providerName); - // Not configured. Just return some high number to put it at the end. - return 100; - }) - .ThenBy(GetOrder); + if (index != -1) + { + return index; + } + + // default to end + return int.MaxValue; } /// @@ -450,21 +444,6 @@ namespace MediaBrowser.Providers.Manager } } - /// - /// Gets the order. - /// - /// The provider. - /// System.Int32. - private int GetOrder(IImageProvider provider) - { - if (provider is not IHasOrder hasOrder) - { - return 0; - } - - return hasOrder.Order; - } - private int GetConfiguredOrder(BaseItem item, IMetadataProvider provider, LibraryOptions libraryOptions, MetadataOptions globalMetadataOptions) { // See if there's a user-defined order @@ -500,6 +479,17 @@ namespace MediaBrowser.Providers.Manager return 100; } + private static int GetOrder(object provider) + { + if (provider is IHasOrder hasOrder) + { + return hasOrder.Order; + } + + // after items that want to be first (~0) but before items that want to be last (~100) + return 50; + } + private int GetDefaultOrder(IMetadataProvider provider) { if (provider is IHasOrder hasOrder) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 31b1913346..590f50b256 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -16,44 +16,34 @@ namespace Jellyfin.Providers.Tests.Manager { public class ProviderManagerTests { - private static TheoryData GetImageProvidersOrderData() + private static TheoryData GetImageProvidersOrderData() => new () { - { 3, null, null, null, null, new[] { 0, 1, 2 } }, // no order options set + { 3, null, null, null, new[] { 0, 1, 2 } }, // no order options set // library options ordering - { 3, null, Array.Empty(), null, null, new[] { 0, 1, 2 } }, // no order provided - { 3, null, new[] { 1 }, null, null, new[] { 1, 0, 2 } }, // one item in order - { 3, null, new[] { 2, 1, 0 }, null, null, new[] { 2, 1, 0 } }, // full reverse order + { 3, Array.Empty(), null, null, new[] { 0, 1, 2 } }, // no order provided + { 3, new[] { 1 }, null, null, new[] { 1, 0, 2 } }, // one item in order + { 3, new[] { 2, 1, 0 }, null, null, new[] { 2, 1, 0 } }, // full reverse order // server options ordering - { 3, null, null, Array.Empty(), null, new[] { 0, 1, 2 } }, // no order provided - { 3, null, null, new[] { 1 }, null, new[] { 1, 0, 2 } }, // one item in order - { 3, null, null, new[] { 2, 1, 0 }, null, new[] { 2, 1, 0 } }, // full reverse order + { 3, null, Array.Empty(), null, new[] { 0, 1, 2 } }, // no order provided + { 3, null, new[] { 1 }, null, new[] { 1, 0, 2 } }, // one item in order + { 3, null, new[] { 2, 1, 0 }, null, new[] { 2, 1, 0 } }, // full reverse order // IHasOrder ordering - // TODO unintuitive - default if not IHasOrder is 0, not max - { 3, null, null, null, new int?[] { null, 0, null }, new[] { 0, 1, 2 } }, // one item with order 0, no change because default order value is 0 - { 3, null, null, null, new int?[] { null, 1, null }, new[] { 0, 2, 1 } }, // one item in order (goes to end, not beginning) - { 3, null, null, null, new int?[] { 2, 1, 0 }, new[] { 2, 1, 0 } }, // full reverse order + { 3, null, null, new int?[] { null, 1, null }, new[] { 1, 0, 2 } }, // one item with defined order + { 3, null, null, new int?[] { 2, 1, 0 }, new[] { 2, 1, 0 } }, // full reverse order // multiple orders set - // TODO should library fall through to server if both are set on different elements? - { 3, null, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // library order first, server order ignored - { 3, null, new[] { 1 }, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby - { 3, null, new[] { 2, 1, 0 }, new[] { 1, 2, 0 }, new int?[] { 2, 0, 1 }, new[] { 2, 1, 0 } }, // library order wins - - // ordering with ILocalImageProvider - // TODO what is the value of testing for ILocalImageProvider on the sort, should this be removed? Behavior is unintuitive - { 3, new[] { false, true, false }, new[] { 1, 0, 2 }, null, null, new[] { 0, 2, 1 } }, // ILocalImageProvider - sorts to end even when set first - { 3, new[] { false, true, false }, new[] { 1 }, null, null, new[] { 0, 1, 2 } }, // ILocalImageProvider - set order ignored when only value set - { 2, new[] { true, true }, new[] { 1, 0 }, null, null, new[] { 0, 1 } }, // ILocalImageProvider - set order ignored - { 2, new[] { true, true }, null, null, new int?[] { 1, 0 }, new[] { 1, 0 } }, // ILocalImageProvider - IHasOrder applies + { 3, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // library order first, server order ignored + { 3, new[] { 1 }, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby + { 3, new[] { 2, 1, 0 }, new[] { 1, 2, 0 }, new int?[] { 2, 0, 1 }, new[] { 2, 1, 0 } }, // library order wins }; [Theory] [MemberData(nameof(GetImageProvidersOrderData))] - public void GetImageProviders_ProviderOrder_MatchesExpected(int providerCount, bool[]? localImageProvider, int[]? libraryOrder, int[]? serverOrder, int?[]? hasOrderOrder, int[] expectedOrder) + public void GetImageProviders_ProviderOrder_MatchesExpected(int providerCount, int[]? libraryOrder, int[]? serverOrder, int?[]? hasOrderOrder, int[] expectedOrder) { var item = new Movie(); @@ -63,14 +53,7 @@ namespace Jellyfin.Providers.Tests.Manager for (var i = 0; i < providerCount; i++) { var order = hasOrderOrder?[i]; - if (localImageProvider != null && localImageProvider[i]) - { - providerList.Add(MockIImageProvider(nameProvider(i), item, order)); - } - else - { - providerList.Add(MockIImageProvider(nameProvider(i), item, order)); - } + providerList.Add(MockIImageProvider(nameProvider(i), item, order)); } var libraryOptions = new LibraryOptions(); From 6221991c630bcbd688316ad35121781c7a52c591 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sun, 5 Dec 2021 21:43:59 +0100 Subject: [PATCH 05/19] Add nullable annotations --- .../Manager/ProviderManager.cs | 32 ++++++++----------- 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 855c467208..9bae738010 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -1,5 +1,3 @@ -#nullable disable - using System; using System.Collections.Concurrent; using System.Collections.Generic; @@ -47,7 +45,7 @@ namespace MediaBrowser.Providers.Manager /// public class ProviderManager : IProviderManager, IDisposable { - private readonly object _refreshQueueLock = new object(); + private readonly object _refreshQueueLock = new (); private readonly ILogger _logger; private readonly IHttpClientFactory _httpClientFactory; private readonly ILibraryMonitor _libraryMonitor; @@ -57,11 +55,11 @@ namespace MediaBrowser.Providers.Manager private readonly ISubtitleManager _subtitleManager; private readonly IServerConfigurationManager _configurationManager; private readonly IBaseItemManager _baseItemManager; - private readonly ConcurrentDictionary _activeRefreshes = new ConcurrentDictionary(); - private readonly CancellationTokenSource _disposeCancellationTokenSource = new CancellationTokenSource(); - private readonly SimplePriorityQueue> _refreshQueue = - new SimplePriorityQueue>(); + private readonly ConcurrentDictionary _activeRefreshes = new (); + private readonly CancellationTokenSource _disposeCancellationTokenSource = new (); + private readonly SimplePriorityQueue> _refreshQueue = new (); + private IImageProvider[] _imageProviders = Array.Empty(); private IMetadataService[] _metadataServices = Array.Empty(); private IMetadataProvider[] _metadataProviders = Array.Empty(); private IMetadataSaver[] _savers = Array.Empty(); @@ -104,15 +102,13 @@ namespace MediaBrowser.Providers.Manager } /// - public event EventHandler> RefreshStarted; + public event EventHandler>? RefreshStarted; /// - public event EventHandler> RefreshCompleted; + public event EventHandler>? RefreshCompleted; /// - public event EventHandler>> RefreshProgress; - - private IImageProvider[] ImageProviders { get; set; } + public event EventHandler>>? RefreshProgress; /// public void AddParts( @@ -122,8 +118,7 @@ namespace MediaBrowser.Providers.Manager IEnumerable metadataSavers, IEnumerable externalIds) { - ImageProviders = imageProviders.ToArray(); - + _imageProviders = imageProviders.ToArray(); _metadataServices = metadataServices.OrderBy(i => i.Order).ToArray(); _metadataProviders = metadataProviders.ToArray(); _externalIds = externalIds.OrderBy(i => i.ProviderName).ToArray(); @@ -313,7 +308,7 @@ namespace MediaBrowser.Providers.Manager var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name); var fetcherOrder = typeOptions?.ImageFetcherOrder ?? options.ImageFetcherOrder; - return ImageProviders.Where(i => CanRefresh(i, item, libraryOptions, refreshOptions, includeDisabled)) + return _imageProviders.Where(i => CanRefresh(i, item, libraryOptions, refreshOptions, includeDisabled)) .OrderBy(i => GetConfiguredOrder(fetcherOrder, i.Name)) .ThenBy(GetOrder); } @@ -758,7 +753,7 @@ namespace MediaBrowser.Providers.Manager where TItemType : BaseItem, new() where TLookupType : ItemLookupInfo { - BaseItem referenceItem = null; + BaseItem? referenceItem = null; if (!searchInfo.ItemId.Equals(default)) { @@ -768,7 +763,7 @@ namespace MediaBrowser.Providers.Manager return GetRemoteSearchResults(searchInfo, referenceItem, cancellationToken); } - private async Task> GetRemoteSearchResults(RemoteSearchQuery searchInfo, BaseItem referenceItem, CancellationToken cancellationToken) + private async Task> GetRemoteSearchResults(RemoteSearchQuery searchInfo, BaseItem? referenceItem, CancellationToken cancellationToken) where TItemType : BaseItem, new() where TLookupType : ItemLookupInfo { @@ -930,7 +925,8 @@ namespace MediaBrowser.Providers.Manager i.UrlFormatString, value) }; - }).Where(i => i != null).Concat(item.GetRelatedUrls()); + }).Where(i => i != null) + .Concat(item.GetRelatedUrls())!; // We just filtered out all the nulls } /// From 56900d0fc3bc791fd3c0a92bda22ca2f23f28be1 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Mon, 6 Dec 2021 00:09:46 +0100 Subject: [PATCH 06/19] Implement CanRefresh tests for ProviderManager.GetImageProviders --- .../Manager/ProviderManagerTests.cs | 93 +++++++++++++++++-- 1 file changed, 87 insertions(+), 6 deletions(-) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 590f50b256..4a1b90895f 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using MediaBrowser.Controller.BaseItemManager; using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.Entities.Movies; @@ -53,7 +54,7 @@ namespace Jellyfin.Providers.Tests.Manager for (var i = 0; i < providerCount; i++) { var order = hasOrderOrder?[i]; - providerList.Add(MockIImageProvider(nameProvider(i), item, order)); + providerList.Add(MockIImageProvider(nameProvider(i), item, order: order)); } var libraryOptions = new LibraryOptions(); @@ -95,7 +96,78 @@ namespace Jellyfin.Providers.Tests.Manager } } - private static IImageProvider MockIImageProvider(string name, BaseItem supportedType, int? order = null) + [Theory] + [InlineData(true, false, true)] + [InlineData(false, false, false)] + [InlineData(true, true, false)] + public void GetImageProviders_CanRefreshBasic_WhenSupportsWithoutError(bool supports, bool errorOnSupported, bool expected) + { + GetImageProviders_CanRefresh_Tester(typeof(IImageProvider), supports, expected, errorOnSupported: errorOnSupported); + } + + [Theory] + [InlineData(typeof(ILocalImageProvider), false, true)] + [InlineData(typeof(ILocalImageProvider), true, true)] + [InlineData(typeof(IImageProvider), false, false)] + [InlineData(typeof(IImageProvider), true, true)] + public void GetImageProviders_CanRefreshLocked_WhenLocalOrFullRefresh(Type providerType, bool fullRefresh, bool expected) + { + GetImageProviders_CanRefresh_Tester(providerType, true, expected, itemLocked: true, fullRefresh: fullRefresh); + } + + [Theory] + [InlineData(typeof(ILocalImageProvider), false, true)] + [InlineData(typeof(IRemoteImageProvider), true, true)] + [InlineData(typeof(IDynamicImageProvider), true, true)] + [InlineData(typeof(IRemoteImageProvider), false, false)] + [InlineData(typeof(IDynamicImageProvider), false, false)] + public void GetImageProviders_CanRefreshEnabled_WhenLocalOrEnabled(Type providerType, bool enabled, bool expected) + { + GetImageProviders_CanRefresh_Tester(providerType, true, expected, baseItemEnabled: enabled); + } + + private static void GetImageProviders_CanRefresh_Tester(Type providerType, bool supports, bool expected, bool errorOnSupported = false, bool itemLocked = false, bool fullRefresh = false, bool baseItemEnabled = true) + { + var item = new Movie + { + IsLocked = itemLocked + }; + + var providerName = "provider"; + IImageProvider provider = providerType.Name switch + { + "IImageProvider" => MockIImageProvider(providerName, item, supports: supports, errorOnSupported: errorOnSupported), + "ILocalImageProvider" => MockIImageProvider(providerName, item, supports: supports, errorOnSupported: errorOnSupported), + "IRemoteImageProvider" => MockIImageProvider(providerName, item, supports: supports, errorOnSupported: errorOnSupported), + "IDynamicImageProvider" => MockIImageProvider(providerName, item, supports: supports, errorOnSupported: errorOnSupported), + _ => throw new ArgumentException("Unexpected provider type") + }; + + var refreshOptions = new ImageRefreshOptions(Mock.Of(MockBehavior.Strict)) + { + ImageRefreshMode = fullRefresh ? MetadataRefreshMode.FullRefresh : MetadataRefreshMode.Default + }; + + var baseItemManager = new Mock(MockBehavior.Strict); + baseItemManager.Setup(i => i.IsImageFetcherEnabled(item, It.IsAny(), providerName)) + .Returns(baseItemEnabled); + + var providerManager = GetProviderManager(baseItemManager: baseItemManager.Object); + AddParts(providerManager, imageProviders: new[] { provider }); + + var actualProviders = providerManager.GetImageProviders(item, refreshOptions); + + if (expected) + { + Assert.Single(actualProviders); + } + else + { + Assert.Empty(actualProviders); + } + } + + private static IImageProvider MockIImageProvider(string name, BaseItem expectedType, bool supports = true, int? order = null, bool errorOnSupported = false) where T : class, IImageProvider { Mock? hasOrder = null; @@ -111,12 +183,21 @@ namespace Jellyfin.Providers.Tests.Manager : hasOrder.As(); provider.Setup(p => p.Name) .Returns(name); - provider.Setup(p => p.Supports(supportedType)) - .Returns(true); + if (errorOnSupported) + { + provider.Setup(p => p.Supports(It.IsAny())) + .Throws(new ArgumentException()); + } + else + { + provider.Setup(p => p.Supports(expectedType)) + .Returns(supports); + } + return provider.Object; } - private static ProviderManager GetProviderManager(ServerConfiguration? serverConfiguration = null, LibraryOptions? libraryOptions = null) + private static ProviderManager GetProviderManager(ServerConfiguration? serverConfiguration = null, LibraryOptions? libraryOptions = null, IBaseItemManager? baseItemManager = null) { var serverConfigurationManager = new Mock(MockBehavior.Strict); serverConfigurationManager.Setup(i => i.Configuration) @@ -135,7 +216,7 @@ namespace Jellyfin.Providers.Tests.Manager null, null, libraryManager.Object, - null); + baseItemManager); return providerManager; } From 11c7c24f0ef06e6366c075062928976ae0d30600 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Mon, 6 Dec 2021 22:31:16 +0100 Subject: [PATCH 07/19] Clarify naming, minor method ordering improvement --- .../Manager/ProviderManager.cs | 40 +++++++++---------- .../Manager/ProviderManagerTests.cs | 14 +++---- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 9bae738010..633b3b1db7 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -297,18 +297,31 @@ namespace MediaBrowser.Providers.Manager return GetRemoteImageProviders(item, true).Select(i => new ImageProviderInfo(i.Name, i.GetSupportedImages(item).ToArray())); } + private IEnumerable GetRemoteImageProviders(BaseItem item, bool includeDisabled) + { + var options = GetMetadataOptions(item); + var libraryOptions = _libraryManager.GetLibraryOptions(item); + + return GetImageProvidersInternal( + item, + libraryOptions, + options, + new ImageRefreshOptions(new DirectoryService(_fileSystem)), + includeDisabled).OfType(); + } + /// public IEnumerable GetImageProviders(BaseItem item, ImageRefreshOptions refreshOptions) { - return GetImageProviders(item, _libraryManager.GetLibraryOptions(item), GetMetadataOptions(item), refreshOptions, false); + return GetImageProvidersInternal(item, _libraryManager.GetLibraryOptions(item), GetMetadataOptions(item), refreshOptions, false); } - private IEnumerable GetImageProviders(BaseItem item, LibraryOptions libraryOptions, MetadataOptions options, ImageRefreshOptions refreshOptions, bool includeDisabled) + private IEnumerable GetImageProvidersInternal(BaseItem item, LibraryOptions libraryOptions, MetadataOptions options, ImageRefreshOptions refreshOptions, bool includeDisabled) { var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name); var fetcherOrder = typeOptions?.ImageFetcherOrder ?? options.ImageFetcherOrder; - return _imageProviders.Where(i => CanRefresh(i, item, libraryOptions, refreshOptions, includeDisabled)) + return _imageProviders.Where(i => CanRefreshImages(i, item, libraryOptions, refreshOptions, includeDisabled)) .OrderBy(i => GetConfiguredOrder(fetcherOrder, i.Name)) .ThenBy(GetOrder); } @@ -342,25 +355,12 @@ namespace MediaBrowser.Providers.Manager var currentOptions = globalMetadataOptions; return _metadataProviders.OfType>() - .Where(i => CanRefresh(i, item, libraryOptions, includeDisabled, forceEnableInternetMetadata)) + .Where(i => CanRefreshMetadata(i, item, libraryOptions, includeDisabled, forceEnableInternetMetadata)) .OrderBy(i => GetConfiguredOrder(item, i, libraryOptions, currentOptions)) .ThenBy(GetDefaultOrder); } - private IEnumerable GetRemoteImageProviders(BaseItem item, bool includeDisabled) - { - var options = GetMetadataOptions(item); - var libraryOptions = _libraryManager.GetLibraryOptions(item); - - return GetImageProviders( - item, - libraryOptions, - options, - new ImageRefreshOptions(new DirectoryService(_fileSystem)), - includeDisabled).OfType(); - } - - private bool CanRefresh( + private bool CanRefreshMetadata( IMetadataProvider provider, BaseItem item, LibraryOptions libraryOptions, @@ -401,7 +401,7 @@ namespace MediaBrowser.Providers.Manager return true; } - private bool CanRefresh( + private bool CanRefreshImages( IImageProvider provider, BaseItem item, LibraryOptions libraryOptions, @@ -535,7 +535,7 @@ namespace MediaBrowser.Providers.Manager var libraryOptions = new LibraryOptions(); - var imageProviders = GetImageProviders( + var imageProviders = GetImageProvidersInternal( dummy, libraryOptions, options, diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 4a1b90895f..98c1e19b20 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -100,9 +100,9 @@ namespace Jellyfin.Providers.Tests.Manager [InlineData(true, false, true)] [InlineData(false, false, false)] [InlineData(true, true, false)] - public void GetImageProviders_CanRefreshBasic_WhenSupportsWithoutError(bool supports, bool errorOnSupported, bool expected) + public void GetImageProviders_CanRefreshImagesBasic_WhenSupportsWithoutError(bool supports, bool errorOnSupported, bool expected) { - GetImageProviders_CanRefresh_Tester(typeof(IImageProvider), supports, expected, errorOnSupported: errorOnSupported); + GetImageProviders_CanRefreshImages_Tester(typeof(IImageProvider), supports, expected, errorOnSupported: errorOnSupported); } [Theory] @@ -110,9 +110,9 @@ namespace Jellyfin.Providers.Tests.Manager [InlineData(typeof(ILocalImageProvider), true, true)] [InlineData(typeof(IImageProvider), false, false)] [InlineData(typeof(IImageProvider), true, true)] - public void GetImageProviders_CanRefreshLocked_WhenLocalOrFullRefresh(Type providerType, bool fullRefresh, bool expected) + public void GetImageProviders_CanRefreshImagesLocked_WhenLocalOrFullRefresh(Type providerType, bool fullRefresh, bool expected) { - GetImageProviders_CanRefresh_Tester(providerType, true, expected, itemLocked: true, fullRefresh: fullRefresh); + GetImageProviders_CanRefreshImages_Tester(providerType, true, expected, itemLocked: true, fullRefresh: fullRefresh); } [Theory] @@ -121,12 +121,12 @@ namespace Jellyfin.Providers.Tests.Manager [InlineData(typeof(IDynamicImageProvider), true, true)] [InlineData(typeof(IRemoteImageProvider), false, false)] [InlineData(typeof(IDynamicImageProvider), false, false)] - public void GetImageProviders_CanRefreshEnabled_WhenLocalOrEnabled(Type providerType, bool enabled, bool expected) + public void GetImageProviders_CanRefreshImagesEnabled_WhenLocalOrEnabled(Type providerType, bool enabled, bool expected) { - GetImageProviders_CanRefresh_Tester(providerType, true, expected, baseItemEnabled: enabled); + GetImageProviders_CanRefreshImages_Tester(providerType, true, expected, baseItemEnabled: enabled); } - private static void GetImageProviders_CanRefresh_Tester(Type providerType, bool supports, bool expected, bool errorOnSupported = false, bool itemLocked = false, bool fullRefresh = false, bool baseItemEnabled = true) + private static void GetImageProviders_CanRefreshImages_Tester(Type providerType, bool supports, bool expected, bool errorOnSupported = false, bool itemLocked = false, bool fullRefresh = false, bool baseItemEnabled = true) { var item = new Movie { From 91e706d3873440a28f107da04143a374d4277b9a Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Wed, 8 Dec 2021 00:52:57 +0100 Subject: [PATCH 08/19] Implement sort test for ProviderManager.GetMetadataProviders --- .../Manager/ProviderManagerTests.cs | 186 +++++++++++++++++- 1 file changed, 177 insertions(+), 9 deletions(-) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 98c1e19b20..7a542f0eba 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -37,7 +37,7 @@ namespace Jellyfin.Providers.Tests.Manager { 3, null, null, new int?[] { 2, 1, 0 }, new[] { 2, 1, 0 } }, // full reverse order // multiple orders set - { 3, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // library order first, server order ignored + { 3, new[] { 1 }, new[] { 2, 0, 1 }, null, new[] { 1, 0, 2 } }, // partial library order first, server order ignored { 3, new[] { 1 }, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby { 3, new[] { 2, 1, 0 }, new[] { 1, 2, 0 }, new int?[] { 2, 0, 1 }, new[] { 2, 1, 0 } }, // library order wins }; @@ -90,10 +90,8 @@ namespace Jellyfin.Providers.Tests.Manager var actualProviders = providerManager.GetImageProviders(item, refreshOptions).ToList(); Assert.Equal(providerList.Count, actualProviders.Count); - for (var i = 0; i < providerList.Count; i++) - { - Assert.Equal(i, actualProviders.IndexOf(providerList[expectedOrder[i]])); - } + var actualOrder = actualProviders.Select(i => providerList.IndexOf(i)).ToArray(); + Assert.Equal(expectedOrder, actualOrder); } [Theory] @@ -167,8 +165,129 @@ namespace Jellyfin.Providers.Tests.Manager } } - private static IImageProvider MockIImageProvider(string name, BaseItem expectedType, bool supports = true, int? order = null, bool errorOnSupported = false) - where T : class, IImageProvider + private static TheoryData GetMetadataProvidersOrderData() + { + var l = "local"; + var r = "remote"; + return new () + { + { new[] { l, l, r, r }, null, null, null, null, null, new[] { 0, 1, 2, 3 } }, // no order options set + + // library options ordering + { new[] { l, l, r, r }, Array.Empty(), Array.Empty(), null, null, null, new[] { 0, 1, 2, 3 } }, // no order provided + // local only + { new[] { r, l, l, l }, new[] { 2 }, null, null, null, null, new[] { 2, 0, 1, 3 } }, // one item in order + { new[] { r, l, l, l }, new[] { 3, 2, 1 }, null, null, null, null, new[] { 3, 2, 1, 0 } }, // full reverse order + // remote only + { new[] { l, r, r, r }, null, new[] { 2 }, null, null, null, new[] { 2, 0, 1, 3 } }, // one item in order + { new[] { l, r, r, r }, null, new[] { 3, 2, 1 }, null, null, null, new[] { 3, 2, 1, 0 } }, // full reverse order + // local and remote, note that results will be interleaved (odd but expected) + { new[] { l, l, r, r }, new[] { 1 }, new[] { 3 }, null, null, null, new[] { 1, 3, 0, 2 } }, // one item in each order + { new[] { l, l, l, r, r, r }, new[] { 2, 1, 0 }, new[] { 5, 4, 3 }, null, null, null, new[] { 2, 5, 1, 4, 0, 3 } }, // full reverse order + + // // server options ordering + { new[] { l, l, r, r }, null, null, Array.Empty(), Array.Empty(), null, new[] { 0, 1, 2, 3 } }, // no order provided + // local only + { new[] { r, l, l, l }, null, null, new[] { 2 }, null, null, new[] { 2, 0, 1, 3 } }, // one item in order + { new[] { r, l, l, l }, null, null, new[] { 3, 2, 1 }, null, null, new[] { 3, 2, 1, 0 } }, // full reverse order + // remote only + { new[] { l, r, r, r }, null, null, null, new[] { 2 }, null, new[] { 2, 0, 1, 3 } }, // one item in order + { new[] { l, r, r, r }, null, null, null, new[] { 3, 2, 1 }, null, new[] { 3, 2, 1, 0 } }, // full reverse order + // local and remote, note that results will be interleaved (odd but expected) + { new[] { l, l, r, r }, null, null, new[] { 1 }, new[] { 3 }, null, new[] { 1, 3, 0, 2 } }, // one item in each order + { new[] { l, l, l, r, r, r }, null, null, new[] { 2, 1, 0 }, new[] { 5, 4, 3 }, null, new[] { 2, 5, 1, 4, 0, 3 } }, // full reverse order + + // IHasOrder ordering (not interleaved, doesn't care about types) + // TODO unset goes to beginning, not end + { new[] { l, l, r, r }, null, null, null, null, new int?[] { 2, null, 1, null }, new[] { 1, 3, 2, 0 } }, // partially defined + { new[] { l, l, r, r }, null, null, null, null, new int?[] { 3, 2, 1, 0 }, new[] { 3, 2, 1, 0 } }, // full reverse order + // note odd interaction - orderby determines order of slot when local and remote both have a slot 0 + { new[] { l, l, r, r }, new[] { 1 }, new[] { 3 }, null, null, new int?[] { null, 2, null, 1 }, new[] { 3, 1, 0, 2 } }, // sorts interleaved results + + // multiple orders set + { new[] { l, l, l, r, r, r }, new[] { 1 }, new[] { 4 }, new[] { 2, 1, 0 }, new[] { 5, 4, 3 }, null, new[] { 1, 4, 0, 2, 3, 5 } }, // partial library order first, server order ignored + { new[] { l, l, l }, new[] { 1 }, null, null, null, new int?[] { 2, 0, 1 }, new[] { 1, 2, 0 } }, // library order first, then orderby + { new[] { l, l, l, r, r, r }, new[] { 2, 1, 0 }, new[] { 5, 4, 3 }, new[] { 1, 2, 0 }, new[] { 4, 5, 3 }, new int?[] { 5, 4, 1, 6, 3, 2 }, new[] { 2, 5, 4, 1, 0, 3 } }, // library order wins (with orderby between local/remote) + }; + } + + [Theory] + [MemberData(nameof(GetMetadataProvidersOrderData))] + public void GetMetadataProviders_ProviderOrder_MatchesExpected(string[] providers, int[]? libraryLocalOrder, int[]? libraryRemoteOrder, int[]? serverLocalOrder, int[]? serverRemoteOrder, int?[]? hasOrderOrder, int[] expectedOrder) + { + var item = new MetadataTestItem(); + var typeNames = new Dictionary + { + { "remote", nameof(IRemoteMetadataProvider) }, + { "local", nameof(ILocalMetadataProvider) }, + { "custom", nameof(ICustomMetadataProvider) } + }; + + var nameProvider = new Func(i => "Provider" + i); + + var providerList = new List>(); + for (var i = 0; i < providers.Length; i++) + { + var order = hasOrderOrder?[i]; + providerList.Add(MockIMetadataProviderMapper(typeNames[providers[i]], nameProvider(i), order: order)); + } + + var libraryOptions = new LibraryOptions(); + if (libraryLocalOrder != null) + { + libraryOptions.LocalMetadataReaderOrder = libraryLocalOrder.Select(nameProvider).ToArray(); + } + + if (libraryRemoteOrder != null) + { + libraryOptions.TypeOptions = new[] + { + new TypeOptions + { + Type = item.GetType().Name, + MetadataFetcherOrder = libraryRemoteOrder.Select(nameProvider).ToArray() + } + }; + } + + var serverConfiguration = new ServerConfiguration(); + if (serverLocalOrder != null || serverRemoteOrder != null) + { + serverConfiguration.MetadataOptions = new[] + { + new MetadataOptions + { + ItemType = item.GetType().Name + } + }; + if (serverLocalOrder != null) + { + serverConfiguration.MetadataOptions[0].LocalMetadataReaderOrder = serverLocalOrder.Select(nameProvider).ToArray(); + } + + if (serverRemoteOrder != null) + { + serverConfiguration.MetadataOptions[0].MetadataFetcherOrder = serverRemoteOrder.Select(nameProvider).ToArray(); + } + } + + var baseItemManager = new Mock(MockBehavior.Strict); + baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), It.IsAny())) + .Returns(true); + + var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, baseItemManager: baseItemManager.Object); + AddParts(providerManager, metadataProviders: providerList); + + // TODO why does this take libraryOptions directly while GetImageProviders did not? + var actualProviders = providerManager.GetMetadataProviders(item, libraryOptions).ToList(); + + Assert.Equal(providerList.Count, actualProviders.Count); + var actualOrder = actualProviders.Select(i => providerList.IndexOf(i)).ToArray(); + Assert.Equal(expectedOrder, actualOrder); + } + + private static IImageProvider MockIImageProvider(string name, BaseItem expectedType, bool supports = true, int? order = null, bool errorOnSupported = false) + where TProviderType : class, IImageProvider { Mock? hasOrder = null; if (order != null) @@ -179,8 +298,8 @@ namespace Jellyfin.Providers.Tests.Manager } var provider = hasOrder == null - ? new Mock(MockBehavior.Strict) - : hasOrder.As(); + ? new Mock(MockBehavior.Strict) + : hasOrder.As(); provider.Setup(p => p.Name) .Returns(name); if (errorOnSupported) @@ -197,6 +316,38 @@ namespace Jellyfin.Providers.Tests.Manager return provider.Object; } + private static IMetadataProvider MockIMetadataProviderMapper(string typeName, string providerName, int? order = null) + where TItemType : BaseItem, IHasLookupInfo + where TLookupInfoType : ItemLookupInfo, new() + => typeName switch + { + "ILocalMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order), + "IRemoteMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order), + "ICustomMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order), + _ => MockIMetadataProvider, TItemType>(providerName, order) + }; + + private static IMetadataProvider MockIMetadataProvider(string name, int? order = null) + where TProviderType : class, IMetadataProvider + where TItemType : BaseItem + { + Mock? hasOrder = null; + if (order != null) + { + hasOrder = new Mock(MockBehavior.Strict); + hasOrder.Setup(i => i.Order) + .Returns((int)order); + } + + var provider = hasOrder == null + ? new Mock(MockBehavior.Strict) + : hasOrder.As(); + provider.Setup(p => p.Name) + .Returns(name); + + return provider.Object; + } + private static ProviderManager GetProviderManager(ServerConfiguration? serverConfiguration = null, LibraryOptions? libraryOptions = null, IBaseItemManager? baseItemManager = null) { var serverConfigurationManager = new Mock(MockBehavior.Strict); @@ -237,5 +388,22 @@ namespace Jellyfin.Providers.Tests.Manager providerManager.AddParts(imageProviders, metadataServices, metadataProviders, metadataSavers, externalIds); } + + /// + /// Simple extension to force SupportsLocalMetadata to true. + /// + public class MetadataTestItem : BaseItem, IHasLookupInfo + { + public override bool SupportsLocalMetadata => true; + + public MetadataTestItemInfo GetLookupInfo() + { + return GetItemLookupInfo(); + } + } + + public class MetadataTestItemInfo : ItemLookupInfo + { + } } } From e7df72de497f25deb7f77bf9de39aeaba1159d11 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Wed, 8 Dec 2021 16:49:09 +0100 Subject: [PATCH 09/19] Improve metadata provider sorting Extract configured order up front instead of for each provider Non-IHasOrder providers default to middle, not beginning Merge image and metadata sort helper methods --- .../Manager/ProviderManager.cs | 80 ++++++------------- .../Manager/ProviderManagerTests.cs | 16 +--- 2 files changed, 27 insertions(+), 69 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 633b3b1db7..82d633e234 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -323,20 +323,7 @@ namespace MediaBrowser.Providers.Manager return _imageProviders.Where(i => CanRefreshImages(i, item, libraryOptions, refreshOptions, includeDisabled)) .OrderBy(i => GetConfiguredOrder(fetcherOrder, i.Name)) - .ThenBy(GetOrder); - } - - private static int GetConfiguredOrder(string[] order, string providerName) - { - var index = Array.IndexOf(order, providerName); - - if (index != -1) - { - return index; - } - - // default to end - return int.MaxValue; + .ThenBy(GetDefaultOrder); } /// @@ -351,12 +338,23 @@ namespace MediaBrowser.Providers.Manager private IEnumerable> GetMetadataProvidersInternal(BaseItem item, LibraryOptions libraryOptions, MetadataOptions globalMetadataOptions, bool includeDisabled, bool forceEnableInternetMetadata) where T : BaseItem { - // Avoid implicitly captured closure - var currentOptions = globalMetadataOptions; + var localMetadataReaderOrder = libraryOptions.LocalMetadataReaderOrder ?? globalMetadataOptions.LocalMetadataReaderOrder; + var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name); + var metadataFetcherOrder = typeOptions?.MetadataFetcherOrder ?? globalMetadataOptions.MetadataFetcherOrder; return _metadataProviders.OfType>() .Where(i => CanRefreshMetadata(i, item, libraryOptions, includeDisabled, forceEnableInternetMetadata)) - .OrderBy(i => GetConfiguredOrder(item, i, libraryOptions, currentOptions)) + .OrderBy(i => + { + // local and remote providers will be interleaved in the final order + // only relative order within a type matters: consumers of the list filter to one or the other + switch (i) + { + case ILocalMetadataProvider: return GetConfiguredOrder(localMetadataReaderOrder, i.Name); + case IRemoteMetadataProvider: return GetConfiguredOrder(metadataFetcherOrder, i.Name); + default: return int.MaxValue; // default to end + } + }) .ThenBy(GetDefaultOrder); } @@ -439,42 +437,20 @@ namespace MediaBrowser.Providers.Manager } } - private int GetConfiguredOrder(BaseItem item, IMetadataProvider provider, LibraryOptions libraryOptions, MetadataOptions globalMetadataOptions) + private static int GetConfiguredOrder(string[] order, string providerName) { - // See if there's a user-defined order - if (provider is ILocalMetadataProvider) + var index = Array.IndexOf(order, providerName); + + if (index != -1) { - var configuredOrder = libraryOptions.LocalMetadataReaderOrder ?? globalMetadataOptions.LocalMetadataReaderOrder; - - var index = Array.IndexOf(configuredOrder, provider.Name); - - if (index != -1) - { - return index; - } + return index; } - // See if there's a user-defined order - if (provider is IRemoteMetadataProvider) - { - var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name); - var typeFetcherOrder = typeOptions?.MetadataFetcherOrder; - - var fetcherOrder = typeFetcherOrder ?? globalMetadataOptions.MetadataFetcherOrder; - - var index = Array.IndexOf(fetcherOrder, provider.Name); - - if (index != -1) - { - return index; - } - } - - // Not configured. Just return some high number to put it at the end. - return 100; + // default to end + return int.MaxValue; } - private static int GetOrder(object provider) + private static int GetDefaultOrder(object provider) { if (provider is IHasOrder hasOrder) { @@ -485,16 +461,6 @@ namespace MediaBrowser.Providers.Manager return 50; } - private int GetDefaultOrder(IMetadataProvider provider) - { - if (provider is IHasOrder hasOrder) - { - return hasOrder.Order; - } - - return 0; - } - /// public MetadataPluginSummary[] GetAllMetadataPlugins() { diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 7a542f0eba..d59e4070f9 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -167,8 +167,8 @@ namespace Jellyfin.Providers.Tests.Manager private static TheoryData GetMetadataProvidersOrderData() { - var l = "local"; - var r = "remote"; + var l = nameof(ILocalMetadataProvider); + var r = nameof(IRemoteMetadataProvider); return new () { { new[] { l, l, r, r }, null, null, null, null, null, new[] { 0, 1, 2, 3 } }, // no order options set @@ -198,8 +198,7 @@ namespace Jellyfin.Providers.Tests.Manager { new[] { l, l, l, r, r, r }, null, null, new[] { 2, 1, 0 }, new[] { 5, 4, 3 }, null, new[] { 2, 5, 1, 4, 0, 3 } }, // full reverse order // IHasOrder ordering (not interleaved, doesn't care about types) - // TODO unset goes to beginning, not end - { new[] { l, l, r, r }, null, null, null, null, new int?[] { 2, null, 1, null }, new[] { 1, 3, 2, 0 } }, // partially defined + { new[] { l, l, r, r }, null, null, null, null, new int?[] { 2, null, 1, null }, new[] { 2, 0, 1, 3 } }, // partially defined { new[] { l, l, r, r }, null, null, null, null, new int?[] { 3, 2, 1, 0 }, new[] { 3, 2, 1, 0 } }, // full reverse order // note odd interaction - orderby determines order of slot when local and remote both have a slot 0 { new[] { l, l, r, r }, new[] { 1 }, new[] { 3 }, null, null, new int?[] { null, 2, null, 1 }, new[] { 3, 1, 0, 2 } }, // sorts interleaved results @@ -216,12 +215,6 @@ namespace Jellyfin.Providers.Tests.Manager public void GetMetadataProviders_ProviderOrder_MatchesExpected(string[] providers, int[]? libraryLocalOrder, int[]? libraryRemoteOrder, int[]? serverLocalOrder, int[]? serverRemoteOrder, int?[]? hasOrderOrder, int[] expectedOrder) { var item = new MetadataTestItem(); - var typeNames = new Dictionary - { - { "remote", nameof(IRemoteMetadataProvider) }, - { "local", nameof(ILocalMetadataProvider) }, - { "custom", nameof(ICustomMetadataProvider) } - }; var nameProvider = new Func(i => "Provider" + i); @@ -229,7 +222,7 @@ namespace Jellyfin.Providers.Tests.Manager for (var i = 0; i < providers.Length; i++) { var order = hasOrderOrder?[i]; - providerList.Add(MockIMetadataProviderMapper(typeNames[providers[i]], nameProvider(i), order: order)); + providerList.Add(MockIMetadataProviderMapper(providers[i], nameProvider(i), order: order)); } var libraryOptions = new LibraryOptions(); @@ -278,7 +271,6 @@ namespace Jellyfin.Providers.Tests.Manager var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, baseItemManager: baseItemManager.Object); AddParts(providerManager, metadataProviders: providerList); - // TODO why does this take libraryOptions directly while GetImageProviders did not? var actualProviders = providerManager.GetMetadataProviders(item, libraryOptions).ToList(); Assert.Equal(providerList.Count, actualProviders.Count); From d5e2c2fb5e8a1328a2e938a7100a1ceb29b28fa7 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Fri, 10 Dec 2021 00:38:41 +0100 Subject: [PATCH 10/19] Implement CanRefreshMetadata tests for GetMetadataProviders Cleanup tests, extract common blocks --- .../Manager/ProviderManagerTests.cs | 285 ++++++++++++------ 1 file changed, 196 insertions(+), 89 deletions(-) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index d59e4070f9..ba91f5ed2d 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -57,33 +57,10 @@ namespace Jellyfin.Providers.Tests.Manager providerList.Add(MockIImageProvider(nameProvider(i), item, order: order)); } - var libraryOptions = new LibraryOptions(); - if (libraryOrder != null) - { - libraryOptions.TypeOptions = new[] - { - new TypeOptions - { - Type = item.GetType().Name, - ImageFetcherOrder = libraryOrder.Select(nameProvider).ToArray() - } - }; - } + var libraryOptions = CreateLibraryOptions(item.GetType().Name, imageFetcherOrder: libraryOrder?.Select(nameProvider).ToArray()); + var serverConfiguration = CreateServerConfiguration(item.GetType().Name, imageFetcherOrder: serverOrder?.Select(nameProvider).ToArray()); - var serverConfiguration = new ServerConfiguration(); - if (serverOrder != null) - { - serverConfiguration.MetadataOptions = new[] - { - new MetadataOptions - { - ItemType = item.GetType().Name, - ImageFetcherOrder = serverOrder.Select(nameProvider).ToArray() - } - }; - } - - var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, libraryOptions: libraryOptions); + using var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, libraryOptions: libraryOptions); AddParts(providerManager, imageProviders: providerList); var refreshOptions = new ImageRefreshOptions(Mock.Of(MockBehavior.Strict)); @@ -119,12 +96,19 @@ namespace Jellyfin.Providers.Tests.Manager [InlineData(typeof(IDynamicImageProvider), true, true)] [InlineData(typeof(IRemoteImageProvider), false, false)] [InlineData(typeof(IDynamicImageProvider), false, false)] - public void GetImageProviders_CanRefreshImagesEnabled_WhenLocalOrEnabled(Type providerType, bool enabled, bool expected) + public void GetImageProviders_CanRefreshImagesBaseItemEnabled_WhenLocalOrEnabled(Type providerType, bool enabled, bool expected) { GetImageProviders_CanRefreshImages_Tester(providerType, true, expected, baseItemEnabled: enabled); } - private static void GetImageProviders_CanRefreshImages_Tester(Type providerType, bool supports, bool expected, bool errorOnSupported = false, bool itemLocked = false, bool fullRefresh = false, bool baseItemEnabled = true) + private static void GetImageProviders_CanRefreshImages_Tester( + Type providerType, + bool supports, + bool expected, + bool errorOnSupported = false, + bool itemLocked = false, + bool fullRefresh = false, + bool baseItemEnabled = true) { var item = new Movie { @@ -150,19 +134,12 @@ namespace Jellyfin.Providers.Tests.Manager baseItemManager.Setup(i => i.IsImageFetcherEnabled(item, It.IsAny(), providerName)) .Returns(baseItemEnabled); - var providerManager = GetProviderManager(baseItemManager: baseItemManager.Object); + using var providerManager = GetProviderManager(baseItemManager: baseItemManager.Object); AddParts(providerManager, imageProviders: new[] { provider }); - var actualProviders = providerManager.GetImageProviders(item, refreshOptions); + var actualProviders = providerManager.GetImageProviders(item, refreshOptions).ToArray(); - if (expected) - { - Assert.Single(actualProviders); - } - else - { - Assert.Empty(actualProviders); - } + Assert.Equal(expected ? 1 : 0, actualProviders.Length); } private static TheoryData GetMetadataProvidersOrderData() @@ -212,7 +189,14 @@ namespace Jellyfin.Providers.Tests.Manager [Theory] [MemberData(nameof(GetMetadataProvidersOrderData))] - public void GetMetadataProviders_ProviderOrder_MatchesExpected(string[] providers, int[]? libraryLocalOrder, int[]? libraryRemoteOrder, int[]? serverLocalOrder, int[]? serverRemoteOrder, int?[]? hasOrderOrder, int[] expectedOrder) + public void GetMetadataProviders_ProviderOrder_MatchesExpected( + string[] providers, + int[]? libraryLocalOrder, + int[]? libraryRemoteOrder, + int[]? serverLocalOrder, + int[]? serverRemoteOrder, + int?[]? hasOrderOrder, + int[] expectedOrder) { var item = new MetadataTestItem(); @@ -225,50 +209,20 @@ namespace Jellyfin.Providers.Tests.Manager providerList.Add(MockIMetadataProviderMapper(providers[i], nameProvider(i), order: order)); } - var libraryOptions = new LibraryOptions(); - if (libraryLocalOrder != null) - { - libraryOptions.LocalMetadataReaderOrder = libraryLocalOrder.Select(nameProvider).ToArray(); - } - - if (libraryRemoteOrder != null) - { - libraryOptions.TypeOptions = new[] - { - new TypeOptions - { - Type = item.GetType().Name, - MetadataFetcherOrder = libraryRemoteOrder.Select(nameProvider).ToArray() - } - }; - } - - var serverConfiguration = new ServerConfiguration(); - if (serverLocalOrder != null || serverRemoteOrder != null) - { - serverConfiguration.MetadataOptions = new[] - { - new MetadataOptions - { - ItemType = item.GetType().Name - } - }; - if (serverLocalOrder != null) - { - serverConfiguration.MetadataOptions[0].LocalMetadataReaderOrder = serverLocalOrder.Select(nameProvider).ToArray(); - } - - if (serverRemoteOrder != null) - { - serverConfiguration.MetadataOptions[0].MetadataFetcherOrder = serverRemoteOrder.Select(nameProvider).ToArray(); - } - } + var libraryOptions = CreateLibraryOptions( + item.GetType().Name, + localMetadataReaderOrder: libraryLocalOrder?.Select(nameProvider).ToArray(), + metadataFetcherOrder: libraryRemoteOrder?.Select(nameProvider).ToArray()); + var serverConfiguration = CreateServerConfiguration( + item.GetType().Name, + localMetadataReaderOrder: serverLocalOrder?.Select(nameProvider).ToArray(), + metadataFetcherOrder: serverRemoteOrder?.Select(nameProvider).ToArray()); var baseItemManager = new Mock(MockBehavior.Strict); baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), It.IsAny())) .Returns(true); - var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, baseItemManager: baseItemManager.Object); + using var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, baseItemManager: baseItemManager.Object); AddParts(providerManager, metadataProviders: providerList); var actualProviders = providerManager.GetMetadataProviders(item, libraryOptions).ToList(); @@ -278,6 +232,87 @@ namespace Jellyfin.Providers.Tests.Manager Assert.Equal(expectedOrder, actualOrder); } + [Theory] + [InlineData(typeof(IMetadataProvider))] + [InlineData(typeof(ILocalMetadataProvider))] + [InlineData(typeof(IRemoteMetadataProvider))] + [InlineData(typeof(ICustomMetadataProvider))] + public void GetMetadataProviders_CanRefreshMetadataBasic_ReturnsTrue(Type providerType) + { + GetMetadataProviders_CanRefreshMetadata_Tester(providerType, true); + } + + [Theory] + [InlineData(typeof(ILocalMetadataProvider), false, true)] + [InlineData(typeof(IRemoteMetadataProvider), false, false)] + [InlineData(typeof(ICustomMetadataProvider), false, false)] + [InlineData(typeof(ILocalMetadataProvider), true, true)] + [InlineData(typeof(ICustomMetadataProvider), true, false)] + public void GetMetadataProviders_CanRefreshMetadataLocked_WhenLocalOrForced(Type providerType, bool forced, bool expected) + { + GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, itemLocked: true, providerForced: forced); + } + + [Theory] + [InlineData(typeof(ILocalMetadataProvider), false, true)] + [InlineData(typeof(ICustomMetadataProvider), false, true)] + [InlineData(typeof(IRemoteMetadataProvider), false, false)] + [InlineData(typeof(IRemoteMetadataProvider), true, true)] + public void GetMetadataProviders_CanRefreshMetadataBaseItemEnabled_WhenEnabledOrNotRemote(Type providerType, bool baseItemEnabled, bool expected) + { + GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, baseItemEnabled: baseItemEnabled); + } + + [Theory] + [InlineData(typeof(IRemoteMetadataProvider), false, true)] + [InlineData(typeof(ICustomMetadataProvider), false, true)] + [InlineData(typeof(ILocalMetadataProvider), false, false)] + [InlineData(typeof(ILocalMetadataProvider), true, true)] + public void GetMetadataProviders_CanRefreshMetadataSupportsLocal_WhenSupportsOrNotLocal(Type providerType, bool supportsLocalMetadata, bool expected) + { + GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, supportsLocalMetadata: supportsLocalMetadata); + } + + [Theory] + [InlineData(typeof(ICustomMetadataProvider), true)] + [InlineData(typeof(IRemoteMetadataProvider), false)] + [InlineData(typeof(ILocalMetadataProvider), false)] + public void GetMetadataProviders_CanRefreshMetadataOwned_WhenNotLocal(Type providerType, bool expected) + { + GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, ownedItem: true); + } + + private static void GetMetadataProviders_CanRefreshMetadata_Tester( + Type providerType, + bool expected, + bool itemLocked = false, + bool baseItemEnabled = true, + bool providerForced = false, + bool supportsLocalMetadata = true, + bool ownedItem = false) + { + var item = new MetadataTestItem + { + IsLocked = itemLocked, + OwnerId = ownedItem ? Guid.NewGuid() : Guid.Empty, + EnableLocalMetadata = supportsLocalMetadata + }; + + var providerName = "provider"; + var provider = MockIMetadataProviderMapper(providerType.Name, providerName, forced: providerForced); + + var baseItemManager = new Mock(MockBehavior.Strict); + baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), providerName)) + .Returns(baseItemEnabled); + + using var providerManager = GetProviderManager(baseItemManager: baseItemManager.Object); + AddParts(providerManager, metadataProviders: new[] { provider }); + + var actualProviders = providerManager.GetMetadataProviders(item, new LibraryOptions()).ToArray(); + + Assert.Equal(expected ? 1 : 0, actualProviders.Length); + } + private static IImageProvider MockIImageProvider(string name, BaseItem expectedType, bool supports = true, int? order = null, bool errorOnSupported = false) where TProviderType : class, IImageProvider { @@ -297,7 +332,7 @@ namespace Jellyfin.Providers.Tests.Manager if (errorOnSupported) { provider.Setup(p => p.Supports(It.IsAny())) - .Throws(new ArgumentException()); + .Throws(new ArgumentException("Provider threw exception on Supports(item)")); } else { @@ -308,25 +343,31 @@ namespace Jellyfin.Providers.Tests.Manager return provider.Object; } - private static IMetadataProvider MockIMetadataProviderMapper(string typeName, string providerName, int? order = null) + private static IMetadataProvider MockIMetadataProviderMapper(string typeName, string providerName, int? order = null, bool forced = false) where TItemType : BaseItem, IHasLookupInfo where TLookupInfoType : ItemLookupInfo, new() => typeName switch { - "ILocalMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order), - "IRemoteMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order), - "ICustomMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order), - _ => MockIMetadataProvider, TItemType>(providerName, order) + "ILocalMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order, forced), + "IRemoteMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order, forced), + "ICustomMetadataProvider" => MockIMetadataProvider, TItemType>(providerName, order, forced), + _ => MockIMetadataProvider, TItemType>(providerName, order, forced) }; - private static IMetadataProvider MockIMetadataProvider(string name, int? order = null) + private static IMetadataProvider MockIMetadataProvider(string name, int? order = null, bool forced = false) where TProviderType : class, IMetadataProvider where TItemType : BaseItem { + Mock? forcedProvider = null; + if (forced) + { + forcedProvider = new Mock(); + } + Mock? hasOrder = null; if (order != null) { - hasOrder = new Mock(MockBehavior.Strict); + hasOrder = forcedProvider == null ? new Mock() : forcedProvider.As(); hasOrder.Setup(i => i.Order) .Returns((int)order); } @@ -340,7 +381,71 @@ namespace Jellyfin.Providers.Tests.Manager return provider.Object; } - private static ProviderManager GetProviderManager(ServerConfiguration? serverConfiguration = null, LibraryOptions? libraryOptions = null, IBaseItemManager? baseItemManager = null) + private static LibraryOptions CreateLibraryOptions( + string typeName, + string[]? imageFetcherOrder = null, + string[]? localMetadataReaderOrder = null, + string[]? metadataFetcherOrder = null) + { + var libraryOptions = new LibraryOptions + { + LocalMetadataReaderOrder = localMetadataReaderOrder + }; + + // only create type options if populating it with something + if (imageFetcherOrder != null || metadataFetcherOrder != null) + { + imageFetcherOrder ??= Array.Empty(); + metadataFetcherOrder ??= Array.Empty(); + + libraryOptions.TypeOptions = new[] + { + new TypeOptions + { + Type = typeName, + ImageFetcherOrder = imageFetcherOrder, + MetadataFetcherOrder = metadataFetcherOrder + } + }; + } + + return libraryOptions; + } + + private static ServerConfiguration CreateServerConfiguration( + string typeName, + string[]? imageFetcherOrder = null, + string[]? localMetadataReaderOrder = null, + string[]? metadataFetcherOrder = null) + { + var serverConfiguration = new ServerConfiguration(); + + // only create type options if populating it with something + if (imageFetcherOrder != null || localMetadataReaderOrder != null || metadataFetcherOrder != null) + { + imageFetcherOrder ??= Array.Empty(); + localMetadataReaderOrder ??= Array.Empty(); + metadataFetcherOrder ??= Array.Empty(); + + serverConfiguration.MetadataOptions = new[] + { + new MetadataOptions + { + ItemType = typeName, + ImageFetcherOrder = imageFetcherOrder, + LocalMetadataReaderOrder = localMetadataReaderOrder, + MetadataFetcherOrder = metadataFetcherOrder + } + }; + } + + return serverConfiguration; + } + + private static ProviderManager GetProviderManager( + ServerConfiguration? serverConfiguration = null, + LibraryOptions? libraryOptions = null, + IBaseItemManager? baseItemManager = null) { var serverConfigurationManager = new Mock(MockBehavior.Strict); serverConfigurationManager.Setup(i => i.Configuration) @@ -382,11 +487,13 @@ namespace Jellyfin.Providers.Tests.Manager } /// - /// Simple extension to force SupportsLocalMetadata to true. + /// Simple extension to make SupportsLocalMetadata directly settable. /// public class MetadataTestItem : BaseItem, IHasLookupInfo { - public override bool SupportsLocalMetadata => true; + public bool EnableLocalMetadata { get; set; } = true; + + public override bool SupportsLocalMetadata => EnableLocalMetadata; public MetadataTestItemInfo GetLookupInfo() { From a7c009e2eb3e21b7b5c07984866419bb8136423f Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sat, 18 Dec 2021 21:40:27 +0100 Subject: [PATCH 11/19] Pass TypeOptions instead of full LibraryOptions --- .../BaseItemManager/BaseItemManager.cs | 14 ++++---- .../BaseItemManager/IBaseItemManager.cs | 8 ++--- .../Manager/ProviderManager.cs | 12 +++---- .../BaseItemManagerTests.cs | 32 +++++++------------ .../Manager/ProviderManagerTests.cs | 6 ++-- 5 files changed, 31 insertions(+), 41 deletions(-) diff --git a/MediaBrowser.Controller/BaseItemManager/BaseItemManager.cs b/MediaBrowser.Controller/BaseItemManager/BaseItemManager.cs index d273b54fc6..61539cae56 100644 --- a/MediaBrowser.Controller/BaseItemManager/BaseItemManager.cs +++ b/MediaBrowser.Controller/BaseItemManager/BaseItemManager.cs @@ -35,7 +35,7 @@ namespace MediaBrowser.Controller.BaseItemManager public SemaphoreSlim MetadataRefreshThrottler { get; private set; } /// - public bool IsMetadataFetcherEnabled(BaseItem baseItem, LibraryOptions libraryOptions, string name) + public bool IsMetadataFetcherEnabled(BaseItem baseItem, TypeOptions? libraryTypeOptions, string name) { if (baseItem is Channel) { @@ -49,10 +49,9 @@ namespace MediaBrowser.Controller.BaseItemManager return !baseItem.EnableMediaSourceDisplay; } - var typeOptions = libraryOptions.GetTypeOptions(baseItem.GetType().Name); - if (typeOptions != null) + if (libraryTypeOptions != null) { - return typeOptions.MetadataFetchers.Contains(name.AsSpan(), StringComparison.OrdinalIgnoreCase); + return libraryTypeOptions.MetadataFetchers.Contains(name.AsSpan(), StringComparison.OrdinalIgnoreCase); } var itemConfig = _serverConfigurationManager.Configuration.MetadataOptions.FirstOrDefault(i => string.Equals(i.ItemType, baseItem.GetType().Name, StringComparison.OrdinalIgnoreCase)); @@ -61,7 +60,7 @@ namespace MediaBrowser.Controller.BaseItemManager } /// - public bool IsImageFetcherEnabled(BaseItem baseItem, LibraryOptions libraryOptions, string name) + public bool IsImageFetcherEnabled(BaseItem baseItem, TypeOptions? libraryTypeOptions, string name) { if (baseItem is Channel) { @@ -75,10 +74,9 @@ namespace MediaBrowser.Controller.BaseItemManager return !baseItem.EnableMediaSourceDisplay; } - var typeOptions = libraryOptions.GetTypeOptions(baseItem.GetType().Name); - if (typeOptions != null) + if (libraryTypeOptions != null) { - return typeOptions.ImageFetchers.Contains(name.AsSpan(), StringComparison.OrdinalIgnoreCase); + return libraryTypeOptions.ImageFetchers.Contains(name.AsSpan(), StringComparison.OrdinalIgnoreCase); } var itemConfig = _serverConfigurationManager.Configuration.MetadataOptions.FirstOrDefault(i => string.Equals(i.ItemType, baseItem.GetType().Name, StringComparison.OrdinalIgnoreCase)); diff --git a/MediaBrowser.Controller/BaseItemManager/IBaseItemManager.cs b/MediaBrowser.Controller/BaseItemManager/IBaseItemManager.cs index e18994214e..b07c80879d 100644 --- a/MediaBrowser.Controller/BaseItemManager/IBaseItemManager.cs +++ b/MediaBrowser.Controller/BaseItemManager/IBaseItemManager.cs @@ -18,18 +18,18 @@ namespace MediaBrowser.Controller.BaseItemManager /// Is metadata fetcher enabled. /// /// The base item. - /// The library options. + /// The type options for baseItem from the library (if defined). /// The metadata fetcher name. /// true if metadata fetcher is enabled, else false. - bool IsMetadataFetcherEnabled(BaseItem baseItem, LibraryOptions libraryOptions, string name); + bool IsMetadataFetcherEnabled(BaseItem baseItem, TypeOptions? libraryTypeOptions, string name); /// /// Is image fetcher enabled. /// /// The base item. - /// The library options. + /// The type options for baseItem from the library (if defined). /// The image fetcher name. /// true if image fetcher is enabled, else false. - bool IsImageFetcherEnabled(BaseItem baseItem, LibraryOptions libraryOptions, string name); + bool IsImageFetcherEnabled(BaseItem baseItem, TypeOptions? libraryTypeOptions, string name); } } diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 82d633e234..d1a5831f98 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -321,7 +321,7 @@ namespace MediaBrowser.Providers.Manager var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name); var fetcherOrder = typeOptions?.ImageFetcherOrder ?? options.ImageFetcherOrder; - return _imageProviders.Where(i => CanRefreshImages(i, item, libraryOptions, refreshOptions, includeDisabled)) + return _imageProviders.Where(i => CanRefreshImages(i, item, typeOptions, refreshOptions, includeDisabled)) .OrderBy(i => GetConfiguredOrder(fetcherOrder, i.Name)) .ThenBy(GetDefaultOrder); } @@ -343,7 +343,7 @@ namespace MediaBrowser.Providers.Manager var metadataFetcherOrder = typeOptions?.MetadataFetcherOrder ?? globalMetadataOptions.MetadataFetcherOrder; return _metadataProviders.OfType>() - .Where(i => CanRefreshMetadata(i, item, libraryOptions, includeDisabled, forceEnableInternetMetadata)) + .Where(i => CanRefreshMetadata(i, item, typeOptions, includeDisabled, forceEnableInternetMetadata)) .OrderBy(i => { // local and remote providers will be interleaved in the final order @@ -361,7 +361,7 @@ namespace MediaBrowser.Providers.Manager private bool CanRefreshMetadata( IMetadataProvider provider, BaseItem item, - LibraryOptions libraryOptions, + TypeOptions? libraryTypeOptions, bool includeDisabled, bool forceEnableInternetMetadata) { @@ -375,7 +375,7 @@ namespace MediaBrowser.Providers.Manager if (provider is IRemoteMetadataProvider) { - if (!forceEnableInternetMetadata && !_baseItemManager.IsMetadataFetcherEnabled(item, libraryOptions, provider.Name)) + if (!forceEnableInternetMetadata && !_baseItemManager.IsMetadataFetcherEnabled(item, libraryTypeOptions, provider.Name)) { return false; } @@ -402,7 +402,7 @@ namespace MediaBrowser.Providers.Manager private bool CanRefreshImages( IImageProvider provider, BaseItem item, - LibraryOptions libraryOptions, + TypeOptions? libraryTypeOptions, ImageRefreshOptions refreshOptions, bool includeDisabled) { @@ -419,7 +419,7 @@ namespace MediaBrowser.Providers.Manager if (provider is IRemoteImageProvider || provider is IDynamicImageProvider) { - if (!_baseItemManager.IsImageFetcherEnabled(item, libraryOptions, provider.Name)) + if (!_baseItemManager.IsImageFetcherEnabled(item, libraryTypeOptions, provider.Name)) { return false; } diff --git a/tests/Jellyfin.Controller.Tests/BaseItemManagerTests.cs b/tests/Jellyfin.Controller.Tests/BaseItemManagerTests.cs index 463e17ad36..f67e6d1ef0 100644 --- a/tests/Jellyfin.Controller.Tests/BaseItemManagerTests.cs +++ b/tests/Jellyfin.Controller.Tests/BaseItemManagerTests.cs @@ -20,17 +20,13 @@ namespace Jellyfin.Controller.Tests { BaseItem item = (BaseItem)Activator.CreateInstance(itemType)!; - var libraryOptions = new LibraryOptions - { - TypeOptions = new[] + var libraryTypeOptions = itemType == typeof(Book) + ? new TypeOptions { - new TypeOptions - { - Type = "Book", - MetadataFetchers = new[] { "LibraryEnabled" } - } + Type = "Book", + MetadataFetchers = new[] { "LibraryEnabled" } } - }; + : null; var serverConfiguration = new ServerConfiguration(); foreach (var typeConfig in serverConfiguration.MetadataOptions) @@ -43,7 +39,7 @@ namespace Jellyfin.Controller.Tests .Returns(serverConfiguration); var baseItemManager = new BaseItemManager(serverConfigurationManager.Object); - var actual = baseItemManager.IsMetadataFetcherEnabled(item, libraryOptions, fetcherName); + var actual = baseItemManager.IsMetadataFetcherEnabled(item, libraryTypeOptions, fetcherName); Assert.Equal(expected, actual); } @@ -57,17 +53,13 @@ namespace Jellyfin.Controller.Tests { BaseItem item = (BaseItem)Activator.CreateInstance(itemType)!; - var libraryOptions = new LibraryOptions - { - TypeOptions = new[] + var libraryTypeOptions = itemType == typeof(Book) + ? new TypeOptions { - new TypeOptions - { - Type = "Book", - ImageFetchers = new[] { "LibraryEnabled" } - } + Type = "Book", + ImageFetchers = new[] { "LibraryEnabled" } } - }; + : null; var serverConfiguration = new ServerConfiguration(); foreach (var typeConfig in serverConfiguration.MetadataOptions) @@ -80,7 +72,7 @@ namespace Jellyfin.Controller.Tests .Returns(serverConfiguration); var baseItemManager = new BaseItemManager(serverConfigurationManager.Object); - var actual = baseItemManager.IsImageFetcherEnabled(item, libraryOptions, fetcherName); + var actual = baseItemManager.IsImageFetcherEnabled(item, libraryTypeOptions, fetcherName); Assert.Equal(expected, actual); } diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index ba91f5ed2d..560b50f091 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -131,7 +131,7 @@ namespace Jellyfin.Providers.Tests.Manager }; var baseItemManager = new Mock(MockBehavior.Strict); - baseItemManager.Setup(i => i.IsImageFetcherEnabled(item, It.IsAny(), providerName)) + baseItemManager.Setup(i => i.IsImageFetcherEnabled(item, It.IsAny(), providerName)) .Returns(baseItemEnabled); using var providerManager = GetProviderManager(baseItemManager: baseItemManager.Object); @@ -219,7 +219,7 @@ namespace Jellyfin.Providers.Tests.Manager metadataFetcherOrder: serverRemoteOrder?.Select(nameProvider).ToArray()); var baseItemManager = new Mock(MockBehavior.Strict); - baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), It.IsAny())) + baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), It.IsAny())) .Returns(true); using var providerManager = GetProviderManager(serverConfiguration: serverConfiguration, baseItemManager: baseItemManager.Object); @@ -302,7 +302,7 @@ namespace Jellyfin.Providers.Tests.Manager var provider = MockIMetadataProviderMapper(providerType.Name, providerName, forced: providerForced); var baseItemManager = new Mock(MockBehavior.Strict); - baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), providerName)) + baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), providerName)) .Returns(baseItemEnabled); using var providerManager = GetProviderManager(baseItemManager: baseItemManager.Object); From 6ab64f4930f61f7f0d0968b88da687a95e0035ad Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sun, 19 Dec 2021 23:33:27 +0100 Subject: [PATCH 12/19] Switch to nameof to simplify theory signatures --- .../Manager/ProviderManagerTests.cs | 82 +++++++++---------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 560b50f091..d76d411a78 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -77,32 +77,32 @@ namespace Jellyfin.Providers.Tests.Manager [InlineData(true, true, false)] public void GetImageProviders_CanRefreshImagesBasic_WhenSupportsWithoutError(bool supports, bool errorOnSupported, bool expected) { - GetImageProviders_CanRefreshImages_Tester(typeof(IImageProvider), supports, expected, errorOnSupported: errorOnSupported); + GetImageProviders_CanRefreshImages_Tester(nameof(IImageProvider), supports, expected, errorOnSupported: errorOnSupported); } [Theory] - [InlineData(typeof(ILocalImageProvider), false, true)] - [InlineData(typeof(ILocalImageProvider), true, true)] - [InlineData(typeof(IImageProvider), false, false)] - [InlineData(typeof(IImageProvider), true, true)] - public void GetImageProviders_CanRefreshImagesLocked_WhenLocalOrFullRefresh(Type providerType, bool fullRefresh, bool expected) + [InlineData(nameof(ILocalImageProvider), false, true)] + [InlineData(nameof(ILocalImageProvider), true, true)] + [InlineData(nameof(IImageProvider), false, false)] + [InlineData(nameof(IImageProvider), true, true)] + public void GetImageProviders_CanRefreshImagesLocked_WhenLocalOrFullRefresh(string providerType, bool fullRefresh, bool expected) { GetImageProviders_CanRefreshImages_Tester(providerType, true, expected, itemLocked: true, fullRefresh: fullRefresh); } [Theory] - [InlineData(typeof(ILocalImageProvider), false, true)] - [InlineData(typeof(IRemoteImageProvider), true, true)] - [InlineData(typeof(IDynamicImageProvider), true, true)] - [InlineData(typeof(IRemoteImageProvider), false, false)] - [InlineData(typeof(IDynamicImageProvider), false, false)] - public void GetImageProviders_CanRefreshImagesBaseItemEnabled_WhenLocalOrEnabled(Type providerType, bool enabled, bool expected) + [InlineData(nameof(ILocalImageProvider), false, true)] + [InlineData(nameof(IRemoteImageProvider), true, true)] + [InlineData(nameof(IDynamicImageProvider), true, true)] + [InlineData(nameof(IRemoteImageProvider), false, false)] + [InlineData(nameof(IDynamicImageProvider), false, false)] + public void GetImageProviders_CanRefreshImagesBaseItemEnabled_WhenLocalOrEnabled(string providerType, bool enabled, bool expected) { GetImageProviders_CanRefreshImages_Tester(providerType, true, expected, baseItemEnabled: enabled); } private static void GetImageProviders_CanRefreshImages_Tester( - Type providerType, + string providerType, bool supports, bool expected, bool errorOnSupported = false, @@ -116,7 +116,7 @@ namespace Jellyfin.Providers.Tests.Manager }; var providerName = "provider"; - IImageProvider provider = providerType.Name switch + IImageProvider provider = providerType switch { "IImageProvider" => MockIImageProvider(providerName, item, supports: supports, errorOnSupported: errorOnSupported), "ILocalImageProvider" => MockIImageProvider(providerName, item, supports: supports, errorOnSupported: errorOnSupported), @@ -233,57 +233,57 @@ namespace Jellyfin.Providers.Tests.Manager } [Theory] - [InlineData(typeof(IMetadataProvider))] - [InlineData(typeof(ILocalMetadataProvider))] - [InlineData(typeof(IRemoteMetadataProvider))] - [InlineData(typeof(ICustomMetadataProvider))] - public void GetMetadataProviders_CanRefreshMetadataBasic_ReturnsTrue(Type providerType) + [InlineData(nameof(IMetadataProvider))] + [InlineData(nameof(ILocalMetadataProvider))] + [InlineData(nameof(IRemoteMetadataProvider))] + [InlineData(nameof(ICustomMetadataProvider))] + public void GetMetadataProviders_CanRefreshMetadataBasic_ReturnsTrue(string providerType) { GetMetadataProviders_CanRefreshMetadata_Tester(providerType, true); } [Theory] - [InlineData(typeof(ILocalMetadataProvider), false, true)] - [InlineData(typeof(IRemoteMetadataProvider), false, false)] - [InlineData(typeof(ICustomMetadataProvider), false, false)] - [InlineData(typeof(ILocalMetadataProvider), true, true)] - [InlineData(typeof(ICustomMetadataProvider), true, false)] - public void GetMetadataProviders_CanRefreshMetadataLocked_WhenLocalOrForced(Type providerType, bool forced, bool expected) + [InlineData(nameof(ILocalMetadataProvider), false, true)] + [InlineData(nameof(IRemoteMetadataProvider), false, false)] + [InlineData(nameof(ICustomMetadataProvider), false, false)] + [InlineData(nameof(ILocalMetadataProvider), true, true)] + [InlineData(nameof(ICustomMetadataProvider), true, false)] + public void GetMetadataProviders_CanRefreshMetadataLocked_WhenLocalOrForced(string providerType, bool forced, bool expected) { GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, itemLocked: true, providerForced: forced); } [Theory] - [InlineData(typeof(ILocalMetadataProvider), false, true)] - [InlineData(typeof(ICustomMetadataProvider), false, true)] - [InlineData(typeof(IRemoteMetadataProvider), false, false)] - [InlineData(typeof(IRemoteMetadataProvider), true, true)] - public void GetMetadataProviders_CanRefreshMetadataBaseItemEnabled_WhenEnabledOrNotRemote(Type providerType, bool baseItemEnabled, bool expected) + [InlineData(nameof(ILocalMetadataProvider), false, true)] + [InlineData(nameof(ICustomMetadataProvider), false, true)] + [InlineData(nameof(IRemoteMetadataProvider), false, false)] + [InlineData(nameof(IRemoteMetadataProvider), true, true)] + public void GetMetadataProviders_CanRefreshMetadataBaseItemEnabled_WhenEnabledOrNotRemote(string providerType, bool baseItemEnabled, bool expected) { GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, baseItemEnabled: baseItemEnabled); } [Theory] - [InlineData(typeof(IRemoteMetadataProvider), false, true)] - [InlineData(typeof(ICustomMetadataProvider), false, true)] - [InlineData(typeof(ILocalMetadataProvider), false, false)] - [InlineData(typeof(ILocalMetadataProvider), true, true)] - public void GetMetadataProviders_CanRefreshMetadataSupportsLocal_WhenSupportsOrNotLocal(Type providerType, bool supportsLocalMetadata, bool expected) + [InlineData(nameof(IRemoteMetadataProvider), false, true)] + [InlineData(nameof(ICustomMetadataProvider), false, true)] + [InlineData(nameof(ILocalMetadataProvider), false, false)] + [InlineData(nameof(ILocalMetadataProvider), true, true)] + public void GetMetadataProviders_CanRefreshMetadataSupportsLocal_WhenSupportsOrNotLocal(string providerType, bool supportsLocalMetadata, bool expected) { GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, supportsLocalMetadata: supportsLocalMetadata); } [Theory] - [InlineData(typeof(ICustomMetadataProvider), true)] - [InlineData(typeof(IRemoteMetadataProvider), false)] - [InlineData(typeof(ILocalMetadataProvider), false)] - public void GetMetadataProviders_CanRefreshMetadataOwned_WhenNotLocal(Type providerType, bool expected) + [InlineData(nameof(ICustomMetadataProvider), true)] + [InlineData(nameof(IRemoteMetadataProvider), false)] + [InlineData(nameof(ILocalMetadataProvider), false)] + public void GetMetadataProviders_CanRefreshMetadataOwned_WhenNotLocal(string providerType, bool expected) { GetMetadataProviders_CanRefreshMetadata_Tester(providerType, expected, ownedItem: true); } private static void GetMetadataProviders_CanRefreshMetadata_Tester( - Type providerType, + string providerType, bool expected, bool itemLocked = false, bool baseItemEnabled = true, @@ -299,7 +299,7 @@ namespace Jellyfin.Providers.Tests.Manager }; var providerName = "provider"; - var provider = MockIMetadataProviderMapper(providerType.Name, providerName, forced: providerForced); + var provider = MockIMetadataProviderMapper(providerType, providerName, forced: providerForced); var baseItemManager = new Mock(MockBehavior.Strict); baseItemManager.Setup(i => i.IsMetadataFetcherEnabled(item, It.IsAny(), providerName)) From bdce435b09b88329dd7f23ff5f4e9bb7998763b5 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sat, 18 Dec 2021 22:30:06 +0100 Subject: [PATCH 13/19] Reorder and flatten provider filtering --- .../Manager/ProviderManager.cs | 115 ++++++++---------- .../Manager/ProviderManagerTests.cs | 4 +- 2 files changed, 55 insertions(+), 64 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index d1a5831f98..e2882ee06e 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -326,6 +326,39 @@ namespace MediaBrowser.Providers.Manager .ThenBy(GetDefaultOrder); } + private bool CanRefreshImages( + IImageProvider provider, + BaseItem item, + TypeOptions? libraryTypeOptions, + ImageRefreshOptions refreshOptions, + bool includeDisabled) + { + try + { + if (!provider.Supports(item)) + { + return false; + } + } + catch (Exception ex) + { + _logger.LogError(ex, "{ProviderName} failed in Supports for type {ItemType} at {ItemPath}", provider.GetType().Name, item.GetType().Name, item.Path); + return false; + } + + if (includeDisabled || provider is ILocalImageProvider) + { + return true; + } + + if (item.IsLocked && refreshOptions.ImageRefreshMode != MetadataRefreshMode.FullRefresh) + { + return false; + } + + return _baseItemManager.IsImageFetcherEnabled(item, libraryTypeOptions, provider.Name); + } + /// public IEnumerable> GetMetadataProviders(BaseItem item, LibraryOptions libraryOptions) where T : BaseItem @@ -365,76 +398,34 @@ namespace MediaBrowser.Providers.Manager bool includeDisabled, bool forceEnableInternetMetadata) { - if (!includeDisabled) - { - // If locked only allow local providers - if (item.IsLocked && provider is not ILocalMetadataProvider && provider is not IForcedProvider) - { - return false; - } - - if (provider is IRemoteMetadataProvider) - { - if (!forceEnableInternetMetadata && !_baseItemManager.IsMetadataFetcherEnabled(item, libraryTypeOptions, provider.Name)) - { - return false; - } - } - } - if (!item.SupportsLocalMetadata && provider is ILocalMetadataProvider) { return false; } - // If this restriction is ever lifted, movie xml providers will have to be updated to prevent owned items like trailers from reading those files - if (!item.OwnerId.Equals(default)) + // Prevent owned items from reading the same local metadata file as their owner + if (!item.OwnerId.Equals(default) && provider is ILocalMetadataProvider) { - if (provider is ILocalMetadataProvider || provider is IRemoteMetadataProvider) - { - return false; - } - } - - return true; - } - - private bool CanRefreshImages( - IImageProvider provider, - BaseItem item, - TypeOptions? libraryTypeOptions, - ImageRefreshOptions refreshOptions, - bool includeDisabled) - { - if (!includeDisabled) - { - // If locked only allow local providers - if (item.IsLocked && provider is not ILocalImageProvider) - { - if (refreshOptions.ImageRefreshMode != MetadataRefreshMode.FullRefresh) - { - return false; - } - } - - if (provider is IRemoteImageProvider || provider is IDynamicImageProvider) - { - if (!_baseItemManager.IsImageFetcherEnabled(item, libraryTypeOptions, provider.Name)) - { - return false; - } - } - } - - try - { - return provider.Supports(item); - } - catch (Exception ex) - { - _logger.LogError(ex, "{ProviderName} failed in Supports for type {ItemType} at {ItemPath}", provider.GetType().Name, item.GetType().Name, item.Path); return false; } + + if (includeDisabled) + { + return true; + } + + // If locked only allow local providers + if (item.IsLocked && provider is not ILocalMetadataProvider && provider is not IForcedProvider) + { + return false; + } + + if (forceEnableInternetMetadata || provider is not IRemoteMetadataProvider) + { + return true; + } + + return _baseItemManager.IsMetadataFetcherEnabled(item, libraryTypeOptions, provider.Name); } private static int GetConfiguredOrder(string[] order, string providerName) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index d76d411a78..8100dcfa6f 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -54,7 +54,7 @@ namespace Jellyfin.Providers.Tests.Manager for (var i = 0; i < providerCount; i++) { var order = hasOrderOrder?[i]; - providerList.Add(MockIImageProvider(nameProvider(i), item, order: order)); + providerList.Add(MockIImageProvider(nameProvider(i), item, order: order)); } var libraryOptions = CreateLibraryOptions(item.GetType().Name, imageFetcherOrder: libraryOrder?.Select(nameProvider).ToArray()); @@ -275,7 +275,7 @@ namespace Jellyfin.Providers.Tests.Manager [Theory] [InlineData(nameof(ICustomMetadataProvider), true)] - [InlineData(nameof(IRemoteMetadataProvider), false)] + [InlineData(nameof(IRemoteMetadataProvider), true)] [InlineData(nameof(ILocalMetadataProvider), false)] public void GetMetadataProviders_CanRefreshMetadataOwned_WhenNotLocal(string providerType, bool expected) { From ee5bd0daa62e68f4f93e7603017c1f028781e387 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Tue, 21 Dec 2021 00:24:07 +0100 Subject: [PATCH 14/19] Implement tests on ProviderManager.RefreshSingleItem --- .../Providers/IMetadataService.cs | 5 + .../Manager/ProviderManagerTests.cs | 110 +++++++++++++++++- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/MediaBrowser.Controller/Providers/IMetadataService.cs b/MediaBrowser.Controller/Providers/IMetadataService.cs index 05fbb18ee4..f0f1d18624 100644 --- a/MediaBrowser.Controller/Providers/IMetadataService.cs +++ b/MediaBrowser.Controller/Providers/IMetadataService.cs @@ -23,6 +23,11 @@ namespace MediaBrowser.Controller.Providers /// true if this instance can refresh the specified item. bool CanRefresh(BaseItem item); + /// + /// Determines whether this instance primarily targets the specified type. + /// + /// The type. + /// true if this instance primarily targets the specified type. bool CanRefreshPrimary(Type type); /// diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 8100dcfa6f..5845b31be8 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -1,6 +1,8 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Threading; +using System.Threading.Tasks; using MediaBrowser.Controller.BaseItemManager; using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; @@ -9,6 +11,7 @@ using MediaBrowser.Controller.Library; using MediaBrowser.Controller.Providers; using MediaBrowser.Model.Configuration; using MediaBrowser.Providers.Manager; +using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Moq; using Xunit; @@ -17,6 +20,95 @@ namespace Jellyfin.Providers.Tests.Manager { public class ProviderManagerTests { + private static readonly ILogger _logger = new NullLogger(); + + private static TheoryData[], int> RefreshSingleItemOrderData() + => new () + { + // no order set, uses provided order + { + new[] + { + MockIMetadataService(true, true), + MockIMetadataService(true, true) + }, + 0 + }, + // sort order sets priority when all match + { + new[] + { + MockIMetadataService(true, true, 1), + MockIMetadataService(true, true, 0), + MockIMetadataService(true, true, 2) + }, + 1 + }, + // CanRefreshPrimary prioritized + { + new[] + { + MockIMetadataService(false, true), + MockIMetadataService(true, true), + }, + 1 + }, + // falls back to CanRefresh + { + new[] + { + MockIMetadataService(false, false), + MockIMetadataService(false, true) + }, + 1 + }, + }; + + [Theory] + [MemberData(nameof(RefreshSingleItemOrderData))] + public void RefreshSingleItem_ServiceOrdering_FollowsPriority(Mock[] servicesList, int expectedIndex) + { + var item = new Movie(); + + using var providerManager = GetProviderManager(); + AddParts(providerManager, metadataServices: servicesList.Select(s => s.Object).ToArray()); + + var refreshOptions = new MetadataRefreshOptions(Mock.Of(MockBehavior.Strict)); + var actual = providerManager.RefreshSingleItem(item, refreshOptions, CancellationToken.None); + + Assert.Equal(ItemUpdateType.MetadataDownload, actual.Result); + for (var i = 0; i < servicesList.Length; i++) + { + if (i == expectedIndex) + { + servicesList[i].Verify(mock => mock.RefreshMetadata(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); + } + else + { + servicesList[i].Verify(mock => mock.RefreshMetadata(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); + } + } + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void RefreshSingleItem_RefreshMetadata_WhenServiceFound(bool serviceFound) + { + var item = new Movie(); + + var servicesList = new[] { MockIMetadataService(false, serviceFound) }; + + using var providerManager = GetProviderManager(); + AddParts(providerManager, metadataServices: servicesList.Select(s => s.Object).ToArray()); + + var refreshOptions = new MetadataRefreshOptions(Mock.Of(MockBehavior.Strict)); + var actual = providerManager.RefreshSingleItem(item, refreshOptions, CancellationToken.None); + + var expectedResult = serviceFound ? ItemUpdateType.MetadataDownload : ItemUpdateType.None; + Assert.Equal(expectedResult, actual.Result); + } + private static TheoryData GetImageProvidersOrderData() => new () { @@ -313,6 +405,20 @@ namespace Jellyfin.Providers.Tests.Manager Assert.Equal(expected ? 1 : 0, actualProviders.Length); } + private static Mock MockIMetadataService(bool refreshPrimary, bool canRefresh, int order = 0) + { + var service = new Mock(MockBehavior.Strict); + service.Setup(s => s.Order) + .Returns(order); + service.Setup(s => s.CanRefreshPrimary(It.IsAny())) + .Returns(refreshPrimary); + service.Setup(s => s.CanRefresh(It.IsAny())) + .Returns(canRefresh); + service.Setup(s => s.RefreshMetadata(It.IsAny(), It.IsAny(), It.IsAny())) + .Returns(Task.FromResult(ItemUpdateType.MetadataDownload)); + return service; + } + private static IImageProvider MockIImageProvider(string name, BaseItem expectedType, bool supports = true, int? order = null, bool errorOnSupported = false) where TProviderType : class, IImageProvider { @@ -460,11 +566,11 @@ namespace Jellyfin.Providers.Tests.Manager null, serverConfigurationManager.Object, null, - new NullLogger(), + _logger, null, null, libraryManager.Object, - baseItemManager); + baseItemManager!); return providerManager; } From ac675318f858e1be6e156e6d9d217cb662ba5c31 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Tue, 21 Dec 2021 00:25:35 +0100 Subject: [PATCH 15/19] Simplify RefreshSingleItem --- .../Manager/ProviderManager.cs | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index e2882ee06e..135b69a95b 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -132,26 +132,15 @@ namespace MediaBrowser.Providers.Manager var type = item.GetType(); var service = _metadataServices.FirstOrDefault(current => current.CanRefreshPrimary(type)); + service ??= _metadataServices.FirstOrDefault(current => current.CanRefresh(item)); if (service == null) { - foreach (var current in _metadataServices) - { - if (current.CanRefresh(item)) - { - service = current; - break; - } - } + _logger.LogError("Unable to find a metadata service for item of type {TypeName}", item.GetType().Name); + return Task.FromResult(ItemUpdateType.None); } - if (service != null) - { - return service.RefreshMetadata(item, options, cancellationToken); - } - - _logger.LogError("Unable to find a metadata service for item of type {TypeName}", item.GetType().Name); - return Task.FromResult(ItemUpdateType.None); + return service.RefreshMetadata(item, options, cancellationToken); } /// From 6e4710d048767292ec027a4992ad3448d826e42e Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@users.noreply.github.com> Date: Fri, 24 Dec 2021 09:50:58 +0100 Subject: [PATCH 16/19] Fix review comment Co-authored-by: Cody Robibero --- MediaBrowser.Providers/Manager/ProviderManager.cs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 135b69a95b..9be4bc479b 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -367,16 +367,15 @@ namespace MediaBrowser.Providers.Manager return _metadataProviders.OfType>() .Where(i => CanRefreshMetadata(i, item, typeOptions, includeDisabled, forceEnableInternetMetadata)) .OrderBy(i => - { // local and remote providers will be interleaved in the final order // only relative order within a type matters: consumers of the list filter to one or the other - switch (i) + i switch { - case ILocalMetadataProvider: return GetConfiguredOrder(localMetadataReaderOrder, i.Name); - case IRemoteMetadataProvider: return GetConfiguredOrder(metadataFetcherOrder, i.Name); - default: return int.MaxValue; // default to end - } - }) + ILocalMetadataProvider => GetConfiguredOrder(localMetadataReaderOrder, i.Name), + IRemoteMetadataProvider => GetConfiguredOrder(metadataFetcherOrder, i.Name), + // Default to end + _ => int.MaxValue + }) .ThenBy(GetDefaultOrder); } From b03f56c3d6e005b615780bcdc676bcd632e80395 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@gmail.com> Date: Sat, 1 Jan 2022 00:22:46 +0100 Subject: [PATCH 17/19] Remove warnings --- .../Manager/ProviderManager.cs | 26 +++++++++------- .../Manager/ProviderManagerTests.cs | 30 +++++++++++-------- 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/MediaBrowser.Providers/Manager/ProviderManager.cs b/MediaBrowser.Providers/Manager/ProviderManager.cs index 9be4bc479b..eb4bc3587f 100644 --- a/MediaBrowser.Providers/Manager/ProviderManager.cs +++ b/MediaBrowser.Providers/Manager/ProviderManager.cs @@ -45,7 +45,7 @@ namespace MediaBrowser.Providers.Manager /// public class ProviderManager : IProviderManager, IDisposable { - private readonly object _refreshQueueLock = new (); + private readonly object _refreshQueueLock = new(); private readonly ILogger _logger; private readonly IHttpClientFactory _httpClientFactory; private readonly ILibraryMonitor _libraryMonitor; @@ -55,9 +55,9 @@ namespace MediaBrowser.Providers.Manager private readonly ISubtitleManager _subtitleManager; private readonly IServerConfigurationManager _configurationManager; private readonly IBaseItemManager _baseItemManager; - private readonly ConcurrentDictionary _activeRefreshes = new (); - private readonly CancellationTokenSource _disposeCancellationTokenSource = new (); - private readonly SimplePriorityQueue> _refreshQueue = new (); + private readonly ConcurrentDictionary _activeRefreshes = new(); + private readonly CancellationTokenSource _disposeCancellationTokenSource = new(); + private readonly SimplePriorityQueue> _refreshQueue = new(); private IImageProvider[] _imageProviders = Array.Empty(); private IMetadataService[] _metadataServices = Array.Empty(); @@ -164,6 +164,10 @@ namespace MediaBrowser.Providers.Manager { contentType = "image/png"; } + else + { + throw new HttpRequestException("Invalid image received: contentType not set.", null, response.StatusCode); + } } // thetvdb will sometimes serve a rubbish 404 html page with a 200 OK code, because reasons... @@ -589,7 +593,7 @@ namespace MediaBrowser.Providers.Manager foreach (var saver in savers.Where(i => IsSaverEnabledForItem(i, item, libraryOptions, updateType, false))) { - _logger.LogDebug("Saving {0} to {1}.", item.Path ?? item.Name, saver.Name); + _logger.LogDebug("Saving {Item} to {Saver}", item.Path ?? item.Name, saver.Name); if (saver is IMetadataFileSaver fileSaver) { @@ -601,7 +605,7 @@ namespace MediaBrowser.Providers.Manager } catch (Exception ex) { - _logger.LogError(ex, "Error in {0} GetSavePath", saver.Name); + _logger.LogError(ex, "Error in {Saver} GetSavePath", saver.Name); continue; } @@ -688,7 +692,7 @@ namespace MediaBrowser.Providers.Manager } catch (Exception ex) { - _logger.LogError(ex, "Error in {0}.IsEnabledFor", saver.Name); + _logger.LogError(ex, "Error in {Saver}.IsEnabledFor", saver.Name); return false; } } @@ -838,7 +842,7 @@ namespace MediaBrowser.Providers.Manager } catch (Exception ex) { - _logger.LogError(ex, "Error in {0}.Supports", i.GetType().Name); + _logger.LogError(ex, "Error in {Type}.Supports", i.GetType().Name); return false; } }); @@ -904,7 +908,7 @@ namespace MediaBrowser.Providers.Manager /// public void OnRefreshStart(BaseItem item) { - _logger.LogDebug("OnRefreshStart {0}", item.Id.ToString("N", CultureInfo.InvariantCulture)); + _logger.LogDebug("OnRefreshStart {Item}", item.Id.ToString("N", CultureInfo.InvariantCulture)); _activeRefreshes[item.Id] = 0; RefreshStarted?.Invoke(this, new GenericEventArgs(item)); } @@ -912,7 +916,7 @@ namespace MediaBrowser.Providers.Manager /// public void OnRefreshComplete(BaseItem item) { - _logger.LogDebug("OnRefreshComplete {0}", item.Id.ToString("N", CultureInfo.InvariantCulture)); + _logger.LogDebug("OnRefreshComplete {Item}", item.Id.ToString("N", CultureInfo.InvariantCulture)); _activeRefreshes.Remove(item.Id, out _); @@ -934,7 +938,7 @@ namespace MediaBrowser.Providers.Manager public void OnRefreshProgress(BaseItem item, double progress) { var id = item.Id; - _logger.LogDebug("OnRefreshProgress {0} {1}", id.ToString("N", CultureInfo.InvariantCulture), progress); + _logger.LogDebug("OnRefreshProgress {Id} {Progress}", id.ToString("N", CultureInfo.InvariantCulture), progress); // TODO: Need to hunt down the conditions for this happening _activeRefreshes.AddOrUpdate( diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 5845b31be8..6179e31359 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -1,21 +1,29 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Net.Http; +using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; +using MediaBrowser.Controller; using MediaBrowser.Controller.BaseItemManager; using MediaBrowser.Controller.Configuration; using MediaBrowser.Controller.Entities; using MediaBrowser.Controller.Entities.Movies; using MediaBrowser.Controller.Library; using MediaBrowser.Controller.Providers; +using MediaBrowser.Controller.Subtitles; using MediaBrowser.Model.Configuration; +using MediaBrowser.Model.IO; using MediaBrowser.Providers.Manager; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Moq; using Xunit; +// Allow Moq to see internal class +[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")] + namespace Jellyfin.Providers.Tests.Manager { public class ProviderManagerTests @@ -23,7 +31,7 @@ namespace Jellyfin.Providers.Tests.Manager private static readonly ILogger _logger = new NullLogger(); private static TheoryData[], int> RefreshSingleItemOrderData() - => new () + => new() { // no order set, uses provided order { @@ -110,7 +118,7 @@ namespace Jellyfin.Providers.Tests.Manager } private static TheoryData GetImageProvidersOrderData() - => new () + => new() { { 3, null, null, null, new[] { 0, 1, 2 } }, // no order options set @@ -238,7 +246,7 @@ namespace Jellyfin.Providers.Tests.Manager { var l = nameof(ILocalMetadataProvider); var r = nameof(IRemoteMetadataProvider); - return new () + return new() { { new[] { l, l, r, r }, null, null, null, null, null, new[] { 0, 1, 2, 3 } }, // no order options set @@ -269,8 +277,6 @@ namespace Jellyfin.Providers.Tests.Manager // IHasOrder ordering (not interleaved, doesn't care about types) { new[] { l, l, r, r }, null, null, null, null, new int?[] { 2, null, 1, null }, new[] { 2, 0, 1, 3 } }, // partially defined { new[] { l, l, r, r }, null, null, null, null, new int?[] { 3, 2, 1, 0 }, new[] { 3, 2, 1, 0 } }, // full reverse order - // note odd interaction - orderby determines order of slot when local and remote both have a slot 0 - { new[] { l, l, r, r }, new[] { 1 }, new[] { 3 }, null, null, new int?[] { null, 2, null, 1 }, new[] { 3, 1, 0, 2 } }, // sorts interleaved results // multiple orders set { new[] { l, l, l, r, r, r }, new[] { 1 }, new[] { 4 }, new[] { 2, 1, 0 }, new[] { 5, 4, 3 }, null, new[] { 1, 4, 0, 2, 3, 5 } }, // partial library order first, server order ignored @@ -562,13 +568,13 @@ namespace Jellyfin.Providers.Tests.Manager .Returns(libraryOptions ?? new LibraryOptions()); var providerManager = new ProviderManager( - null, - null, + Mock.Of(), + Mock.Of(), serverConfigurationManager.Object, - null, + Mock.Of(), _logger, - null, - null, + Mock.Of(), + Mock.Of(), libraryManager.Object, baseItemManager!); @@ -595,7 +601,7 @@ namespace Jellyfin.Providers.Tests.Manager /// /// Simple extension to make SupportsLocalMetadata directly settable. /// - public class MetadataTestItem : BaseItem, IHasLookupInfo + internal class MetadataTestItem : BaseItem, IHasLookupInfo { public bool EnableLocalMetadata { get; set; } = true; @@ -607,7 +613,7 @@ namespace Jellyfin.Providers.Tests.Manager } } - public class MetadataTestItemInfo : ItemLookupInfo + internal class MetadataTestItemInfo : ItemLookupInfo { } } From 6bf71c0fd355e9c95a1e142019d9bc5cce34200d Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@users.noreply.github.com> Date: Sun, 16 Jan 2022 14:18:44 +0100 Subject: [PATCH 18/19] Combine verify calls Co-authored-by: Claus Vium --- .../Manager/ProviderManagerTests.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index 6179e31359..a5673ad838 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -87,14 +87,8 @@ namespace Jellyfin.Providers.Tests.Manager Assert.Equal(ItemUpdateType.MetadataDownload, actual.Result); for (var i = 0; i < servicesList.Length; i++) { - if (i == expectedIndex) - { - servicesList[i].Verify(mock => mock.RefreshMetadata(It.IsAny(), It.IsAny(), It.IsAny()), Times.Once()); - } - else - { - servicesList[i].Verify(mock => mock.RefreshMetadata(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never()); - } + var times = i == expectedIndex ? Times.Once() : Times.Never(); + servicesList[i].Verify(mock => mock.RefreshMetadata(It.IsAny(), It.IsAny(), It.IsAny()), times); } } From 6252bc399a3a56fc684f11523218093f8ff5f2b0 Mon Sep 17 00:00:00 2001 From: Joe Rogers <1337joe@users.noreply.github.com> Date: Wed, 23 Nov 2022 15:59:50 +0100 Subject: [PATCH 19/19] Fix unit tests after merge from master Co-authored-by: Bond-009 --- .../Manager/ProviderManagerTests.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs index a5673ad838..725e295b9a 100644 --- a/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs +++ b/tests/Jellyfin.Providers.Tests/Manager/ProviderManagerTests.cs @@ -30,7 +30,7 @@ namespace Jellyfin.Providers.Tests.Manager { private static readonly ILogger _logger = new NullLogger(); - private static TheoryData[], int> RefreshSingleItemOrderData() + public static TheoryData[], int> RefreshSingleItemOrderData() => new() { // no order set, uses provided order @@ -74,7 +74,7 @@ namespace Jellyfin.Providers.Tests.Manager [Theory] [MemberData(nameof(RefreshSingleItemOrderData))] - public void RefreshSingleItem_ServiceOrdering_FollowsPriority(Mock[] servicesList, int expectedIndex) + public async Task RefreshSingleItem_ServiceOrdering_FollowsPriority(Mock[] servicesList, int expectedIndex) { var item = new Movie(); @@ -82,9 +82,9 @@ namespace Jellyfin.Providers.Tests.Manager AddParts(providerManager, metadataServices: servicesList.Select(s => s.Object).ToArray()); var refreshOptions = new MetadataRefreshOptions(Mock.Of(MockBehavior.Strict)); - var actual = providerManager.RefreshSingleItem(item, refreshOptions, CancellationToken.None); + var actual = await providerManager.RefreshSingleItem(item, refreshOptions, CancellationToken.None).ConfigureAwait(false); - Assert.Equal(ItemUpdateType.MetadataDownload, actual.Result); + Assert.Equal(ItemUpdateType.MetadataDownload, actual); for (var i = 0; i < servicesList.Length; i++) { var times = i == expectedIndex ? Times.Once() : Times.Never(); @@ -95,7 +95,7 @@ namespace Jellyfin.Providers.Tests.Manager [Theory] [InlineData(true)] [InlineData(false)] - public void RefreshSingleItem_RefreshMetadata_WhenServiceFound(bool serviceFound) + public async Task RefreshSingleItem_RefreshMetadata_WhenServiceFound(bool serviceFound) { var item = new Movie(); @@ -105,13 +105,13 @@ namespace Jellyfin.Providers.Tests.Manager AddParts(providerManager, metadataServices: servicesList.Select(s => s.Object).ToArray()); var refreshOptions = new MetadataRefreshOptions(Mock.Of(MockBehavior.Strict)); - var actual = providerManager.RefreshSingleItem(item, refreshOptions, CancellationToken.None); + var actual = await providerManager.RefreshSingleItem(item, refreshOptions, CancellationToken.None).ConfigureAwait(false); var expectedResult = serviceFound ? ItemUpdateType.MetadataDownload : ItemUpdateType.None; - Assert.Equal(expectedResult, actual.Result); + Assert.Equal(expectedResult, actual); } - private static TheoryData GetImageProvidersOrderData() + public static TheoryData GetImageProvidersOrderData() => new() { { 3, null, null, null, new[] { 0, 1, 2 } }, // no order options set @@ -236,7 +236,7 @@ namespace Jellyfin.Providers.Tests.Manager Assert.Equal(expected ? 1 : 0, actualProviders.Length); } - private static TheoryData GetMetadataProvidersOrderData() + public static TheoryData GetMetadataProvidersOrderData() { var l = nameof(ILocalMetadataProvider); var r = nameof(IRemoteMetadataProvider);