Samples for Newtonsoft migration doc

[tdykstra]

No description provided.

[Thraka]

I didn't download and run the code. I didn't see any glaring errors.

[tdykstra]

@Thraka Thanks for the review!

[ahsonkhan]

Should we state why? cc @steveharter

[layomia]

Do we have guidance against this? We do it internally: https://github.com/dotnet/runtime/blob/f77914d4c9045a01b3a91b63c28805f24850c274/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonValueConverterKeyValuePair.cs#L127

[ahsonkhan]

Well, it would cause a stackoverflow in this case. I don't know if this is a bug or expected.

https://github.com/dotnet/runtime/blob/f77914d4c9045a01b3a91b63c28805f24850c274/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoNotNullable.cs#L89

System.Text.Json.Tests.dll!System.Text.Json.Serialization.Tests.ReadValueTests.ImmutablePointConverter.Write(System.Text.Json.Utf8JsonWriter writer, System.Text.Json.Serialization.Tests.ReadValueTests.ImmutablePoint value, System.Text.Json.JsonSerializerOptions options) Line 1611 C#
System.Text.Json.dll!System.Text.Json.JsonPropertyInfoNotNullable<object, System.Text.Json.Serialization.Tests.ReadValueTests.ImmutablePoint, System.Text.Json.Serialization.Tests.ReadValueTests.ImmutablePoint>.OnWrite(ref System.Text.Json.WriteStackFrame current, System.Text.Json.Utf8JsonWriter writer) Line 91 C#
System.Text.Json.dll!System.Text.Json.JsonPropertyInfo.Write(ref System.Text.Json.WriteStack state, System.Text.Json.Utf8JsonWriter writer) Line 571 C#
System.Text.Json.dll!System.Text.Json.JsonSerializer.Write(System.Text.Json.Utf8JsonWriter writer, int originalWriterDepth, int flushThreshold, System.Text.Json.JsonSerializerOptions options, ref System.Text.Json.WriteStack state) Line 37 C#
System.Text.Json.dll!System.Text.Json.JsonSerializer.WriteCore(System.Text.Json.Utf8JsonWriter writer, object value, System.Type type, System.Text.Json.JsonSerializerOptions options) Line 139 C#
System.Text.Json.dll!System.Text.Json.JsonSerializer.WriteValueCore(System.Text.Json.Utf8JsonWriter writer, object value, System.Type type, System.Text.Json.JsonSerializerOptions options) Line 107 C#
System.Text.Json.dll!System.Text.Json.JsonSerializer.Serialize<System.Text.Json.Serialization.Tests.ReadValueTests.ImmutablePoint>(System.Text.Json.Utf8JsonWriter writer, System.Text.Json.Serialization.Tests.ReadValueTests.ImmutablePoint value, System.Text.Json.JsonSerializerOptions options) Line 22 C#

In the key value pair case, looks like we only call it for object and end up calling GetType on it the next time, so it doesn't usually stackoverflow.

But, this sample breaks that:

var kvp = new KeyValuePair<object, int>("foo", 0);
KeyValuePair<object, int> parent = default;

for (int i = 0; i < 1_000; i++)
{
    parent = new KeyValuePair<object, int>(kvp, i);
    kvp = parent;
}

Console.WriteLine(JsonSerializer.Serialize(parent));

[tdykstra]

[EDIT] The bug here is caused by my mistake in adapting the KeyValueConverter code. I have corrected it now to call the int converter for each property instead of calling into Serialize(). So the advice to not pass in options doesn't apply in this case.

public override void Write(
    Utf8JsonWriter writer,
    ImmutablePoint value,
    JsonSerializerOptions options)
{
    writer.WriteStartObject();
    WriteProperty(writer, value.X, XName, options);
    WriteProperty(writer, value.Y, YName, options);
    writer.WriteEndObject();
}

[tdykstra]

The corrected code is here: https://github.com/dotnet/samples/blob/5dee66d52080f302775790bea4a25fe5afa938ac/snippets/core/system-text-json/csharp/ImmutablePointConverter.cs

[ahsonkhan]

I don't know if this is the right place for this, but this converter can be written more efficiently. So, maybe we suggest that?

1. reader.GetString() for property name matching isn't necessary. You could use ValueTextEquals
2. Comparison with JsonEncodedText is faster than comparison with string.
3. Rather than asking for an int converter every time, it's probably better to get it once and either pass it in, or store it as a field by getting it in the custom ctor. cc @steveharter, @layomia
4. You can remove one of the checks at the end (if (!xSet || !ySet)) by crafting the conditions differently.

public class ImmutablePointNewConverter : JsonConverter<ImmutablePoint>
{
    private readonly JsonEncodedText XName = JsonEncodedText.Encode("X");
    private readonly JsonEncodedText YName = JsonEncodedText.Encode("Y");

    private readonly JsonConverter<int> _intConverter;

    public ImmutablePointNewConverter(JsonSerializerOptions options)
    {
        if (options?.GetConverter(typeof(int)) is JsonConverter<int> intConverter)
        {
            _intConverter = intConverter;
        }
        else
        {
            throw new InvalidOperationException();
        }
    }

    public override ImmutablePoint Read(
        ref Utf8JsonReader reader,
        Type typeToConvert,
        JsonSerializerOptions options)
    {
        if (reader.TokenType != JsonTokenType.StartObject)
        {
            throw new JsonException();
        };

        int x = default;
        bool xSet = false;

        int y = default;
        bool ySet = false;

        // Get the first property.
        reader.Read();
        if (reader.TokenType != JsonTokenType.PropertyName)
        {
            throw new JsonException();
        }

        if (reader.ValueTextEquals(XName.EncodedUtf8Bytes))
        {
            x = ReadProperty(ref reader, options);
            xSet = true;
        }
        else if (reader.ValueTextEquals(YName.EncodedUtf8Bytes))
        {
            y = ReadProperty(ref reader, options);
            ySet = true;
        }
        else
        {
            throw new JsonException();
        }

        // Get the second property.
        reader.Read();
        if (reader.TokenType != JsonTokenType.PropertyName)
        {
            throw new JsonException();
        }

        if (xSet && reader.ValueTextEquals(YName.EncodedUtf8Bytes))
        {
            y = ReadProperty(ref reader, options);
        }
        else if (ySet && reader.ValueTextEquals(XName.EncodedUtf8Bytes))
        {
            x = ReadProperty(ref reader, options);
        }
        else
        {
            throw new JsonException();
        }

        reader.Read();

        if (reader.TokenType != JsonTokenType.EndObject)
        {
            throw new JsonException();
        }

        return new ImmutablePoint(x, y);
    }

    private int ReadProperty(ref Utf8JsonReader reader, JsonSerializerOptions options)
    {
        Debug.Assert(reader.TokenType == JsonTokenType.PropertyName);

        reader.Read();
        return _intConverter.Read(ref reader, typeof(int), options);
    }

    public override void Write(
        Utf8JsonWriter writer,
        ImmutablePoint value,
        JsonSerializerOptions options)
    {
        // Don't pass in options when recursively calling Serialize.
        JsonSerializer.Serialize(writer, value);
    }
}

[tdykstra]

@ahsonkhan @steveharter @layomia Updates in response to feedback are in #1871

[ahsonkhan]

I think this could be problematic. If the Date did exist in the JSON payload, but just happen to be the default value, then we still throw an exception.

For example, if jsonString was the following (or you serialize a POCO containing DateTime date = default).

{
  "Date": "0001-01-01T00:00:00+00:00",
  "TemperatureCelsius": 25,
  "Summary": "Hot"
}

Not sure if there is a great way to do this without using nullable, at least for value types like DateTime, int, etc.

[tdykstra]

Do you think it better to add some text warning about the case where the date has the default value in the JSON, or remove the converter as a workaround from the doc?

[ahsonkhan]

I can't think of a great/general purpose workaround for Required properties atm. For now, text warning about the case is probably fine. I will have to think about the scenario some more. One option could be to create a sentinel default value and check against that (like for an int property that is known to be positive, choose -1), but again, you wouldn't know if the payload actually contained that value or it just didn't exist.

Even for reference types (or nullable), how do you differentiate between the property not existing vs one that exists but is explicitly set to null.

[ahsonkhan]

@steveharter, @layomia - do you have any ideas or suggestions?

[ahsonkhan]

Re-iterating dotnet/docs#16225 (comment) since it makes more sense in the samples PR:

Looks like using value as parameter names isn't great in the docs. They show up colored blue which makes C# harder to read. Maybe use result or forecast (or maybe this issue can be fixed altogether and we can keep value)?