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

fix: prevent request being serialized twice #11

Closed

Conversation

m-schmoock
Copy link

json-ipc-lib will make use of json-rpc-protocol module.
In its current form it already does a JSON.stringify inside the submodule.
Client.js tries this a second time, which will result in
a compliant server not understand the request.

{ Error: JSON-IPC Client Exception::Expected {} for json command
at Socket. (./node_modules/json-ipc-lib/dist/Client.js:171:36)
at Socket.emit (events.js:197:13)

Note:
This will not happen for 'method only' calls but for
'object' {method, params, ...} calls.

Relevant code snippets:

Client.js

import * as protocol from 'json-rpc-protocol';
// ...
protocol.format.request(
	Date.now(),
	method,
	args);
...
socket.write(JSON.strinify(request));

node_modules/json-rpc-protocol/dist/format.js

var toJson = JSON.stringify;
...
  return toJson({
    jsonrpc: '2.0',
    id,
    error: _error
  });

json-ipc-lib will make use of json-rpc-protocol module.
In its current form it already does a JSON.stringify inside the submodule.
Client.js tries this a second time, which will result in
a compliant server not understand the request.

{ Error: JSON-IPC Client Exception::Expected {} for json command
    at Socket.<anonymous> (./node_modules/json-ipc-lib/dist/Client.js:171:36)
    at Socket.emit (events.js:197:13)

Note:
This will not happen for 'method only' calls but for
'object' {method, params, ...} calls.

Relevant code snippets:

Client.js
```JS
import * as protocol from 'json-rpc-protocol';
// ...
protocol.format.request(
	Date.now(),
	method,
	args);
...
socket.write(JSON.strinify(request));
```

node_modules/json-rpc-protocol/dist/format.js
```JS
var toJson = JSON.stringify;
...
  return toJson({
    jsonrpc: '2.0',
    id,
    error: _error
  });
```
brozeph added a commit that referenced this pull request Feb 11, 2019
@brozeph
Copy link
Contributor

brozeph commented Feb 11, 2019

Added #12 to stringify in the event the payload isn't a string... this should, in theory, fix the double stringify (and it passes unit testing)... not sure of the exact scenario you're running into, so this may not fully resolve. Any further hints or a test harness for validation would help too, if you can share... no worries if not.

@m-schmoock
Copy link
Author

As I describe with the code samples above, we see that stringify will be called twice:

  • first by protocol.format.request
  • second by Client.js

This only happens for requests with pamaeters that are not just plain metho calls. If you need I write a test.

@m-schmoock
Copy link
Author

But your fix does the same. thx

brozeph added a commit that referenced this pull request Feb 11, 2019
@brozeph
Copy link
Contributor

brozeph commented Feb 11, 2019

Closing as merged and released v1.1.2 from other PR 🥂

@brozeph brozeph closed this Feb 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants