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

[release/5.0-rc2] Fix default handling for value types when converter based on interface (#42319) #42406

Closed
wants to merge 1 commit into from

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Sep 17, 2020

Backport of #42319 to release/5.0-rc2.

Customer Impact


JsonIgnoreCondition.WhenWritingDefault is not honored when a property is a struct, the property is the default value, a custom converter is provided, and the converter's TypeToConvert is typeof(object) or an interface. The expected behavior is for the property to be ignored and to not call the converter.

This is not a common usage pattern (polymorphic converter implementations). The workaround is to specialize the converter to handle to specific type of the property, or to not use the converter if unnecessary.

Example highlighting the issue:

public class MyClass
{
    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
    [JsonConverter(typeof(MyPolymorphicConverter))] // polymorphic converter causes issue
    public MyStruct MyStruct { get; set; }
}

public interface IInterface { }

public struct MyStruct : IInterface { }

// workaround is for this to be specialized to JsonConverter<MyStruct> or converter should not be used if not needed.
public class MyPolymorphicConverter : JsonConverter<IInterface>
{
    // Converter implementation
}

string json = JsonSerializer.Serialize(new MyClass()); // custom converter will be called
Console.WriteLine(json);
// Expected {}
// Actual (assuming converter writes this) {"MyStruct":{}}

Testing

Extensive tests have been added. Fix has been verified in reporting project. We don't expect a theme around issues polymorphic converter usages to develop, but will this will be an area to focus on in an oncoming fuzz testing and validation effort.

Risk

The fix is targeted to the scenario it fixes. It is unlikely to break other areas. There is no perf regression to the happy path.

@layomia layomia added this to the 5.0.0 milestone Sep 17, 2020
@layomia layomia self-assigned this Sep 17, 2020
@layomia
Copy link
Contributor Author

layomia commented Sep 17, 2020

Marking as servicing-consider but should not be merged until #42359 is for proper layering. This commit as-is contains changes from that PR - this was done to fix a merge conflict with cherry-picking #42319 to release/5.0-rc2.

@layomia layomia added Servicing-consider Issue for next servicing release review NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Sep 17, 2020
@layomia
Copy link
Contributor Author

layomia commented Sep 17, 2020

Edge case scenario that has workaround

Closing for this reason given the stage of the release. We don't expect many apps to hit this and can provide guidance on how to workaround it in 5.0.

@layomia layomia closed this Sep 17, 2020
@layomia layomia removed the Servicing-consider Issue for next servicing release review label Sep 17, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@layomia layomia deleted the ignore_default_net5 branch May 18, 2021 07:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants