Skip to content

Commit 62e8a69

Browse files
committed
Normalize ModelState keys for collection binding
Remap ModelState entries for collections when explicit numeric indices are provided so rendered views lookup values correctly. Adds tracking of explicit index names, a NormalizeCollectionModelStateKeys helper that snapshots and remaps keys from non-sequential explicit indices (e.g. [0],[10],[1]) to sequential indices (0,1,2...), and removes the previous explicit-index validation strategy in favor of the default sequential strategy. Includes unit tests for non-sequential and sequential index scenarios and a test binder helper (CreateIntBinderWithModelState) to exercise ModelState behavior. This fixes incorrect ModelState lookups observed when re-rendering views after validation failures (regression referenced in issue #26217).
1 parent 83c5b49 commit 62e8a69

File tree

2 files changed

+194
-12
lines changed

2 files changed

+194
-12
lines changed

src/Mvc/Mvc.Core/src/ModelBinding/Binders/CollectionModelBinder.cs

Lines changed: 80 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -343,8 +343,12 @@ internal async Task<CollectionResult> BindComplexCollectionFromIndexes(
343343

344344
var boundCollection = new List<TElement?>();
345345

346+
// Track explicit index names for normalization when using finite indices.
347+
List<string>? explicitIndexNames = indexNamesIsFinite ? new List<string>() : null;
348+
346349
foreach (var indexName in indexNames)
347350
{
351+
explicitIndexNames?.Add(indexName);
348352
var fullChildName = ModelNames.CreateIndexModelName(bindingContext.ModelName, indexName);
349353

350354
ModelBindingResult? result;
@@ -394,20 +398,87 @@ internal async Task<CollectionResult> BindComplexCollectionFromIndexes(
394398
bindingContext.ModelMetadata.ElementType));
395399
}
396400

401+
// Normalize ModelState keys from explicit indices to sequential indices so that
402+
// re-rendered views (which iterate collections sequentially) look up the correct
403+
// ModelState values. Without this, non-sequential explicit indices like [0],[10],[1],[2]
404+
// cause data corruption when the page is re-rendered after a validation failure.
405+
if (indexNamesIsFinite && explicitIndexNames is not null)
406+
{
407+
NormalizeCollectionModelStateKeys(bindingContext.ModelState, bindingContext.ModelName, explicitIndexNames);
408+
}
409+
397410
return new CollectionResult(boundCollection)
398411
{
399-
// If we're working with a fixed set of indexes then this is the format like:
400-
//
401-
// ?parameter.index=zero&parameter.index=one&parameter.index=two&parameter[zero]=0&parameter[one]=1&parameter[two]=2...
402-
//
403-
// We need to provide this data to the validation system so it can 'replay' the keys.
404-
// But we can't just set ValidationState here, because it needs the 'real' model.
405-
ValidationStrategy = indexNamesIsFinite ?
406-
new ExplicitIndexCollectionValidationStrategy(indexNames) :
407-
null,
412+
// After normalizing explicit indices to sequential indices, the default
413+
// sequential validation strategy is correct. For non-finite (sequential) indices,
414+
// the default strategy is also used.
415+
ValidationStrategy = null,
408416
};
409417
}
410418

