dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.57k stars 10.05k forks source link

[SignalR] Support union in the js client #7298

Open sharok opened 5 years ago

sharok commented 5 years ago

MessagePack for C# supports Union type and I can use it for interfaces. For example, I have the following model that I send to js client:

[MessagePackObject]
public class Prices
{
        [Key("last")]
        public decimal Last { get; set; }

        [Key("sell")]
        public decimal Sell { get; set; }

        [Key("buy")]
        public decimal Buy { get; set; }

        [Key("product")]
        public IProductModel Product { get; set; }
}

[MessagePack.Union(0, typeof(ProductEx0))]
[MessagePack.Union(1, typeof(ProductEx1))]
public interface IProductModel 
{
   string Name{ get; set; }
}

I can create different IProductModel and send to the client. I handle creating the corresponding objects manually on the client and all works fine. But, it does't work when I want to deserialize message to a class that contain union. I have the following class on the server side:

public RegisterProduct 
{
   [Key("product")]
   public IProductModel Product { get; set; }
}

I send the following object from the client:

{
      product: new ProductEx0()     
}

MessagePack on server side throws error:

System.InvalidOperationException: code is invalid. code:129 format:fixmap

I understand that Union types are encoded in different way, but from the client I send simple model. therefore messagepack cannot handle it.

bradygaster commented 5 years ago

cc @muratg to come up with a repro and potential path forward. assigning to @BrennanConroy to come up with a test case so we can investigate. thanks for the detailed repro above!

peppy commented 3 years ago

Until this is resolved, I'd highly recommend adding mention of this in the SignalR documentation. Right now it is recommended to use MessagePack for serialisation of messages, but with limitations like this (which you can hit late into development) not listed from the outset it's a bit of a frustrating situation.

Also a bit weird that this is titled as a javascript client issue, but also applies to the c# client.

Sonorpearl commented 8 months ago

Will there ever be a fix for it?

@peppy I heard you have some kind of workaround?

Sonorpearl commented 8 months ago

Mentioning this issue, so it's also linked: https://github.com/MessagePack-CSharp/MessagePack-CSharp/issues/1171

Sonorpearl commented 8 months ago

Also throwing it in, but it seems like MemoryPack (MessagePack Alternative), has solved this issue: https://github.com/Cysharp/MemoryPack/issues/146 Haven't done an investigation yet.

SylvainGantois commented 8 months ago

The standard hub Json protocol is much easier to use and supports polymorphism. In our real life complex solution, we couldn't use MessagePack everywhere, and MemoryPack seems to have even more constraints. Unless there is a real performance issue related to message size, better stay away from those.

olegsavelos commented 8 months ago

Guys its been years now, any chance you could actually fix this ? @Sonorpearl As a solution I have just copied their protocol implementation from the aspnet source and made the required changes so my models serialized by desired type, its literally 2 lines of code you need to change.

image

BFS-JWesseler commented 8 months ago

Thanks @olegsavelos, I will look into that tomorrow. My plan was to also look at the implementation of osu!, which was done here by @peppy: https://github.com/ppy/osu/pull/14389

BrennanConroy commented 7 months ago

any chance you could actually fix this ?

its literally 2 lines of code you need to change

We would gladly accept a PR to fix this. This is an open-source project 😄

Tests would likely go in https://github.com/dotnet/aspnetcore/blob/main/src/SignalR/common/SignalR.Common/test/Internal/Protocol/MessagePackHubProtocolTests.cs

Mitchman215 commented 7 hours ago

Now that .NET 9 finally added System.Text.Json polymorphic support for SignalR (#53035), it would be really nice to also have this capability with MessagePack for feature parity. As it stands, the release notes for .NET 9 don't explicitly indicate that polymorphic support is only available for System.Text.Json and not MessagePack, which is a bit misleading.

Also to echo @peppy's previous comment, it would be great if this issue were renamed to indicate that it also applies to c# client and SignalR's documentation were updated to mention this limitation.