Skip to content

Commit

Permalink
Changes per PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
pranavkm committed Jul 23, 2020
1 parent 3f366ea commit e2887b8
Show file tree
Hide file tree
Showing 18 changed files with 375 additions and 264 deletions.
6 changes: 3 additions & 3 deletions src/Components/Web.JS/dist/Release/blazor.server.js

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public abstract class ModelBindingMessageProvider
/// <summary>
/// Error message the model binding system adds when <see cref="ModelError.Exception"/> is of type
/// <see cref="FormatException"/> or <see cref="OverflowException"/>, value is known, and error is associated
/// with a property or parameter.
/// with a property.
/// </summary>
/// <value>Default <see cref="string"/> is "The value '{0}' is not valid for {1}.".</value>
public virtual Func<string, string, string> AttemptedValueIsInvalidAccessor { get; } = default!;
Expand Down
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
get
{
// In record types, each constructor parameter in the primary constructor is also a settable property with the same name.
// Executing model binding on these parameters twice may have detrimental effects, such as duplicate validation entries,
// Executing model binding on these parameters twice may have detrimental effects, such as duplicate ModelState entries,
// or failures if a model expects to be bound exactly ones.
// Consequently when binding to a constructor, we only bind and validate the subset of properties whose names
// haven't appeared as parameters.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ public sealed class ComplexObjectModelBinder : IModelBinder
private readonly ILogger _logger;
private Func<object> _modelCreator;


internal ComplexObjectModelBinder(
IDictionary<ModelMetadata, IModelBinder> propertyBinders,
IReadOnlyList<IModelBinder> parameterBinders,
Expand Down Expand Up @@ -83,7 +82,7 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
if (boundConstructor != null)
{
var values = new object[boundConstructor.BoundConstructorParameters.Count];
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParameters(
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParametersAsync(
bindingContext,
propertyData,
boundConstructor.BoundConstructorParameters,
Expand All @@ -103,7 +102,7 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
}
}

var (attemptedPropertyBinding, propertyBindingSucceeded) = await BindProperties(
var (attemptedPropertyBinding, propertyBindingSucceeded) = await BindPropertiesAsync(
bindingContext,
propertyData,
modelMetadata.BoundProperties);
Expand Down Expand Up @@ -225,7 +224,7 @@ internal void CreateModel(ModelBindingContext bindingContext)
bindingContext.Model = _modelCreator();
}

private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindParameters(
private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindParametersAsync(
ModelBindingContext bindingContext,
int propertyData,
IReadOnlyList<ModelMetadata> parameters,
Expand Down Expand Up @@ -322,7 +321,7 @@ internal void CreateModel(ModelBindingContext bindingContext)
return (attemptedBinding, bindingSucceeded);
}

private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindProperties(
private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindPropertiesAsync(
ModelBindingContext bindingContext,
int propertyData,
IReadOnlyList<ModelMetadata> boundProperties)
Expand All @@ -344,9 +343,6 @@ internal void CreateModel(ModelBindingContext bindingContext)
continue;
}

var fieldName = property.BinderModelName ?? property.PropertyName;
var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);

var propertyBinder = _propertyBinders[property];
if (propertyBinder is PlaceholderBinder)
{
Expand All @@ -372,6 +368,8 @@ internal void CreateModel(ModelBindingContext bindingContext)
}
}

var fieldName = property.BinderModelName ?? property.PropertyName;
var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
var result = await BindPropertyAsync(bindingContext, property, propertyBinder, fieldName, modelName);

if (result.IsModelSet)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand All @@ -25,6 +25,7 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)
if (metadata.IsComplexType && !metadata.IsCollectionType)
{
var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
var logger = loggerFactory.CreateLogger<ComplexObjectModelBinder>();
var parameterBinders = GetParameterBinders(context);

var propertyBinders = new Dictionary<ModelMetadata, IModelBinder>();
Expand All @@ -34,7 +35,7 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)
propertyBinders.Add(property, context.CreateBinder(property));
}

