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

Commit

Permalink
[Fixes #5801] Move call to validate constructor in ComplexTypeModelBi…
Browse files Browse the repository at this point in the history 
…nder into CreateModel
  • Loading branch information
@kichalla
kichalla committed Feb 15, 2017
1 parent 531c11d commit 29647fd
Show file tree
Hide file tree
Showing 3 changed files with 153 additions and 33 deletions.
43 changes: 20 additions & 23 deletions src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/Binders/ComplexTypeModelBinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,28 +48,6 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
return TaskCache.CompletedTask;
}

// The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as
// reflection does not provide information about the implicit parameterless constructor for a struct.
// This binder would eventually fail to construct an instance of the struct as the Linq's NewExpression
// compile fails to construct it.
var modelTypeInfo = bindingContext.ModelType.GetTypeInfo();
if (bindingContext.Model == null &&
(modelTypeInfo.IsAbstract ||
modelTypeInfo.GetConstructor(Type.EmptyTypes) == null))
{
if (bindingContext.IsTopLevelObject)
{
throw new InvalidOperationException(
Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(modelTypeInfo.FullName));
}

throw new InvalidOperationException(
Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty(
modelTypeInfo.FullName,
bindingContext.ModelName,
bindingContext.ModelMetadata.ContainerType.FullName));
}

// Perf: separated to avoid allocating a state machine when we don't
// need to go async.
return BindModelCoreAsync(bindingContext);
Expand Down Expand Up @@ -345,13 +323,32 @@ protected virtual object CreateModel(ModelBindingContext bindingContext)
// application developer should know that this was an invalid type to try to bind to.
if (_modelCreator == null)
{
// The following check causes the ComplexTypeModelBinder to NOT participate in binding structs as
// reflection does not provide information about the implicit parameterless constructor for a struct.
// This binder would eventually fail to construct an instance of the struct as the Linq's NewExpression
// compile fails to construct it.
var modelTypeInfo = bindingContext.ModelType.GetTypeInfo();
if (modelTypeInfo.IsAbstract || modelTypeInfo.GetConstructor(Type.EmptyTypes) == null)
{
if (bindingContext.IsTopLevelObject)
{
throw new InvalidOperationException(
Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject(modelTypeInfo.FullName));
}

throw new InvalidOperationException(
Resources.FormatComplexTypeModelBinder_NoParameterlessConstructor_ForProperty(
modelTypeInfo.FullName,
bindingContext.ModelName,
bindingContext.ModelMetadata.ContainerType.FullName));
}

_modelCreator = Expression
.Lambda<Func<object>>(Expression.New(bindingContext.ModelType))
.Compile();
}

return _modelCreator();

}

/// <summary>
Expand Down
79 changes: 70 additions & 9 deletions test/Microsoft.AspNetCore.Mvc.Core.Test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@
using System.ComponentModel;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Reflection;
using System.Runtime.Serialization;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Internal;
using Microsoft.AspNetCore.Mvc.ModelBinding.Internal;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
using Microsoft.AspNetCore.Testing;
using Microsoft.Extensions.DependencyInjection;
using Moq;
Expand Down Expand Up @@ -349,6 +348,52 @@ public void CreateModel_InstantiatesInstanceOfMetadataType()
Assert.IsType<Person>(model);
}

[Fact]
public void CreateModel_ForStructModelType_AsTopLevelObject_ThrowsException()
{
// Arrange
var bindingContext = new DefaultModelBindingContext
{
ModelMetadata = GetMetadataForType(typeof(PointStruct)),
IsTopLevelObject = true
};
var binder = CreateBinder(bindingContext.ModelMetadata);

// Act & Assert
var exception = Assert.Throws<InvalidOperationException>(() => binder.CreateModelPublic(bindingContext));
Assert.Equal(
string.Format(
"Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " +
"value types and must have a parameterless constructor.",
typeof(PointStruct).FullName),
exception.Message);
}

[Fact]
public void CreateModel_ForStructModelType_AsProperty_ThrowsException()
{
// Arrange
var bindingContext = new DefaultModelBindingContext
{
ModelMetadata = GetMetadataForProperty(typeof(Location), nameof(Location.Point)),
ModelName = nameof(Location.Point),
IsTopLevelObject = false
};
var binder = CreateBinder(bindingContext.ModelMetadata);

// Act & Assert
var exception = Assert.Throws<InvalidOperationException>(() => binder.CreateModelPublic(bindingContext));
Assert.Equal(
string.Format(
"Could not create an instance of type '{0}'. Model bound complex types must not be abstract or " +
"value types and must have a parameterless constructor. Alternatively, set the '{1}' property to" +
" a non-null value in the '{2}' constructor.",
typeof(PointStruct).FullName,
nameof(Location.Point),
typeof(Location).FullName),
exception.Message);
}

[Fact]
public async Task BindModelAsync_ModelIsNotNull_DoesNotCallCreateModel()
{
Expand Down Expand Up @@ -758,7 +803,7 @@ public void SetProperty_PropertyHasDefaultValue_DefaultValueAttributeDoesNothing
// Arrange
var model = new Person();
var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);

var metadata = GetMetadataForType(typeof(Person));
var propertyMetadata = metadata.Properties[nameof(model.PropertyWithDefaultValue)];

Expand All @@ -780,7 +825,7 @@ public void SetProperty_PropertyIsPreinitialized_NoValue_DoesNothing()
// Arrange
var model = new Person();
var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);

