SignalR Java client: allow customisation of the Gson deserialisation process

iannewson commented 5 years ago

Hi, I'm using SignalR in an Android app. One of my models includes a date. SignalR will not deserialise a model which includes a Date (either java.util.Date or jodatime DateTime) as it doesn't know how to deserialise a date from a string.

Gson provides the ability to support this by customising the instance of Gson via GsonBuilder and the registerType* methods.

Please expose a way to customise Gson by allowing us to provide our own instance of Gson.

analogrelay commented 5 years ago

This certainly seems useful. Any interest in contributing this change? One important thing to keep in mind when doing this is that we must only use the customized settings when serializing/deserializing user payloads (arguments, return values), not the protocol messages that carry them.

iannewson commented 5 years ago

This certainly seems useful. Any interest in contributing this change? One important thing to keep in mind when doing this is that we must only use the customized settings when serializing/deserializing user payloads (arguments, return values), not the protocol messages that carry them.

@anurse Yes definitely. I get intimidated when it comes to contributing to large companies like Microsoft though, so could you point me towards some style guidelines? Hopefully you have some that are specific to not only Java but also Android development, as the feature I'm suggesting is clearly crying out for some kind of dependency injection but I don't really want to attempt that without knowing what your formal use cases are. Thanks again. :)

analogrelay commented 5 years ago

We don't have much in the way of style guidelines, particularly for Java :). Don't worry, we'll happily help you get the style aligned, there might be a bunch of PR comments to refine the style, but that's expected (even when our team members make contributions 😆). Take a look at the existing code as you go and try to align where you can, if you're not sure, that's OK, we can refine it in the PR. We know external contributors don't have the same experience with the code as we do, it's OK and we're happy to help!

As for how to architect this, we do use DI in the .NET Client but I don't think we want to do that here right now. I'd just start by adding a method to HttpHubConnectionBuilder called withJsonProtocol (to match the .NET client). It would take a JsonHubProtocolOptions object, which could have a GsonBuilder property. Then we just need to flow it in to the HubConnection then JsonHubProtocol.

In the JsonHubProtocol we already have a separate Gson instance for serializing/deserializing user payloads and then use JsonReader to read the protocol parts. So we just need to build the Gson from the provided GsonBuilder (or use a default one if none was provided).

We're happy to help you out as you go! Feel free to open a draft PR if you've got some of the way there but need some help!

iannewson commented 5 years ago

@anurse That is reassuring and helpful, I will give it a go later in the week.

SergeyKharuk commented 2 years ago

So, did you finish current feature or didn't? I was going to setup HubConnection instance with custom Gson instance but don't know how to do that.

p.s. Android project. library version: com.microsoft.signalr:signalr:5.0.4

halter73 commented 1 year ago

Background and Motivation

See original issue.

Hi, I'm using SignalR in an Android app. One of my models includes a date. SignalR will not deserialise a model which includes a Date (either java.util.Date or jodatime DateTime) as it doesn't know how to deserialise a date from a string.

Gson provides the ability to support this by customising the instance of Gson via GsonBuilder and the registerType* methods.

Please expose a way to customise Gson by allowing us to provide our own instance of Gson.

Proposed API

JsonHubProtocol is currently package-private unlike MessagePackHubProtocol which is public. To add constructors that can be used outside of tests, we'll have to make JsonHubProtocol public too.

package com.microsoft.signalr;

+ public class JsonHubProtocol implements HubProtocol {
+     public JsonHubProtocol();
+     public JsonHubProtocol(Gson gson);
+     public JsonHubProtocol(JsonParser jsonParser);
+     public JsonHubProtocol(Gson gson, JsonParser jsonParser);
+ 
+     // From HubProtocol. Interface methods must be public
+     public String getName();
+     public int getVersion();
+     public List<HubMessage> parseMessages(ByteBuffer message, InvocationBinder binder);
+     public ByteBuffer writeMessage(HubMessage message);
+ }

Usage Examples

Gson gson = new GsonBuilder().registerTypeAdapter(...).create();
HubConnectionBuilder.create(url).withHubProtocol(new JsonHubProtocol(gson, new JsonParser()));

Alternative Designs

We could get rid of some of the public constructors. Only the two-argument version is really necessary. Most people probably won't construct this this type and just use defaults, so this could be okay. At the very least we should probably get rid of the JsonHubProtocol(JsonParser jsonParser) constructor as JsonParser moves from the first to second argument depending on the overload which can hurt auto-completion. Additional constructors could always be added later.

We could also reconsider adding HttpHubConnectionBuilder.withJsonProtocolOptions) and a new JsonHubProtocolOptions. Perhaps with a custom options class, we could expose options that are not directly tied to Gson.

Risks

More API surface locks us into supporting a com.google.gson-based JSON protocol forever more. Dropping com.google.gson for another JSON serializer/deserializer will be more breaking out of this change. Maybe as part of making this public, we should rename JsonHubProtocol to GsonHubProtocol.

ghost commented 1 year ago

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

halter73 commented 1 year ago

API Review Notes:

We are worried about tying JsonHubProtocol to the com.google.gson library. It sounds like we're okay with calling it GsonHubProtocol.
We are not sure of the use case for customizing the JsonParser, so we'll leave that out for now.
Should we make this sealed errr I mean final 😆 ? Yes.
Are we fine keeping it in the com.microsoft.signalr package now that it's public? Yes. We already have the Gson dependency.

API Approved! Thanks for the patience @hcshmk!

package com.microsoft.signalr;

+ public final class GsonHubProtocol implements HubProtocol {
+     public GsonHubProtocol();
+     public GsonHubProtocol(Gson gson);
+ 
+     // From HubProtocol. Interface methods must be public
+     public String getName();
+     public int getVersion();
+     public List<HubMessage> parseMessages(ByteBuffer message, InvocationBinder binder);
+     public ByteBuffer writeMessage(HubMessage message);
+ }

BrennanConroy commented 1 year ago

Done via https://github.com/dotnet/aspnetcore/pull/45555

dotnet / aspnetcore