Runtime: Custom struct not ignored during serialization when default

[Turnerj]

Description

In a test project of mine in my ongoing effort to convert from Newtonsoft.Json to System.Text.Json, I found an issue relating to the handling of default values during serialization.

The test project has a class like the following:

public class CustomType : Thing, IThing
{
    [JsonPropertyName("@type")]
    public override string Type => "CustomType";

    [JsonPropertyName("number")]
    [JsonConverter(typeof(ValuesJsonConverter))]
    public OneOrMany<int?> Number { get; set; }

    [JsonPropertyName("uri")]
    [JsonConverter(typeof(ValuesJsonConverter))]
    public Values<string, Uri> Uri { get; set; }
}

The types OneOrMany<T> and Values<T1,T2> are both struct and I believe this is where the problem lies (as changing them to class avoids this issue altogether).

As Object

new CustomType
{
    Number = 123
}

The JSON output by S.T.J

{"@type":"CustomType","number":123,"uri":null,"@context":"https://schema.org"}

The desired JSON output

{"@type":"CustomType","number":123,"@context":"https://schema.org"}

Serializer Options

new JsonSerializerOptions
{
    DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault,
    Encoder = System.Text.Encodings.Web.JavaScriptEncoder.UnsafeRelaxedJsonEscaping,
}

Now, I know where the null value comes from - it comes from my ValuesJsonConverter - however given that the property Uri is a default value (default(Values<string,Uri>)), my understanding is that it never should have got to my converter.

At the point my converter knows it is a default value, the property name has already been written out to the JSON so I can't do anything about it (writing nothing causes invalid JSON).

Configuration

• .NET Core 3.1
• System.Text.Json 5.0.0-rc.1.20451.14
• Windows 10 Home (10.0.18362 Build 18362)

Regression?

Unable to determine if this is a regression due to dependency on other fixes in RC1.

Other information

You can see a working example of the issue here: https://github.com/Turnerj/SchemaNet_SystemTextJson/blob/master/SchemaNet_SystemTextJson/Program.cs

One known workaround for this is change the OneOrMany<T> and Values<T1,T2> to be a class instead of a struct. That isn't ruled out at this point though I think this is still a legitimate bug regardless which direction I go in my project.

[devsko]

Just a guess...
https://github.com/dotnet/runtime/blob/2182936365696952aca1ff9796c204db45604f6a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs#L134-L141
T is your interface IValues and default(T) is null but your value isn't

[Turnerj]

Great find @devsko ! That is really interesting - it really comes down to the combination of interface + struct that is causing grief.

[am11]

Interesting that even with DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull, we get "uri":nul, with custom converter. I think this goes against what has been documented:

|||
|---|---|
| WhenWritingNull | If the value is null, the property is ignored during serialization. This is applied only to reference-type properties and fields. |

Edit: ah This is applied only to reference-type properties and fields. in this case we have value type. nvm 😄

[Turnerj]

@am11 it is kinda a Catch-22, it is treated both as a reference type in one case and a value type in another but in either case, it isn't doing what I am expecting.

[steveharter]

@Turnerj thanks for reporting this issue. I have a PR that should address the issue.

If possible, can you test the change out locally?

Also, for perf, it will be faster to use a class, not a value type, for default handling on types that contain many properties. This is because a simple null check can be performed on reference types.

[Turnerj]

I'll give it a shot - is there a build artifact from your PR that I use or do I build and run your branch locally?

[steveharter]

For now, until it is in master and there's a build, you'll need to make a build with my branch or the changes manually applied.

We may consider this for 5.0. Currently 5.0 is done taking changes for its last RC so the bar is high to get it in and requires validation that it has been tested, which is why I'm asking.

[Turnerj]

Yep, no problem. Will have a go today and get back to you ASAP.

[Turnerj]

Took about an hour and a half of working how what/how to build the PR locally, working around various issues (needing to install Python and C++, building my test application via command line for reasons I'll explain), I believe I've managed to test it out correctly.

I built your "IgnoreNull" branch (via build.cmd clr+libs -rc Release) though couldn't work out how to reference the whole build so I only individually referenced the "System.Text.Json.dll" build artifact and used the 5.0 RC1 SDK (but because I don't run preview versions of VS, I needed to build my test application via dotnet build). Updated the TargetFramework in my test application for net5.0 then built and ran it - the JSON output now matches what I'm expecting!

The reason I mentioned any of that process was to 1. make sure it was a valid test so the results are helpful to you and 2. hopefully find out an easier way to do this type of thing in the future.

Thanks for the quick work on this @steveharter !

[am11]

@Turnerj, for adhoc testing (and comparing results etc.) of local dotnet/runtime library build, I use the good old <Reference path-to-dll> item in my csproj (which still takes precedence over shared framework in installation directory). e.g.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworks>net5.0;netcoreapp3.1</TargetFrameworks>
  </PropertyGroup>

  <ItemGroup Condition="'$(TargetFramework)' == 'net5.0'">
    <Reference Include="/Users/am11/projects/runtime_pr/artifacts/bin/System.Configuration.ConfigurationManager/netstandard2.0-Release/System.Configuration.ConfigurationManager.dll" />
    <Reference Include="/Users/am11/projects/runtime_pr/artifacts/bin/System.IO.IsolatedStorage/net5.0-Unix-Release/System.IO.IsolatedStorage.dll"/>
  </ItemGroup>
</Project>

then ~/.dotnet5/dotnet run -f net5.0 (or dotnet run -f netcoreapp3.1). I have RC2 installed at ~/.dotnet5 and 3.1 is in PATH.