Messagepack-csharp: Typeless decimal serialized as string

[kstreichergb]

Bug description

Types are deserialized as different incompatible types (e.g. int as uint, decimal as string).

Examples

using System.Collections.Generic;
using MessagePack;
using Xunit;

    public class TestMsgPackCSharp
    {
        [MessagePackObject(keyAsPropertyName: false)]
        public class MsgPack
        {
            [Key(0)] public Dictionary<string, object> Data = new Dictionary<string, object>();
        }

        [Fact]
        public void IntShouldBeDeserializedAsInt()
        {
            var msgPack = new MsgPack();
            const string type = "int";
            int before = 4;
            msgPack.Data[type] = 4;
            var bytes = MessagePackSerializer.Serialize(msgPack);
            var deserialized = MessagePackSerializer.Deserialize<MsgPack>(bytes);
            var after = deserialized.Data[type];
            Assert.Equal(before.GetType(), after.GetType());
/*
Assert.Equal() Failure
Expected: typeof(int)
Actual:   typeof(byte)
*/
        }

                [Fact]
        public void MediumSizedIntShouldBeDeserializedAsInt()
        {
            var msgPack = new MsgPack();
            const string type = "int";
            int before = 2049905;
            msgPack.Data[type] = 4;
            var bytes = MessagePackSerializer.Serialize(msgPack);
            var deserialized = MessagePackSerializer.Deserialize<MsgPack>(bytes);
            var after = deserialized.Data[type];
            Assert.Equal(before.GetType(), after.GetType());
/*
Assert.Equal() Failure
Expected: typeof(int)
Actual:   typeof(uint)
*/
        }
[Fact]
        public void DecimalShouldBeDeserializedAsDecimal()
        {
            var msgPack = new MsgPack();
            const string type = "int";
            decimal before = 2049905m;
            msgPack.Data[type] = before;
            var bytes = MessagePackSerializer.Serialize(msgPack);
            var deserialized = MessagePackSerializer.Deserialize<MsgPack>(bytes);
            var after = deserialized.Data[type];
            Assert.Equal(before.GetType(), after.GetType());
        }

    }

Yes the specification of MessagePack allows to pack numeric values into smaller data types.

However, the deserialized type should be the one it had when it is serialized.

Problem 1

Packing the values into smaller types is less of a problem, but there are two things which are:
the MediumSizedInt which is unpacked as uint, which means the unpacked value cannot be safely cast back to the int.

Problem 2

Unpacking the decimal as a string. It is fine to use this as a intermediate packing, but once unpacked it must be a decimal again, as this is not even a numeric value anymore.

Problem 3

Which culture variant rules are used for decimal to string conversion. There is a huge difference on how to convert a string back to a decimal. How can I specify which rules should be used?

Repro steps

Run the XUnit test cases.

Expected behavior

No matter what format is used during packing, the unpacked types should match the original or a smaller type which can be safely assigned to the larger one.

Actual behavior

The types changed from int to uint and from decimal to string.
The string case is in particular dangerous as string serialization is culture variant.

• Version used: 1.7.3.7 (nuget)
• Runtime: .NET Core 2.2

Other resources

See messagepack-c for a similar issue: https://github.com/msgpack/msgpack-c/issues/247

[AArnott]

Preserving integer types is tracked by #425
The only unique bit to this issue is that we pack a decimal as a string, which sounds bizarre.

[AArnott]

Ok, it looks like the DecimalFormatter specifically formats as a string:

https://github.com/neuecc/MessagePack-CSharp/blob/90f784ff6dba0bcc2e98a6709d2044e8c75a7436/src/MessagePack.UnityClient/Assets/Scripts/MessagePack/Formatters/StandardClassLibraryFormatter.cs#L109-L113

I guess that's because there's no better format that retains all the precision that Decimal guarantees.

The shortcoming then seems to be that the typeless formatter doesn't recognize this type as needing a type annotation, and as a result simply deserializes it using the type it appears to be in the msgpack sequence.

@neuecc Do you already have a pattern for dealing with this?

[AArnott]

I added a test case for this. We'll merge it hopefully with a fix, or I might merge it as a skipped test just to document the bug if we can't fix right away.

[neuecc]

for DateTime, Typeless already using NativeDateTimeResolver so I think good to use NativeGuidResolver and NativeDecimalResolver.
But it breaks binary compatibility.

[AArnott]

So should we get this in for 2.0 then on the backward compat basis?

[AArnott]

Adding NativeGuidResolver to TypelessContractlessStandardResolver didn't make the test that I added for GUIDs pass.
And given that NativeGuidFormatter isn't safe in general (it's documented as only being allowed on little endian systems) it isn't actually the approach I was thinking of.

I think we should solve this by adding a type annotation extension when we serialize guid's or decimals when they were statically typed just as object so that when deserialized we know to convert them back to their original types instead of leaving them as strings.

[neuecc]

v1-> v2 should have binary compatible.
I don't think breaking changes for this are good.

[AArnott]

So we resolve this as Won't Fix then? I don't see any alternative without some kind of msgpack formatter change.

[neuecc]

You can create a binary compatibility formatter for Typeless that determines the type when deserializing (string or binary).
However, Typeless is not very performance oriented in the first place, so I don't know if these changes are worthwhile.
I think Won't fix is good.