fix(rpc): Handle rpc request with empty params #122

tomonari-t · 2020-06-06T07:42:44Z

Why this PR is needed

By JSON-RPC 2.0 Specification, params member may be omitted.

But I send below request, application is crushed.

% curl -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","id":1,"method":"web3_clientVersion", "id": 1}' http://localhost:8545

TODO

Add Test

coveralls · 2020-06-06T07:44:26Z

Coverage increased (+0.004%) to 92.708% when pulling d2ef871 on tomonari-t:handle-rpc-params-empty into 8a05f49 on ethereumjs:master.

tomonari-t · 2020-06-06T07:55:00Z

This is my first time PR to this project, so I would be grateful if you could give me any advice

ryanio · 2020-06-06T17:48:04Z

thanks @tomonari-t!

i think in the default case we want to set params to an empty array [] instead of an empty string.

the additional test is helpful as well, thanks for doing that! i think it would be best to test two cases:

it should complete the request as normal with an empty params if the method does not require any params (e.g. web3_clientVersion)
it should throw if the specific method requires params to complete the request (e.g. eth_getBlockByNumber)

tomonari-t · 2020-06-07T02:01:28Z

Thanks @ryanio for reviewing it!

Yes, you are right. params is array. I'll fix it and add tests 👍

tomonari-t · 2020-06-07T02:42:57Z

I have applied the review.

ryanio · 2020-06-07T19:30:58Z

thanks, looks great at first glance!
I will give it a proper review Monday morning.

holgerd77 · 2020-06-08T10:22:53Z

test/rpc/validation.js

