feat(jsonrpc): clearer errors for malformed JSON and invalid params#3702
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3702 +/- ##
==========================================
+ Coverage 76.67% 76.69% +0.01%
==========================================
Files 409 410 +1
Lines 37055 37181 +126
==========================================
+ Hits 28411 28515 +104
- Misses 6668 6679 +11
- Partials 1976 1987 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
rodrodros
left a comment
There was a problem hiding this comment.
Half done PR. Yes now more information is given to the user, but it is given in such way that is not human friendly. Let's improve upon that.
You can look into other RPCs error and get inspiration from those.
|
Here are some of the new errors: {]
^
unexpected ']', expected a string key or '}' [line 1, column 2]{"a" 1}
^
unexpected '1', expected ':' [line 1, column 6]{"a":}
^
unexpected '}', expected a value [line 1, column 6]{"a":1, }
^
unexpected trailing comma before '}' [line 1, column 9][1,2,]
^
unexpected trailing comma before ']' [line 1, column 6]{"id":@}
^
unexpected '@', expected a value [line 1, column 7]{"a": "unterminated
^
unexpected end of input [line 1, column 20]{"jsonrpc": 5, "method": "x", "id": 1}
^
field "jsonrpc" should be string, got number [line 1, column 13]^
unexpected end of input [line 1, column 1] {]
^
unexpected ']', expected a string key or '}' [line 3, column 4]...xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" z}
^
unexpected 'z', expected ',' or '}' [line 1, column 230]{@yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy...
^
unexpected '@', expected a string key or '}' [line 1, column 2] |
|
Claude finished @RafaelGranza's task in 2m 46s —— View job Re-review:
|
|
Claude finished @RafaelGranza's task in 2m 5s —— View job Re-review:
|
|
Claude finished @RafaelGranza's task in 1m 7s —— View job Re-review:
|
rodrodros
left a comment
There was a problem hiding this comment.
Looks good, thank you!
Before merging, could you please swap the column which makes sense for us programmers but might not be for non-programmers. My suggestion is position or similar.
Finally, in the tests, can you please add at least one real Starknet request (multi line) with an error. The examples I saw in the unit tests are limited to 1 to 2 lines.
b479925 to
1cbb5e9
Compare
|
Claude finished @RafaelGranza's task in 3m 25s —— View job Re-review:
|
|
Claude finished @RafaelGranza's task in 3m 14s —— View job Re-review:
|
|
Claude finished @RafaelGranza's task in 3m 24s —— View job Re-review:
|
9399128 to
ad165d0
Compare
|
Claude finished @RafaelGranza's task in 2m 58s —— View job Re-review:
|
|
Claude finished @RafaelGranza's task in 3m 17s —— View job Re-review:
|
|
Claude finished @RafaelGranza's task in 1m 6s —— View job Re-review:
|
Context:
Related to Issue #3581
Before, a malformed JSON request returned only the raw decoder message.
Errors now point at what's wrong.
Also, long lines are truncated to 80 cols, so the marker stays aligned.
Errors it Covers
Examples:
Errors ate python styled.
{"id":@} ^ unexpected '@', expected a value [line 1, column 7]{"jsonrpc": 5, "method": "x", "id": 1} ^ field "jsonrpc" should be string, got number [line 1, column 13]Benchmark:
We no longer copy the whole stream, we only keep the last 512 bytes read. So the overhead stays roughly constant instead of growing with the request size.
Large requests,
main(no buffering) vs this PR:SONIC
When SONIC arrives it won't break.
Its errors already carry position, so it will just work, only not printed as nicely until we add a case for it (see the
TODOinerrorOffset).