Skip to content

Commit 7686c0b

Browse files
pranavkmJamesNKdougbu
authored
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 <[email protected]> * 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 <[email protected]> * Update src/Mvc/Mvc.Core/src/Resources.resx Co-authored-by: Doug Bunting <[email protected]> * Apply suggestions from code review Co-authored-by: James Newton-King <[email protected]> Co-authored-by: Doug Bunting <[email protected]>
1 parent 748b368 commit 7686c0b

14 files changed

+319
-120
lines changed

‎src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadata.cs

Lines changed: 65 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,10 @@
77
using System.Collections.ObjectModel;
88
using System.ComponentModel;
99
using System.Diagnostics;
10+
using System.Diagnostics.CodeAnalysis;
1011
using System.Linq;
1112
using System.Reflection;
13+
using Microsoft.AspNetCore.Mvc.Abstractions;
1214
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
1315
using Microsoft.AspNetCore.Mvc.ModelBinding.Validation;
1416
using Microsoft.Extensions.Internal;
@@ -26,11 +28,11 @@ public abstract class ModelMetadata : IEquatable<ModelMetadata?>, IModelMetadata
2628
/// </summary>
2729
public static readonly int DefaultOrder = 10000;
2830

29-
private static readonly IReadOnlyDictionary<ModelMetadata, ModelMetadata> EmptyParameterMapping = new Dictionary<ModelMetadata, ModelMetadata>(0);
30-
3131
private int? _hashCode;
3232
private IReadOnlyList<ModelMetadata>? _boundProperties;
3333
private IReadOnlyDictionary<ModelMetadata, ModelMetadata>? _parameterMapping;
34+
private Exception? _recordTypeValidatorsOnPropertiesError;
35+
private bool _recordTypeConstructorDetailsCalculated;
3436

3537
/// <summary>
3638
/// Creates a new <see cref="ModelMetadata"/>.
@@ -137,37 +139,16 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
137139
}
138140
}
139141

142+
/// <summary>
143+
/// A mapping from parameters to their corresponding properties on a record type.
144+
/// </summary>
140145
internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParameterMapping
141146
{
142147
get
143148
{
144-
if (_parameterMapping != null)
145-
{
146-
return _parameterMapping;
147-
}
148-
149-
if (BoundConstructor is null)
150-
{
151-
_parameterMapping = EmptyParameterMapping;
152-
return _parameterMapping;
153-
}
154-
155-
var boundParameters = BoundConstructor.BoundConstructorParameters!;
156-
var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();
157-
158-
foreach (var parameter in boundParameters)
159-
{
160-
var property = Properties.FirstOrDefault(p =>
161-
string.Equals(p.Name, parameter.ParameterName, StringComparison.Ordinal) &&
162-
p.ModelType == parameter.ModelType);
163-
164-
if (property != null)
165-
{
166-
parameterMapping[parameter] = property;
167-
}
168-
}
149+
Debug.Assert(BoundConstructor != null, "This API can be only called for types with bound constructors.");
150+
CalculateRecordTypeConstructorDetails();
169151

170-
_parameterMapping = parameterMapping;
171152
return _parameterMapping;
172153
}
173154
}
@@ -494,6 +475,62 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParam
494475
/// </summary>
495476
public virtual Func<object[], object>? BoundConstructorInvoker => null;
496477

478+
/// <summary>
479+
/// Gets a value that determines if validators can be constructed using metadata exclusively defined on the property.
480+
/// </summary>
481+
internal virtual bool PropertyHasValidators => false;
482+
483+
/// <summary>
484+
/// Throws if the ModelMetadata is for a record type with validation on properties.
485+
/// </summary>
486+
internal void ThrowIfRecordTypeHasValidationOnProperties()
487+
{
488+
CalculateRecordTypeConstructorDetails();
489+
if (_recordTypeValidatorsOnPropertiesError != null)
490+
{
491+
throw _recordTypeValidatorsOnPropertiesError;
492+
}
493+
}
494+
495+
[MemberNotNull(nameof(_parameterMapping))]
496+
private void CalculateRecordTypeConstructorDetails()
497+
{
498+
if (_recordTypeConstructorDetailsCalculated)
499+
{
500+
Debug.Assert(_parameterMapping != null);
501+
return;
502+
}
503+
504+
505+
var boundParameters = BoundConstructor!.BoundConstructorParameters!;
506+
var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();
507+
508+
foreach (var parameter in boundParameters)
509+
{
510+
var property = Properties.FirstOrDefault(p =>
511+
string.Equals(p.Name, parameter.ParameterName, StringComparison.Ordinal) &&
512+
p.ModelType == parameter.ModelType);
513+
514+
if (property != null)
515+
{
516+
parameterMapping[parameter] = property;
517+
518+
if (property.PropertyHasValidators)
519+
{
520+
// When constructing the mapping of paramets -> properties, also determine
521+
// if the property has any validators (without looking at metadata on the type).
522+
// This will help us throw during validation if a user defines validation attributes
523+
// on the property of a record type.
524+
_recordTypeValidatorsOnPropertiesError = new InvalidOperationException(
525+
Resources.FormatRecordTypeHasValidationOnProperties(ModelType, property.Name));
526+
}
527+
}
528+
}
529+
530+
_recordTypeConstructorDetailsCalculated = true;
531+
_parameterMapping = parameterMapping;
532+
}
533+
497534
/// <summary>
498535
/// Gets a display name for the model.
499536
/// </summary>

‎src/Mvc/Mvc.Abstractions/src/Resources.resx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -177,4 +177,7 @@
177177
<data name="BinderType_MustBeIModelBinder" xml:space="preserve">
178178
<value>The type '{0}' must implement '{1}' to be used as a model binder.</value>
179179
</data>
180-
</root>
180+
<data name="RecordTypeHasValidationOnProperties" xml:space="preserve">
181+
<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>
182+
</data>
183+
</root>

‎src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -75,32 +75,33 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
7575
var bindingSucceeded = false;
7676

7777
var modelMetadata = bindingContext.ModelMetadata;
78+
var boundConstructor = modelMetadata.BoundConstructor;
7879

79-
if (bindingContext.Model == null)
80+
if (boundConstructor != null)
8081
{
81-
var boundConstructor = modelMetadata.BoundConstructor;
82-
if (boundConstructor != null)
83-
{
84-
var values = new object[boundConstructor.BoundConstructorParameters.Count];
85-
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParametersAsync(
86-
bindingContext,
87-
propertyData,
88-
boundConstructor.BoundConstructorParameters,
89-
values);
82+
// Only record types are allowed to have a BoundConstructor. Binding a record type requires
83+
// instantiating the type. This means we'll ignore a previously assigned bindingContext.Model value.
84+
// This behaior is identical to input formatting with S.T.Json and Json.NET.
85+
86+
var values = new object[boundConstructor.BoundConstructorParameters.Count];
87+
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParametersAsync(
88+
bindingContext,
89+
propertyData,
90+
boundConstructor.BoundConstructorParameters,
91+
values);
9092

91-
attemptedBinding |= attemptedParameterBinding;
92-
bindingSucceeded |= parameterBindingSucceeded;
93+
attemptedBinding |= attemptedParameterBinding;
94+
bindingSucceeded |= parameterBindingSucceeded;
9395

94-
if (!CreateModel(bindingContext, boundConstructor, values))
95-
{
96-
return;
97-
}
98-
}
99-
else
96+
if (!CreateModel(bindingContext, boundConstructor, values))
10097
{
101-
CreateModel(bindingContext);
98+
return;
10299
}
103100
}
101+
else if (bindingContext.Model == null)
102+
{
103+
CreateModel(bindingContext);
104+
}
104105

105106
var (attemptedPropertyBinding, propertyBindingSucceeded) = await BindPropertiesAsync(
106107
bindingContext,

‎src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,7 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
143143
static bool IsRecordType(Type type)
144144
{
145145
// Based on the state of the art as described in https://github.com/dotnet/roslyn/issues/45777
146-
var cloneMethod = type.GetMethod("<Clone>$", BindingFlags.Public | BindingFlags.Instance) ??
147-
type.GetMethod("<>Clone", BindingFlags.Public | BindingFlags.Instance);
146+
var cloneMethod = type.GetMethod("<Clone>$", BindingFlags.Public | BindingFlags.Instance);
148147
return cloneMethod != null && cloneMethod.ReturnType == type;
149148
}
150149
}

‎src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadata.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,8 @@ public override bool? HasValidators
468468
}
469469
}
470470

471+
internal override bool PropertyHasValidators => ValidationMetadata.PropertyHasValidators;
472+
471473
internal static bool CalculateHasValidators(HashSet<DefaultModelMetadata> visited, ModelMetadata metadata)
472474
{
473475
RuntimeHelpers.EnsureSufficientExecutionStack();

‎src/Mvc/Mvc.Core/src/ModelBinding/Metadata/HasValidatorsValidationMetadataProvider.cs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (c) .NET Foundation. All rights reserved.
1+
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

44
using System;
@@ -40,7 +40,23 @@ public void CreateValidationMetadata(ValidationMetadataProviderContext context)
4040
if (provider.HasValidators(context.Key.ModelType, context.ValidationMetadata.ValidatorMetadata))
4141
{
4242
context.ValidationMetadata.HasValidators = true;
43-
return;
43+
44+
if (context.Key.MetadataKind == ModelMetadataKind.Property)
45+
{
46+
// For properties, additionally determine that if there's validators defined exclusively
47+
// from property attributes. This is later used to produce a error for record types
48+
// where a record type property that is bound as a parameter defines validation attributes.
49+
50+
if (!(context.PropertyAttributes is IList<object> propertyAttributes))
51+
{
52+
propertyAttributes = context.PropertyAttributes.ToList();
53+
}
54+
55+
if (provider.HasValidators(typeof(object), propertyAttributes))
56+
{
57+
context.ValidationMetadata.PropertyHasValidators = true;
58+
}
59+
}
4460
}
4561
}
4662

‎src/Mvc/Mvc.Core/src/ModelBinding/Metadata/ValidationMetadata.cs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,5 +46,10 @@ public class ValidationMetadata
4646
/// Gets a value that indicates if the model has validators .
4747
/// </summary>
4848
public bool? HasValidators { get; set; }
49+
50+
/// <summary>
51+
/// Gets or sets a value that determines if validators can be constructed using metadata on properties.
52+
/// </summary>
53+
internal bool PropertyHasValidators { get; set; }
4954
}
50-
}
55+
}

‎src/Mvc/Mvc.Core/src/ModelBinding/ModelBindingHelper.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,12 @@ public static async Task<bool> TryUpdateModelAsync(
268268
}
269269

270270
var modelMetadata = metadataProvider.GetMetadataForType(modelType);
271+
272+
if (modelMetadata.BoundConstructor != null)
273+
{
274+
throw new NotSupportedException(Resources.FormatTryUpdateModel_RecordTypeNotSupported(nameof(TryUpdateModelAsync), modelType));
275+
}
276+
271277
var modelState = actionContext.ModelState;
272278

273279
var modelBindingContext = DefaultModelBindingContext.CreateBindingContext(

‎src/Mvc/Mvc.Core/src/ModelBinding/Validation/DefaultComplexObjectValidationStrategy.cs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ public Enumerator(
5858
}
5959
else
6060
{
61+
_modelMetadata.ThrowIfRecordTypeHasValidationOnProperties();
6162
_parameters = _modelMetadata.BoundConstructor.BoundConstructorParameters;
6263
}
6364

‎src/Mvc/Mvc.Core/src/ModelBinding/Validation/ValidationVisitor.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -304,6 +304,11 @@ private bool VisitImplementation(ref ModelMetadata metadata, ref string key, obj
304304
else if (metadata.HasValidators == false &&
305305
ModelState.GetFieldValidationState(key) != ModelValidationState.Invalid)
306306
{
307+
if (metadata.BoundConstructor != null)
308+
{
309+
metadata.ThrowIfRecordTypeHasValidationOnProperties();
310+
}
311+
307312
// No validators will be created for this graph of objects. Mark it as valid if it wasn't previously validated.
308313
var entries = ModelState.FindKeysWithPrefix(key);
309314
foreach (var item in entries)

0 commit comments

Comments
 (0)