Skip to content

Record type follow ups: #25218

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

Merged
merged 8 commits into from
Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 65 additions & 28 deletions src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
using System.Collections.ObjectModel;
using System.ComponentModel;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reflection;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
using Microsoft.Extensions.Internal;
Expand All @@ -26,11 +28,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;
private Exception? _recordTypeValidatorsOnPropertiesError;
private bool _recordTypeConstructorDetailsCalculated;

/// <summary>
/// Creates a new <see cref="ModelMetadata"/>.
Expand Down Expand Up @@ -137,37 +139,16 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
}
}

/// <summary>
/// A mapping from parameters to their corresponding properties on a record type.
/// </summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to have doc comments for internal API given the complexities in this area❕

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;
}
}
Debug.Assert(BoundConstructor != null, "This API can be only called for types with bound constructors.");
CalculateRecordTypeConstructorDetails();

_parameterMapping = parameterMapping;
return _parameterMapping;
}
}
Expand Down Expand Up @@ -494,6 +475,62 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParam
/// </summary>
public virtual Func<object[], object>? BoundConstructorInvoker => null;

/// <summary>
/// Gets a value that determines if validators can be constructed using metadata exclusively defined on the property.
/// </summary>
internal virtual bool PropertyHasValidators => false;

/// <summary>
/// Throws if the ModelMetadata is for a record type with validation on properties.
/// </summary>
internal void ThrowIfRecordTypeHasValidationOnProperties()
{
CalculateRecordTypeConstructorDetails();
if (_recordTypeValidatorsOnPropertiesError != null)
{
throw _recordTypeValidatorsOnPropertiesError;
}
}

[MemberNotNull(nameof(_parameterMapping))]
private void CalculateRecordTypeConstructorDetails()
{
if (_recordTypeConstructorDetailsCalculated)
{
Debug.Assert(_parameterMapping != null);
return;
}


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;

if (property.PropertyHasValidators)
{
// When constructing the mapping of paramets -> properties, also determine
// if the property has any validators (without looking at metadata on the type).
// This will help us throw during validation if a user defines validation attributes
// on the property of a record type.
_recordTypeValidatorsOnPropertiesError = new InvalidOperationException(
Resources.FormatRecordTypeHasValidationOnProperties(ModelType, property.Name));
}
}
}

_recordTypeConstructorDetailsCalculated = true;
_parameterMapping = parameterMapping;
}

/// <summary>
/// Gets a display name for the model.
/// </summary>
Expand Down
3 changes: 3 additions & 0 deletions src/Mvc/Mvc.Abstractions/src/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -177,4 +177,7 @@
<data name="BinderType_MustBeIModelBinder" xml:space="preserve">
<value>The type '{0}' must implement '{1}' to be used as a model binder.</value>
</data>
<data name="RecordTypeHasValidationOnProperties" xml:space="preserve">
<value>Record type '{0}' has validation metadata defined on property '{1}' that will be ignored. '{1}' is a parameter in the record primary constructor and validation metadata must be specified against the constructor parameter.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -75,32 +75,33 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
var bindingSucceeded = false;

var modelMetadata = bindingContext.ModelMetadata;
var boundConstructor = modelMetadata.BoundConstructor;

if (bindingContext.Model == null)
if (boundConstructor != null)
{
var boundConstructor = modelMetadata.BoundConstructor;
if (boundConstructor != null)
{
var values = new object[boundConstructor.BoundConstructorParameters.Count];
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParametersAsync(
bindingContext,
propertyData,
boundConstructor.BoundConstructorParameters,
values);
// Only record types are allowed to have a BoundConstructor. Binding a record type requires
// instantiating the type. This means we'll ignore a previously assigned bindingContext.Model value.
// This behaior is identical to input formatting with S.T.Json and Json.NET.

var values = new object[boundConstructor.BoundConstructorParameters.Count];
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParametersAsync(
bindingContext,
propertyData,
boundConstructor.BoundConstructorParameters,
values);

attemptedBinding |= attemptedParameterBinding;
bindingSucceeded |= parameterBindingSucceeded;
attemptedBinding |= attemptedParameterBinding;
bindingSucceeded |= parameterBindingSucceeded;

if (!CreateModel(bindingContext, boundConstructor, values))
{
return;
}
}
else
if (!CreateModel(bindingContext, boundConstructor, values))
{
CreateModel(bindingContext);
return;
}
}
else if (bindingContext.Model == null)
{
CreateModel(bindingContext);
}

