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

JSON Encoding issues #1255

Closed
fdmarc opened this issue Aug 17, 2023 · 17 comments
Closed

JSON Encoding issues #1255

fdmarc opened this issue Aug 17, 2023 · 17 comments
Assignees

Comments

@fdmarc
Copy link

fdmarc commented Aug 17, 2023

I've had two problems encoding C# objects into JSON with the Ably SDK.

First, when trying to encode a TokenRequest from IRestClient.Auth.CreateTokenRequestObjectAsync, by calling JSON.net in my HTTP controller. The call to JsonConvert.SerializeObject(tokenRequest) was returning JSON with the original field names from the TokenRequest class, and ignoring the JsonProperty attributes. So the resulting JSON had field names KeyName, Ttl rather than keyName, ttl, etc.

The issue seemed to be that version of JSON.net in my app was not able to understand the JsonProperty attributes from your SDK.

I solved this problem by calling the (deprecated) CreateTokenRequestAsync function, which returned the correct JSON (I can only guess as to who this worked).

The second issue that I hit was converting the other way - I was trying to send domain objects from my app as the payload of Ably messages. This time my JsonProperty attributes were being ignored. I solved this problem using this trick:

#92 (comment)

Do you have thoughts on how to address these problems?

If not, it would be nice to include this code snippet in the docs, since it's very powerful when you want to have fine-grained control over the JSON.

Thanks

@sync-by-unito
Copy link

sync-by-unito bot commented Aug 17, 2023

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-3809

@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 17, 2023

@fdmarc sure we will most likely update the docs 👍 . Thanks for noticing : )

@sacOO7 sacOO7 self-assigned this Aug 19, 2023
@sacOO7 sacOO7 mentioned this issue Aug 19, 2023
@sacOO7
Copy link
Collaborator

sacOO7 commented Aug 19, 2023

@fdmarc as you can see, I have created a PR to update README -> #1256.
Let me know if any other README updates are needed, thanks : )

@fdmarc
Copy link
Author

fdmarc commented Sep 15, 2023

I have a bit more information to help debug this. Here is a minimal repro from our codebase with the error message:

static class AblyExtensions
{
    public static Task<HttpPaginatedResponse> PublishAsync(this AblyRest ably)
    {
        var batch = JToken.Parse("{}");
        return ably.Request("POST", "/messages", null, batch, null);
    }
}

Produces this error:

AblyPublishRealTimeEvents.cs(127,60): Error CS1503 : Argument 4:
cannot convert from 'Newtonsoft.Json.Linq.JToken [C:\Users\User\.nuget\packages\newtonsoft.json\13.0.3\lib\net45\Newtonsoft.Json.dll]'
to 'Newtonsoft.Json.Linq.JToken [C:\Users\User\.nuget\packages\ably.io\1.2.12\lib\net46\IO.Ably.dll]'

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

@fdmarc got it 👍 .
I think it's better to send serialized strings externally. We will merge updated README soon 👍

@fdmarc
Copy link
Author

fdmarc commented Sep 15, 2023

That's not an option unfortunately - there is no Request method on AblyRest that takes a string, only JToken.

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

Understood. Your requirement is publishing multiple messages on multiple channels at the same time, right?
We will update request method to take payload as a string instead 👍
Meanwhile, you need to iterate over each channel to publish messages.

@fdmarc
Copy link
Author

fdmarc commented Sep 15, 2023

My immediate need is to send the same message to roughly 1-5 connected clients.

This is how I was approaching it:

    static class AblyExtensions
    {
        public static Task<HttpPaginatedResponse> PublishAsync(this AblyRest ably, IEnumerable<string> channels, IEnumerable<Message> messages)
        {
            JObject batch = new() { 
                { "channels", new JObject(channels) },
                { "messages", new JObject(messages) } 
            };
            return ably.Request("POST", "/messages", null, batch, null);
        }
    }

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

For the time being, you can do

    static class AblyExtensions
    {
        public static Task<HttpPaginatedResponse> PublishAsync(this AblyRest ably, IEnumerable<string> channels, IEnumerable<Message> messages)
        {
           for (var channelName in channels) {
            var restChannel = ably.Channels.Get(channelName);
            await restChannel.PublishAsync(messages);
           }
        }
    }

@fdmarc
Copy link
Author

fdmarc commented Sep 15, 2023

Yup, but I'm making the HTTP calls in in parallel:

            var message = new Message
            {
                Id = eventBase.FlipdishEventId.ToString(),
                Name = eventBase.EventName,
                Data = JsonConvert.SerializeObject(eventBase.GenericMapToPublicModel()),
                Encoding = "json",
            };

            // TODO:
            // https://github.com/ably/ably-dotnet/issues/1151
            // The Ably C# SDK does not support batch sending at present.
            var tasks = channels.Select(channel =>
                _restClient.Channels.Get(channel).PublishAsync(message));

            await Task.WhenAll(tasks);

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

I don't see it as a problem as long as the number of channels are limited ( ~10/12 channels).
If you have more channels ~100, you can use Parallel.ForEachAsync Method instead
There's a blog for this -> https://medium.com/@nayanava.de01/controlling-degree-of-parallelism-with-task-whenall-in-c-1ef1e9ff2bc

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

However, this should be a small change to be made from our side to the request method. I will check and create the PR soon 👍

@fdmarc
Copy link
Author

fdmarc commented Sep 15, 2023

Could you add an AblyRest.PublishAsync(channels, messages) function instead please? :) Then I don't need to concern myself with the lower level REST API.

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

It's not a part of standard ably public API specification : (
Though I will check and try to make a change that suits the requirement.

@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 15, 2023

@fdmarc I have created a PR to fix your issue #1260. You can comment there if needed 👍

@sacOO7
Copy link
Collaborator

sacOO7 commented Oct 13, 2023

Closed via #1260.
@fdmarc you can open a new issue if there are JSON encoding issues in the future.

@sacOO7 sacOO7 closed this as completed Oct 13, 2023
@sacOO7
Copy link
Collaborator

sacOO7 commented Oct 23, 2023

@fdmarc we have created a new release for this. Please use 1.2.14, you will find all fixes available there.

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