Skip to content

Fix default handling for value types when converter based on interface - #42319

Merged
steveharter merged 5 commits into
dotnet:masterfrom
steveharter:IgnoreNull
Sep 17, 2020
Merged

Fix default handling for value types when converter based on interface#42319
steveharter merged 5 commits into
dotnet:masterfrom
steveharter:IgnoreNull

Conversation

@steveharter

@stevehartersteveharter commented Sep 16, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes#42237

When a custom converter is based on an interface and a corresponding property is based on a value type that implements the interface, default handling was not working. This PR fixes that.

Some local benchmarks showed no regression for the happy path that doesn't hit this edge case.

@stevehartersteveharter self-assigned this Sep 16, 2020
steveharter
steveharter commented Sep 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
}

[Fact]
public static void JsonIgnoreCondition_WhenWritingDefault_OnBoxedPrimitive()

@stevehartersteveharterSep 16, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This was passing before, just added the test.

steveharter
steveharter commented Sep 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
}

[Fact]
public static void JsonIgnoreCondition_WhenWritingDefault_OnRootTypes()

@stevehartersteveharterSep 16, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This was passing before, just added the test.

layomia
layomia reviewed Sep 16, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs Outdated
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs Outdated
Comment threadsrc/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs Outdated
layomia
layomia approved these changes Sep 17, 2020
View reviewed changes

@layomialayomia left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Would like to present to ship-room for .NET 5 consideration (incomplete scenario for ignoring default values feature new in 5.0).

@layomialayomia added this to the 5.0.0 milestone Sep 17, 2020
@steveharter

steveharter commented Sep 17, 2020

Copy link
Copy Markdown
ContributorAuthor

Runtime failure in mono unrelated:

##[error]Extraction failed for file: F:\workspace\_work\1\s\__download__\libraries_test_assets_Windows_NT_x64_Debug\libraries_test_assets_Windows_NT_x64_Debug.zip code: 2

Publishing build artifacts failed with an error: Not found PathtoPublish: F:\workspace\_work\1\s\artifacts\log\Debug

@steveharter
steveharter merged commit 710118d into dotnet:masterSep 17, 2020
@steveharter
steveharter deleted the IgnoreNull branch September 17, 2020 14:58
layomia pushed a commit to layomia/dotnet_runtime that referenced this pull request Sep 17, 2020
@steveharter@layomia
Fix default handling for value types when converter based on interface (
1374897
dotnet#42319)
@stevehartersteveharter mentioned this pull request Sep 17, 2020
Merged
@layomialayomia mentioned this pull request Sep 17, 2020
Closed
@TurnerjTurnerj mentioned this pull request Sep 18, 2020
Merged
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jozkeejozkeeAwaiting requested review from jozkee
1 more reviewer
@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@stevehartersteveharter

Labels

area-System.Text.Json

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

Custom struct not ignored during serialization when default

3 participants

@steveharter@layomia@Dotnet-GitSync-Bot