+const { startRPC, closeRPC } = require('./helpers')
+const { middleware } = require('../../lib/rpc/validation')
+
+test('should comlete the request with non-params if the method does not require any params', t => {


comlete -> complete 😄

I find both of these test description with this "non-params" wording a bit hard too read, can we make it a bit more descriptive here what happens, since there is a lot of ground for confusion around around what is meant by "param" in this case.

Some first suggestion:

'should work with no request parameter "params" provided if the API endpoint has no parameter input'

But feel free to improve on that.

a suggestion:

Suggested change

test('should comlete the request with non-params if the method does not require any params', t => {

test('should work without `params` when it\'s optional', t => {

Thank you!
I think it is better should work without params when it\'s optional.
and I have installed type checker ;)

holgerd77 · 2020-06-08T10:27:39Z

test/rpc/validation.js

+const { startRPC, closeRPC } = require('./helpers')
+const { middleware } = require('../../lib/rpc/validation')
+
+test('should comlete the request with non-params if the method does not require any params', t => {


I find both of these test description with this "non-params" wording a bit hard too read, can we make it a bit more descriptive here what happens, since there is a lot of ground for confusion around around what is meant by "param" in this case.

Some first suggestion:

'should work with no request parameter "params" provided if the API endpoint has no parameter input'

But feel free to improve on that.

holgerd77 · 2020-06-08T10:29:36Z

test/rpc/validation.js

+    })
+})
+
+test('should throw if the method requires params but non-params', t => {


Here a bit the same, suggestion:

'...if the API endpoint requires parameters as an input but no "params" request parameter is provided'

holgerd77 · 2020-06-08T10:35:54Z

test/rpc/validation.js

+    .post('/')
+    .set('Content-Type', 'application/json')
+    .send(req)
+    .expect(res => {


We additionally need a success case for this - like e.g. here - this should likely be expect(200) when looking at the code.

Awesome! I'll merge and add tests.

holgerd77 · 2020-06-08T10:43:34Z

test/rpc/validation.js

+    .send(req)
+    .expect(res => {
+      if (!res.body.error) {
+        throw new Error('should return an error object')


There is also not an explicit "what should have happened?" case here, we should think about that.

Currently a request like curl -X POST -H "Content-Type: application/json" --data '{"jsonrpc":"2.0","method":"eth_getBlockByNumber","params":["0x1b4", true],"id":1}' http://127.0.0.1:8545 is actually crashing the whole client, this should likely be fixed? 😄

Ouput:

ERROR [06-08|12:38:02] uncaughtException: Cannot read property 'length' of undefined TypeError: Cannot read property 'length' of undefined at Eth.getBlockByNumber (/ethereumjs-client/lib/rpc/validation.js:12:18) at Method.execute (/ethereumjs-client/node_modules/jayson/lib/method.js:123:20) at /ethereumjs-client/node_modules/jayson/lib/server/index.js:303:12 at maybeParse (/ethereumjs-client/node_modules/jayson/lib/server/index.js:428:5) at Server.call (/ethereumjs-client/node_modules/jayson/lib/server/index.js:231:3) at /ethereumjs-client/node_modules/jayson/lib/utils.js:151:14 at /ethereumjs-client/node_modules/lodash/lodash.js:10050:25 at /ethereumjs-client/node_modules/jayson/lib/utils.js:117:7 at Object.Utils.JSON.parse (/ethereumjs-client/node_modules/jayson/lib/utils.js:254:3) at IncomingMessage.<anonymous> (/ethereumjs-client/node_modules/jayson/lib/utils.js:113:16)

What is the default/specified behavior here?

Update: ah sorry, got a bit confused here. Just realizing that I just tested on my local master and that this case WAS actually fixed by your, PR, lol (that's what I was referring too with "learning/work out together" in the other post 😄 ).

Anyhow, so the type of test seems ok here. This might also take an additional expect(200) here, but not too pressing.

So the output that this actually gives is:

HTTP/1.1 200 OK Connection: keep-alive Date: Mon, 08 Jun 2020 12:13:37 GMT content-length: 100 content-type: application/json; charset=utf-8 { "error": { "code": -32602, "message": "missing value for required argument 0" }, "id": "1", "jsonrpc": "2.0" }

Just for the sake of completeness.

Thank you for trying this by yourself!

tomonari-t · 2020-06-10T00:31:57Z

I have applied comment

holgerd77 · 2020-06-10T07:48:28Z

Oh, just seeing, the number of commit messages got a bit out of hand here for such a rather small change. 😄 Can you please either rebase and squash your commits into one or two commits or - might be easier if you are not so familiar with rebasing - open a new PR from current master and manually re-add your changes to 1-2 commits?

holgerd77 · 2020-06-10T07:48:55Z

This is for having a somewhat clean and not too bloated commit history in the library.

tomonari-t · 2020-06-10T08:38:22Z

I got it and I should squash.

tomonari-t · 2020-06-10T13:21:15Z

I'll make new PR

tomonari-t marked this pull request as ready for review June 6, 2020 07:53

tomonari-t force-pushed the handle-rpc-params-empty branch from 1348503 to f629d29 Compare June 6, 2020 07:56

holgerd77 mentioned this pull request Jun 8, 2020

Added JSON-RPC documentation to README #124

Merged

holgerd77 reviewed Jun 8, 2020

View reviewed changes

holgerd77 suggested changes Jun 8, 2020

View reviewed changes

fix(rpc): handle correctly a rpc request with empty params

32978e8

tomonari-t force-pushed the handle-rpc-params-empty branch from d2ef871 to 32978e8 Compare June 10, 2020 13:19

tomonari-t closed this Jun 10, 2020

tomonari-t mentioned this pull request Jun 10, 2020

fix(rpc): handle rpc request with empty params #130

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rpc): Handle rpc request with empty params #122

fix(rpc): Handle rpc request with empty params #122

tomonari-t commented Jun 6, 2020 •

edited

Loading

coveralls commented Jun 6, 2020 •

edited

Loading

tomonari-t commented Jun 6, 2020

ryanio commented Jun 6, 2020

tomonari-t commented Jun 7, 2020

tomonari-t commented Jun 7, 2020

ryanio commented Jun 7, 2020

holgerd77 Jun 8, 2020

holgerd77 Jun 8, 2020

evertonfraga Jun 9, 2020

tomonari-t Jun 9, 2020

holgerd77 Jun 8, 2020

holgerd77 Jun 8, 2020

holgerd77 Jun 8, 2020

tomonari-t Jun 9, 2020

holgerd77 Jun 8, 2020

holgerd77 Jun 8, 2020

holgerd77 Jun 8, 2020

tomonari-t Jun 9, 2020

tomonari-t commented Jun 10, 2020

holgerd77 commented Jun 10, 2020

holgerd77 commented Jun 10, 2020

tomonari-t commented Jun 10, 2020

tomonari-t commented Jun 10, 2020

	test('should comlete the request with non-params if the method does not require any params', t => {
	test('should work without `params` when it\'s optional', t => {

fix(rpc): Handle rpc request with empty params #122

fix(rpc): Handle rpc request with empty params #122

Conversation

tomonari-t commented Jun 6, 2020 • edited Loading

Why this PR is needed

TODO

coveralls commented Jun 6, 2020 • edited Loading

tomonari-t commented Jun 6, 2020

ryanio commented Jun 6, 2020

tomonari-t commented Jun 7, 2020

tomonari-t commented Jun 7, 2020

ryanio commented Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomonari-t commented Jun 10, 2020

holgerd77 commented Jun 10, 2020

holgerd77 commented Jun 10, 2020

tomonari-t commented Jun 10, 2020

tomonari-t commented Jun 10, 2020

tomonari-t commented Jun 6, 2020 •

edited

Loading

coveralls commented Jun 6, 2020 •

edited

Loading