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.

Sign up for GitHub

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

Jump to bottom

Add JSON null support for the built-in (ReadOnly)Memory converters. #95275

Merged
merged 2 commits into from
Nov 28, 2023
Merged

Add JSON null support for the built-in (ReadOnly)Memory converters. #95275

merged 2 commits into from
Nov 28, 2023

Conversation

eiriktsarpalis
Copy link
Member

Fix #95267.

@ghost ghost assigned eiriktsarpalis Nov 27, 2023
@ghost
Copy link

ghost commented Nov 27, 2023

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

Fix #95267.

Author: eiriktsarpalis
Assignees: -
Labels:

area-System.Text.Json
Milestone: -

@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Nov 27, 2023
eiriktsarpalis
eiriktsarpalis commented Nov 27, 2023
View reviewed changes
...tem.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs
Converter = converter;
ConverterStrategy = converter.ConverterStrategy;
Copy link
Member Author

Choose a reason for hiding this comment

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

This change adds some wiring that was being dropped so far.

Copy link
Member

Choose a reason for hiding this comment

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

I was about to ask :) Does that mean there's another bug that was lurking and this was fixing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, it was surfaced with the newly added tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

For context this is a converter wrapper type that is being used by the source generator APIs. The logic I just added was copied from a similar wrapper type being used internally:

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/CastingConverter.cs

Lines 27 to 36 in acf7c85

_sourceConverter = sourceConverter;
IsInternalConverter = sourceConverter.IsInternalConverter;
IsInternalConverterForNumberType = sourceConverter.IsInternalConverterForNumberType;
ConverterStrategy = sourceConverter.ConverterStrategy;
CanBePolymorphic = sourceConverter.CanBePolymorphic;
// Ensure HandleNull values reflect the exact configuration of the source converter
HandleNullOnRead = sourceConverter.HandleNullOnRead;
HandleNullOnWrite = sourceConverter.HandleNullOnWrite;
HandleNull = sourceConverter.HandleNullOnWrite;

@jozkee
Copy link
Member

jozkee commented Nov 27, 2023

Should this be backported to 8.0.x?

This was referenced Nov 27, 2023
Open
Closed
@eiriktsarpalis eiriktsarpalis merged commit fe79ce8 into dotnet:main Nov 28, 2023
107 of 109 checks passed
@eiriktsarpalis
Copy link
Member Author

/backport to release/8.0-staging

@github-actions GitHub Actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7019012556

@github-actions github-actions bot mentioned this pull request Nov 28, 2023
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@tarekgh tarekgh tarekgh approved these changes

@jozkee jozkee jozkee approved these changes

@krwq krwq Awaiting requested review from krwq

Assignees

@eiriktsarpalis eiriktsarpalis

Labels
area-System.Text.Json
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

JsonConverter for {ReadOnly}Memory doesn't deserialize nulls
4 participants
@eiriktsarpalis @jozkee @stephentoub @tarekgh