Make `CollectionModelBinder` better extensible for custom model binders
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 290
Description
## 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](https://stackoverflow.com/questions/7807127/non-sequential-list-binding-not-working?rq=1) [not](https://stackoverflow.com/questions/36753338/non-sequential-indices-model-binding) [alone](https://www.mattbutton.com/2013/03/02/asp-net-mvc-issues-withbinding-a-non-sequential-list-with-the-default-model-binde/) with this, and that this behavior seems to bother people [at least since 2008](https://haacked.com/archive/2008/10/23/model-binding-to-a-list.aspx/).
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](https://github.com/dotnet/aspnetcore/blob/b8f5b5ea8cc007535b37f9625e6facd72dd7c857/src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs#L372) 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:
```diff
namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
public partial class CollectionModelBinder : ICollectionModelBinder
{
- private readonly int _maxModelBindingCollectionSize = MvcOptions.DefaultMaxModelBindingCollectionSize;
+ protected readonly int _maxModelBindingCollectionSize = MvcOptions.DefaultMaxModelBindingCollectionSize;
- internal async Task BindComplexCollectionFromIndexes(
+ protected internal virtual async Task BindComplexCollectionFromIndexes(
+ ModelBindingContext bindingContext,
+ IEnumerable? indexNames);
}
```
Second, the `CollectionModelBinderProvider` should gain a type parameter, to be able to provide custom collection model binders:
```diff
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();
var mvcOptions = context.Services.GetRequiredService>().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:
```diff
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:
```csharp
public class NonSequentialIndexCollectionModelBinder : CollectionModelBinder
{
// ... other ctors
[ActivatorUtilitiesConstructorAttribute]
public NonSequentialIndexCollectionModelBinder(IModelBinder elementBinder, ILoggerFactory loggerFactory, bool allowValidatingTopLevelNodes, MvcOptions mvcOptions) : base(elementBinder, loggerFactory, allowValidatingTopLevelNodes, mvcOptions)
{
}
protected override async Task BindComplexCollectionFromIndexes(
ModelBindingContext bindingContext,
IEnumerable? 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();
var foundIndexNames = new List();
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(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
};
}
}
```
```csharp
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.
Contributor guide
Assessment
This issue has not been assessed yet.