Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Commit

Permalink
Fixing #2466, #2446.
Browse files Browse the repository at this point in the history 
The assumption is ModelState should have entries if
1. An error is explicitly added by a model binder.
2. There is validation error reported while validating the model.
3. There is value bound by the model binder.

With this change there should be no extra entry other than for the cases mentioned above.

Also enabling the integration test cases.
  • Loading branch information
@harshgMSFT
harshgMSFT committed May 15, 2015
1 parent 12e4307 commit 88ac4b9
Show file tree
Hide file tree
Showing 19 changed files with 352 additions and 274 deletions.
4 changes: 3 additions & 1 deletion src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,9 @@ private static bool ShallowValidate(
}
}

if (isValid)
// Add an entry only if there was an entry which was added by a model binder.
// This prevents adding spurious entries.
if (modelState.ContainsKey(modelKey) && isValid)
{
validationContext.ModelValidationContext.ModelState.MarkFieldValid(modelKey);
}
Expand Down
39 changes: 20 additions & 19 deletions test/Microsoft.AspNet.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,20 @@ public Person3()
public List<Address> Address { get; }
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_NonSettableCollectionModel_EmptyPrefix_GetsBound()
{
// Arrange
var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
var parameter = new ParameterDescriptor()
{
Name = "Address",
Name = "prefix",
ParameterType = typeof(Person3)
};

var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
{
request.QueryString = QueryString.Create("[0].Street", "SomeStreet");
request.QueryString = QueryString.Create("Address[0].Street", "SomeStreet");
});

var modelState = new ModelStateDictionary();
Expand All @@ -64,7 +64,7 @@ public async Task ActionParameter_NonSettableCollectionModel_EmptyPrefix_GetsBou
Assert.True(modelState.IsValid);

Assert.Equal(1, modelState.Keys.Count);
var key = Assert.Single(modelState.Keys, k => k == "[0].Street");
var key = Assert.Single(modelState.Keys, k => k == "Address[0].Street");
Assert.NotNull(modelState[key].Value);
Assert.Equal("SomeStreet", modelState[key].Value.AttemptedValue);
Assert.Equal("SomeStreet", modelState[key].Value.RawValue);
Expand All @@ -77,19 +77,19 @@ private class Person6
public CustomReadOnlyCollection<Address> Address { get; set; }
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_ReadOnlyCollectionModel_EmptyPrefix_DoesNotGetBound()
{
// Arrange
var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
var parameter = new ParameterDescriptor()
{
Name = "Address",
Name = "prefix",
ParameterType = typeof(Person6)
};
var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
{
request.QueryString = QueryString.Create("[0].Street", "SomeStreet");
request.QueryString = QueryString.Create("Address[0].Street", "SomeStreet");
});

var modelState = new ModelStateDictionary();
Expand Down Expand Up @@ -119,20 +119,20 @@ private class Person4
public Address[] Address { get; set; }
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_SettableArrayModel_EmptyPrefix_GetsBound()
{
// Arrange
var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
var parameter = new ParameterDescriptor()
{
Name = "Address",
Name = "prefix",
ParameterType = typeof(Person4)
};

var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
{
request.QueryString = QueryString.Create("[0].Street", "SomeStreet");
request.QueryString = QueryString.Create("Address[0].Street", "SomeStreet");
});

var modelState = new ModelStateDictionary();
Expand All @@ -147,14 +147,15 @@ public async Task ActionParameter_SettableArrayModel_EmptyPrefix_GetsBound()
// Model
Assert.NotNull(modelBindingResult.Model);
var boundModel = Assert.IsType<Person4>(modelBindingResult.Model);
Assert.NotNull(boundModel.Address);
Assert.Equal(1, boundModel.Address.Count());
Assert.Equal("SomeStreet", boundModel.Address[0].Street);

// ModelState
Assert.True(modelState.IsValid);