var metadata = GetMetadataForType(typeof(Person));
var propertyMetadata = metadata.Properties[nameof(model.PropertyWithInitializedValue)];

Expand All @@ -804,7 +849,7 @@ public void SetProperty_PropertyIsPreinitialized_DefaultValueAttributeDoesNothin
// Arrange
var model = new Person();
var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);

var metadata = GetMetadataForType(typeof(Person));
var propertyMetadata = metadata.Properties[nameof(model.PropertyWithInitializedValueAndDefault)];

Expand All @@ -828,7 +873,7 @@ public void SetProperty_PropertyIsReadOnly_DoesNothing()
// Arrange
var model = new Person();
var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);

var metadata = GetMetadataForType(typeof(Person));
var propertyMetadata = metadata.Properties[nameof(model.NonUpdateableProperty)];

Expand Down Expand Up @@ -917,7 +962,7 @@ public void SetProperty_PropertyIsSettable_CallsSetter()
// Arrange
var model = new Person();
var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);

var metadata = GetMetadataForType(typeof(Person));
var propertyMetadata = bindingContext.ModelMetadata.Properties[nameof(model.DateOfBirth)];

Expand All @@ -943,7 +988,7 @@ public void SetProperty_PropertyIsSettable_SetterThrows_RecordsError()
};

var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);

var metadata = GetMetadataForType(typeof(Person));
var propertyMetadata = bindingContext.ModelMetadata.Properties[nameof(model.DateOfDeath)];

Expand All @@ -967,7 +1012,7 @@ public void SetProperty_PropertySetterThrows_CapturesException()
var model = new ModelWhosePropertySetterThrows();
var bindingContext = CreateContext(GetMetadataForType(model.GetType()), model);
bindingContext.ModelName = "foo";

var metadata = GetMetadataForType(typeof(ModelWhosePropertySetterThrows));
var propertyMetadata = bindingContext.ModelMetadata.Properties[nameof(model.NameNoAttribute)];

Expand Down Expand Up @@ -1036,6 +1081,22 @@ private static ModelMetadata GetMetadataForProperty(Type type, string propertyNa
return _metadataProvider.GetMetadataForProperty(type, propertyName);
}

private class Location
{
public PointStruct Point { get; set; }
}

private struct PointStruct
{
public PointStruct(double x, double y)
{
X = x;
Y = y;
}
public double X { get; }
public double Y { get; }
}

private class BindingOptionalProperty
{
[BindingBehavior(BindingBehavior.Optional)]
Expand Down
64 changes: 63 additions & 1 deletion test/Microsoft.AspNetCore.Mvc.IntegrationTests/ActionParametersIntegrationTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Mvc.ModelBinding;
using Microsoft.AspNetCore.Mvc.ModelBinding.Binders;
using Xunit;

namespace Microsoft.AspNetCore.Mvc.IntegrationTests
Expand Down Expand Up @@ -460,6 +461,34 @@ public async Task ActionParameter_BindingToTypeWithNoParameterlessConstructor_Th
exception.Message);
}

[Fact]
public async Task ActionParameter_CustomModelBinder_CanCreateModels_ForParameterlessConstructorTypes()
{
// Arrange
var argumentBinder = ModelBindingTestHelper.GetArgumentBinder(binderProvider: new CustomComplexTypeModelBinderProvider());
var parameter = new ParameterDescriptor()
{
Name = "prefix",
ParameterType = typeof(ClassWithNoDefaultConstructor)
};
var testContext = ModelBindingTestHelper.GetTestContext();
var modelState = testContext.ModelState;

// Act
var modelBindingResult = await argumentBinder.BindModelAsync(parameter, testContext);

// Assert
Assert.True(modelBindingResult.IsModelSet);

// Model
Assert.NotNull(modelBindingResult.Model);
var boundModel = Assert.IsType<ClassWithNoDefaultConstructor>(modelBindingResult.Model);
Assert.Equal(100, boundModel.Id);

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

private struct PointStruct
{
public PointStruct(double x, double y)
Expand All @@ -479,8 +508,12 @@ private class Class1

private class ClassWithNoDefaultConstructor
{
public ClassWithNoDefaultConstructor(int id) { }
public ClassWithNoDefaultConstructor(int id)
{
Id = id;
}
public string City { get; set; }
public int Id { get; }
}

private abstract class AbstractClassWithNoDefaultConstructor
Expand Down Expand Up @@ -562,5 +595,34 @@ System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator()
return GetEnumerator();
}
}

// By default the ComplexTypeModelBinder fails to construct models for types with no parameterless constructor,
// but a developer could change this behavior by overridng CreateModel
private class CustomComplexTypeModelBinder : ComplexTypeModelBinder
{
public CustomComplexTypeModelBinder(IDictionary<ModelMetadata, IModelBinder> propertyBinders)
: base(propertyBinders)
{
}

protected override object CreateModel(ModelBindingContext bindingContext)
{
Assert.Equal(typeof(ClassWithNoDefaultConstructor), bindingContext.ModelType);
return new ClassWithNoDefaultConstructor(100);
}
}

private class CustomComplexTypeModelBinderProvider : IModelBinderProvider
{
public IModelBinder GetBinder(ModelBinderProviderContext context)
{
var propertyBinders = new Dictionary<ModelMetadata, IModelBinder>();
foreach (var property in context.Metadata.Properties)
{
propertyBinders.Add(property, context.CreateBinder(property));
}
return new CustomComplexTypeModelBinder(propertyBinders);
}
}
}
}

0 comments on commit 29647fd

Please sign in to comment.