Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UnitsNetBaseJsonConverter fails for custom quantities that are declared nullable #1465

Open
lipchev opened this issue Dec 18, 2024 · 0 comments
Labels

Comments

@lipchev
Copy link
Collaborator

lipchev commented Dec 18, 2024

Describe the bug
This looks like a duplicate of this issue but I was lead there by my attempt to remove the IConvertible interface...
In essence, none of the UnitsNetBaseJsonConverter we have are ever getting called for a property such as public HowMuch? NullableQuantity { get; set; } ... because it doesn't implement the IConvertible interface 😆 ..

To Reproduce
I took inspiration from this test, and tried to replicate it in the HowMuchTests

Here's the new HowMuchTets.cs:

    public class HowMuchTests
    {
        [Fact]
        public static void SerializeAndDeserializeCreatesSameObjectForIQuantity()
        {
            var jsonSerializerSettings = new JsonSerializerSettings { Formatting = Formatting.Indented };
            var quantityConverter = new UnitsNetIQuantityJsonConverter();
            quantityConverter.RegisterCustomType(typeof(HowMuch), typeof(HowMuchUnit));
            jsonSerializerSettings.Converters.Add(quantityConverter);

            var quantity = new HowMuch(12.34, HowMuchUnit.ATon);

            var serializedQuantity = JsonConvert.SerializeObject(quantity, jsonSerializerSettings);

            var deserializedQuantity = JsonConvert.DeserializeObject<HowMuch>(serializedQuantity, jsonSerializerSettings);
            Assert.Equal(quantity, deserializedQuantity);
        }
        
        [Fact]
        public static void SerializeObjectWithNullQuantity()
        {
            var jsonSerializerSettings = new JsonSerializerSettings { Formatting = Formatting.Indented };
            var quantityConverter = new UnitsNetIQuantityJsonConverter();
            quantityConverter.RegisterCustomType(typeof(HowMuch), typeof(HowMuchUnit));
            jsonSerializerSettings.Converters.Add(quantityConverter);

            var quantity = new HowMuch(12.34, HowMuchUnit.ATon);
            var objectToSerialize = new TestObject { NonNullableQuantity = quantity};

            var serializedQuantity = JsonConvert.SerializeObject(objectToSerialize, jsonSerializerSettings);
            TestObject deserializedQuantity = JsonConvert.DeserializeObject<TestObject>(serializedQuantity, jsonSerializerSettings);
            
            Assert.Equal(quantity, deserializedQuantity.NonNullableQuantity);
            Assert.Null(deserializedQuantity.NullableQuantity);
        }
        
        [Fact]
        public static void SerializeObjectWithNullableQuantity()
        {
            var jsonSerializerSettings = new JsonSerializerSettings { Formatting = Formatting.Indented};
            var quantityConverter = new UnitsNetIQuantityJsonConverter();
            quantityConverter.RegisterCustomType(typeof(HowMuch), typeof(HowMuchUnit));
            jsonSerializerSettings.Converters.Add(quantityConverter);

            var quantity = new HowMuch(12.34, HowMuchUnit.ATon);
            var objectToSerialize = new TestObject { NonNullableQuantity = quantity, NullableQuantity = quantity };

            var serializedQuantity = JsonConvert.SerializeObject(objectToSerialize, jsonSerializerSettings);
            TestObject deserializedQuantity = JsonConvert.DeserializeObject<TestObject>(serializedQuantity, jsonSerializerSettings); // exception here
            
            Assert.Equal(quantity, deserializedQuantity.NonNullableQuantity);
            Assert.Equal(quantity, deserializedQuantity.NullableQuantity);
        }
    }
    
    public sealed class TestObject
    {
        public HowMuch? NullableQuantity { get; set; }
        public HowMuch NonNullableQuantity { get; set; }
    }

SerializeObjectWithNullableQuantity fails. with

Newtonsoft.Json.JsonSerializationException
Unable to find a constructor to use for type UnitsNet.UnitInfo. A class should either have a default constructor, one constructor with arguments or a constructor marked with the JsonConstructor attribute. Path 'NullableQuantity.QuantityInfo.UnitInfos[0].Value', line 18, position 18.

This is because the nullable property is not being matched against the converter, so the serializedQuantity string contains all public properties and their descendants (not crashing yet), and on the way back- it once again ignores the converter, and attempts to reconstruct everything, and fails (because of the constructors).

And what about the IConvertible - the exact same test passes when we use the internal quantities:

    public sealed class TestObject
    {
        public Frequency? NullableFrequency { get; set; }
        public Frequency NonNullableFrequency { get; set; }
    }

... frankly I'm not sure, but what I know is that as soon as as I removed the IConvertible from the Frequence (and all other quantities), the nullable-struct-property tests started failing in the same fashion. Meanwhile, I know that none of the IConvertible interface members are being called in the original tests- yet there must be something about the interface that is obviously being checked for somewhere..

Puzzled by it, I looked around and found a few open issues that seem to be related...
The suggested workarounds mentioned using the JsonConverter instead of the JsonConverter<T> and overriding the CanConvert such that it handles the Nullable<T> case as well..

So I did (comments omitted for brevity)..

public abstract class NullableQuantityConverter<T> : JsonConverter
{
    public override bool CanConvert(Type objectType)
    {
        // Check if the type directly implements TQuantity
        if (typeof(T).IsAssignableFrom(objectType))
        {
            return true;
        }

        // Check if the type is a Nullable<T> where T implements IQuantity
        Type? underlyingType = Nullable.GetUnderlyingType(objectType);
        return underlyingType != null && typeof(T).IsAssignableFrom(underlyingType);
    }

    public sealed override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer)
    {
        switch (value)
        {
            case T quantity:
                WriteJson(writer, quantity, serializer);
                break;
            case null:
                WriteJson(writer, default, serializer);
                break;
            default:
                throw new JsonSerializationException("Converter cannot write specified value to JSON. An IQuantity is required.");
        }
    }

    public abstract void WriteJson(JsonWriter writer, T? quantity, JsonSerializer serializer);

    public sealed override object? ReadJson(JsonReader reader, Type objectType, object? existingValue, JsonSerializer serializer)
    {
        return existingValue switch
        {
            T existingQuantity => ReadJson(reader, objectType, existingQuantity, true, serializer),
            null => ReadJson(reader, objectType, default, false, serializer),
            _ => throw new JsonSerializationException("Converter cannot read JSON with the specified existing value. An IQuantity is required.")
        };
    }
    
    public abstract T? ReadJson(JsonReader reader, Type objectType, T? existingValue, bool hasExistingValue, JsonSerializer serializer);
}

After which I simply switched the UnitsNetBaseJsonConverters base class from JsonConverter<T> to the NullableQuantityConverter<T> and all tests are now green (including in the version where Frequence is not IConvertible)...

As it stands, the current issue only affects the nullable external quantities, however if we decide to move away from the IConvertible interface, this issue will really become relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant