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 all commits
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
Expand Up @@ -6,12 +6,17 @@
namespace Microsoft.AspNetCore.Mvc.ModelBinding
{
/// <summary>
/// Provides a predicate which can determines which model properties should be bound by model binding.
/// Provides a predicate which can determines which model properties or parameters should be bound by model binding.
/// </summary>
public interface IPropertyFilterProvider
{
/// <summary>
/// <para>
/// Gets a predicate which can determines which model properties should be bound by model binding.
/// </para>
/// <para>
/// This predicate is also used to determine which parameters are bound when a model's constructor is bound.
/// </para>
pranavkm marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
Func<ModelMetadata, bool> PropertyFilter { get; }
}
Expand Down
Expand Up @@ -51,7 +51,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 collection element or action parameter.
/// with a collection element or parameter.
/// </summary>
/// <value>Default <see cref="string"/> is "The value '{0}' is not valid.".</value>
public virtual Func<string, string> NonPropertyAttemptedValueIsInvalidAccessor { get; } = default!;
Expand All @@ -67,7 +67,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 unknown, and error is associated
/// with a collection element or action parameter.
/// with a collection element or parameter.
/// </summary>
/// <value>Default <see cref="string"/> is "The supplied value is invalid.".</value>
public virtual Func<string> NonPropertyUnknownValueIsInvalidAccessor { get; } = default!;
Expand Down
Expand Up @@ -16,12 +16,14 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
Type modelType,
string? name = null,
Type? containerType = null,
object? fieldInfo = null)
object? fieldInfo = null,
ConstructorInfo? constructorInfo = null)
{
ModelType = modelType;
Name = name;
ContainerType = containerType;
FieldInfo = fieldInfo;
ConstructorInfo = constructorInfo;
}

/// <summary>
Expand Down Expand Up @@ -130,6 +132,28 @@ public static ModelMetadataIdentity ForParameter(ParameterInfo parameter, Type m
return new ModelMetadataIdentity(modelType, parameter.Name, fieldInfo: parameter);
}

/// <summary>
/// Creates a <see cref="ModelMetadataIdentity"/> for the provided parameter with the specified
/// model type.
/// </summary>
/// <param name="constructor">The <see cref="ConstructorInfo" />.</param>
/// <param name="modelType">The model type.</param>
/// <returns>A <see cref="ModelMetadataIdentity"/>.</returns>
public static ModelMetadataIdentity ForConstructor(ConstructorInfo constructor, Type modelType)
{
if (constructor == null)
{
throw new ArgumentNullException(nameof(constructor));
}

if (modelType == null)
{
throw new ArgumentNullException(nameof(modelType));
}

return new ModelMetadataIdentity(modelType, constructor.Name, constructorInfo: constructor);
}

/// <summary>
/// Gets the <see cref="Type"/> defining the model property represented by the current
/// instance, or <c>null</c> if the current instance does not represent a property.
Expand All @@ -152,6 +176,10 @@ public ModelMetadataKind MetadataKind
{
return ModelMetadataKind.Parameter;
}
else if (ConstructorInfo != null)
{
return ModelMetadataKind.Constructor;
}
else if (ContainerType != null && Name != null)
{
return ModelMetadataKind.Property;
Expand Down Expand Up @@ -183,6 +211,12 @@ public ModelMetadataKind MetadataKind
/// </summary>
public PropertyInfo? PropertyInfo => FieldInfo as PropertyInfo;

/// <summary>
/// Gets a descriptor for the constructor, or <c>null</c> if this instance
/// does not represent a constructor.
/// </summary>
public ConstructorInfo? ConstructorInfo { get; }

/// <inheritdoc />
public bool Equals(ModelMetadataIdentity other)
{
Expand All @@ -191,7 +225,8 @@ public bool Equals(ModelMetadataIdentity other)
ModelType == other.ModelType &&
Name == other.Name &&
ParameterInfo == other.ParameterInfo &&
PropertyInfo == other.PropertyInfo;
PropertyInfo == other.PropertyInfo &&
ConstructorInfo == other.ConstructorInfo;
}

/// <inheritdoc />
Expand All @@ -210,6 +245,7 @@ public override int GetHashCode()
hash.Add(Name, StringComparer.Ordinal);
hash.Add(ParameterInfo);
hash.Add(PropertyInfo);
hash.Add(ConstructorInfo);
return hash.ToHashCode();
}
}
Expand Down
Expand Up @@ -22,5 +22,10 @@ public enum ModelMetadataKind
/// Used for <see cref="ModelMetadata"/> for a parameter.
/// </summary>
Parameter,

/// <summary>
/// <see cref="ModelMetadata"/> for a constructor.
/// </summary>
Constructor,
}
}
}
97 changes: 96 additions & 1 deletion src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs
Expand Up @@ -4,8 +4,10 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.ComponentModel;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
Expand All @@ -24,7 +26,11 @@ public abstract class ModelMetadata : IEquatable<ModelMetadata?>, IModelMetadata
/// </summary>
public static readonly int DefaultOrder = 10000;

private static readonly IReadOnlyDictionary<ModelMetadata, ModelMetadata> EmptyParameterMapping = new Dictionary<ModelMetadata, ModelMetadata>(0);

private int? _hashCode;
private IReadOnlyList<ModelMetadata>? _boundProperties;
private IReadOnlyDictionary<ModelMetadata, ModelMetadata>? _parameterMapping;

/// <summary>
/// Creates a new <see cref="ModelMetadata"/>.
Expand Down Expand Up @@ -83,7 +89,7 @@ public virtual ModelMetadata ContainerMetadata
/// <summary>
/// Gets the key for the current instance.
/// </summary>
protected ModelMetadataIdentity Identity { get; }
protected internal ModelMetadataIdentity Identity { get; }

/// <summary>
/// Gets a collection of additional information about the model.
Expand All @@ -95,6 +101,88 @@ public virtual ModelMetadata ContainerMetadata
/// </summary>
public abstract ModelPropertyCollection Properties { get; }

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 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
if (BoundConstructor is null)
{
return Properties;
}

if (_boundProperties is null)
{
var boundParameters = BoundConstructor.BoundConstructorParameters!;
var boundProperties = new List<ModelMetadata>();

foreach (var metadata in Properties)
{
if (!boundParameters.Any(p =>
string.Equals(p.ParameterName, metadata.PropertyName, StringComparison.Ordinal)
&& p.ModelType == metadata.ModelType))
{
boundProperties.Add(metadata);
}
}

_boundProperties = boundProperties;
}

return _boundProperties;
}
}

internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParameterMapping
{
get
{
if (_parameterMapping != null)
{
return _parameterMapping;
}

if (BoundConstructor is null)
{
_parameterMapping = EmptyParameterMapping;
return _parameterMapping;
}

var boundParameters = BoundConstructor.BoundConstructorParameters!;
var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();

foreach (var parameter in boundParameters)
{
var property = Properties.FirstOrDefault(p =>
string.Equals(p.Name, parameter.ParameterName, StringComparison.Ordinal) &&
p.ModelType == parameter.ModelType);

if (property != null)
{
parameterMapping[parameter] = property;
}
}

_parameterMapping = parameterMapping;
return _parameterMapping;
}
}

/// <summary>
/// Gets <see cref="ModelMetadata"/> instance for a constructor of a record type that is used during binding and validation.
/// </summary>
public virtual ModelMetadata? BoundConstructor { get; }

/// <summary>
/// Gets the collection of <see cref="ModelMetadata"/> instances for parameters on a <see cref="BoundConstructor"/>.
/// This is only available when <see cref="MetadataKind"/> is <see cref="ModelMetadataKind.Constructor"/>.
/// </summary>
public virtual IReadOnlyList<ModelMetadata>? BoundConstructorParameters { get; }

/// <summary>
/// Gets the name of a model if specified explicitly using <see cref="IModelNameProvider"/>.
/// </summary>
Expand Down Expand Up @@ -401,6 +489,11 @@ public virtual ModelMetadata ContainerMetadata
/// </summary>
public abstract Action<object, object> PropertySetter { get; }

/// <summary>
/// Gets a delegate that invokes the bound constructor <see cref="BoundConstructor" /> if non-<see langword="null" />.
/// </summary>
public virtual Func<object[], object>? BoundConstructorInvoker => null;

/// <summary>
/// Gets a display name for the model.
/// </summary>
Expand Down Expand Up @@ -500,6 +593,8 @@ private string DebuggerToString()
return $"ModelMetadata (Property: '{ContainerType!.Name}.{PropertyName}' Type: '{ModelType.Name}')";
case ModelMetadataKind.Type:
return $"ModelMetadata (Type: '{ModelType.Name}')";
case ModelMetadataKind.Constructor:
return $"ModelMetadata (Constructor: '{ModelType.Name}')";
default:
return $"Unsupported MetadataKind '{MetadataKind}'.";
}
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 Down Expand Up @@ -54,5 +54,16 @@ public virtual ModelMetadata GetMetadataForProperty(PropertyInfo propertyInfo, T
{
throw new NotSupportedException();
}

/// <summary>
/// Supplies metadata describing a constructor.
/// </summary>
/// <param name="constructor">The <see cref="ConstructorInfo"/>.</param>
/// <param name="modelType">The type declaring the constructor.</param>
/// <returns>A <see cref="ModelMetadata"/> instance describing the <paramref name="constructor"/>.</returns>
public virtual ModelMetadata GetMetadataForConstructor(ConstructorInfo constructor, Type modelType)
{
throw new NotSupportedException();
}
}
}
Expand Up @@ -298,6 +298,7 @@ public bool TryAddModelError(string key, Exception exception, ModelMetadata meta
// "The value '' is not valid." (when no value was provided, not even an empty string) and
// "The supplied value is invalid for Int32." (when error is for an element or parameter).
var messageProvider = metadata.ModelBindingMessageProvider;

var name = metadata.DisplayName ?? metadata.PropertyName;
string errorMessage;
if (entry == null && name == null)
Expand Down
14 changes: 7 additions & 7 deletions src/Mvc/Mvc.Abstractions/test/ModelBinding/BindingInfoTest.cs
Expand Up @@ -83,7 +83,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesValuesFromBindingI
// Arrange
var attributes = new object[]
{
new ModelBinderAttribute { BinderType = typeof(ComplexTypeModelBinder), Name = "Test" },
new ModelBinderAttribute { BinderType = typeof(ComplexObjectModelBinder), Name = "Test" },
};
var modelType = typeof(Guid);
var provider = new TestModelMetadataProvider();
Expand All @@ -100,7 +100,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesValuesFromBindingI

// Assert
Assert.NotNull(bindingInfo);
Assert.Same(typeof(ComplexTypeModelBinder), bindingInfo.BinderType);
Assert.Same(typeof(ComplexObjectModelBinder), bindingInfo.BinderType);
Assert.Same("Test", bindingInfo.BinderModelName);
}

Expand All @@ -110,7 +110,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesBinderNameFromMode
// Arrange
var attributes = new object[]
{
new ModelBinderAttribute(typeof(ComplexTypeModelBinder)),
new ModelBinderAttribute(typeof(ComplexObjectModelBinder)),
new ControllerAttribute(),
new BindNeverAttribute(),
};
Expand All @@ -129,7 +129,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesBinderNameFromMode

// Assert
Assert.NotNull(bindingInfo);
Assert.Same(typeof(ComplexTypeModelBinder), bindingInfo.BinderType);
Assert.Same(typeof(ComplexObjectModelBinder), bindingInfo.BinderType);
Assert.Same("Different", bindingInfo.BinderModelName);
Assert.Same(BindingSource.Custom, bindingInfo.BindingSource);
}
Expand All @@ -143,7 +143,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesModelBinderFromMod
var provider = new TestModelMetadataProvider();
provider.ForType(modelType).BindingDetails(metadata =>
{
metadata.BinderType = typeof(ComplexTypeModelBinder);
metadata.BinderType = typeof(ComplexObjectModelBinder);
});
var modelMetadata = provider.GetMetadataForType(modelType);

Expand All @@ -152,7 +152,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesModelBinderFromMod

// Assert
Assert.NotNull(bindingInfo);
Assert.Same(typeof(ComplexTypeModelBinder), bindingInfo.BinderType);
Assert.Same(typeof(ComplexObjectModelBinder), bindingInfo.BinderType);
}

[Fact]
Expand Down Expand Up @@ -187,7 +187,7 @@ public void GetBindingInfo_WithAttributesAndModelMetadata_UsesPropertyPredicateP
// Arrange
var attributes = new object[]
{
new ModelBinderAttribute(typeof(ComplexTypeModelBinder)),
new ModelBinderAttribute(typeof(ComplexObjectModelBinder)),
new ControllerAttribute(),
new BindNeverAttribute(),
};
Expand Down
Expand Up @@ -728,6 +728,12 @@ public override IReadOnlyList<object> ValidatorMetadata
throw new NotImplementedException();
}
}

public override ModelMetadata BoundConstructor => throw new NotImplementedException();

public override Func<object[], object> BoundConstructorInvoker => throw new NotImplementedException();

public override IReadOnlyList<ModelMetadata> BoundConstructorParameters => throw new NotImplementedException();
}

private class CollectionImplementation : ICollection<string>
Expand Down
Expand Up @@ -4,7 +4,7 @@ namespace Microsoft.AspNetCore.Mvc.Analyzers.TopLevelParameterNameAnalyzerTestFi
{
public class IsProblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter
{
[ModelBinder(typeof(ComplexTypeModelBinder), Name = "model")]
[ModelBinder(typeof(ComplexObjectModelBinder), Name = "model")]
dougbu marked this conversation as resolved.
Show resolved Hide resolved
public string Different { get; set; }

public void ActionMethod(
Expand Down