Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

messagepack GUID failing #1043

Closed
Jon-Murray opened this issue Oct 21, 2017 · 8 comments
Closed

messagepack GUID failing #1043

Jon-Murray opened this issue Oct 21, 2017 · 8 comments

Comments

@Jon-Murray
Copy link

Jon-Murray commented Oct 21, 2017

Hi,

When i have a function inside a hub such as the following:

    public async Task<bool?> myFunc(Guid ID)

And am using the messagepack protocol on the browser, I get the following error thrown in DeserializeObject (UnpackFrom) in MessagePackHubProtocol.cs:

"System.Runtime.Serialization.SerializationException: 'The unpacked value is not expected type. Byte array for GUID must be exactly 16 bytes long.'"

When I change "ID" in my hub function to be a string, and then manually parse the GUID, it is fine, and there are no problems with anything. Am i doing something wrong here, or is this a bug?

Thanks!

@moozzyk
Copy link
Contributor

moozzyk commented Oct 21, 2017

Likely a bug or a limitation. Needs some investigation.

@BrennanConroy
Copy link
Member

How are you passing the GUID to the hub from Typescript/Javascript?

I could be wrong but it looks like the server is expecting the value from the client to be 16 bytes but the client is probably sending a string instead since a GUID in Typescript/javascript is usually represented by a string.

@Jon-Murray
Copy link
Author

That's correct, and i believe that is the issue. There is no native GUID in JS/TS (as you've said) so i can do something like:

// push a native guid from the server:
Guid g = Guid.NewGuid();
clients.all.invoke('something', g);
// receive a guid on the client's browser
myconnection.on('something', function(g)
{
   // send the received "proper" guid right back to the server
   myconnection.invoke('serverSomething', g);
});
// on the server
public async task serverSomething(Guid g)
{
  // do something
}

"serverSomething" will fail, despite having a "proper" guid pushed out to it with the error message above. As you've said before as there's nothing in JS/TS to turn this into a proper guid such that messagepack will see it as such, it will fail despite having a proper guid passed back to it. I guess this is a limitation, though it is interesting why it is throwing that particular error given that it is a properly formatted guid in the first place. I'm unsure as to why this is happening which is why i've raised it here. I'd guess it is not an issue with signalr, but messagepack itself. For now, using it as a string and converting to a guid server-side is fine. I'll continue to investigate if i get the time to try and find a fix.

I've done some pretty extensive testing with messagpack/signalr (alpha 2) and this is the first issue that i've run into really. Other than that its been great!

Thanks again

@moozzyk
Copy link
Contributor

moozzyk commented Nov 1, 2017

This is a bit tricky. I just merged a change where it is possible to send bytes from the JavaScript client to the server by creating a Uint8Array. So if you have a GUID on the client side you can do GUID: new Uint8Array([0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01, 0x00]), and the corresponding GUID property on the server side will be set correctly. The tricky part is to get GUIDs from the server to the client - if you have a GUID property on the server side what you get on the client is a string created by utf8 encoding the original GUID bytes. Extracting the actual GUID will not be easy and if you need to send guids from the server to the client I would recommend just using byte arrays or strings.

@moozzyk
Copy link
Contributor

moozzyk commented Nov 2, 2017

I don't think there is any work to be done here. I opened an issue in the msgpack-cli repo to ask if they can serialize GUIDs as bin8. If they did handling guids in JS would be quite easy because they would be serialized to Uint8Array/Buffer. Using raw family is valid as per spec though and since raw data is being sent with string header JS clients reads is as string the problem is that the result is hard to process in JS.

@moozzyk moozzyk closed this as completed Nov 2, 2017
@Jon-Murray
Copy link
Author

Jon-Murray commented Nov 3, 2017

I know this has been marked as closed now. I might be wrong here but i swear i remember reading through and seeing signalr using reflection to get the target types, target args, etc. etc. of the hub methods. Would it not be better to throw an exception stating that GUID is not supported and to use a string instead (assuming a hub method has a System.Guid as an argument)? It would save a lot of people a lot of headaches further down the line if they had this passed to their clients i'm sure

@moozzyk
Copy link
Contributor

moozzyk commented Nov 3, 2017

It depends on what your client is. If you are using C# client it will work just fine. Note that JavaScript does not have a built-in GUID type and this by itself should be a warning sign that the user needs to understand how serialization/deserialization would work for their case. I think we will need to have some documentation on this.

@moozzyk
Copy link
Contributor

moozzyk commented Nov 6, 2017

As per this issue - if MsgPack-Cli - respects CompatibilityOptions and GUIDs are serialized as bin8 the experience in the JS will be much better. We would get uint8Array -> GUID, GUID ->uint8Array serialization.

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

No branches or pull requests

3 participants