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

Commit

Permalink
[Fixes #5698] Regression in 1.1 model binding for model types without…
Browse files Browse the repository at this point in the history
… default constructor

- Also reverts "Check for default constructor in ComplexTypeModelBinderProvider" commit d09e921.
  • Loading branch information
kichalla committed Feb 10, 2017
1 parent bd9e431 commit 842d661
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 156 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Linq.Expressions;
using System.Reflection;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Mvc.Core;
using Microsoft.AspNetCore.Mvc.Internal;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
Expand Down Expand Up @@ -47,6 +48,28 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Reflection;
using System.Collections.Generic;

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
Expand All @@ -20,9 +19,7 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)
throw new ArgumentNullException(nameof(context));
}

if (context.Metadata.IsComplexType &&
!context.Metadata.IsCollectionType &&
HasDefaultConstructor(context.Metadata.ModelType.GetTypeInfo()))
if (context.Metadata.IsComplexType && !context.Metadata.IsCollectionType)
{
var propertyBinders = new Dictionary<ModelMetadata, IModelBinder>();
foreach (var property in context.Metadata.Properties)
Expand All @@ -35,14 +32,5 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)

return null;
}

private bool HasDefaultConstructor(TypeInfo modelTypeInfo)
{
// The following check causes the ComplexTypeModelBinder to NOT participate in binding structs.
// - Reflection does not provide information about the implicit parameterless constructor for a struct.
// - Also this binder would eventually fail to construct an instance of the struct as the Linq's
// NewExpression compile fails to construct it.
return !modelTypeInfo.IsAbstract && modelTypeInfo.GetConstructor(Type.EmptyTypes) != null;
}
}
}
68 changes: 50 additions & 18 deletions src/Microsoft.AspNetCore.Mvc.Core/Properties/Resources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/Microsoft.AspNetCore.Mvc.Core/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -387,4 +387,10 @@
<value>Unable to create an instance of type '{0}'. The type specified in {1} must not be abstract and must have a parameterless constructor.</value>
<comment>0 is the type to configure. 1 is the name of the parameter, configurationType.</comment>
</data>
<data name="ComplexTypeModelBinder_NoParameterlessConstructor_TopLevelObject" xml:space="preserve">
<value>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.</value>
</data>
<data name="ComplexTypeModelBinder_NoParameterlessConstructor_ForProperty" xml:space="preserve">
<value>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.</value>
</data>
</root>

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -55,107 +55,11 @@ public void Create_ForSupportedTypes_ReturnsBinder()
Assert.IsType<ComplexTypeModelBinder>(result);
}

[Theory]
[InlineData(typeof(PointStructWithExplicitConstructor))]
[InlineData(typeof(PointStructWithNoExplicitConstructor))]
public void Create_ForStructModel_ReturnsNull(Type modelType)
{
// Arrange
var provider = new ComplexTypeModelBinderProvider();
var context = new TestModelBinderProviderContext(modelType);

// Act
var result = provider.GetBinder(context);

// Assert
Assert.Null(result);
}

[Theory]
[InlineData(typeof(ClassWithNoDefaultConstructor))]
[InlineData(typeof(ClassWithStaticConstructor))]
[InlineData(typeof(ClassWithInternalDefaultConstructor))]
public void Create_ForModelTypeWithNoDefaultPublicConstructor_ReturnsNull(Type modelType)
{
// Arrange
var provider = new ComplexTypeModelBinderProvider();
var context = new TestModelBinderProviderContext(modelType);

// Act
var result = provider.GetBinder(context);

// Assert
Assert.Null(result);
}

[Fact]
public void Create_ForAbstractModelTypeWithDefaultPublicConstructor_ReturnsNull()
{
// Arrange
var provider = new ComplexTypeModelBinderProvider();
var context = new TestModelBinderProviderContext(typeof(AbstractClassWithDefaultConstructor));

// Act
var result = provider.GetBinder(context);

// Assert
Assert.Null(result);
}

private struct PointStructWithNoExplicitConstructor
{
public double X { get; set; }
public double Y { get; set; }
}

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

private class Person
{
public string Name { get; set; }

public int Age { get; set; }
}

private class ClassWithNoDefaultConstructor
{
public ClassWithNoDefaultConstructor(int id) { }
}

private class ClassWithInternalDefaultConstructor
{
internal ClassWithInternalDefaultConstructor() { }
}

private class ClassWithStaticConstructor
{
static ClassWithStaticConstructor() { }

public ClassWithStaticConstructor(int id) { }
}

private abstract class AbstractClassWithDefaultConstructor
{
private readonly string _name;

public AbstractClassWithDefaultConstructor()
: this("James")
{
}

public AbstractClassWithDefaultConstructor(string name)
{
_name = name;
}
}
}
}
Loading

0 comments on commit 842d661

Please sign in to comment.