Skip to content
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

Make JSON support required properties #72937

Merged
merged 11 commits into from
Jul 29, 2022
Merged

Conversation

krwq
Copy link
Member

@krwq krwq commented Jul 27, 2022

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

@ghost
Copy link

ghost commented Jul 27, 2022

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: -

@eiriktsarpalis
Copy link
Member

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?

@krwq
Copy link
Member Author

krwq commented Jul 28, 2022

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

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@krwq krwq merged commit 9b63001 into dotnet:main Jul 29, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure JsonSerializer / source generator works well with required members
3 participants