Skip to content

Commit cdb8a47

Browse files
pranavkmpranavkm
committed
Changes per PR comments
1 parent 00cb06f commit cdb8a47

File tree

12 files changed

+42
-53
lines changed

12 files changed

+42
-53
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@
66
namespace Microsoft.AspNetCore.Mvc.ModelBinding
77
{
88
/// <summary>
9-
/// Provides a predicate which can determines which model properties should be bound by model binding.
9+
/// Provides a predicate which can determines which model properties or parameters should be bound by model binding.
1010
/// </summary>
1111
public interface IPropertyFilterProvider
1212
{
1313
/// <summary>
14+
/// <para>
1415
/// Gets a predicate which can determines which model properties should be bound by model binding.
16+
/// </para>
1517
/// <para>
1618
/// This predicate is also used to determine which parameters are bound when a model's constructor is bound.
1719
/// </para>

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

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,10 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
105105
{
106106
get
107107
{
108-
// An item may appear as both a constructor parameter and a property. For instance, in record types,
109-
// each constructor parameter is also a settable property and will have the same name, possibly with a difference in case.
108+
// In record types, each constructor parameter in the primary constructor is also a settable property with the same name.
110109
// Executing model binding on these parameters twice may have detrimental effects, such as duplicate validation entries,
111110
// or failures if a model expects to be bound exactly ones.
112-
// Consequently when a bound constructor is present, we only bind and validate the subset of properties whose names
111+
// Consequently when binding to a constructor, we only bind and validate the subset of properties whose names
113112
// haven't appeared as parameters.
114113
if (BoundConstructor is null)
115114
{
@@ -118,7 +117,7 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
118117

119118
if (_boundProperties is null)
120119
{
121-
var boundParameters = BoundConstructor.Parameters!;
120+
var boundParameters = BoundConstructor.BoundConstructorParameters!;
122121
var boundProperties = new List<ModelMetadata>();
123122

124123
foreach (var metadata in Properties)
@@ -138,7 +137,7 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
138137
}
139138
}
140139

141-
internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
140+
internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> BoundConstructorParameterMapping
142141
{
143142
get
144143
{
@@ -153,7 +152,7 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
153152
return _parameterMapping;
154153
}
155154

156-
var boundParameters = BoundConstructor.Parameters!;
155+
var boundParameters = BoundConstructor.BoundConstructorParameters!;
157156
var parameterMapping = new Dictionary<ModelMetadata, ModelMetadata>();
158157

159158
foreach (var parameter in boundParameters)
@@ -174,15 +173,15 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
174173
}
175174

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

181180
/// <summary>
182-
/// Gets the collection of <see cref="ModelMetadata"/> instances for a constructor's parameters.
181+
/// Gets the collection of <see cref="ModelMetadata"/> instances for parameters on a <see cref="BoundConstructor"/>.
183182
/// This is only available when <see cref="MetadataKind"/> is <see cref="ModelMetadataKind.Constructor"/>.
184183
/// </summary>
185-
public virtual IReadOnlyList<ModelMetadata>? Parameters { get; }
184+
public virtual IReadOnlyList<ModelMetadata>? BoundConstructorParameters { get; }
186185

187186
/// <summary>
188187
/// Gets the name of a model if specified explicitly using <see cref="IModelNameProvider"/>.
@@ -491,9 +490,9 @@ internal IReadOnlyDictionary<ModelMetadata, ModelMetadata> ParameterMapping
491490
public abstract Action<object, object> PropertySetter { get; }
492491

493492
/// <summary>
494-
/// Gets a delegate that invokes the constructor.
493+
/// Gets a delegate that invokes the bound constructor <see cref="BoundConstructor" /> if non-<see langword="null" />.
495494
/// </summary>
496-
public virtual Func<object[], object>? ConstructorInvoker => null;
495+
public virtual Func<object[], object>? BoundConstructorInvoker => null;
497496

498497
/// <summary>
499498
/// Gets a display name for the model.

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99
using System.Runtime.CompilerServices;
1010
using Microsoft.AspNetCore.Mvc.Abstractions;
1111
using Microsoft.AspNetCore.Mvc.Formatters;
12-
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
1312
using Microsoft.Extensions.Primitives;
1413

1514
namespace Microsoft.AspNetCore.Mvc.ModelBinding

‎src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelMetadataTest.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -731,9 +731,9 @@ public override Action<object, object> PropertySetter
731731

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

734-
public override Func<object[], object> ConstructorInvoker => throw new NotImplementedException();
734+
public override Func<object[], object> BoundConstructorInvoker => throw new NotImplementedException();
735735

736-
public override IReadOnlyList<ModelMetadata> Parameters => throw new NotImplementedException();
736+
public override IReadOnlyList<ModelMetadata> BoundConstructorParameters => throw new NotImplementedException();
737737
}
738738

739739
private class CollectionImplementation : ICollection<string>

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,11 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
8282
var boundConstructor = modelMetadata.BoundConstructor;
8383
if (boundConstructor != null)
8484
{
85-
var values = new object[boundConstructor.Parameters.Count];
85+
var values = new object[boundConstructor.BoundConstructorParameters.Count];
8686
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParameters(
8787
bindingContext,
8888
propertyData,
89-
boundConstructor.Parameters,
89+
boundConstructor.BoundConstructorParameters,
9090
values);
9191

9292
attemptedBinding |= attemptedParameterBinding;
@@ -162,7 +162,7 @@ internal static bool CreateModel(ModelBindingContext bindingContext, ModelMetada
162162
{
163163
try
164164
{
165-
bindingContext.Model = boundConstructor.ConstructorInvoker(values);
165+
bindingContext.Model = boundConstructor.BoundConstructorInvoker(values);
166166
return true;
167167
}
168168
catch (Exception ex)
@@ -552,7 +552,7 @@ private int CanBindAnyModelItem(ModelBindingContext bindingContext)
552552
var performsConstructorBinding = bindingContext.Model == null && modelMetadata.BoundConstructor != null;
553553

554554
if (modelMetadata.Properties.Count == 0 &&
555-
(!performsConstructorBinding || modelMetadata.BoundConstructor.Parameters.Count == 0))
555+
(!performsConstructorBinding || modelMetadata.BoundConstructor.BoundConstructorParameters.Count == 0))
556556
{
557557
Log.NoPublicSettableItems(_logger, bindingContext);
558558
return NoDataAvailable;
@@ -617,7 +617,7 @@ private int CanBindAnyModelItem(ModelBindingContext bindingContext)
617617

618618
if (performsConstructorBinding)
619619
{
620-
var parameters = bindingContext.ModelMetadata.BoundConstructor.Parameters;
620+
var parameters = bindingContext.ModelMetadata.BoundConstructor.BoundConstructorParameters;
621621
for (var i = 0; i < parameters.Count; i++)
622622
{
623623
var parameterMetadata = parameters[i];

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,11 +48,11 @@ private static IReadOnlyList<IModelBinder> GetParameterBinders(ModelBinderProvid
4848
return Array.Empty<IModelBinder>();
4949
}
5050

51-
var parameterBinders = boundConstructor.Parameters.Count == 0 ? Array.Empty<IModelBinder>() : new IModelBinder[boundConstructor.Parameters.Count];
51+
var parameterBinders = boundConstructor.BoundConstructorParameters.Count == 0 ? Array.Empty<IModelBinder>() : new IModelBinder[boundConstructor.BoundConstructorParameters.Count];
5252

5353
for (var i = 0; i < parameterBinders.Length; i++)
5454
{
55-
parameterBinders[i] = context.CreateBinder(boundConstructor.Parameters[i]);
55+
parameterBinders[i] = context.CreateBinder(boundConstructor.BoundConstructorParameters[i]);
5656
}
5757

5858
return parameterBinders;

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,9 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
104104
}
105105

106106
// For record types, we will support binding and validating the primary constructor.
107-
// There isn't metadata to identify a primary constructor. We require that at most one constructor is defined on the type,
108-
// and that every parameter on the constructor has a corresponding property.
107+
// There isn't metadata to identify a primary constructor. Our heuristic is:
108+
// We require exactly one constructor to be defined on the type, and that every parameter on
109+
// that constructor is mapped to a property with the same name and type.
109110

110111
if (constructors.Length > 1)
111112
{
@@ -135,7 +136,9 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
135136

136137
static bool IsRecordType(Type type)
137138
{
138-
return type.GetProperty("EqualityContract", BindingFlags.NonPublic | BindingFlags.Instance) != null;
139+
// Based on the state of the art as described in https://github.com/dotnet/roslyn/issues/45777
140+
var cloneMethod = type.GetMethod("<>Clone", BindingFlags.Public | BindingFlags.Instance);
141+
return cloneMethod != null && cloneMethod.ReturnType == type;
139142
}
140143
}
141144

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public DefaultMetadataDetails(ModelMetadataIdentity key, ModelAttributes attribu
5757
/// <summary>
5858
/// Gets or sets the <see cref="ModelMetadata"/> entries for constructor parameters.
5959
/// </summary>
60-
public ModelMetadata[] ConstructorParameters { get; set; }
60+
public ModelMetadata[] BoundConstructorParameters { get; set; }
6161

6262
/// <summary>
6363
/// Gets or sets a property getter delegate to get the property value from a model object.
@@ -70,7 +70,7 @@ public DefaultMetadataDetails(ModelMetadataIdentity key, ModelAttributes attribu
7070
public Action<object, object> PropertySetter { get; set; }
7171

7272
/// <summary>
73-
/// Gets or sets a delegate used to construct an object using the model binding constructor.
73+
/// Gets or sets a delegate used to invoke the bound constructor for record types.
7474
/// </summary>
7575
public Func<object[], object> BoundConstructorInvoker { get; set; }
7676

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,7 @@ public override ModelMetadata BoundConstructor
407407
}
408408
}
409409

410-
public override IReadOnlyList<ModelMetadata> Parameters => _details.ConstructorParameters;
410+
public override IReadOnlyList<ModelMetadata> BoundConstructorParameters => _details.BoundConstructorParameters;
411411

412412
/// <inheritdoc />
413413
public override IPropertyFilterProvider PropertyFilterProvider => BindingMetadata.PropertyFilterProvider;
@@ -517,7 +517,7 @@ internal static bool CalculateHasValidators(HashSet<DefaultModelMetadata> visite
517517
}
518518
else if (defaultModelMetadata.IsComplexType)
519519
{
520-
var parameters = defaultModelMetadata.BoundConstructor?.Parameters ?? Array.Empty<ModelMetadata>();
520+
var parameters = defaultModelMetadata.BoundConstructor?.BoundConstructorParameters ?? Array.Empty<ModelMetadata>();
521521
foreach (var parameter in parameters)
522522
{
523523
if (CalculateHasValidators(visited, parameter))
@@ -559,7 +559,7 @@ public override IReadOnlyList<object> ValidatorMetadata
559559
/// <inheritdoc />
560560
public override Action<object, object> PropertySetter => _details.PropertySetter;
561561

562-
public override Func<object[], object> ConstructorInvoker => _details.BoundConstructorInvoker;
562+
public override Func<object[], object> BoundConstructorInvoker => _details.BoundConstructorInvoker;
563563

564564
internal DefaultMetadataDetails Details => _details;
565565

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

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,8 @@
55
using System.Collections.Concurrent;
66
using System.Collections.Generic;
77
using System.Diagnostics;
8-
using System.Linq;
98
using System.Linq.Expressions;
109
using System.Reflection;
11-
using Microsoft.AspNetCore.Mvc.Infrastructure;
12-
using Microsoft.Extensions.DependencyInjection;
1310
using Microsoft.Extensions.Internal;
1411
using Microsoft.Extensions.Options;
1512

@@ -275,7 +272,7 @@ private DefaultMetadataDetails CreateConstructorDetails(ModelMetadataIdentity co
275272
}
276273

277274
var constructorDetails = new DefaultMetadataDetails(constructorKey, ModelAttributes.Empty);
278-
constructorDetails.ConstructorParameters = parameterMetadata;
275+
constructorDetails.BoundConstructorParameters = parameterMetadata;
279276
constructorDetails.BoundConstructorInvoker = CreateObjectFactory(constructor);
280277

281278
return constructorDetails;
@@ -453,13 +450,5 @@ public ModelMetadataCacheEntry(ModelMetadata metadata, DefaultMetadataDetails de
453450

454451
public DefaultMetadataDetails Details { get; }
455452
}
456-
457-
private class NullServiceProvider : IServiceProvider
458-
{
459-
public static readonly NullServiceProvider Instance = new NullServiceProvider();
460-
461-
// We do not expect this to be invoked at all.
462-
public object GetService(Type serviceType) => throw new NotSupportedException();
463-
}
464453
}
465454
}

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

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,7 @@
44
using System;
55
using System.Collections;
66
using System.Collections.Generic;
7-
using System.Linq;
8-
using System.Reflection;
97
using Microsoft.AspNetCore.Mvc.Core;
10-
using Microsoft.AspNetCore.Mvc.ModelBinding.Metadata;
118

129
namespace Microsoft.AspNetCore.Mvc.ModelBinding.Validation
1310
{
@@ -19,7 +16,7 @@ internal class DefaultComplexObjectValidationStrategy : IValidationStrategy
1916
/// <summary>
2017
/// Gets an instance of <see cref="DefaultComplexObjectValidationStrategy"/>.
2118
/// </summary>
22-
public static readonly DefaultComplexObjectValidationStrategy Instance = new DefaultComplexObjectValidationStrategy();
19+
public static readonly IValidationStrategy Instance = new DefaultComplexObjectValidationStrategy();
2320

2421
private DefaultComplexObjectValidationStrategy()
2522
{
@@ -61,7 +58,7 @@ public Enumerator(
6158
}
6259
else
6360
{
64-
_parameters = _modelMetadata.BoundConstructor.Parameters;
61+
_parameters = _modelMetadata.BoundConstructor.BoundConstructorParameters;
6562
}
6663

6764
_properties = _modelMetadata.BoundProperties;
@@ -95,7 +92,7 @@ public bool MoveNext()
9592
}
9693
else
9794
{
98-
if (!_modelMetadata.ParameterMapping.TryGetValue(parameter, out var property))
95+
if (!_modelMetadata.BoundConstructorParameterMapping.TryGetValue(parameter, out var property))
9996
{
10097
throw new InvalidOperationException(
10198
Resources.FormatValidationStrategy_MappedPropertyNotFound(parameter, _modelMetadata.ModelType));

‎src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultModelMetadataTest.cs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,7 +1275,7 @@ public void CalculateHasValidators_RecordType_ParametersWithNoValidators()
12751275
var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: false);
12761276

12771277
var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
1278-
constructorMetadata.Details.ConstructorParameters = new[]
1278+
constructorMetadata.Details.BoundConstructorParameters = new[]
12791279
{
12801280
parameterMetadata,
12811281
};
@@ -1316,7 +1316,7 @@ public void CalculateHasValidators_RecordType_ParametersWithValidators()
13161316
var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: true);
13171317

13181318
var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
1319-
constructorMetadata.Details.ConstructorParameters = new[]
1319+
constructorMetadata.Details.BoundConstructorParameters = new[]
13201320
{
13211321
parameterMetadata,
13221322
};
@@ -1365,7 +1365,7 @@ public void CalculateHasValidators_RecordTypeWithProperty_NoValidators()
13651365
var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: false);
13661366

13671367
var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
1368-
constructorMetadata.Details.ConstructorParameters = new[]
1368+
constructorMetadata.Details.BoundConstructorParameters = new[]
13691369
{
13701370
parameterMetadata,
13711371
};
@@ -1409,7 +1409,7 @@ public void CalculateHasValidators_RecordTypeWithProperty_PropertyHasValidators(
14091409
var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: true);
14101410

14111411
var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
1412-
constructorMetadata.Details.ConstructorParameters = new[]
1412+
constructorMetadata.Details.BoundConstructorParameters = new[]
14131413
{
14141414
parameterMetadata,
14151415
};
@@ -1453,7 +1453,7 @@ public void CalculateHasValidators_RecordTypeWithProperty_MappedPropertyHasValid
14531453
var parameterMetadata = CreateModelMetadata(parameterId, metadataProvider.Object, hasValidators: false);
14541454

14551455
var constructorMetadata = CreateModelMetadata(ModelMetadataIdentity.ForConstructor(constructor, modelType), metadataProvider.Object, null);
1456-
constructorMetadata.Details.ConstructorParameters = new[]
1456+
constructorMetadata.Details.BoundConstructorParameters = new[]
14571457
{
14581458
parameterMetadata,
14591459
};

0 commit comments

Comments
 (0)