Rework of UnitsNetJsonConverter

[dschuermans]

I've bumped into a few issue while trying to use the provided UnitsNetJsonConverter:

1. Initially, there was no support for arrays of units (Fixed in New unit: TorquePerLength and fix for array serialization & deserialization #712)
2. The UnitsNetJsonConverter does not take into account any settings defined by the user (e.g. custom contract resolver being used, additional converters, date time handling, null value handling etc... Pretty much any of the JsonSerializerSettings that can be provided the JsonConvert.Serialize() or JsonConvert.DeserializeObject() methods. (see Unit deserialization does not work when unit or value are in lowercase #568)
3. The UnitsNetJsonConverter does not respect the configured naming conventions (see also Unit deserialization does not work when unit or value are in lowercase #568)

I've attempted to create a more simplified and straight forward converter, since the current one also breaks the single responsiblity principle because it handles both ValueUnit types and IComparable.

In my opinion, providing support for deserializing IComparable is tricky, due the generic nature of IComparable. There are so many types that also implement IComparable that I feel it's a dangerous path to walk and it could potentionally interfere with any existing converter(s) the user might have for IComparable types.

So what I propose is the following:

• 2 JsonConverter implementations instead of one.
• Each converter inherits from JsonConverter<T> instead of JsonConverter
  • 1 converter that inherits from JsonConverter<IQuantity> - This will deal with serializing & deserializing any UnitsNet unit
  • 1 converter that inherits from JsonConverter<IComparable> - This will deal with deserializing an IComparable into a UnitsNet unit.

By inheriting from JsonConverter<T> the code for type checking and all of the custom Array functionality becomes obsolete, which in turn simplifies the implementation.
If the user is indeed using IComparable as types in their code base, they can now opt-in to use the UnitsNetIComparableJsonConverter.
But i'm sure that in 99% of the cases it will be sufficient to only register the UnitsNetIQuantityJsonConverter

Mind you, the order in which the converters are registered is important: First the converter for IQuantity and then optionally the IComparable converter

Here's my working implementation, tested against the existing UT's for the current V4 converter:

public abstract class UnitsNetBaseJsonConverter<T> : JsonConverter<T>
    {
        /// <summary>
        /// Reads the "Unit" and "Value" properties from a JSON string
        /// </summary>
        /// <param name="jsonToken">The JSON data to read from</param>
        /// <returns>A <see cref="ValueUnit"/></returns>
        protected ValueUnit ReadValueUnit(JToken jsonToken)
        {
            if (!jsonToken.HasValues)
            {
                return null;
            }

            JObject jsonObject = (JObject) jsonToken;

            JToken unit = jsonObject.GetValue(nameof(ValueUnit.Unit), StringComparison.OrdinalIgnoreCase);
            JToken value = jsonObject.GetValue(nameof(ValueUnit.Value), StringComparison.OrdinalIgnoreCase);

            if (unit == null || value == null)
            {
                return null;
            }

            return new ValueUnit()
            {
                Unit = unit.Value<string>(),
                Value = value.Value<double>()
            };
        }

        /// <summary>
        /// Convert a <see cref="ValueUnit"/> to an <see cref="IQuantity"/>
        /// </summary>
        /// <param name="valueUnit">The value unit to convert</param>
        /// <exception cref="UnitsNetException">Thrown when an invalid Unit has been provided</exception>
        /// <returns>An IQuantity</returns>
        protected IQuantity ConvertValueUnit(ValueUnit valueUnit)
        {
            if (valueUnit == null || string.IsNullOrWhiteSpace(valueUnit.Unit))
            {
                return null;
            }

            // "MassUnit.Kilogram" => "MassUnit" and "Kilogram"
            string unitEnumTypeName = valueUnit.Unit.Split('.')[0];
            string unitEnumValue = valueUnit.Unit.Split('.')[1];

            // "UnitsNet.Units.MassUnit,UnitsNet"
            string unitEnumTypeAssemblyQualifiedName = "UnitsNet.Units." + unitEnumTypeName + ",UnitsNet";

            // -- see http://stackoverflow.com/a/6465096/1256096 for details
            Type unitEnumType = Type.GetType(unitEnumTypeAssemblyQualifiedName);
            if (unitEnumType == null)
            {
                var ex = new UnitsNetException("Unable to find enum type.");
                ex.Data["type"] = unitEnumTypeAssemblyQualifiedName;
                throw ex;
            }

            double value = valueUnit.Value;
            Enum unitValue = (Enum)Enum.Parse(unitEnumType, unitEnumValue); // Ex: MassUnit.Kilogram

            return Quantity.From(value, unitValue);
        }

        /// <summary>
        /// Convert an <see cref="IQuantity"/> to a <see cref="ValueUnit"/>
        /// </summary>
        /// <param name="quantity">The quantity to convert</param>
        /// <returns></returns>
        protected ValueUnit ConvertIQuantity(IQuantity quantity)
        {
            return new ValueUnit
            {
                // See ValueUnit about precision loss for quantities using decimal type.
                Value = quantity.Value,
                Unit = $"{quantity.QuantityInfo.UnitType.Name}.{quantity.Unit}"
            };
        }

        /// <summary>
        /// Create a copy of a serializer, retaining any settings but leaving out a converter to prevent loops
        /// </summary>
        /// <param name="serializer">The serializer to copy</param>
        /// <param name="currentConverter">The converter to leave out</param>
        /// <returns>A serializer with the same settings and all converters except the current one.</returns>
        protected JsonSerializer CreateLocalSerializer(JsonSerializer serializer, JsonConverter currentConverter)
        {
            JsonSerializer localSerializer = new JsonSerializer()
            {
                Culture = serializer.Culture,
                CheckAdditionalContent = serializer.CheckAdditionalContent,
                Context = serializer.Context,
                ContractResolver = serializer.ContractResolver,
                TypeNameHandling = serializer.TypeNameHandling,
                TypeNameAssemblyFormatHandling = serializer.TypeNameAssemblyFormatHandling,
                Formatting = serializer.Formatting,
                ConstructorHandling =  serializer.ConstructorHandling,
                DateFormatHandling = serializer.DateFormatHandling,
                DateFormatString = serializer.DateFormatString,
                DateParseHandling = serializer.DateParseHandling,
                DateTimeZoneHandling = serializer.DateTimeZoneHandling,
                DefaultValueHandling = serializer.DefaultValueHandling,
                EqualityComparer = serializer.EqualityComparer,
                FloatFormatHandling  = serializer.FloatFormatHandling,
                FloatParseHandling = serializer.FloatParseHandling,
                MaxDepth = serializer.MaxDepth,
                MetadataPropertyHandling = serializer.MetadataPropertyHandling,
                MissingMemberHandling = serializer.MissingMemberHandling,
                NullValueHandling = serializer.NullValueHandling,
                ObjectCreationHandling = serializer.ObjectCreationHandling,
                PreserveReferencesHandling = serializer.PreserveReferencesHandling,
                ReferenceLoopHandling = serializer.ReferenceLoopHandling,
                ReferenceResolver = serializer.ReferenceResolver,
                SerializationBinder = serializer.SerializationBinder,
                StringEscapeHandling = serializer.StringEscapeHandling,
                TraceWriter = serializer.TraceWriter
            };

            foreach (JsonConverter converter in serializer.Converters.Where(x => x != currentConverter))
            {
                localSerializer.Converters.Add(converter);
            }

            return localSerializer;
        }

        /// <summary>
        ///     A structure used to serialize/deserialize Units.NET unit instances.
        /// </summary>
        /// <remarks>
        ///     Quantities may use decimal, long or double as base value type and this might result
        ///     in a loss of precision when serializing/deserializing to decimal.
        ///     Decimal is the highest precision type available in .NET, but has a smaller
        ///     range than double.
        ///
        ///     Json: Support decimal precision #503
        ///     https://github.com/angularsen/UnitsNet/issues/503
        /// </remarks>
        protected sealed class ValueUnit
        {
            public string Unit { get; [UsedImplicitly] set; }
            public double Value { get; [UsedImplicitly] set; }
        }
    }

    public sealed class UnitsNetIComparableJsonConverter : UnitsNetBaseJsonConverter<IComparable>
    {
        public override bool CanWrite => false;

        public override void WriteJson(JsonWriter writer, IComparable value, JsonSerializer serializer)
        {
            throw new NotImplementedException("Serialization of IComparable is handled by default serialization");
        }

        /// <summary>
        /// Attempts to deserialize a JSON string as UnitsNet type, assigned to property of type IComparable
        /// </summary>
        /// <param name="reader"></param>
        /// <param name="objectType"></param>
        /// <param name="existingValue"></param>
        /// <param name="hasExistingValue"></param>
        /// <param name="serializer"></param>
        /// <returns></returns>
        public override IComparable ReadJson(JsonReader reader, Type objectType, IComparable existingValue, bool hasExistingValue,
            JsonSerializer serializer)
        {
            Guard.IsNotNull(reader, nameof(reader));
            Guard.IsNotNull(serializer, nameof(serializer));

            if (reader.TokenType == JsonToken.Null)
            {
                return null;
            }

            JsonSerializer localSerializer = CreateLocalSerializer(serializer, this);

            JToken token = JToken.Load(reader);

            // If objectType is not IComparable but a type that implements IComparable, deserialize directly as this type instead.
            if (objectType != typeof(IComparable))
            {
                return token.ToObject(objectType, localSerializer) as IComparable;
            }

            ValueUnit valueUnit = ReadValueUnit(token);

            if (valueUnit == null)
            {
                return token.ToObject<IComparable>(localSerializer);
            }

            return ConvertValueUnit(valueUnit) as IComparable;
        }
    }

    public sealed class UnitsNetIQuantityJsonConverter : UnitsNetBaseJsonConverter<IQuantity>
    {
        /// <summary>
        ///     Writes the JSON representation of the object.
        /// </summary>
        /// <param name="writer">The <see cref="T:Newtonsoft.Json.JsonWriter" /> to write to.</param>
        /// <param name="value">The value to write.</param>
        /// <param name="serializer">The calling serializer.</param>
        public override void WriteJson(JsonWriter writer, IQuantity value, JsonSerializer serializer)
        {
            Guard.IsNotNull(writer, nameof(writer));
            Guard.IsNotNull(serializer, nameof(serializer));

            if (value == null)
            {
                writer.WriteNull();
            }

            ValueUnit valueUnit = ConvertIQuantity(value);

            serializer.Serialize(writer, valueUnit);
        }

        /// <summary>
        ///     Reads the JSON representation of the object.
        /// </summary>
        /// <param name="reader">The <see cref="T:Newtonsoft.Json.JsonReader" /> to read from.</param>
        /// <param name="objectType">Type of the object.</param>
        /// <param name="existingValue">The existing value of object being read.</param>
        /// <param name="hasExistingValue">Indicates if an existing value has been provided</param>
        /// <param name="serializer">The calling serializer.</param>
        /// <returns>
        ///     The object value.
        /// </returns>
        /// <exception cref="UnitsNetException">Unable to parse value and unit from JSON.</exception>
        public override IQuantity ReadJson(JsonReader reader, Type objectType, IQuantity existingValue, bool hasExistingValue, JsonSerializer serializer)
        {
            Guard.IsNotNull(reader, nameof(reader));
            Guard.IsNotNull(serializer, nameof(serializer));

            if (reader.TokenType == JsonToken.Null)
            {
                return null;
            }

            JToken token = JToken.Load(reader);

            ValueUnit valueUnit = ReadValueUnit(token);

            return ConvertValueUnit(valueUnit);
        }

    }

When I run the tests with both converters registered:

When I run the tests with only the UnitsNetIQuantityJsonConverter registered:

Those 2 tests fail because it uses the UnitsNetIQuantityConverter to serialize the properties that are actually UnitsNet types but it doesn't include the $type magic property, so when deserializing it doesn't know to which type it should deserialize.

Thoughts?

[angularsen]

Hi, I agree with your reasoning and your proposed change here looks good. Excellent work describing it.

I can't think of a usecase for supporting IComparable myself, but I found it was added in #200 so the usecase there is described as

I had a class that I needed to serialize that had a unit as a IComparable.

I do, however, agree that it would be much better and safer to serialize a type that has more specific interfaces, but if we can maintain backwards compatibility without too much work then fine, we can do that.

In order to avoid a breaking change, I propose we mark UnitsNetJsonConverter as [Obsolete("some explanation on what to use instead")] and add the two new converters as you have described above. Alternatively, we decide to not support this moving forward and simply drop support for it in the next major version bump.

@eriove You authored #200, do you have any input on this proposed change?

[eriove]

Thanks for the good description. I no longer work at the company where I used that code, but as I remember it the idea seemed good at the time. But later on we stopped serializing UnitsNet types and started using pure POCOs for serialization.

I think we had a property with an IComparable which could be different quantities but was always just compared, so this "trick" made it possible to stay with just one property instead of three (and switch cases).

Anyway, it was an edge case. There might be others that rely on it it though so the proposed change seems very good.

[dschuermans]

Okay great, seems everyone is on board with this proposed change?

I'll turn it into a PR soon, unless someone else beats me to it

[angularsen]

Go go :)