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

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);
+                .ThenBy(GetDefaultOrder);
-        }
-
-        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;
         }
 
         /// <inheritdoc />
@@ -351,12 +338,23 @@ namespace MediaBrowser.Providers.Manager
         private IEnumerable<IMetadataProvider<T>> GetMetadataProvidersInternal<T>(BaseItem item, LibraryOptions libraryOptions, MetadataOptions globalMetadataOptions, bool includeDisabled, bool forceEnableInternetMetadata)
             where T : BaseItem
         {
-            // Avoid implicitly captured closure
+            var localMetadataReaderOrder = libraryOptions.LocalMetadataReaderOrder ?? globalMetadataOptions.LocalMetadataReaderOrder;
-            var currentOptions = globalMetadataOptions;
+            var typeOptions = libraryOptions.GetTypeOptions(item.GetType().Name);
+            var metadataFetcherOrder = typeOptions?.MetadataFetcherOrder ?? globalMetadataOptions.MetadataFetcherOrder;
 
             return _metadataProviders.OfType<IMetadataProvider<T>>()
                 .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
+            var index = Array.IndexOf(order, providerName);
-            if (provider is ILocalMetadataProvider)
-            {
-                var configuredOrder = libraryOptions.LocalMetadataReaderOrder ?? globalMetadataOptions.LocalMetadataReaderOrder;
-
-                var index = Array.IndexOf(configuredOrder, provider.Name);
 
             if (index != -1)
             {
                 return index;
             }
+
+            // default to end
+            return int.MaxValue;
         }
 
-            // See if there's a user-defined order
+        private static int GetDefaultOrder(object provider)
-            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;
-        }
-
-        private static int GetOrder(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;
-        }
-
         /// <inheritdoc/>
         public MetadataPluginSummary[] GetAllMetadataPlugins()
         {

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

@@ -167,8 +167,8 @@ namespace Jellyfin.Providers.Tests.Manager
 
         private static TheoryData<string[], int[]?, int[]?, int[]?, int[]?, int?[]?, int[]> GetMetadataProvidersOrderData()
         {
-            var l = "local";
+            var l = nameof(ILocalMetadataProvider);
-            var r = "remote";
+            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[] { 2, 0, 1, 3 } }, // partially defined
-                { 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
@@ -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<string, string>
-            {
-                { "remote", nameof(IRemoteMetadataProvider) },
-                { "local", nameof(ILocalMetadataProvider) },
-                { "custom", nameof(ICustomMetadataProvider) }
-            };
 
             var nameProvider = new Func<int, string>(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<MetadataTestItem, MetadataTestItemInfo>(typeNames[providers[i]], nameProvider(i), order: order));
+                providerList.Add(MockIMetadataProviderMapper<MetadataTestItem, MetadataTestItemInfo>(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<MetadataTestItem>(item, libraryOptions).ToList();
 
             Assert.Equal(providerList.Count, actualProviders.Count);