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 13, 2015
1 parent c6b47ea commit 55dfa4a
Show file tree
Hide file tree
Showing 19 changed files with 349 additions and 276 deletions.
7 changes: 5 additions & 2 deletions src/Microsoft.AspNet.Mvc.Core/ModelBinding/Validation/DefaultObjectValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,12 @@ private static bool ShallowValidate(
}
}

if (isValid)
if (modelState.ContainsKey(modelKey))
{
validationContext.ModelValidationContext.ModelState.MarkFieldValid(modelKey);
if (isValid)
{
validationContext.ModelValidationContext.ModelState.MarkFieldValid(modelKey);
}
}

return isValid;
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
44 changes: 27 additions & 17 deletions test/Microsoft.AspNet.Mvc.IntegrationTests/BinderTypeBasedModelBinderIntegrationTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ public async Task BindParameter_WithData_WithPrefix_GetsBound()
Assert.True(modelState.IsValid);
var key = Assert.Single(modelState.Keys);
Assert.Equal("CustomParameter", key);
Assert.Null(modelState[key].Value); // value is only set if the custom model binder sets it.
Assert.NotNull(modelState[key].Value); //Do not validate as its the test model binder which sets the value.
}

private class Person
Expand All @@ -177,7 +177,7 @@ private class Address
public string Street { get; set; }
}

[Fact(Skip = "Extra entries in model state #2446")]
[Fact]
public async Task BindProperty_WithData_EmptyPrefix_GetsBound()
{
// Arrange
Expand Down Expand Up @@ -209,16 +209,12 @@ public async Task BindProperty_WithData_EmptyPrefix_GetsBound()
// ModelState
Assert.True(modelState.IsValid);

// Should there be another key for what is there in the complex object ?
// This should probably behave like body binder, where even the body gets validated by default.
Assert.Equal(2, modelState.Keys.Count);
Assert.Equal(1, modelState.Keys.Count);
var key = Assert.Single(modelState.Keys, k => k == "Parameter1.Address.Street");
Assert.Null(modelState[key].Value); // value is only set if the custom model binder sets it.
key = Assert.Single(modelState.Keys, k => k == "Parameter1.Address");
Assert.Null(modelState[key].Value); // value is only set if the custom model binder sets it.
Assert.NotNull(modelState[key].Value); // Value is set by test model binder, no need to validate it.
}

[Fact(Skip = "Extra entries in model state #2446")]
[Fact]
public async Task BindProperty_WithData_WithPrefix_GetsBound()
{
// Arrange
Expand Down Expand Up @@ -252,14 +248,9 @@ public async Task BindProperty_WithData_WithPrefix_GetsBound()

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

// Should there be another key for what is there in the complex object ?
// This should probably behave like body binder, where even the body gets validated by default.
Assert.Equal(2, modelState.Keys.Count);
Assert.Equal(1, modelState.Keys.Count);
var key = Assert.Single(modelState.Keys, k => k == "CustomParameter.Address.Street");
Assert.Null(modelState[key].Value); // value is only set if the custom model binder sets it.
key = Assert.Single(modelState.Keys, k => k == "CustomParameter.Address");
Assert.Null(modelState[key].Value); // value is only set if the custom model binder sets it.
Assert.NotNull(modelState[key].Value); // Value is set by test model binder, no need to validate it.
}

private class AddressModelBinder : IModelBinder
Expand All @@ -273,7 +264,22 @@ public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContex

var address = new Address() { Street = "SomeStreet" };

return Task.FromResult(new ModelBindingResult(address, bindingContext.ModelName, true));
bindingContext.ModelState.SetModelValue(
bindingContext.ModelName + ".Street",
new ValueProviderResult(
address.Street,
address.Street,
System.Globalization.CultureInfo.CurrentCulture));

var validationNode = new ModelValidationNode(
bindingContext.ModelName,
bindingContext.ModelMetadata,
address)
{
ValidateAllProperties = true
};

return Task.FromResult(new ModelBindingResult(address, bindingContext.ModelName, true, validationNode));
}
}

Expand All @@ -282,6 +288,10 @@ private class SuccessModelBinder : IModelBinder
public Task<ModelBindingResult> BindModelAsync(ModelBindingContext bindingContext)
{
var model = "Success";
bindingContext.ModelState.SetModelValue(
bindingContext.ModelName,
new ValueProviderResult(model, model, System.Globalization.CultureInfo.CurrentCulture));

var modelValidationNode = new ModelValidationNode(
bindingContext.ModelName,
bindingContext.ModelMetadata,
Expand Down
Loading

0 comments on commit 55dfa4a

Please sign in to comment.