419+
/// <summary>
420+
/// Remaps ModelState entries from explicit (potentially non-sequential) collection indices
421+
/// to sequential indices (0, 1, 2, ...) so that rendered views can correctly look up
422+
/// ModelState values for collection elements.
423+
/// </summary>
424+
private static void NormalizeCollectionModelStateKeys(
425+
ModelStateDictionary modelState,
426+
string modelName,
427+
List<string> explicitIndexNames)
428+
{
429+
// Check if any index differs from its sequential position.
430+
var needsNormalization = false;
431+
for (var i = 0; i < explicitIndexNames.Count; i++)
432+
{
433+
if (!string.Equals(explicitIndexNames[i], i.ToString(CultureInfo.InvariantCulture), StringComparison.Ordinal))
434+
{
435+
needsNormalization = true;
436+
break;
437+
}
438+
}
439+
440+
if (!needsNormalization)
441+
{
442+
return;
443+
}
444+
445+
// Collect all entries that need remapping. We snapshot the data before making any
446+
// changes to avoid issues with overlapping indices (e.g., swapping [0] and [1]).
447+
var entriesToRemap = new List<(string OldKey, string NewKey, object? RawValue, string? AttemptedValue)>();
448+
449+
for (var i = 0; i < explicitIndexNames.Count; i++)
450+
{
451+
var explicitIndex = explicitIndexNames[i];
452+
var sequentialIndex = i.ToString(CultureInfo.InvariantCulture);
453+
454+
if (string.Equals(explicitIndex, sequentialIndex, StringComparison.Ordinal))
455+
{
456+
continue;
457+
}
458+
459+
var explicitPrefix = ModelNames.CreateIndexModelName(modelName, explicitIndex);
460+
var sequentialPrefix = ModelNames.CreateIndexModelName(modelName, sequentialIndex);
461+
462+
foreach (var kvp in modelState.FindKeysWithPrefix(explicitPrefix))
463+
{
464+
var oldKey = kvp.Key;
465+
var newKey = string.Concat(sequentialPrefix, oldKey.AsSpan(explicitPrefix.Length));
466+
entriesToRemap.Add((oldKey, newKey, kvp.Value!.RawValue, kvp.Value.AttemptedValue));
467+
}
468+
}
469+
470+
// Remove old entries first, then add new ones to avoid key conflicts.
471+
foreach (var (oldKey, _, _, _) in entriesToRemap)
472+
{
473+
modelState.Remove(oldKey);
474+
}
475+
476+
foreach (var (_, newKey, rawValue, attemptedValue) in entriesToRemap)
477+
{
478+
modelState.SetModelValue(newKey, rawValue, attemptedValue);
479+
}
480+
}
481+
411482
// Internal for testing.
412483
internal sealed class CollectionResult
413484
{

src/Mvc/Mvc.Core/test/ModelBinding/Binders/CollectionModelBinderTest.cs

Lines changed: 114 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,86 @@ public async Task BindComplexCollectionFromIndexes_FiniteIndexes()
3232
// Assert
3333
Assert.Equal(new[] { 42, 0, 200 }, collectionResult.Model.ToArray());
3434

35-
// This requires a non-default IValidationStrategy
36-
var strategy = Assert.IsType<ExplicitIndexCollectionValidationStrategy>(collectionResult.ValidationStrategy);
37-
Assert.Equal(new[] { "foo", "bar", "baz" }, strategy.ElementKeys);
35+
// After normalization, explicit indices use the default sequential validation strategy
36+
Assert.Null(collectionResult.ValidationStrategy);
37+
}
38+
39+
[Fact]
40+
public async Task BindComplexCollectionFromIndexes_NonSequentialNumericIndexes_NormalizesModelStateKeys()
41+
{
42+
// Arrange - reproduces https://github.com/dotnet/aspnetcore/issues/26217
43+
// When explicit indices are non-sequential (e.g., [0],[10],[1],[2]), the ModelState keys
44+
// must be normalized to sequential indices so that re-rendered views display correct values.
45+
var valueProvider = new SimpleValueProvider
46+
{
47+
{ "someName[0]", "100" },
48+
{ "someName[10]", "200" },
49+
{ "someName[1]", "300" },
50+
{ "someName[2]", "400" }
51+
};
52+
var bindingContext = GetModelBindingContext(valueProvider);
53+
var modelState = bindingContext.ModelState;
54+
var binder = new CollectionModelBinder<int>(CreateIntBinderWithModelState(), NullLoggerFactory.Instance);
55+
56+
// Act
57+
var collectionResult = await binder.BindComplexCollectionFromIndexes(
58+
bindingContext,
59+
new[] { "0", "10", "1", "2" });
60+
61+
// Assert - collection is bound in explicit index order
62+
Assert.Equal(new[] { 100, 200, 300, 400 }, collectionResult.Model.ToArray());
63+
64+
// ModelState keys should be normalized to sequential indices
65+
Assert.True(modelState.TryGetValue("someName[0]", out var entry0));
66+
Assert.Equal("100", entry0!.AttemptedValue);
67+
68+
Assert.True(modelState.TryGetValue("someName[1]", out var entry1));
69+
Assert.Equal("200", entry1!.AttemptedValue);
70+
71+
Assert.True(modelState.TryGetValue("someName[2]", out var entry2));
72+
Assert.Equal("300", entry2!.AttemptedValue);
73+
74+
Assert.True(modelState.TryGetValue("someName[3]", out var entry3));
75+
Assert.Equal("400", entry3!.AttemptedValue);
76+
77+
// Old non-sequential keys should no longer exist
78+
Assert.False(modelState.ContainsKey("someName[10]"));
79+
80+
// Uses default sequential validation strategy after normalization
81+
Assert.Null(collectionResult.ValidationStrategy);
82+
}
83+
84+
[Fact]
85+
public async Task BindComplexCollectionFromIndexes_SequentialIndexes_NoNormalizationNeeded()
86+
{
87+
// Arrange - sequential indices should not trigger normalization
88+
var valueProvider = new SimpleValueProvider
89+
{
90+
{ "someName[0]", "100" },
91+
{ "someName[1]", "200" },
92+
{ "someName[2]", "300" }
93+
};
94+
var bindingContext = GetModelBindingContext(valueProvider);
95+
var modelState = bindingContext.ModelState;
96+
var binder = new CollectionModelBinder<int>(CreateIntBinderWithModelState(), NullLoggerFactory.Instance);
97+
98+
// Act
99+
var collectionResult = await binder.BindComplexCollectionFromIndexes(
100+
bindingContext,
101+
new[] { "0", "1", "2" });
102+
103+
// Assert
104+
Assert.Equal(new[] { 100, 200, 300 }, collectionResult.Model.ToArray());
105+
106+
// ModelState keys should remain unchanged
107+
Assert.True(modelState.TryGetValue("someName[0]", out var entry0));
108+
Assert.Equal("100", entry0!.AttemptedValue);
109+
110+
Assert.True(modelState.TryGetValue("someName[1]", out var entry1));
111+
Assert.Equal("200", entry1!.AttemptedValue);
112+
113+
Assert.True(modelState.TryGetValue("someName[2]", out var entry2));
114+
Assert.Equal("300", entry2!.AttemptedValue);
38115
}
39116

40117
[Fact]
@@ -526,6 +603,40 @@ private static IModelBinder CreateIntBinder()
526603
});
527604
}
528605

606+
private static IModelBinder CreateIntBinderWithModelState()
607+
{
608+
return new StubModelBinder(context =>
609+
{
610+
var value = context.ValueProvider.GetValue(context.ModelName);
611+
if (value == ValueProviderResult.None)
612+
{
613+
return ModelBindingResult.Failed();
614+
}
615+
616+
context.ModelState.SetModelValue(context.ModelName, value);
617+
618+
object valueToConvert = null;
619+
if (value.Values.Count == 1)
620+
{
621+
valueToConvert = value.Values[0];
622+
}
623+
else if (value.Values.Count > 1)
624+
{
625+
valueToConvert = value.Values.ToArray();
626+
}
627+
628+
var model = ModelBindingHelper.ConvertTo(valueToConvert, context.ModelType, value.Culture);
629+
if (model == null)
630+
{
631+
return ModelBindingResult.Failed();
632+
}
633+
else
634+
{
635+
return ModelBindingResult.Success(model);
636+
}
637+
});
638+
}
639+
529640
private static DefaultModelBindingContext CreateContext()
530641
{
531642
var actionContext = new ActionContext()

0 commit comments

Comments
 (0)