Record type follow ups: (#25218)

* Record type follow ups:

* Throw an error if a record type property has validation metadata
* Disallow TryUpdateModel on a top-level record type
* Ignore previously specified model value when binding a record type
* Unskip record type tests
* Clean up record type detection

* Update src/Mvc/Mvc.Abstractions/src/Resources.resx

Co-authored-by: James Newton-King <james@newtonking.com>

* Fixup tests

* Update src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs

* Update src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs

* Update src/Mvc/Mvc.Abstractions/src/Resources.resx

Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>

* Update src/Mvc/Mvc.Core/src/Resources.resx

Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: James Newton-King <james@newtonking.com>
Co-authored-by: Doug Bunting <6431421+dougbu@users.noreply.github.com>
This commit is contained in:
Pranav K 2020-08-28 09:42:11 -07:00 committed by GitHub
parent 748b368d54
commit 7686c0b4e7
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 319 additions and 120 deletions

View File

@ -7,8 +7,10 @@ using System.Collections.Generic;
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;
@ -26,11 +28,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
/// </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"/>.
@ -137,37 +139,16 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
}
}
/// <summary>
/// A mapping from parameters to their corresponding properties on a record type.
/// </summary>
internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParameterMapping
{
get
{
if (_parameterMapping != null)
{
return _parameterMapping;
}
Debug.Assert(BoundConstructor != null, "This API can be only called for types with bound constructors.");
CalculateRecordTypeConstructorDetails();
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;
}
}
@ -494,6 +475,62 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
/// </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>

View File

@ -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>
</root>
<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 associated with the constructor parameter.</value>
</data>
</root>

View File

@ -75,32 +75,33 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
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,

View File

@ -143,8 +143,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
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;
}
}

View File

@ -468,6 +468,8 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
}
}
internal override bool PropertyHasValidators => ValidationMetadata.PropertyHasValidators;
internal static bool CalculateHasValidators(HashSet<DefaultModelMetadata> visited, ModelMetadata metadata)
{
RuntimeHelpers.EnsureSufficientExecutionStack();

View File

@ -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;
@ -40,7 +40,23 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
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;
}
}
}
}

View File

@ -46,5 +46,10 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Metadata
/// 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; }
}
}
}

View File

@ -268,6 +268,12 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding
}
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(

View File

@ -58,6 +58,7 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
}
else
{
_modelMetadata.ThrowIfRecordTypeHasValidationOnProperties();
_parameters = _modelMetadata.BoundConstructor.BoundConstructorParameters;
}

View File

@ -304,6 +304,11 @@ namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
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)

View File

@ -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>
<data name="TryUpdateModel_RecordTypeNotSupported" xml:space="preserve">
<value>{0} cannot update a record type model. If a '{1}' must be updated, include it in an object type.</value>
</data>
</root>

View File

@ -17,11 +17,11 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests
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();
}

View File

@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
@ -1145,7 +1146,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests
}
[Fact]
public async Task TryUpdateModel_RecordTypeModel_DoesNotOverwriteConstructorParameters()
public async Task TryUpdateModel_RecordTypeModel_Throws()
{
// Arrange
var testContext = ModelBindingTestHelper.GetTestContext(request =>
@ -1160,61 +1161,10 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests
};
var oldModel = model;
// Act
var result = await TryUpdateModelAsync(model, string.Empty, testContext);
// Act & Assert
var ex = await Assert.ThrowsAsync<NotSupportedException>(() => TryUpdateModelAsync(model, string.Empty, testContext));
Assert.Equal($"TryUpdateModelAsync cannot update a record type model. If a '{model.GetType()}' must be updated, include it in an object type." , ex.Message);
// Assert
Assert.True(result);
// Model
Assert.Same(oldModel, model);
Assert.Equal("DefaultStreet", model.Street);
Assert.Equal("Toronto", model.City);
Assert.Equal("98001", model.ZipCode);
// ModelState
Assert.True(modelState.IsValid);
Assert.Empty(modelState);
}
[Fact]
public async Task TryUpdateModel_RecordTypeModel_UpdatesProperties()
{
// Arrange
var testContext = ModelBindingTestHelper.GetTestContext(request =>
{
request.QueryString = QueryString.Create("ZipCode", "98007").Add("Street", "SomeStreet");
});
var modelState = testContext.ModelState;
var model = new AddressRecord("DefaultStreet", "Toronto")
{
ZipCode = "98001",
};
var oldModel = model;
// Act
var result = await TryUpdateModelAsync(model, string.Empty, testContext);
// Assert
Assert.True(result);
// Model
Assert.Same(oldModel, model);
Assert.Equal("DefaultStreet", model.Street);
Assert.Equal("Toronto", model.City);
Assert.Equal("98007", model.ZipCode);
// ModelState
Assert.True(modelState.IsValid);
var entry = Assert.Single(modelState);
Assert.Equal("ZipCode", entry.Key);
var state = entry.Value;
Assert.Equal("98007", state.AttemptedValue);
Assert.Equal("98007", state.RawValue);
Assert.Empty(state.Errors);
Assert.Equal(ModelValidationState.Valid, state.ValidationState);
}
private class ModelWithRecordTypeProperty
@ -1269,7 +1219,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests
}
[Fact]
public async Task TryUpdateModel_RecordTypeProperty_InitializedDoesNotOverwriteConstructorParameters()
public async Task TryUpdateModel_RecordTypePropertyIsOverwritten()
{
// Arrange
var testContext = ModelBindingTestHelper.GetTestContext(request =>
@ -1297,19 +1247,33 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests
Assert.Same(oldModel, model);
Assert.NotNull(model.Address);
var address = model.Address;
Assert.Equal("DefaultStreet", address.Street);
Assert.Equal("DefaultCity", address.City);
Assert.Equal("SomeStreet", address.Street);
Assert.Null(address.City);
Assert.Equal("98007", address.ZipCode);
// ModelState
Assert.True(modelState.IsValid);
var entry = Assert.Single(modelState);
var state = entry.Value;
Assert.Equal("98007", state.AttemptedValue);
Assert.Equal("98007", state.RawValue);
Assert.Empty(state.Errors);
Assert.Equal(ModelValidationState.Valid, state.ValidationState);
Assert.Collection(
modelState.OrderBy(kvp => kvp.Key),
kvp =>
{
Assert.Equal("Address.Street", kvp.Key);
var state = kvp.Value;
Assert.Equal("SomeStreet", state.AttemptedValue);
Assert.Equal("SomeStreet", state.RawValue);
Assert.Empty(state.Errors);
Assert.Equal(ModelValidationState.Valid, state.ValidationState);
},
kvp =>
{
Assert.Equal("Address.ZipCode", kvp.Key);
var state = kvp.Value;
Assert.Equal("98007", state.AttemptedValue);
Assert.Equal("98007", state.RawValue);
Assert.Empty(state.Errors);
Assert.Equal(ModelValidationState.Valid, state.ValidationState);
});
}
private void UpdateRequest(HttpRequest request, string data, string name)

View File

@ -1151,7 +1151,7 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests
Assert.Equal("The value '-123' is not valid.", error.ErrorMessage);
}
private record NeverValid(string NeverValidProperty) : IValidatableObject
private record NeverValid(string NeverValidProperty) : IValidatableObject
{
public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
@ -2298,6 +2298,163 @@ namespace Microsoft.AspNetCore.Mvc.IntegrationTests
private static void Validation_InifnitelyRecursiveModel_ValidationOnTopLevelParameterMethod([Required] RecursiveModel model) { }
private record RecordTypeWithValidatorsOnProperties(string Property1)
{
[Required]
public string Property1 { get; init; }
}
[Fact]
public async Task Validation_ValidatorsDefinedOnRecordTypeProperties()
{
// Arrange
var modelType = typeof(RecordTypeWithValidatorsOnProperties);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var modelMetadata = modelMetadataProvider.GetMetadataForType(modelType);
var parameterBinder = ModelBindingTestHelper.GetParameterBinder(modelMetadataProvider);
var expected = $"Record type '{modelType}' has validation metadata defined on property 'Property1' that will be ignored. " +
"'Property1' is a parameter in the record primary constructor and validation metadata must be associated with the constructor parameter.";
var parameter = new ParameterDescriptor()
{
Name = "parameter",
ParameterType = modelType,
};
var testContext = ModelBindingTestHelper.GetTestContext(request =>
{
request.QueryString = new QueryString("?Property1=8");
});
var modelState = testContext.ModelState;
// Act & Assert
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
parameterBinder.BindModelAsync(parameter, testContext, modelMetadataProvider, modelMetadata));
Assert.Equal(expected, ex.Message);
}
private record RecordTypeWithValidatorsOnPropertiesAndParameters([Required] string Property1)
{
[Required]
public string Property1 { get; init; }
}
[Fact]
public async Task Validation_ValidatorsDefinedOnRecordTypePropertiesAndParameters()
{
// Arrange
var modelType = typeof(RecordTypeWithValidatorsOnPropertiesAndParameters);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var modelMetadata = modelMetadataProvider.GetMetadataForType(modelType);
var parameterBinder = ModelBindingTestHelper.GetParameterBinder(modelMetadataProvider);
var expected = $"Record type '{modelType}' has validation metadata defined on property 'Property1' that will be ignored. " +
"'Property1' is a parameter in the record primary constructor and validation metadata must be associated with the constructor parameter.";
var parameter = new ParameterDescriptor()
{
Name = "parameter",
ParameterType = modelType,
};
var testContext = ModelBindingTestHelper.GetTestContext(request =>
{
request.QueryString = new QueryString("?Property1=8");
});
var modelState = testContext.ModelState;
// Act & Assert
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
parameterBinder.BindModelAsync(parameter, testContext, modelMetadataProvider, modelMetadata));
Assert.Equal(expected, ex.Message);
}
private record RecordTypeWithValidatorsOnMixOfPropertiesAndParameters([Required] string Property1, string Property2)
{
[Required]
public string Property2 { get; init; }
}
[Fact]
public async Task Validation_ValidatorsDefinedOnMixOfRecordTypePropertiesAndParameters()
{
// Variation of Validation_ValidatorsDefinedOnRecordTypePropertiesAndParameters, but validators
// appear on a mix of properties and parameters.
// Arrange
var modelType = typeof(RecordTypeWithValidatorsOnMixOfPropertiesAndParameters);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var modelMetadata = modelMetadataProvider.GetMetadataForType(modelType);
var parameterBinder = ModelBindingTestHelper.GetParameterBinder(modelMetadataProvider);
var expected = $"Record type '{modelType}' has validation metadata defined on property 'Property2' that will be ignored. " +
"'Property2' is a parameter in the record primary constructor and validation metadata must be associated with the constructor parameter.";
var parameter = new ParameterDescriptor()
{
Name = "parameter",
ParameterType = modelType,
};
var testContext = ModelBindingTestHelper.GetTestContext(request =>
{
request.QueryString = new QueryString("?Property1=8");
});
var modelState = testContext.ModelState;
// Act & Assert
var ex = await Assert.ThrowsAsync<InvalidOperationException>(() =>
parameterBinder.BindModelAsync(parameter, testContext, modelMetadataProvider, modelMetadata));
Assert.Equal(expected, ex.Message);
}
private record RecordTypeWithPropertiesAndParameters([Required] string Property1)
{
[Required]
public string Property2 { get; init; }
}
[Fact]
public async Task Validation_ValidatorsOnParametersAndProperties()
{
// Arrange
var modelType = typeof(RecordTypeWithPropertiesAndParameters);
var modelMetadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
var modelMetadata = modelMetadataProvider.GetMetadataForType(modelType);
var parameterBinder = ModelBindingTestHelper.GetParameterBinder(modelMetadataProvider);
var parameter = new ParameterDescriptor()
{
Name = "parameter",
ParameterType = modelType,
};
var testContext = ModelBindingTestHelper.GetTestContext(request =>
{
request.QueryString = new QueryString("?Property1=SomeValue");
});
var modelState = testContext.ModelState;
// Act
var modelBindingResult = await parameterBinder.BindModelAsync(parameter, testContext);
Assert.Equal(2, modelState.Count);
Assert.Equal(1, modelState.ErrorCount);
Assert.False(modelState.IsValid);
var entry = Assert.Single(modelState, e => e.Key == "Property1").Value;
Assert.Equal("SomeValue", entry.AttemptedValue);
Assert.Equal("SomeValue", entry.RawValue);
Assert.Equal(ModelValidationState.Valid, entry.ValidationState);
entry = Assert.Single(modelState, e => e.Key == "Property2").Value;
Assert.Equal(ModelValidationState.Invalid, entry.ValidationState);
}
private static void AssertRequiredError(string key, ModelError error)
{
Assert.Equal(ValidationAttributeUtil.GetRequiredErrorMessage(key), error.ErrorMessage);