Skip to content

Typeless decimal serialized as string #651

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

Closed
kstreichergb opened this issue Nov 20, 2019 · 11 comments
Closed

Typeless decimal serialized as string #651

kstreichergb opened this issue Nov 20, 2019 · 11 comments
Assignees

Comments

@kstreichergb
Copy link

kstreichergb commented Nov 20, 2019

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: msgpack/msgpack-c#247

@AArnott AArnott changed the title Deserialized types change Typeless decimal serialized as string Dec 11, 2019
@AArnott
Copy link
Collaborator

AArnott commented Dec 11, 2019

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 AArnott added the bug label Dec 11, 2019
@AArnott AArnott self-assigned this Dec 11, 2019
@AArnott
Copy link
Collaborator

AArnott commented Dec 11, 2019

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 added a commit to AArnott/MessagePack-CSharp that referenced this issue Dec 11, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
AArnott Andrew Arnott
Demonstrates bug MessagePack-CSharp#651
@AArnott
Copy link
Collaborator

AArnott commented Dec 11, 2019

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
Copy link
Member

neuecc commented Dec 12, 2019

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

@AArnott
Copy link
Collaborator

AArnott commented Dec 12, 2019

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

AArnott added a commit to AArnott/MessagePack-CSharp that referenced this issue Dec 12, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
AArnott Andrew Arnott
Demonstrates bug MessagePack-CSharp#651
@AArnott
Copy link
Collaborator

AArnott commented Dec 12, 2019

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
Copy link
Member

neuecc commented Dec 12, 2019

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

@AArnott
Copy link
Collaborator

AArnott commented Dec 12, 2019

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

@neuecc
Copy link
Member

neuecc commented Dec 12, 2019

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.

@RyanLiu99
Copy link

I am trying latest stable nuget package as of 2/18/2013. Guid and decimal will be string after serialization and deserialization. I have a property whose type is object/dynamic, since it generic ID for any entity, could be composite key, could be any type. Is there a way to preserver run time type info for each object when serialization and use it for deserialization?

@AArnott
Copy link
Collaborator

AArnott commented Mar 11, 2023

@RyanLiu99 Please don't double post.

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

No branches or pull requests

4 participants