json: 🐛Fix stack overflow deserializing empty JSON

[shaeussler]

Fixes #1292

The call to "token.ToObject(serializer)" with an empty or invalid json calls JsonConvert.DeserializeObject again and results in infinite recursion and stack overflow.

Changes

• Throw JsonSerializationException on invalid JSON in UnitsNetIQuantityJsonConverter
  • Include helpful info in exception, link to wiki, truncated JSON token in message and full JSON token in Data["JsonToken"] property.
• Add tests for new behavior

[angularsen]

Looks good, a minor suggestion

[shaeussler]

Why do you call "token.ToObject(serializer);" inside ReadJson?
Here is another fix:

            if (valueUnit == null)
            {
                return existingValue; // token.ToObject<IQuantity>(serializer);
            }

[angularsen]

Why do you call "token.ToObject(serializer);" inside ReadJson? Here is another fix:

I think this was bug. UnitsNetIComparableJsonConverter uses UnitsNetBaseJsonConverter<T>.CreateLocalSerializer to create a new serializer internally, excluding itself.

I'm guessing a similar approach was intended here, but we should not need to deserialize a second time here and can instead just return existingValue as you proposed.

I pushed some changes in that direction, take a look

[angularsen]

It's hard to find any official documentation on how to handle errors in deserialization, but I changed it to explicitly throw an exception if it fails to deserialize the JSON.

It seems an exception is the convention when trying to deserialize "{}" to DateTime?, DateTime and other built-in supported types in json.net, rather than returning null or the existing value.

[shaeussler]

Throwing a JsonSerializationException in case of an empty json is ok in my eyes. I added the token to the error message.

[angularsen]

Great! JSON tokens can potentially be huge, maybe we should truncate it to a max length of say 100?

This example is 50 long:
{ "Value": 5.9999999, "Unit": "LengthUnit.Meter" }

We can also include the full token in the Data dictionary property instead of in the message, which allows debuggers to view it as well as log frameworks to log it if configured so.

We use this extension method in a separate project, which we could add as an internal class in UnitsNet:

    internal static class StringExtensions
    {
        /// <summary>
        /// Truncates a string to the given length, with truncation suffix.
        /// </summary>
        /// <param name="value">The string.</param>
        /// <param name="maxLength">The max length, including <paramref name="truncationSuffix"/>.</param>
        /// <param name="truncationSuffix">Suffix to append if truncated, defaults to "…".</param>
        /// <returns>The truncated string if longer, otherwise the original string.</returns>
        [return: NotNullIfNotNull("value")]
        public static string? Truncate(this string? value, int maxLength, string truncationSuffix = "…")
        {
            return value?.Length > maxLength
                ? value.Substring(0, maxLength - truncationSuffix.Length) + truncationSuffix
                : value;
        }
}

[angularsen]

Thanks a lot!

Nuget should be out shortly.

Release UnitsNet/5.28.0 · angularsen/UnitsNet

Release JsonNet/5.28.0 · angularsen/UnitsNet