var (attemptedPropertyBinding, propertyBindingSucceeded) = await BindPropertiesAsync(
bindingContext,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,7 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
static bool IsRecordType(Type type)
{
// Based on the state of the art as described in https://github.com/dotnet/roslyn/issues/45777
var cloneMethod = type.GetMethod("<Clone>$", BindingFlags.Public | BindingFlags.Instance) ??
type.GetMethod("<>Clone", BindingFlags.Public | BindingFlags.Instance);
var cloneMethod = type.GetMethod("<Clone>$", BindingFlags.Public | BindingFlags.Instance);
return cloneMethod != null && cloneMethod.ReturnType == type;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,8 @@ public override bool? HasValidators
}
}

internal override bool PropertyHasValidators => ValidationMetadata.PropertyHasValidators;

internal static bool CalculateHasValidators(HashSet<DefaultModelMetadata> visited, ModelMetadata metadata)
{
RuntimeHelpers.EnsureSufficientExecutionStack();
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 Down Expand Up @@ -40,7 +40,23 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
if (provider.HasValidators(context.Key.ModelType, context.ValidationMetadata.ValidatorMetadata))
{
context.ValidationMetadata.HasValidators = true;
return;

if (context.Key.MetadataKind == ModelMetadataKind.Property)
{
// For properties, additionally determine that if there's validators defined exclusively
// from property attributes. This is later used to produce a error for record types
// where a record type property that is bound as a parameter defines validation attributes.

if (!(context.PropertyAttributes is IList<object> propertyAttributes))
{
propertyAttributes = context.PropertyAttributes.ToList();
}

if (provider.HasValidators(typeof(object), propertyAttributes))
{
context.ValidationMetadata.PropertyHasValidators = true;
}
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,5 +46,10 @@ public class ValidationMetadata
/// Gets a value that indicates if the model has validators .
/// </summary>
public bool? HasValidators { get; set; }

/// <summary>
/// Gets or sets a value that determines if validators can be constructed using metadata on properties.
/// </summary>
internal bool PropertyHasValidators { get; set; }
}
}
}
6 changes: 6 additions & 0 deletions src/Mvc/Mvc.Core/src/ModelBinding/ModelBindingHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,12 @@ public static async Task<bool> TryUpdateModelAsync(
}

var modelMetadata = metadataProvider.GetMetadataForType(modelType);

if (modelMetadata.BoundConstructor != null)
{
throw new NotSupportedException(Resources.FormatTryUpdateModel_RecordTypeNotSupported(nameof(TryUpdateModelAsync), modelType));
}

var modelState = actionContext.ModelState;

var modelBindingContext = DefaultModelBindingContext.CreateBindingContext(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ public Enumerator(
}
else
{
_modelMetadata.ThrowIfRecordTypeHasValidationOnProperties();
_parameters = _modelMetadata.BoundConstructor.BoundConstructorParameters;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,11 @@ private bool VisitImplementation(ref ModelMetadata metadata, ref string key, obj
else if (metadata.HasValidators == false &&
ModelState.GetFieldValidationState(key) != ModelValidationState.Invalid)
{
if (metadata.BoundConstructor != null)
{
metadata.ThrowIfRecordTypeHasValidationOnProperties();
}

// No validators will be created for this graph of objects. Mark it as valid if it wasn't previously validated.
var entries = ModelState.FindKeysWithPrefix(key);
foreach (var item in entries)
Expand Down
5 changes: 4 additions & 1 deletion src/Mvc/Mvc.Core/src/Resources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -534,4 +534,7 @@
<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 of a record type's primary constructor must have a property to read the value.</value>
</data>
</root>
<data name="TryUpdateModel_RecordTypeNotSupported" xml:space="preserve">
<value>{0} is not supported on record type model '{1}'.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ public SystemTextJsonInputFormatterTest(MvcTestFixture<FormatterWebSite.StartupW
public override Task JsonInputFormatter_RoundtripsRecordType()
=> base.JsonInputFormatter_RoundtripsRecordType();

[Fact(Skip = "https://github.com/dotnet/runtime/issues/38539")]
[Fact]
public override Task JsonInputFormatter_ValidationWithRecordTypes_NoValidationErrors()
=> base.JsonInputFormatter_ValidationWithRecordTypes_NoValidationErrors();

[Fact(Skip = "https://github.com/dotnet/runtime/issues/38539")]
[Fact]
public override Task JsonInputFormatter_ValidationWithRecordTypes_ValidationErrors()
=> base.JsonInputFormatter_ValidationWithRecordTypes_ValidationErrors();
}
Expand Down
Loading