Skip to content
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

Address JSON serialisation #1143

Open
AndyNicks opened this issue Apr 7, 2022 · 1 comment
Open

Address JSON serialisation #1143

AndyNicks opened this issue Apr 7, 2022 · 1 comment

Comments

@AndyNicks
Copy link

AndyNicks commented Apr 7, 2022

From conversation with a customer:
the root cause of the problem is that during the serialization of the ProtocolMessage the ably client lib is using a serializer created by this method:

private static JsonSerializer GetSerializer()
{
    var settings = GetJsonSettings();

    return JsonSerializer.CreateDefault(settings);
}

when calling JsonSerializer.CreateDefault the settings passed to the method are merged with the static JsonConvert.DefaultSettings. in our case the later are:

JsonConvert.DefaultSettings = () => new JsonSerializerSettings
{
    Converters = new List<JsonConverter>() { new StringEnumConverter() },
};

the ably client lib should be making a difference between the serialization of the user's messages (stored in ProtocolMessage.Messages property) and other protocol message's properties, so two different instances of JsonSerializerSettings are needed.

the user's message serialization settings should be customizable in the ably client and it's only a matter of contract negotiation between the publisher and the consumer so ably's lib code should not interfere with those settings
the protocol message serialization settings should remain static and not customizable, otherwise you cannot ensure the same json settings apply for serialization on the client and de-serialization on the server. those settings should not depend on JsonConvert.DefaultSettings

Workaround

var customSettings = new JsonSerializerSettings
{
    Converters = new List<JsonConverter>
    {
        new StringEnumConverter() // more converters here
    },
};

var serializedJSON = JsonConvert.SerializeObject(new { payload = $@"published at: {DateTime.UtcNow}" }, customSettings);
// Don't update JsonConvert.DefaultSettings
var result = await channel.PublishAsync(new Message
{
        Data = serializedJSON ,
        Encoding = "json",
        Id = Guid.NewGuid().ToString().ToLower(),
});

see https://ably-real-time.slack.com/archives/CURL4U2FP/p1649240863594569 for details

┆Issue is synchronized with this Jira Story by Unito

@tomkirbygreen
Copy link
Contributor

The whole area of JSON serialization in the Ably SDK has been under discussion for some time. Since JSON contributes to the public surface area of the SDK this work is waiting for the SDK to be able to take back control of its version number.

We have a number of existing issues that pertain to JSON:

Allow to send raw jtoken/jobject instead object

Unity: Newtonsoft JSON library conflicts with version that Unity includes by default

In considering revisiting the whole JSON area we'd ideally like to drop the dependency on Newtonsoft or indeed any third party library and use the .NET Base Class Library namespace System.Text.Json .

@AndyNicks AndyNicks added the enhancement New feature or improved functionality. label May 13, 2022
@sync-by-unito sync-by-unito bot removed the enhancement New feature or improved functionality. label Jan 23, 2023
@sacOO7 sacOO7 mentioned this issue Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants