Make JSON support required properties #72937

krwq · 2022-07-27T13:04:32Z

Fixes #70945
Contributes to #29861

Implements new required keyword (added by language in this release) according to proposal shared here: #70945 (comment)

required keyword will map internally to JsonPropertyInfo.IsRequired - in #29861 we should make it public. One exception to this rule is when constructor has SetsRequiredMembersAttribute which cancels effects of required

…SetsRequiredMembers'

ghost · 2022-07-27T13:05:11Z

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #70945
Contributes to #29861

Implements new required keyword according to proposal shared here: #70945 (comment)

required keyword will map internally to JsonPropertyInfo.IsRequired - in Implement a concept of "Required" properties #29861 we should make it public
- one exception to this rule is when constructor has SetsRequiredMembersAttribute which cancels effects of required
fixes bug (didn't see any report) for null not always being ignored when IgnoreNullValues is set - this was found while testing - we actually sometimes did not ignore null for streams or when continuations were happening in combination with parametrized constructor (we used to set value using extension data property APIs which never checked for IgnoreNullValue)

Author:	krwq
Assignees:	-
Labels:	`area-System.Text.Json`
Milestone:	-

src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs

...stem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs

src/libraries/System.Text.Json/src/Resources/Strings.resx

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs

...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs

...tem/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs

...xt/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs

...es/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs

eiriktsarpalis · 2022-07-27T19:11:35Z

fixes bug (didn't see any report) for null not always being ignored when IgnoreNullValues is set - this was found while testing - we actually sometimes did not ignore null for streams or when continuations were happening in combination with parametrized constructor (we used to set value using extension data property APIs which never checked for IgnoreNullValue)

Can you elaborate? Perhaps add a dedicated test that was failing before this got fixed?

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

krwq · 2022-07-28T11:02:40Z

Can you elaborate? Perhaps add a dedicated test that was failing before this got fixed?

Actually my bad - seems like we're not ignoring calling custom setters for IgnoreNullValues so I'll change the logic so that we don't throw exception for required + IgnoreNullValues and rather than that we will just read the null - similarly to what we do for custom setters

...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs

...es/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs

src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs

...es/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs

src/libraries/System.Text.Json/src/Resources/Strings.resx

eiriktsarpalis

Other than a few pending issues, looks good to me. Thanks!

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs

…peof on net7.0

krwq added 2 commits July 26, 2022 16:33

Throw exception when 'required' keyword used but ctor does not have '…

a0f5c3e

…SetsRequiredMembers'

Support required keyword (and internal IsRequired)

ae9b84b

krwq requested review from eiriktsarpalis and stephentoub July 27, 2022 13:04

ghost added the area-System.Text.Json label Jul 27, 2022

krwq mentioned this pull request Jul 27, 2022

Implement a concept of "Required" properties #29861

Closed

remove local variable (forgot to press Save All)

fddae30

krwq commented Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs Show resolved Hide resolved

krwq commented Jul 27, 2022

View reviewed changes

...stem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs Outdated Show resolved Hide resolved

krwq commented Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs Outdated Show resolved Hide resolved

revert added namespaces in two files

2df48d2

eiriktsarpalis assigned krwq Jul 27, 2022

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/Resources/Strings.resx Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs Outdated Show resolved Hide resolved

runfoapp bot mentioned this pull request Jul 27, 2022

System.Net.* cancellation tests fail in CI #72818

Closed

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...tem/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...xt/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs Outdated Show resolved Hide resolved

eiriktsarpalis reviewed Jul 27, 2022

View reviewed changes

...es/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs Outdated Show resolved Hide resolved