return new ComplexObjectModelBinder(propertyBinders, parameterBinders, loggerFactory.CreateLogger<ComplexObjectModelBinder>());
return new ComplexObjectModelBinder(propertyBinders, parameterBinders, logger);
}

return null;
Expand All @@ -48,7 +49,9 @@ private static IReadOnlyList<IModelBinder> GetParameterBinders(ModelBinderProvid
return Array.Empty<IModelBinder>();
}

var parameterBinders = boundConstructor.BoundConstructorParameters.Count == 0 ? Array.Empty<IModelBinder>() : new IModelBinder[boundConstructor.BoundConstructorParameters.Count];
var parameterBinders = boundConstructor.BoundConstructorParameters.Count == 0 ?
Array.Empty<IModelBinder>() :
new IModelBinder[boundConstructor.BoundConstructorParameters.Count];

for (var i = 0; i < parameterBinders.Length; i++)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,12 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
var constructor = constructors[0];

var parameters = constructor.GetParameters();
if (parameters.Length == 0)
{
// We do not need to do special handling for parameterless constructors.
return null;
}

var properties = PropertyHelper.GetVisibleProperties(type);

for (var i = 0; i < parameters.Length; i++)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,7 @@ static Func<object[], object> CreateObjectFactory(ConstructorInfo constructor)
var args = Expression.Parameter(typeof(object[]), "args");
var factoryExpressionBody = BuildFactoryExpression(constructor, args);

var factoryLamda = Expression.Lambda<Func<object[], object>>(
factoryExpressionBody, args);
var factoryLamda = Expression.Lambda<Func<object[], object>>(factoryExpressionBody, args);

return factoryLamda.Compile();
}
Expand Down
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/ModelBinding/ParameterBinder.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,6 @@
<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. Record types must have a single primary constructor.</value>
</data>
<data name="ValidationStrategy_MappedPropertyNotFound" xml:space="preserve">
<value>No property found that maps to constructor parameter '{0}' for type '{1}'. Validation requires that each bound parameter must have a property to read the value.</value>
<value>No property found that maps to pconstructor parameter '{0}' for type '{1}'. Validation requires that each bound parameter of a record type's primary constructor must have a property to read the value.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
{
[Obsolete]
public class ComplexObjectModelBinderProviderTest
{
[Theory]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
{
[Obsolete]
#pragma warning disable CS0618 // Type or member is obsolete
public class ComplexTypeModelBinderTest
{
private static readonly IModelMetadataProvider _metadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
Expand Down Expand Up @@ -1662,4 +1662,5 @@ protected override void SetProperty(
}
}
}
#pragma warning restore CS0618 // Type or member is obsolete
}
Original file line number Diff line number Diff line change
Expand Up @@ -748,17 +748,27 @@ public void GetBoundConstructor_ReturnsPrimaryConstructor_ForRecordType()
p => Assert.Equal("name", p.Name));
}

private record RecordTypeWithPrimaryAndParameterlessConstructor(string name)
private record RecordTypeWithDefaultConstructor
{
public RecordTypeWithPrimaryAndParameterlessConstructor() : this(string.Empty) {}
public string Name { get; init; }

public int Age { get; init; }
}

[Fact]
public void GetBoundConstructor_ReturnsNull_ForRecordTypeWithParameterlessConstructor()
private record RecordTypeWithParameterlessConstructor
{
// Arrange
var type = typeof(RecordTypeWithPrimaryAndParameterlessConstructor);
public RecordTypeWithParameterlessConstructor() { }

public string Name { get; init; }

public int Age { get; init; }
}

[Theory]
[InlineData(typeof(RecordTypeWithDefaultConstructor))]
[InlineData(typeof(RecordTypeWithParameterlessConstructor))]
public void GetBoundConstructor_ReturnsNull_ForRecordTypeWithParameterlessConstructor(Type type)
{
// Act
var result = DefaultBindingMetadataProvider.GetBoundConstructor(type);

Expand Down
Loading

0 comments on commit e2887b8

Please sign in to comment.