Use the declared type to infer NullableContextOptions - #15134
Conversation
Prior to this change MVC used the runtime type rather than the declared type to determine the nullability of a property based on state. In the case of inheritance, this can be erroneous since the declared type may have a different nullability context than the model type. This change addresses this by adding content to `ModelIdentity` that allows inspecting the declared type. * Add an overload to `ModelMetadataIdentity` that allows flowing `PropertyInfo` * Use the declared type in `DataAnnotationsMetadataProvider` to determine nullability based on context. Fixes #14812
NTaylorMullen
left a comment
NTaylorMullen
left a comment
There was a problem hiding this comment.
I mostly have dumb questions. Signing off since you know better 😉
|
|
||
| if (HasNullableAttribute(context.PropertyAttributes, out var propertyHasNullableAttribute)) | ||
| { | ||
| addInferredRequiredAttribute = propertyHasNullableAttribute; |
There was a problem hiding this comment.
Hmm, i'm confused, so if it has the nullable attribute then we DO add the required attribute? Isn't that backwards? I'm sure it's just me misunderstanding the code since your mass amount of tests show differently.
There was a problem hiding this comment.
This might help: https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-metadata.md#type-parameters, but essentially non-null is represented as [Nullable(1)]. HasNullableAttribute returns true if the attribute is present, the propertyHasNullableAttribute says if the value is 1.
| else | ||
| { | ||
| addInferredRequiredAttribute = IsNullableReferenceType( | ||
| property.DeclaringType, |
There was a problem hiding this comment.
And just to clarify, this is the key piece that makes the customers scenario work correct?
There was a problem hiding this comment.
Also, one last question when a user shadows a nullable property are there two model identitys? Aka, one for each property?
There was a problem hiding this comment.
And just to clarify, this is the key piece that makes the customers scenario work correct?
Yeah. Well more like fixes our bug
Also, one last question when a user shadows a nullable property are there two model identitys? Aka, one for each property?
Yup. Although our property discovery will skip over shadowed properties: https://github.com/aspnet/AspNetCore/blob/master/src/Shared/PropertyHelper/PropertyHelper.cs#L441-L477
mkArtakMSFT
left a comment
mkArtakMSFT
left a comment
There was a problem hiding this comment.
My main concern here is about the breaking change. Can we use a flag to avoid making a breaking change?
…erTest.cs Co-Authored-By: N. Taylor Mullen <nimullen@microsoft.com>
mkArtakMSFT
added
the
area-mvc
3 participants
Prior to this change MVC used the runtime type rather than the declared type to
determine the nullability of a property based on state. In the case of inheritance,
this can be erroneous since the declared type may have a different nullability context
than the model type.
This change addresses this by adding content to
ModelIdentitythat allows inspecting thedeclared type.
ModelMetadataIdentitythat allows flowingPropertyInfoDataAnnotationsMetadataProviderto determine nullability based on context.Fixes #14812