JSON support required properties #73063
Conversation
dotnet-issue-labeler
bot
added
area-System.Text.Json
new-api-needs-documentation
labels
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsFixes: #29861 In #72937 I've added internal support for required properties and Since APIs haven't been reviewed yet and source gen part is not finished yet marking this as draft.
|
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonRequiredAttribute.cs
Outdated
Show resolved
Hide resolved
...ext.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonPropertyInfo.cs
Show resolved
Hide resolved
|
Some note for the last two commits: there is no test logic change in there, tests are only adapted so that they can be run both to use source gen and regular reflection resolver. The last commit actually makes it work on source gen (all source gen variants of tests are failing without that change and all pass with that change) |
...ries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonRequiredAttribute.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonRequiredAttribute.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonRequiredAttribute.cs
Show resolved
Hide resolved
...ext.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonPropertyInfo.cs
Show resolved
Hide resolved
| if (memberMetadata.IsRequired) | ||
| { | ||
| sb.Append($@" | ||
| {propertyInfoVarName}.IsRequired = true;"); |
There was a problem hiding this comment.
Do we wish to set other applicable properties directly on JsonPropertyInfo rather than the intermediate JsonPropertyInfoValues (e.g. IsExtensionData)?
There was a problem hiding this comment.
Generally speaking we could probably get rid of most of the usages of JsonMetadataServices but that might be a bit more work since we'd have to refactor also JsonTypeInfo part. While I could start doing it gradually it probably makes more sense to do it in one go but for that we need to be able to customize parameters which currently contract customization doesn't support. The reason this one is separate is because it doesn't have API in JsonMetadataServices but because we can avoid adding such API I went ahead and did it this way. I'll keep changes in this PR to minimum, we can revisit this once we get full parameters support
...ries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonRequiredAttribute.cs
Outdated
Show resolved
Hide resolved
...Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/Attributes/JsonRequiredAttribute.cs
Show resolved
Hide resolved
| public JsonRequiredAttribute() | ||
| { | ||
| } | ||
| public JsonRequiredAttribute() { } |
There was a problem hiding this comment.
Can't the default constructor be removed here? Or is it required by ApiCompat?
There was a problem hiding this comment.
it's not, I followed existing pattern with JsonInclude. We can put better xml doc comment I guess
eiriktsarpalis
added
the
NO-MERGE
|
Marking |
eiriktsarpalis
removed
the
NO-MERGE
ghost
locked as resolved and limited conversation to collaborators
Fixes: #29861
In #72937 I've added internal support for required properties and
requiredkeyword. This is a continuation of that which makes it public.Since APIs haven't been reviewed yet
and source gen part is not finished yetmarking this as draft.