Skip to content

Commit e2887b8

Browse files
committed
Changes per PR comments
1 parent 3f366ea commit e2887b8

18 files changed

+375
-264
lines changed

src/Components/Web.JS/dist/Release/blazor.server.js

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/Mvc/Mvc.Abstractions/src/ModelBinding/Metadata/ModelBindingMessageProvider.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public abstract class ModelBindingMessageProvider
4343
/// <summary>
4444
/// Error message the model binding system adds when <see cref="ModelError.Exception"/> is of type
4545
/// <see cref="FormatException"/> or <see cref="OverflowException"/>, value is known, and error is associated
46-
/// with a property or parameter.
46+
/// with a property.
4747
/// </summary>
4848
/// <value>Default <see cref="string"/> is "The value '{0}' is not valid for {1}.".</value>
4949
public virtual Func<string, string, string> AttemptedValueIsInvalidAccessor { get; } = default!;

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ internal IReadOnlyList<ModelMetadata> BoundProperties
106106
get
107107
{
108108
// In record types, each constructor parameter in the primary constructor is also a settable property with the same name.
109-
// Executing model binding on these parameters twice may have detrimental effects, such as duplicate validation entries,
109+
// Executing model binding on these parameters twice may have detrimental effects, such as duplicate ModelState entries,
110110
// or failures if a model expects to be bound exactly ones.
111111
// Consequently when binding to a constructor, we only bind and validate the subset of properties whose names
112112
// haven't appeared as parameters.

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

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ public sealed class ComplexObjectModelBinder : IModelBinder
3636
private readonly ILogger _logger;
3737
private Func<object> _modelCreator;
3838

39-
4039
internal ComplexObjectModelBinder(
4140
IDictionary<ModelMetadata, IModelBinder> propertyBinders,
4241
IReadOnlyList<IModelBinder> parameterBinders,
@@ -83,7 +82,7 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
8382
if (boundConstructor != null)
8483
{
8584
var values = new object[boundConstructor.BoundConstructorParameters.Count];
86-
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParameters(
85+
var (attemptedParameterBinding, parameterBindingSucceeded) = await BindParametersAsync(
8786
bindingContext,
8887
propertyData,
8988
boundConstructor.BoundConstructorParameters,
@@ -103,7 +102,7 @@ private async Task BindModelCoreAsync(ModelBindingContext bindingContext, int pr
103102
}
104103
}
105104

106-
var (attemptedPropertyBinding, propertyBindingSucceeded) = await BindProperties(
105+
var (attemptedPropertyBinding, propertyBindingSucceeded) = await BindPropertiesAsync(
107106
bindingContext,
108107
propertyData,
109108
modelMetadata.BoundProperties);
@@ -225,7 +224,7 @@ internal void CreateModel(ModelBindingContext bindingContext)
225224
bindingContext.Model = _modelCreator();
226225
}
227226

228-
private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindParameters(
227+
private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindParametersAsync(
229228
ModelBindingContext bindingContext,
230229
int propertyData,
231230
IReadOnlyList<ModelMetadata> parameters,
@@ -322,7 +321,7 @@ internal void CreateModel(ModelBindingContext bindingContext)
322321
return (attemptedBinding, bindingSucceeded);
323322
}
324323

325-
private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindProperties(
324+
private async ValueTask<(bool attemptedBinding, bool bindingSucceeded)> BindPropertiesAsync(
326325
ModelBindingContext bindingContext,
327326
int propertyData,
328327
IReadOnlyList<ModelMetadata> boundProperties)
@@ -344,9 +343,6 @@ internal void CreateModel(ModelBindingContext bindingContext)
344343
continue;
345344
}
346345

347-
var fieldName = property.BinderModelName ?? property.PropertyName;
348-
var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
349-
350346
var propertyBinder = _propertyBinders[property];
351347
if (propertyBinder is PlaceholderBinder)
352348
{
@@ -372,6 +368,8 @@ internal void CreateModel(ModelBindingContext bindingContext)
372368
}
373369
}
374370

371+
var fieldName = property.BinderModelName ?? property.PropertyName;
372+
var modelName = ModelNames.CreatePropertyModelName(bindingContext.ModelName, fieldName);
375373
var result = await BindPropertyAsync(bindingContext, property, propertyBinder, fieldName, modelName);
376374

377375
if (result.IsModelSet)

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

Lines changed: 6 additions & 3 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;
@@ -25,6 +25,7 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)
2525
if (metadata.IsComplexType && !metadata.IsCollectionType)
2626
{
2727
var loggerFactory = context.Services.GetRequiredService<ILoggerFactory>();
28+
var logger = loggerFactory.CreateLogger<ComplexObjectModelBinder>();
2829
var parameterBinders = GetParameterBinders(context);
2930

3031
var propertyBinders = new Dictionary<ModelMetadata, IModelBinder>();
@@ -34,7 +35,7 @@ public IModelBinder GetBinder(ModelBinderProviderContext context)
3435
propertyBinders.Add(property, context.CreateBinder(property));
3536
}
3637

37-
return new ComplexObjectModelBinder(propertyBinders, parameterBinders, loggerFactory.CreateLogger<ComplexObjectModelBinder>());
38+
return new ComplexObjectModelBinder(propertyBinders, parameterBinders, logger);
3839
}
3940

4041
return null;
@@ -48,7 +49,9 @@ private static IReadOnlyList<IModelBinder> GetParameterBinders(ModelBinderProvid
4849
return Array.Empty<IModelBinder>();
4950
}
5051

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

5356
for (var i = 0; i < parameterBinders.Length; i++)
5457
{

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ private static ConstructorInfo GetRecordTypeConstructor(Type type, ConstructorIn
116116
var constructor = constructors[0];
117117

118118
var parameters = constructor.GetParameters();
119+
if (parameters.Length == 0)
120+
{
121+
// We do not need to do special handling for parameterless constructors.
122+
return null;
123+
}
124+
119125
var properties = PropertyHelper.GetVisibleProperties(type);
120126

121127
for (var i = 0; i < parameters.Length; i++)

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -282,8 +282,7 @@ static Func<object[], object> CreateObjectFactory(ConstructorInfo constructor)
282282
var args = Expression.Parameter(typeof(object[]), "args");
283283
var factoryExpressionBody = BuildFactoryExpression(constructor, args);
284284

285-
var factoryLamda = Expression.Lambda<Func<object[], object>>(
286-
factoryExpressionBody, args);
285+
var factoryLamda = Expression.Lambda<Func<object[], object>>(factoryExpressionBody, args);
287286

288287
return factoryLamda.Compile();
289288
}

src/Mvc/Mvc.Core/src/ModelBinding/ParameterBinder.cs

Lines changed: 1 addition & 1 deletion
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;

src/Mvc/Mvc.Core/src/Resources.resx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -532,6 +532,6 @@
532532
<value>Could not create an instance of type '{0}'. Model bound complex types must not be abstract or value types and must have a parameterless constructor. Record types must have a single primary constructor.</value>
533533
</data>
534534
<data name="ValidationStrategy_MappedPropertyNotFound" xml:space="preserve">
535-
<value>No property found that maps to constructor parameter '{0}' for type '{1}'. Validation requires that each bound parameter must have a property to read the value.</value>
535+
<value>No property found that maps to pconstructor 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>
536536
</data>
537537
</root>

src/Mvc/Mvc.Core/test/ModelBinding/Binders/ComplexObjectModelBinderProviderTest.cs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
1010
{
11-
[Obsolete]
1211
public class ComplexObjectModelBinderProviderTest
1312
{
1413
[Theory]

src/Mvc/Mvc.Core/test/ModelBinding/Binders/ComplexTypeModelBinderTest.cs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@
1919

2020
namespace Microsoft.AspNetCore.Mvc.ModelBinding.Binders
2121
{
22-
[Obsolete]
22+
#pragma warning disable CS0618 // Type or member is obsolete
2323
public class ComplexTypeModelBinderTest
2424
{
2525
private static readonly IModelMetadataProvider _metadataProvider = TestModelMetadataProvider.CreateDefaultProvider();
@@ -1662,4 +1662,5 @@ protected override void SetProperty(
16621662
}
16631663
}
16641664
}
1665+
#pragma warning restore CS0618 // Type or member is obsolete
16651666
}

src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -748,17 +748,27 @@ public void GetBoundConstructor_ReturnsPrimaryConstructor_ForRecordType()
748748
p => Assert.Equal("name", p.Name));
749749
}
750750

751-
private record RecordTypeWithPrimaryAndParameterlessConstructor(string name)
751+
private record RecordTypeWithDefaultConstructor
752752
{
753-
public RecordTypeWithPrimaryAndParameterlessConstructor() : this(string.Empty) {}
753+
public string Name { get; init; }
754+
755+
public int Age { get; init; }
754756
}
755757

756-
[Fact]
757-
public void GetBoundConstructor_ReturnsNull_ForRecordTypeWithParameterlessConstructor()
758+
private record RecordTypeWithParameterlessConstructor
758759
{
759-
// Arrange
760-
var type = typeof(RecordTypeWithPrimaryAndParameterlessConstructor);
760+
public RecordTypeWithParameterlessConstructor() { }
761+
762+
public string Name { get; init; }
761763

764+
public int Age { get; init; }
765+
}
766+
767+
[Theory]
768+
[InlineData(typeof(RecordTypeWithDefaultConstructor))]
769+
[InlineData(typeof(RecordTypeWithParameterlessConstructor))]
770+
public void GetBoundConstructor_ReturnsNull_ForRecordTypeWithParameterlessConstructor(Type type)
771+
{
762772
// Act
763773
var result = DefaultBindingMetadataProvider.GetBoundConstructor(type);
764774

0 commit comments

Comments
 (0)