Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for binding record types #23976

Merged
merged 10 commits into from Jul 24, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/Components/Web.JS/dist/Release/blazor.server.js

Large diffs are not rendered by default.

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
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.
pranavkm marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Expand Up @@ -36,7 +36,6 @@ public sealed class ComplexObjectModelBinder : IModelBinder
private readonly ILogger _logger;
private Func<object> _modelCreator;


internal ComplexObjectModelBinder(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the constructor internal❔ This prevents extension through delegation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. Our recommendation would be to use the binder provider to construct this type rather than new up these instances. Let's talk about this during API review next week.

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
@@ -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
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++)
pranavkm marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Expand Up @@ -282,8 +282,7 @@ private DefaultMetadataDetails CreateConstructorDetails(ModelMetadataIdentity co
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
@@ -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
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>
pranavkm marked this conversation as resolved.
Show resolved Hide resolved
</data>
</root>
Expand Up @@ -8,7 +8,6 @@

namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
{
[Obsolete]
public class ComplexObjectModelBinderProviderTest
{
[Theory]
Expand Down
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 object CreateModel(ModelBindingContext bindingContext)
}
}
}
#pragma warning restore CS0618 // Type or member is obsolete
}
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