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

serialize the id field as a string #329

Closed
blelem opened this issue Aug 28, 2019 · 5 comments · Fixed by #348
Closed

serialize the id field as a string #329

blelem opened this issue Aug 28, 2019 · 5 comments · Fixed by #348
Assignees

Comments

@blelem
Copy link

blelem commented Aug 28, 2019

Would the vs-streamjsonrpc maintainers consider adding an option/setting to serialize the id field as a string?

{
    "jsonrpc": "2.0",
    "method": "ExampleMethod",
    "params": {},
    "id": "pt-1"
}

Here is the reasoning behind this request:

I am connecting to a terminal via jsonrpc. The terminal follows the JSON/RPC specifications, but defines the following additional constraint:

The id field must be a string. The suggested format is "x-" where x is the endpoint short name (e.g. pt for payment terminal) and is a counter starting from 1 and increasing after every method request sent by that party. Example: "pt-1"

Right now the library serializes the id generated by the invoke* methods as integers. It was easy enough to fork the streamjsonrpc library to convert that id to a string. And that was enough to fullfil the terminal additional constraint.

  protected async Task<TResult> InvokeCoreAsync<TResult>(long? id, string targetName, IReadOnlyList<object> arguments, CancellationToken cancellationToken, bool isParameterObject)
        {
            var request = new JsonRpcRequest
            {
                Id = id.toString(), // <--- toString() added here.
                Method = targetName,
         };

Would the vs-streamjsonrpc maintainers consider adding an option/setting to serialize the id field as a string? Optionally, providing the user an option to use a define an id_prefix would be a nice addition.

@AArnott
Copy link
Member

AArnott commented Aug 28, 2019

I'm not opposed to the change.

It isn't going to be as simple as what you propose though. We have lookup tables that will have to be updated to handle both string and long. And we have an active PR that adds several more of these lookup tables, with more on the way.

I think the way we might do this is to define an internal struct that represents the long, string union that includes Equals and GetHashCode overrides so we can use this struct as the key in our dictionaries.

@blelem
Copy link
Author

blelem commented Aug 29, 2019

I guess I have been lucky, adding the toString() just worked for my use case, I didn't have to look what was happening further down in the library.
Thank you so much for considering this!

@AArnott
Copy link
Member

AArnott commented Oct 16, 2019

I'm looking into this now. No promises yet.

@AArnott AArnott self-assigned this Oct 16, 2019
@AArnott
Copy link
Member

AArnott commented Oct 18, 2019

It looks like we can technically do it. I'm working on minimizing impact to public API. Then we'll have to evaluate risk and which milestone it might go in.

@blelem
Copy link
Author

blelem commented Oct 24, 2019

I tested the new code on my project, and it worked beautifully! The requestId prefix was nice and easy to setup.

Thanks @AArnott!

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

Successfully merging a pull request may close this issue.

2 participants