-
Notifications
You must be signed in to change notification settings - Fork 10k
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
Add support for binding record types #23976
Conversation
src/Mvc/Mvc.Abstractions/ref/Microsoft.AspNetCore.Mvc.Abstractions.netcoreapp.cs
Outdated
Show resolved
Hide resolved
/// </summary> | ||
/// <value>Default <see cref="string"/> is "The supplied value is invalid for {0}.".</value> | ||
public virtual Func<string, string> UnknownValueIsInvalidAccessor { get; } = default!; | ||
|
||
/// <summary> | ||
/// Error message the model binding system adds when <see cref="ModelError.Exception"/> is of type | ||
/// <see cref="FormatException"/> or <see cref="OverflowException"/>, value is unknown, and error is associated | ||
/// with a collection element or action parameter. | ||
/// with a collection element . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// with a collection element . | |
/// with a collection element. |
{ | ||
internal ComplexObjectModelBinder() { } | ||
public System.Threading.Tasks.Task BindModelAsync(Microsoft.AspNetCore.Mvc.ModelBinding.ModelBindingContext bindingContext) { throw null; } | ||
public static partial class Log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private/internal
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs
Outdated
Show resolved
Hide resolved
return true; | ||
} | ||
|
||
private async Task<ModelBindingResult> BindProperty( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: BindPropertyAsync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be ValueTask?
return result; | ||
} | ||
|
||
private async Task<ModelBindingResult> BindParameter( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: BindParameterAsync
Also can this be ValueTask?
} | ||
} | ||
|
||
public static class Log |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private/internal
|
||
static bool IsRecordType(Type type) | ||
{ | ||
return type.GetProperty("EqualityContract", BindingFlags.NonPublic | BindingFlags.Instance) != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone could have EqualityContract
property on a type.
Is there no attribute on record type classes to say it is a record type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that I can tell. I have #23975 to follow up with the compiler team. What we really want is the ability to identify the primary constructor. If we had a reliable way to identify that, we could remove this check, unambiguously always say that it gets bound \ validated and remove the 1-constructor limit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "primary" constructor isn't required as far as I can tell. What documentation am I missing❔
And, I'd say we need to unambiguously identify record types before trying to find how best to bind it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "primary" constructor isn't required as far as I can tell. What documentation am I missing❔
Nevermind. If we need to bind a constructor, the primary constructor will exist.
However it's a fairly significant deficit that we can't find it or even detect record
types reliably and have this single constructor requirement.
I didn't look closely at the runtime logic. I'm not familiar with model binding implementation. I'll look at the unit tests later 😄 |
get | ||
{ | ||
// An item may appear as both a constructor parameter and a property. For instance, in record types, | ||
// each constructor parameter is also a settable property and will have the same name, possibly with a difference in case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this if there are more than one constructor? I thought the casing on the property and the parameter would match in a primary constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any restrictions on overriding or even requiring the "positional" constructor of a record. Is this documented❔ Because, if not, there's no way we should assume a relationship between constructor parameter names and the init-only property names.
As https://devblogs.microsoft.com/dotnet/welcome-to-c-9-0/#positional-records says:
It’s perfectly possible to specify your own constructor and deconstructor in a record.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc I linked to is older and https://github.com/dotnet/csharplang/blob/master/proposals/records.md#primary-constructor makes it clear(ish) users can't write their own primary constructor:
It is an error to have a primary constructor and a constructor with the same signature already present in the class.
To @javiercn's point, src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs won't bind unless there is just one constructor.
@@ -2850,6 +2850,21 @@ public partial class CollectionModelBinder<TElement> : Microsoft.AspNetCore.Mvc. | |||
protected virtual object CreateEmptyCollection(System.Type targetType) { throw null; } | |||
protected object CreateInstance(System.Type targetType) { throw null; } | |||
} | |||
public partial class ComplexObjectModelBinder : Microsoft.AspNetCore.Mvc.ModelBinding.IModelBinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new modelbinder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only scratched the surface but I need to do other work today. I feel like we're rushing to get this into P8 but would rather consider the implications carefully.
src/Mvc/Mvc.Abstractions/src/ModelBinding/IPropertyFilterProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs
Outdated
Show resolved
Hide resolved
7c48769
to
32211f0
Compare
@dougbu If this doesn't go out in preview 8 then there is less time to get customer feedback. If large API changes are required then we still have RC1 to make them. |
I was planning on discussing some of these offline with @dougbu since he was one the last people to touch some of these types. There's a couple of interesting changes in here, but given that we have excellent test coverage in this area, I feel pretty confident that these is a very compatible change. As @JamesNK pointed out, we want to get as early feedback as we can. Our rc schedule doesn't really afford too much time for changes so I'd like to push for a preview8 change barring major concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling with this PR and not seeing what makes it a reliable way to bind records. The amount of churn and number of breaking changes is also a big cause for concern.
get | ||
{ | ||
// An item may appear as both a constructor parameter and a property. For instance, in record types, | ||
// each constructor parameter is also a settable property and will have the same name, possibly with a difference in case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of any restrictions on overriding or even requiring the "positional" constructor of a record. Is this documented❔ Because, if not, there's no way we should assume a relationship between constructor parameter names and the init-only property names.
As https://devblogs.microsoft.com/dotnet/welcome-to-c-9-0/#positional-records says:
It’s perfectly possible to specify your own constructor and deconstructor in a record.
src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelMetadataProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Abstractions/test/ModelBinding/ModelStateDictionaryTest.cs
Outdated
Show resolved
Hide resolved
...roblematicParameter_ReturnsTrue_IfPropertyWithModelBindingAttributeHasSameNameAsParameter.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs
Outdated
Show resolved
Hide resolved
|
||
static bool IsRecordType(Type type) | ||
{ | ||
return type.GetProperty("EqualityContract", BindingFlags.NonPublic | BindingFlags.Instance) != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "primary" constructor isn't required as far as I can tell. What documentation am I missing❔
And, I'd say we need to unambiguously identify record types before trying to find how best to bind it.
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs
Show resolved
Hide resolved
@@ -357,5 +453,13 @@ public ModelMetadataCacheEntry(ModelMetadata metadata, DefaultMetadataDetails de | |||
|
|||
public DefaultMetadataDetails Details { get; } | |||
} | |||
|
|||
private class NullServiceProvider : IServiceProvider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this used❔
@@ -3232,7 +3251,7 @@ public static partial class ModelValidatorProviderExtensions | |||
public static void RemoveType(this System.Collections.Generic.IList<Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IModelValidatorProvider> list, System.Type type) { } | |||
public static void RemoveType<TModelValidatorProvider>(this System.Collections.Generic.IList<Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IModelValidatorProvider> list) where TModelValidatorProvider : Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IModelValidatorProvider { } | |||
} | |||
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Property, AllowMultiple=false, Inherited=true)] | |||
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Parameter | System.AttributeTargets.Property, AllowMultiple=false, Inherited=true)] | |||
public sealed partial class ValidateNeverAttribute : System.Attribute, Microsoft.AspNetCore.Mvc.ModelBinding.Validation.IPropertyValidationFilter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very strange to apply IPropertyValdiationFilter
s to parameters.
ac569ac
to
38d4302
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few naming and comment suggestions
src/Mvc/Mvc.Abstractions/src/ModelBinding/ModelStateDictionary.cs
Outdated
Show resolved
Hide resolved
get | ||
{ | ||
// An item may appear as both a constructor parameter and a property. For instance, in record types, | ||
// each constructor parameter is also a settable property and will have the same name, possibly with a difference in case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc I linked to is older and https://github.com/dotnet/csharplang/blob/master/proposals/records.md#primary-constructor makes it clear(ish) users can't write their own primary constructor:
It is an error to have a primary constructor and a constructor with the same signature already present in the class.
To @javiercn's point, src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs won't bind unless there is just one constructor.
|
||
static bool IsRecordType(Type type) | ||
{ | ||
return type.GetProperty("EqualityContract", BindingFlags.NonPublic | BindingFlags.Instance) != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A "primary" constructor isn't required as far as I can tell. What documentation am I missing❔
Nevermind. If we need to bind a constructor, the primary constructor will exist.
However it's a fairly significant deficit that we can't find it or even detect record
types reliably and have this single constructor requirement.
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultMetadataDetails.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultMetadataDetails.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// Gets an instance of <see cref="DefaultComplexObjectValidationStrategy"/>. | ||
/// </summary> | ||
public static readonly IValidationStrategy Instance = new DefaultComplexObjectValidationStrategy(); | ||
public static readonly DefaultComplexObjectValidationStrategy Instance = new DefaultComplexObjectValidationStrategy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another breaking change
38d4302
to
1a6c7c9
Compare
I addressed the naming issues and updated the detection to use the cool new method. Could you have another look? |
1a6c7c9
to
cdb8a47
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great set of changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with implementation of binding, but I think new behavior here is good.
Hopefully we can improve on record type and primary constructor detection before 5.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wave 1 of 3: Hurray, I've reached the cut-over to tests 😺
@@ -43,31 +43,31 @@ public abstract class ModelBindingMessageProvider | |||
/// <summary> | |||
/// Error message the model binding system adds when <see cref="ModelError.Exception"/> is of type | |||
/// <see cref="FormatException"/> or <see cref="OverflowException"/>, value is known, and error is associated | |||
/// with a property. | |||
/// with a property or parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you reverted changes in the ModelBindingMessageProvider
semantics in existing scenarios. Is this <summary />
correct❔ In particular are AttemptedValueIsInvalidAccessor
and UnknownValueIsInvalidAccessor
really used for both action and constructor parameters now❔
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultModelMetadataProvider.cs
Show resolved
Hide resolved
/// applied to a type, the validation system excludes all properties within that type. | ||
/// Indicates that a property or parameter should be excluded from validation. | ||
/// When applied to a property, the validation system excludes that property. | ||
/// When applied to a parameter, the validation system excludes that parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm: This property now works on action parameters too❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently not: I filed an issue to follow up on this: #24241
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taking longer than I expected, sorry. Still need to look at a few remaining huge test files like src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs as well as compare the new and old model binders (locally).
To be a bit more concrete, haven't looked closely enough at
- src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs
- src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinderProvider.cs
- src/Mvc/test/Mvc.IntegrationTests/ComplexRecordIntegrationTest.cs
- src/Mvc/test/Mvc.IntegrationTests/ComplexTypeIntegrationTestBase.cs
- src/Mvc/test/Mvc.IntegrationTests/ComplexTypeModelBinderIntegrationTest.cs
- src/Mvc/test/Mvc.IntegrationTests/ValidationWithRecordIntegrationTests.cs
src/Mvc/Mvc.Core/test/ModelBinding/Binders/ComplexObjectModelBinderProviderTest.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/test/ModelBinding/Metadata/DefaultBindingMetadataProviderTest.cs
Show resolved
Hide resolved
{ | ||
// Arrange | ||
var modelType = typeof(SimpleRecordType); | ||
var constructor = modelType.GetConstructors().First(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Most .First()
calls in this class should be .Single
or even Assert.Single(...)
guards to ensure Reflection doesn't mess the tests up.
} | ||
|
||
[Fact] | ||
public async Task ActionParameter_MultipleConstructors_ParameterlessConstructor() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this checked a billion other ways❔ That is, this isn't checking a record
type and it's unclear why the test was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we can remove it. I started off writing these tests before I got to the unit tests. I'll reduce this to a single test that verifies record types with multiple ctors throw.
src/Mvc/test/Mvc.IntegrationTests/ActionParametersIntegrationTest.cs
Outdated
Show resolved
Hide resolved
/// <summary> | ||
/// <see cref="IModelBinder"/> implementation for binding complex types. | ||
/// </summary> | ||
public sealed class ComplexObjectModelBinder : IModelBinder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Obsolete]
ing the old type is very different from preventing subclassing the new one. This means those subclassing the old type are at a dead end. They either need to copy the entire new class or stick with subclassing the old one. Either way, they won't automatically pick up fixes we make when they rebuild.
In addition, we have no other sealed
model binders and I'm not sure this is the right precedent. Suggest leaving this class unsealed
and making equivalent extension points virtual
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW I remembered the usual reason devs want to wrap or extend the ComplexTypeModelBinder
: Polymorphic model binding. Not sure we want to strand them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition, we have no other sealed model binders and I'm not sure this is the right precedent. Suggest leaving this class unsealed and making equivalent extension points virtual.
Let's discuss this during API reviews. I'm leaving this type as-is for the time being.
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs
Outdated
Show resolved
Hide resolved
private Func<object> _modelCreator; | ||
|
||
|
||
internal ComplexObjectModelBinder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the constructor internal
❔ This prevents extension through delegation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. Our recommendation would be to use the binder provider to construct this type rather than new up these instances. Let's talk about this during API review next week.
// If there are no properties on the model, and no constructor parameters, there is nothing to bind. We are here means this is not a top | ||
// level object. So we return false. | ||
var modelMetadata = bindingContext.ModelMetadata; | ||
var performsConstructorBinding = bindingContext.Model == null && modelMetadata.BoundConstructor != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking: Does Model ==null
break some validations when updating records
because we don't check e.g. [BindRequired]
and will happily change init
-only properties❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding a couple for this - when calling TryUpdateModel
, we would expect properties that do not appear as parameters to be updated, but not the parameters. e.g.
public record Person(string Name)
{
public int Age { get; set; }
}
var person = new Person("initial-name");
TryUpdateModel(person, ...);
In this case, Name
will never be updated we treat it as immutable once the object is constructed, but Age
is up for grabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again this violates the principle of least surprise, especially when the record
is a property of something else. We replace struct
s even if they have an existing value, why not record
s❔ What happens when devs use with
to attempt to create a new record
with new values that are passed to the constructor e.g. does it fail or does it use the copy constructor or clone method then overwrite the values❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You get a new instance though. TryUpdateModel scenarios require that you update the model in place. For record types this would require us to set parameter values via properties, which feels iffy to me. But let's discuss this with the larger team, maybe they feel differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, getting a new instance is exactly what I'd expect if I have
public record Person(string Name);
or
public record Person
{
public string Name { get; init; }
}
as long as I'm calling TryUpdateModel(...)
on a class containing Person
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some tests and it admittedly feels wonky. Maybe a fix for this is to always bind the constructor unless it's a top-level type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #24265 for tracking
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinder.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinderProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Binders/ComplexObjectModelBinderProvider.cs
Outdated
Show resolved
Hide resolved
src/Mvc/test/Mvc.IntegrationTests/ComplexTypeIntegrationTestBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel this is quite ready without addressing some of my comments. Most urgent is to make it possible to extend or wrap the new model binder. Minimal change there would likely be to make the constructor public
, allowing its use within a polymorphic binder.
Assert.Null(state.Value.RawValue); | ||
} | ||
|
||
private record TestModel(TestInnerModel[] InnerModels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this property / parameter lose its Array.Empty<TestInnerModel>()
default❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaults values have to compile time constants. Enumerable.Empty isn't unfortunately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think new TestInnerModel[] {}
or new TestInnerModel[0]
should work
ParameterType = typeof(RecordTypeWithSettableProperty1) | ||
}; | ||
|
||
// Need to have a key here so that the ComplexObjectModelBinder will recurse to bind elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment. The data is used when binding.
ParameterType = typeof(RecordTypeWithSettableProperty1) | ||
}; | ||
|
||
// Need to have a key here so that the ComplexObjectModelBinder will recurse to bind elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another copy-'n-paste error❔ This looks consistent in the last few tests though incorrect in many (all❔) cases.
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs
Show resolved
Hide resolved
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/DefaultBindingMetadataProvider.cs
Show resolved
Hide resolved
b6fc8b9
to
e2887b8
Compare
} | ||
|
||
[Fact] | ||
public async Task TryUpdateModel_RecordTypeModel_DoesNotOverwriteConstructorParameters() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, this violates the principle of least surprise. Devs likely expect the requested updates to be honoured and, because you're finding the validators for the properties in the constructor parameters, it could work just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's bring this up doing the design review. We could tweak this if we choose to. I'd thought it would be more surprising to change what are considered immutable properties
@dougbu I think this should be good to get in. We can discuss the design and API choices next week and fix them during rc. |
@@ -51,23 +51,23 @@ public abstract class ModelBindingMessageProvider | |||
/// <summary> | |||
/// Error message the model binding system adds when <see cref="ModelError.Exception"/> is of type | |||
/// <see cref="FormatException"/> or <see cref="OverflowException"/>, value is known, and error is associated | |||
/// with a collection element or action parameter. | |||
/// with a collection element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still confused by these comments. What actually changed❔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, missed reverting this.
This change adds support for binding and validating record types with exactly one constructor.
While binding can be performed on arbitrary constructors, validation requires that we can deterministically tell what constructor was chosen and that we have the ability to read the values assigned to the parameters. The primary constructor on a record type has both these traits, so we can safely model bind and validate these.
Right now, we don't have a very good way to identify a primary constructor. If we're able to address this as part of #23975, we can remove this limitation.
Contributes to #23465
A summary of what works and what does not:
We allow binding to and validating record types.
For this to work, we require that the type
We do not bind to regular POCOs that do not have parameterless constructors.
e.g., this will result in an exception saying that the type must have a parameterless constructor.
When a record type has more than one constructor, binding will fail stating this is unsupported:
Record types with manually authored constructors that look like primary constructors will work:
For record types, validation and binding metadata on parameters is used. Any metadata on properties is ignored.
Validation uses metadata on the parameter but uses the property to read the value. In the ordinary case with primary constructors, the two would be identical. However, there are ways to defeat it:
TryUpdateModel does not update parameters on a record type
In this case, MVC will not attempt to bind
Name
again. However,Age
is allowed to be updated