JSON deserialization of quantities that use decimal internally may fail

[bitbonk]

Describe the bug
When serializing and deserializing quantities that use the decimal type for storage interally, JSON deserialization using the converters from UnitsNet.Serialization.JsonNet may fail with an OverflowException if the value is too big or too large.

To Reproduce
Steps to reproduce the behavior (just an example):

1. Add nuget UnitsNet 4.72.0, UnitsNet.Serialization.JsonNet 4.2.0, Xunit, and FluentAssertions to a .NET Core 3.1 class library project
2. Build and run the test code below.
3. See 6 failing tests.

    using System.Collections.Generic;
    using System.Linq;
    using FluentAssertions;
    using Newtonsoft.Json;
    using UnitsNet;
    using UnitsNet.Serialization.JsonNet;
    using Xunit;

    public class QuantitySerializationTests
    {
        public static IEnumerable<object[]> QuantityZeros { get; } =
            Quantity.Infos.Select(
                info => new object[]
                {
                    (IQuantity)info.Zero
                });
        
        public static IEnumerable<object[]> QuantityMinValues { get; } =
            Quantity.Infos.Select(
                info => new object[]
                {
                    (IQuantity)info.ValueType.GetProperty("MinValue").GetValue(null)
                });
        
        public static IEnumerable<object[]> QuantityMaxValues { get; } =
            Quantity.Infos.Select(
                info => new object[]
                {
                    (IQuantity)info.ValueType.GetProperty("MaxValue").GetValue(null)
                });

        [Theory]
        [MemberData(nameof(QuantityZeros))]
        public void Zero_IsMessagePackSerializable(IQuantity value)
        {
            var sutClone =
                JsonConvert.DeserializeObject(
                    JsonConvert.SerializeObject(value, Settings),
                    value.GetType(), Settings);

            sutClone.Should().NotBeSameAs(value);
            sutClone.Should().BeEquivalentTo(value, o => o.RespectingRuntimeTypes());
        }

        [Theory]
        [MemberData(nameof(QuantityMinValues))]
        public void MinValue_IsMessagePackSerializable(IQuantity value)
        {
            var sutClone =
                JsonConvert.DeserializeObject(
                    JsonConvert.SerializeObject(value, Settings),
                    value.GetType(), Settings);

            sutClone.Should().NotBeSameAs(value);
            sutClone.Should().BeEquivalentTo(value, o => o.RespectingRuntimeTypes());
        }

        [Theory]
        [MemberData(nameof(QuantityMaxValues))]
        public void MaxValue_IsMessagePackSerializable(IQuantity value)
        {
            var sutClone =
                JsonConvert.DeserializeObject(
                    JsonConvert.SerializeObject(value, Settings),
                    value.GetType(), Settings);

            sutClone.Should().NotBeSameAs(value);
            sutClone.Should().BeEquivalentTo(value, o => o.RespectingRuntimeTypes());
        }
        
        private static readonly JsonSerializerSettings Settings = new JsonSerializerSettings
        {
            TypeNameHandling = TypeNameHandling.Auto,
            Converters =
            {
                new UnitsNetIQuantityJsonConverter(),
                new UnitsNetIComparableJsonConverter()
            }
        };
    }

Example failed test result:

Tests.QuantitySerializationTests.MaxValue_IsMessagePackSerializable.MaxValue_IsMessagePackSerializable(value: 7,92e+28 W)

System.OverflowException: Value was either too large or too small for a Decimal.

System.OverflowException
Value was either too large or too small for a Decimal.
at System.Number.ThrowOverflowException(TypeCode type)
at System.Decimal.DecCalc.VarDecFromR8(Double input, DecCalc& result)
at System.Decimal.op_Explicit(Double value)
at UnitsNet.QuantityValue.op_Explicit(QuantityValue number)
at UnitsNet.Power.From(QuantityValue value, PowerUnit fromUnit)
at UnitsNet.Quantity.TryFrom(QuantityValue value, Enum unit, IQuantity& quantity)
at UnitsNet.Quantity.From(QuantityValue value, Enum unit)
at UnitsNet.Serialization.JsonNet.UnitsNetBaseJsonConverter1.ConvertValueUnit(ValueUnit valueUnit) at UnitsNet.Serialization.JsonNet.UnitsNetIQuantityJsonConverter.ReadJson(JsonReader reader, Type objectType, IQuantity existingValue, Boolean hasExistingValue, JsonSerializer > serializer) at Newtonsoft.Json.JsonConverter1.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer)
at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)
at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
at Tests.QuantitySerializationTests

Expected behavior
No tests fail.

[angularsen]

Thank you for a good repro on this issue. I believe this can be solved by making the serializer smarter in handling double vs decimal. If I recall correctly it forces serialization via double.

Would you be interested in attempting a pull request on this? I'll be happy to assist.
My time is very limited these days, so things take a long time for me to get around to it.

[rohahn]

I created PR #868 to fix the issue.

Since decimal values are not serialized/deserialized bit perfect by Json.NET as numbers, I had to resort to serializing them as a string.

But I still don't understand what is so special about the Power quantity that it needs a decimal base type. When I try to convert 1 Petawatt to Femtowatt, it results in an overflow. The quantity Luminosity supports the same prefixes for example but uses the basetype double.

[angularsen]

Great, yes JSON number is probably restricted to represent the precision provided by decimal.

The reason Power has decimal, but not Luminosity is not intentional. Introducing decimal was a design mistake to begin with, because it complicates a lot of things having different number types for different quantities. We haven't found a good solution for this yet.

The reason decimal was added was that round-trip conversion unit tests failed when adding very large/small units to certain quantities, and I believe Power was one of the first to encounter this. Why Luminosity didn't receive the same treatment I'm not sure, maybe the round-trip conversion stopped being a problem at some point - I haven't investigated.

At any rate, I'd love to get rid of decimal altogether, but I'm not pursuing this myself. For now we will have to deal with it until someone ™️ gets rid of it while still keeping everything functional.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[angularsen]

Fixed by #868