Make `CollectionModelBinder` better extensible for custom model binders

[dmorawetz]

Background and Motivation

I came across the need to bind a collection with non-sequential indices in my project. While researching I found that I am not alone with this, and that this behavior seems to bother people at least since 2008.

Our use case is, to have a list of objects in a form. Via HTMX new lines can be appended to the end, and rows in the middle can be removed. Thus we can guarantee, that indices are unique, but might have gaps between them.

I looked at the CollectionModelBinder and found, that our desired behavior could be achieved by changing the break here to a continue.

However, since the BindComplexCollectionFromIndexes method is internal, I can not derive a custom model binder and overwrite just this one part.

Furthermore, to reuse as much as possible from the CollectionModelBinderProvider, I suggest adding an optional model binder type parameter to the constructor.

I do not propose adding the NonSequentialIndexCollectionModelBinder to ASP.NET core. I am fine if this lives in our project code. However, with these few changes, we could reuse most of the standard code.

Proposed API

I suggest simple changes in three classes. First, the CollectionModelBinder should allow BindComplexCollectionFromIndexes to be overridden and make the needed _maxModelBindingCollectionSize available to derived classes:

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

public partial class CollectionModelBinder<TElement> : ICollectionModelBinder
{
-  private readonly int _maxModelBindingCollectionSize = MvcOptions.DefaultMaxModelBindingCollectionSize;
+  protected readonly int _maxModelBindingCollectionSize = MvcOptions.DefaultMaxModelBindingCollectionSize;

-    internal async Task<CollectionResult> BindComplexCollectionFromIndexes(
+    protected internal virtual async Task<CollectionResult> BindComplexCollectionFromIndexes(
+        ModelBindingContext bindingContext,
+        IEnumerable<string>? indexNames);
}

Second, the CollectionModelBinderProvider should gain a type parameter, to be able to provide custom collection model binders:

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;

-  public class CollectionModelBinderProvider : IModelBinderProvider
+ public class CollectionModelBinderProvider(Type? modelBinderType = null) : IModelBinderProvider
{
+    private readonly Type _modelBinderType = modelBinderType ?? typeof(CollectionModelBinder<>);
+
-    private static IModelBinder CreateInstance(ModelBinderProviderContext context, Type collectionType)
+    private IModelBinder CreateInstance(ModelBinderProviderContext context, Type collectionType)
    {
-       var binderType = typeof(CollectionModelBinder<>).MakeGenericType(collectionType.GenericTypeArguments);
+       var binderType = _modelBinderType.MakeGenericType(collectionType.GenericTypeArguments);
        var elementType = collectionType.GenericTypeArguments[0];
        var elementBinder = context.CreateBinder(context.MetadataProvider.GetMetadataForType(elementType));

        var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
        var mvcOptions = context.Services.GetRequiredService<IOptions<MvcOptions>>().Value;
        var binder = (IModelBinder)Activator.CreateInstance(
            binderType,
            elementBinder,
            loggerFactory,
            true /* allowValidatingTopLevelNodes */,
            mvcOptions)!;

        return binder;
    }
}

Third, the ExplicitIndexCollectionValidationStrategy should become public, so that it can be used in custom model binders:

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation;

-  internal sealed class ExplicitIndexCollectionValidationStrategy : IValidationStrategy
+  public sealed class ExplicitIndexCollectionValidationStrategy : IValidationStrategy

Usage Examples

Then we can derive a simple deviating model binder, that allows gaps and non-zero based indices:

public class NonSequentialIndexCollectionModelBinder<TElement> : CollectionModelBinder<TElement>
{
    // ... other ctors

    [ActivatorUtilitiesConstructorAttribute]
    public NonSequentialIndexCollectionModelBinder(IModelBinder elementBinder, ILoggerFactory loggerFactory, bool allowValidatingTopLevelNodes, MvcOptions mvcOptions) : base(elementBinder, loggerFactory, allowValidatingTopLevelNodes, mvcOptions)
    {
    }

    protected override async Task<CollectionResult> BindComplexCollectionFromIndexes(
            ModelBindingContext bindingContext,
            IEnumerable<string>? indexNames)
    {
        bool indexNamesIsFinite;
        if (indexNames != null)
        {
            indexNamesIsFinite = true;
        }
        else
        {
            indexNamesIsFinite = false;
            var limit = _maxModelBindingCollectionSize == int.MaxValue ? int.MaxValue : _maxModelBindingCollectionSize + 1;
            indexNames = Enumerable
                .Range(0, limit)
                .Select(i => i.ToString(CultureInfo.InvariantCulture));
        }

        var elementMetadata = bindingContext.ModelMetadata.ElementMetadata!;

        var boundCollection = new List<TElement?>();
        var foundIndexNames = new List<string>();

        foreach (var indexName in indexNames)
        {
            var fullChildName = ModelNames.CreateIndexModelName(bindingContext.ModelName, indexName);

            ModelBindingResult? result;
            using (bindingContext.EnterNestedScope(
                       elementMetadata,
                       fieldName: indexName,
                       modelName: fullChildName,
                       model: null))
            {
                await ElementBinder.BindModelAsync(bindingContext);
                result = bindingContext.Result;
            }

            var didBind = false;
            object? boundValue = null;
            if (result != null && result.Value.IsModelSet)
            {
                didBind = true;
                boundValue = result.Value.Model;
            }

            // infinite size collection stops on first bind failure
            if (!didBind && !indexNamesIsFinite)
            {
                continue;
            }

            boundCollection.Add(CastOrDefault<TElement>(boundValue));
            foundIndexNames.Add(indexName);
        }

        // Did the collection grow larger than the limit?
        if (boundCollection.Count > _maxModelBindingCollectionSize)
        {
            throw new InvalidOperationException("Exceeded maximum number of elements per model.");
        }

        return new CollectionResult(boundCollection)
        {
            ValidationStrategy = indexNamesIsFinite
                ? new ExplicitIndexCollectionValidationStrategy(indexNames)
                : new ExplicitIndexCollectionValidationStrategy(foundIndexNames), // this is needed for validation to work properly
        };
    }
}

public class NonSequentialIndexCollectionModelBinderProvider : CollectionModelBinderProvider(typeof(NonSequentialIndexCollectionModelBinder<>))
{
    
}

Alternative Designs

The most suggested workaround seems to be to add hidden fields for the Index, but this feels clunky.

Risks

As this change only allows to reuse ASP.NET code as much as possible in a custom model binder and not change any behavior or usage, I see no risks.