Assert.Equal(1, modelState.Keys.Count);
var key = Assert.Single(modelState.Keys, k => k == "[0].Street");
var key = Assert.Single(modelState.Keys, k => k == "Address[0].Street");
Assert.NotNull(modelState[key].Value);
Assert.Equal("SomeStreet", modelState[key].Value.AttemptedValue);
Assert.Equal("SomeStreet", modelState[key].Value.RawValue);
Expand All @@ -167,20 +168,20 @@ private class Person5
public Address[] Address { get; } = new Address[] { };
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_NonSettableArrayModel_EmptyPrefix_DoesNotGetBound()
{
// Arrange
var argumentBinder = ModelBindingTestHelper.GetArgumentBinder();
var parameter = new ParameterDescriptor()
{
Name = "Address",
Name = "prefix",
ParameterType = typeof(Person5)
};

var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
{
request.QueryString = QueryString.Create("[0].Street", "SomeStreet");
request.QueryString = QueryString.Create("Address[0].Street", "SomeStreet");
});

var modelState = new ModelStateDictionary();
Expand All @@ -204,7 +205,7 @@ public async Task ActionParameter_NonSettableArrayModel_EmptyPrefix_DoesNotGetBo
Assert.Empty(modelState.Keys);
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_NonSettableCollectionModel_WithPrefix_GetsBound()
{
// Arrange
Expand Down Expand Up @@ -251,7 +252,7 @@ public async Task ActionParameter_NonSettableCollectionModel_WithPrefix_GetsBoun
Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState);
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_ReadOnlyCollectionModel_WithPrefix_DoesNotGetBound()
{
// Arrange
Expand Down Expand Up @@ -292,7 +293,7 @@ public async Task ActionParameter_ReadOnlyCollectionModel_WithPrefix_DoesNotGetB
Assert.Empty(modelState.Keys);
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_SettableArrayModel_WithPrefix_GetsBound()
{
// Arrange
Expand Down Expand Up @@ -339,7 +340,7 @@ public async Task ActionParameter_SettableArrayModel_WithPrefix_GetsBound()
Assert.Equal(ModelValidationState.Valid, modelState[key].ValidationState);
}

[Fact(Skip = "Extra entries in model state dictionary. #2466")]
[Fact]
public async Task ActionParameter_NonSettableArrayModel_WithPrefix_DoesNotGetBound()
{
// Arrange
Expand Down Expand Up @@ -369,7 +370,7 @@ public async Task ActionParameter_NonSettableArrayModel_WithPrefix_DoesNotGetBou

// Model
Assert.NotNull(modelBindingResult.Model);
var boundModel = Assert.IsType<Person4>(modelBindingResult.Model);
var boundModel = Assert.IsType<Person5>(modelBindingResult.Model);

// Arrays should not be updated.
Assert.Equal(0, boundModel.Address.Count());
Expand Down
26 changes: 13 additions & 13 deletions test/Microsoft.AspNet.Mvc.IntegrationTests/ArrayModelBinderIntegrationTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace Microsoft.AspNet.Mvc.IntegrationTests
// Integration tests targeting the behavior of the ArrayModelBinder with other model binders.
public class ArrayModelBinderIntegrationTest
{
[Fact(Skip = "Extra ModelState key because of #2446")]
[Fact]
public async Task ArrayModelBinder_BindsArrayOfSimpleType_WithPrefix_Success()
{
// Arrange
Expand Down Expand Up @@ -39,7 +39,7 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_WithPrefix_Success()
var model = Assert.IsType<int[]>(modelBindingResult.Model);
Assert.Equal(new int[] { 10, 11 }, model);

Assert.Equal(2, modelState.Count); // This fails due to #2446
Assert.Equal(2, modelState.Count);
Assert.Equal(0, modelState.ErrorCount);
Assert.True(modelState.IsValid);

Expand All @@ -52,7 +52,7 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_WithPrefix_Success()
Assert.Equal("11", entry.Value.RawValue);
}

[Fact(Skip = "Extra ModelState key because of #2446")]
[Fact]
public async Task ArrayModelBinder_BindsArrayOfSimpleType_WithExplicitPrefix_Success()
{
// Arrange
Expand Down Expand Up @@ -84,7 +84,7 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_WithExplicitPrefix_Suc
var model = Assert.IsType<int[]>(modelBindingResult.Model);
Assert.Equal(new int[] { 10, 11 }, model);

Assert.Equal(2, modelState.Count); // This fails due to #2446
Assert.Equal(2, modelState.Count);
Assert.Equal(0, modelState.ErrorCount);
Assert.True(modelState.IsValid);

Expand All @@ -97,7 +97,7 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_WithExplicitPrefix_Suc
Assert.Equal("11", entry.Value.RawValue);
}

[Fact(Skip = "Extra ModelState key because of #2446")]
[Fact]
public async Task ArrayModelBinder_BindsArrayOfSimpleType_EmptyPrefix_Success()
{
// Arrange
Expand Down Expand Up @@ -125,7 +125,7 @@ public async Task ArrayModelBinder_BindsArrayOfSimpleType_EmptyPrefix_Success()
var model = Assert.IsType<int[]>(modelBindingResult.Model);
Assert.Equal(new int[] { 10, 11 }, model);

Assert.Equal(2, modelState.Count); // This fails due to #2446
Assert.Equal(2, modelState.Count);
Assert.Equal(0, modelState.ErrorCount);
Assert.True(modelState.IsValid);

Expand Down Expand Up @@ -174,7 +174,7 @@ private class Person
public string Name { get; set; }
}

[Fact(Skip = "Extra ModelState key because of #2446")]
[Fact]
public async Task ArrayModelBinder_BindsArrayOfComplexType_WithPrefix_Success()
{
// Arrange
Expand Down Expand Up @@ -203,7 +203,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_WithPrefix_Success()
Assert.Equal("bill", model[0].Name);
Assert.Equal("lang", model[1].Name);

Assert.Equal(2, modelState.Count); // This fails due to #2446
Assert.Equal(2, modelState.Count);
Assert.Equal(0, modelState.ErrorCount);
Assert.True(modelState.IsValid);

Expand All @@ -216,7 +216,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_WithPrefix_Success()
Assert.Equal("lang", entry.Value.RawValue);
}

[Fact(Skip = "Extra ModelState key because of #2446")]
[Fact]
public async Task ArrayModelBinder_BindsArrayOfComplexType_WithExplicitPrefix_Success()
{
// Arrange
Expand All @@ -233,7 +233,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_WithExplicitPrefix_Su

var operationContext = ModelBindingTestHelper.GetOperationBindingContext(request =>
{
request.QueryString = new QueryString("?prefix[0].Name=bill&prefix[1]=lang");
request.QueryString = new QueryString("?prefix[0].Name=bill&prefix[1].Name=lang");
});

var modelState = new ModelStateDictionary();
Expand All @@ -249,7 +249,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_WithExplicitPrefix_Su
Assert.Equal("bill", model[0].Name);
Assert.Equal("lang", model[1].Name);

Assert.Equal(2, modelState.Count); // This fails due to #2446
Assert.Equal(2, modelState.Count);
Assert.Equal(0, modelState.ErrorCount);
Assert.True(modelState.IsValid);

Expand All @@ -262,7 +262,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_WithExplicitPrefix_Su
Assert.Equal("lang", entry.Value.RawValue);
}

[Fact(Skip = "Extra ModelState key because of #2446")]
[Fact]
public async Task ArrayModelBinder_BindsArrayOfComplexType_EmptyPrefix_Success()
{
// Arrange
Expand Down Expand Up @@ -291,7 +291,7 @@ public async Task ArrayModelBinder_BindsArrayOfComplexType_EmptyPrefix_Success()
Assert.Equal("bill", model[0].Name);
Assert.Equal("lang", model[1].Name);

Assert.Equal(2, modelState.Count); // This fails due to #2446
Assert.Equal(2, modelState.Count);
Assert.Equal(0, modelState.ErrorCount);
Assert.True(modelState.IsValid);

Expand Down
Loading

0 comments on commit 88ac4b9

Please sign in to comment.