msgpack / msgpack-cli

MessagePack implementation for Common Language Infrastructure / msgpack.org[C#]
http://msgpack.org
Apache License 2.0
835 stars 175 forks source link

PackObject packs byte[] as fixstr #261

Closed moozzyk closed 7 years ago

moozzyk commented 7 years ago

Is there any reason why Packer.PackObject packs byte arrays to a string? There is even a test testing this but it seems to me that byte[] should be packed as fixarray and not as fixstr.

yfakariya commented 7 years ago

In msgpack format specification, str (formally raw) type is "byte array which may be UTF-8 encoded string" (and it was just "byte array" in old spec). So PackObject should pack byte array as str or bin instead of treating it as "array of unsigned 8bit integers". In addition, msgpack str type can be unpacked as String or byte[] depends on its content -- byte sequence [0x0, 0x1, 0x2, 0x3] is valid UTF-8 sequence even if it only contains control chars.

yfakariya commented 7 years ago

As far as I know, it is affected Ruby's string handling.

davidfowl commented 7 years ago

How do we get it to pack as bin instead of str? We'd like to treat byte[] as a raw binary payload and string as str.

yfakariya commented 7 years ago

You can do it by setting PackerCompatibilityOptions.None to SerializationContext.CompatibilityOptions.PackerCompatibilityOptions = PackerCompatibilityOptions.None (str is default because of backward compatibility)

moozzyk commented 7 years ago

I am trying to set the PackerCompatibilityOptions to None but it does not seem to have any effect. Here are my test:

        [Fact]
        public void PackerPacksByteArraysAsStringsWithDefaultSerializationSettings()
        {
            using (var ms = new MemoryStream())
            {
                var packer = Packer.Create(ms);
                packer.Pack(new byte[] { 1, 2, 3 });
                Assert.Equal(new byte[] { 0xA3, 0x01, 0x02, 0x03 }, ms.ToArray());
            }
        }

        [Fact]
        public void PackerPacksByteArraysAsByteArraysIfCompatibilitySettingsAreSetToNone()
        {
            using (var ms = new MemoryStream())
            {
                var packer = Packer.Create(ms);
                var serializationContext = new SerializationContext();
                serializationContext.CompatibilityOptions.PackerCompatibilityOptions = PackerCompatibilityOptions.None;
                packer.Pack(new byte[] { 1, 2, 3 }, serializationContext);
                Assert.Equal(new byte[] { 0x93, 0x01, 0x02, 0x03 }, ms.ToArray());
            }
        }

The first test passes as expected but the second test fails:

[9/28/2017 9:21:41 PM Error] [xUnit.net 00:00:01.3106688]     MsgPackTest.UnitTest1.PackerPacksByteArraysAsByteArraysIfCompatibilitySettingsAreSetToNone [FAIL]
[9/28/2017 9:21:41 PM Informational] [xUnit.net 00:00:01.3143498]       Assert.Equal() Failure
[9/28/2017 9:21:41 PM Informational] [xUnit.net 00:00:01.3150366]       Expected: Byte[] [147, 1, 2, 3]
[9/28/2017 9:21:41 PM Informational] [xUnit.net 00:00:01.3153703]       Actual:   Byte[] [163, 1, 2, 3]
[9/28/2017 9:21:41 PM Informational] [xUnit.net 00:00:01.3175092]       Stack Trace:
[9/28/2017 9:21:41 PM Informational] [xUnit.net 00:00:01.3196604]         C:\Users\XXX\Documents\Visual Studio 2017\Projects\MsgPackTest\MsgPackTest\UnitTest1.cs(31,0): at MsgPackTest.UnitTest1.PackerPacksByteArraysAsByteArraysIfCompatibilitySettingsAreSetToNone()

As you can see the expected is 147 (i.e. 0x93) but the actual is 163 which is 0xa3 which is a string of 3 items. Looks like passing the compatibility options have no effect. Or am I doing something wrong?

moozzyk commented 7 years ago

As per your other comment I should probably expect 0xc4 for bin8. Nevertheless, this does not happen either.

yfakariya commented 7 years ago

Sorry for my insufficient comment.

If you explicitly instantiate a Packer outside of a MessagePackSerializer.Pack method, you have to specify PackerCompatibilityOptions via

In fact, MessagePackSerializer<T> instances obtain PackerCompatibilityOptions from a SerializationContext they belongs when they instantiate Packer internally.

moozzyk commented 7 years ago

When I pass PackerCompatibilityOptions.None to the Create method the byte[] is serialized as bin8 indeed.

I think I can close this issue for now. Thanks for your help.

moozzyk commented 7 years ago

@yfakariya - one more question. I now understand how packing works. However, I have a question about unpacking. Specifically, if I have a fixarray of bytes why can't I deserialize it as a byte[] array? Here is an example: This payload:

0x94 0x01 0x02 0x03 0xcc 0xff

causes:

System.InvalidOperationException: Do not convert System.UInt32 (binary:0x4) MessagePackObject to MsgPack.MessagePackString.
   at MsgPack.MessagePackObject.ThrowInvalidTypeAs[T](MessagePackObject instance)
   at MsgPack.MessagePackObject.VerifyUnderlyingType[T](MessagePackObject instance, String parameterName)
   at MsgPack.Serialization.DefaultSerializers.System_ByteArrayMessagePackSerializer.UnpackFromCore(Unpacker unpacker)
   at MsgPack.Serialization.MessagePackSerializer`1.UnpackFrom(Unpacker unpacker)
   at MsgPack.Serialization.MessagePackSerializer`1.InternalUnpackFrom(Unpacker unpacker)
   at MsgPack.Serialization.ReflectionSerializers.ReflectionObjectMessagePackSerializer`1.UnpackSingleValue(Unpacker unpacker, Int32 index)
   at MsgPack.Serialization.ReflectionSerializers.ReflectionObjectMessagePackSerializer`1.UnpackMemberValue(Object objectGraph, Unpacker unpacker, Int32 itemsCount, Int32 unpacked, Int32 index, Int32 unpackerOffset)
   at MsgPack.Serialization.ReflectionSerializers.ReflectionObjectMessagePackSerializer`1.UnpackFromCore(Unpacker unpacker)
   at MsgPack.Serialization.MessagePackSerializer`1.UnpackFrom(Unpacker unpacker)
   at MsgPack.Serialization.MessagePackSerializer`1.InternalUnpackFrom(Unpacker unpacker)

Interestingly the same payload can be deserialized to int[] just fine.

As a side note: I tried compiling the code so that I can debug it myself but I am hitting many issues like not being able to restore package because the mpu.csproj seems to reference a version of Mono.Options that does not exist on NuGet. Since I don't need Mono, UWP, Xamarin etc. I tried to strip these pieces from the build but I still could not make it work. Can you update the build instructions and required dependencies (e.g. I also had to install .NET Framework 3.5 on my machine) ?

yfakariya commented 7 years ago

Thank you for follow ups!

Side note: Mono.Options is a simple and powerful command line parameters parser which takes advantage of C#'s collection initializer syntax and lambda expression -- it does not relate to Mono and/or Xamarin at all.

moozzyk commented 7 years ago

Would you open a new issue for it If you really think that unpacking an array of uint8 range integers should be unpacked as byte[] ?

I am not 100% sure. I just encountered a problem when trying to send an array of bytes from JavaScript with msgpack5. For arrays of numbers they use fixarray. It makes sense but can't be deserialized to byte[] MsgPack-Cli. On the other hand I was not able to force msgpack5 to send any bin family from the browser due to: https://github.com/mcollina/msgpack5/issues/58. I think I will try fixing this on the msgpack5 side.

As per the update to docs:

If you do not want to install options, edit <TargetFrameworks> element in *.csproj files to exclude platforms you want to exclude.

I think I actually tried that but still could not build... Maybe because I did not remove the mpu.csproj from sln?

yfakariya commented 7 years ago

I see. I will be very happy from your report when you face another interoperability scenario for deserializing byte array as a uint8 array.

And I will revise readme fix. Thank you!