Merge pull request #4473 from crobibero/binder-no-throw

Don't throw exception when converting values using binder or JsonConv…

Jellyfin.Api/ModelBinders/CommaDelimitedArrayModelBinder.cs

@@ -1,7 +1,9 @@
 using System;
+using System.Collections.Generic;
 using System.ComponentModel;
 using System.Threading.Tasks;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
+using Microsoft.Extensions.Logging;
 
 namespace Jellyfin.Api.ModelBinders
 {
@@ -11,6 +13,17 @@ namespace Jellyfin.Api.ModelBinders
     /// </summary>
     public class CommaDelimitedArrayModelBinder : IModelBinder
     {
+        private readonly ILogger<CommaDelimitedArrayModelBinder> _logger;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="CommaDelimitedArrayModelBinder"/> class.
+        /// </summary>
+        /// <param name="logger">Instance of the <see cref="ILogger{CommaDelimitedArrayModelBinder}"/> interface.</param>
+        public CommaDelimitedArrayModelBinder(ILogger<CommaDelimitedArrayModelBinder> logger)
+        {
+            _logger = logger;
+        }
+
         /// <inheritdoc/>
         public Task BindModelAsync(ModelBindingContext bindingContext)
         {
@@ -20,16 +33,8 @@ namespace Jellyfin.Api.ModelBinders
 
             if (valueProviderResult.Length > 1)
             {
-                var result = Array.CreateInstance(elementType, valueProviderResult.Length);
-
-                for (int i = 0; i < valueProviderResult.Length; i++)
-                {
-                    var value = converter.ConvertFromString(valueProviderResult.Values[i].Trim());
-
-                    result.SetValue(value, i);
-                }
-
-                bindingContext.Result = ModelBindingResult.Success(result);
+                var typedValues = GetParsedResult(valueProviderResult.Values, elementType, converter);
+                bindingContext.Result = ModelBindingResult.Success(typedValues);
             }
             else
             {
@@ -37,13 +42,8 @@ namespace Jellyfin.Api.ModelBinders
 
                 if (value != null)
                 {
-                    var values = Array.ConvertAll(
-                        value.Split(new[] { "," }, StringSplitOptions.RemoveEmptyEntries),
-                        x => converter.ConvertFromString(x?.Trim()));
-
-                    var typedValues = Array.CreateInstance(elementType, values.Length);
-                    values.CopyTo(typedValues, 0);
-
+                    var splitValues = value.Split(',', StringSplitOptions.RemoveEmptyEntries);
+                    var typedValues = GetParsedResult(splitValues, elementType, converter);
                     bindingContext.Result = ModelBindingResult.Success(typedValues);
                 }
                 else
@@ -55,5 +55,36 @@ namespace Jellyfin.Api.ModelBinders
 
             return Task.CompletedTask;
         }
+
+        private Array GetParsedResult(IReadOnlyList<string> values, Type elementType, TypeConverter converter)
+        {
+            var parsedValues = new object?[values.Count];
+            var convertedCount = 0;
+            for (var i = 0; i < values.Count; i++)
+            {
+                try
+                {
+                    parsedValues[i] = converter.ConvertFromString(values[i].Trim());
+                    convertedCount++;
+                }
+                catch (FormatException e)
+                {
+                    _logger.LogWarning(e, "Error converting value.");
+                }
+            }
+
+            var typedValues = Array.CreateInstance(elementType, convertedCount);
+            var typedValueIndex = 0;
+            for (var i = 0; i < parsedValues.Length; i++)
+            {
+                if (parsedValues[i] != null)
+                {
+                    typedValues.SetValue(parsedValues[i], typedValueIndex);
+                    typedValueIndex++;
+                }
+            }
+
+            return typedValues;
+        }
     }
 }

MediaBrowser.Common/Json/Converters/JsonCommaDelimitedArrayConverter.cs

@@ -32,13 +32,34 @@ namespace MediaBrowser.Common.Json.Converters
                     return Array.Empty<T>();
                 }
 
-                var entries = new T[stringEntries.Length];
+                var parsedValues = new object[stringEntries.Length];
+                var convertedCount = 0;
                 for (var i = 0; i < stringEntries.Length; i++)
                 {
-                    entries[i] = (T)_typeConverter.ConvertFrom(stringEntries[i].Trim());
+                    try
+                    {
+                        parsedValues[i] = _typeConverter.ConvertFrom(stringEntries[i].Trim());
+                        convertedCount++;
+                    }
+                    catch (FormatException)
+                    {
+                        // TODO log when upgraded to .Net5
+                        // _logger.LogWarning(e, "Error converting value.");
+                    }
                 }
 
-                return entries;
+                var typedValues = new T[convertedCount];
+                var typedValueIndex = 0;
+                for (var i = 0; i < stringEntries.Length; i++)
+                {
+                    if (parsedValues[i] != null)
+                    {
+                        typedValues.SetValue(parsedValues[i], typedValueIndex);
+                        typedValueIndex++;
+                    }
+                }
+
+                return typedValues;
             }
 
             return JsonSerializer.Deserialize<T[]>(ref reader, options);

tests/Jellyfin.Api.Tests/ModelBinders/CommaDelimitedArrayModelBinderTests.cs

@@ -5,6 +5,7 @@ using System.Threading.Tasks;
 using Jellyfin.Api.ModelBinders;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc.ModelBinding;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Primitives;
 using Moq;
 using Xunit;
@@ -21,7 +22,7 @@ namespace Jellyfin.Api.Tests.ModelBinders
             var queryParamString = "lol,xd";
             var queryParamType = typeof(string[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
                     new QueryCollection(new Dictionary<string, StringValues> { { queryParamName, new StringValues(queryParamString) } }),
@@ -46,7 +47,7 @@ namespace Jellyfin.Api.Tests.ModelBinders
             var queryParamString = "42,0";
             var queryParamType = typeof(int[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
                     new QueryCollection(new Dictionary<string, StringValues> { { queryParamName, new StringValues(queryParamString) } }),
@@ -71,7 +72,7 @@ namespace Jellyfin.Api.Tests.ModelBinders
             var queryParamString = "How,Much";
             var queryParamType = typeof(TestType[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
                     new QueryCollection(new Dictionary<string, StringValues> { { queryParamName, new StringValues(queryParamString) } }),
@@ -96,7 +97,7 @@ namespace Jellyfin.Api.Tests.ModelBinders
             var queryParamString = "How,,Much";
             var queryParamType = typeof(TestType[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
                     new QueryCollection(new Dictionary<string, StringValues> { { queryParamName, new StringValues(queryParamString) } }),
@@ -122,7 +123,7 @@ namespace Jellyfin.Api.Tests.ModelBinders
             var queryParamString2 = "Much";
             var queryParamType = typeof(TestType[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
 
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
@@ -150,7 +151,7 @@ namespace Jellyfin.Api.Tests.ModelBinders
             var queryParamValues = Array.Empty<TestType>();
             var queryParamType = typeof(TestType[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
 
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
@@ -172,13 +173,13 @@ namespace Jellyfin.Api.Tests.ModelBinders
         }
 
         [Fact]
-        public async Task BindModelAsync_ThrowsIfCommaDelimitedEnumArrayQueryIsInvalid()
+        public async Task BindModelAsync_EnumArrayQuery_BindValidOnly()
         {
             var queryParamName = "test";
             var queryParamString = "🔥,😢";
             var queryParamType = typeof(TestType[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
                     new QueryCollection(new Dictionary<string, StringValues> { { queryParamName, new StringValues(queryParamString) } }),
@@ -189,20 +190,20 @@ namespace Jellyfin.Api.Tests.ModelBinders
             bindingContextMock.Setup(b => b.ModelType).Returns(queryParamType);
             bindingContextMock.SetupProperty(b => b.Result);
 
-            Func<Task> act = async () => await modelBinder.BindModelAsync(bindingContextMock.Object);
-
-            await Assert.ThrowsAsync<FormatException>(act);
+            await modelBinder.BindModelAsync(bindingContextMock.Object);
+            Assert.True(bindingContextMock.Object.Result.IsModelSet);
+            Assert.Empty((TestType[])bindingContextMock.Object.Result.Model);
         }
 
         [Fact]
-        public async Task BindModelAsync_ThrowsIfCommaDelimitedEnumArrayQueryIsInvalid2()
+        public async Task BindModelAsync_EnumArrayQuery_BindValidOnly_2()
         {
             var queryParamName = "test";
             var queryParamString1 = "How";
             var queryParamString2 = "😱";
             var queryParamType = typeof(TestType[]);
 
-            var modelBinder = new CommaDelimitedArrayModelBinder();
+            var modelBinder = new CommaDelimitedArrayModelBinder(new NullLogger<CommaDelimitedArrayModelBinder>());
 
             var valueProvider = new QueryStringValueProvider(
                     new BindingSource(string.Empty, string.Empty, false, false),
@@ -217,9 +218,9 @@ namespace Jellyfin.Api.Tests.ModelBinders
             bindingContextMock.Setup(b => b.ModelType).Returns(queryParamType);
             bindingContextMock.SetupProperty(b => b.Result);
 
-            Func<Task> act = async () => await modelBinder.BindModelAsync(bindingContextMock.Object);
-
-            await Assert.ThrowsAsync<FormatException>(act);
+            await modelBinder.BindModelAsync(bindingContextMock.Object);
+            Assert.True(bindingContextMock.Object.Result.IsModelSet);
+            Assert.Single((TestType[])bindingContextMock.Object.Result.Model);
         }
     }
 }