Add custom HttpStatusCodeProvider to adjust to JSON-RPC 2.0 conventions #1522

marcciosilva · 2021-05-05T22:59:04Z

Description

Since the jsonrpc4j keeps on returning internal error HTTP status codes upon JSON-RPC errors (it shouldn't, since the transport layer protocol should be agnostic to JSON-RPC), we add custom logic to handle them.

Inspired on this, any valid JSON-RPC request should generate a JSON-RPC response, the HTTP status code should be 200. No matter if the response is a success or error response.
Calling a method that hasn't been enabled or a method that doesn't exist are valid JSON-RPC requests, therefore the expected response is a JSON-RPC error (and status code = 200)
Any invalid JSON-RPC request (malformed JSON, etc.) should generate a 400 status code.

Motivation and Context

Issue #1387

How Has This Been Tested?

Unit testing

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Tests for the changes have been added (for bug fixes / features)
Requires Activation Code (Hard Fork)

marcciosilva · 2021-05-06T14:39:08Z

rskj-core/src/main/java/co/rsk/rpc/netty/Web3HttpStatusCodeProvider.java

+import static com.googlecode.jsonrpc4j.ErrorResolver.JsonError;
+import static com.googlecode.jsonrpc4j.ErrorResolver.JsonError.*;
+
+public class Web3HttpStatusCodeProvider implements HttpStatusCodeProvider {


Based on this class (which we used prior to this change).
FYI: version 1.6 of jsonrpc4j doesn't adjust error handling to the JSON-RPC guidelines, that's why I decided not to update it this PR.

marcciosilva · 2021-05-06T14:40:09Z

rskj-core/src/main/java/co/rsk/rpc/netty/Web3HttpStatusCodeProvider.java

+     */
+    @Override
+    public Integer getJsonRpcCode(int httpStatusCode) {
+        throw new UnsupportedOperationException("Cannot possibly determine a single JSON RPC from an HTTP status code");


jsonrpc4j maps each supported HTTP status code to a single JSON-RPC code - this is not entirely correct in my opinion, since a single HTTP code could map to N JSON-RPC codes (this even happens in jsonrpc4j).
Since we don't use this method in rskj, I decided to leave it like this.

Take into account that a JSON RPC request could be an array of valid JSON RPC requests. Is this case covered in the process of responses/responses code?

Since I just swapped the DefaultHttpStatusCodeProvider (from jsonrpc4j library) for a custom implementation that adheres to the same signatures (but mapping HTTP codes differently), the actual processing of requests is not involved (this is only used in the response handling part of the pipeline).

marcciosilva · 2021-05-06T16:18:21Z

pipeline: run

fedejinich · 2021-05-06T18:47:31Z

pipeline:run

ajlopezrsk · 2021-05-06T20:02:06Z

rskj-core/src/main/java/co/rsk/rpc/netty/Web3HttpStatusCodeProvider.java

+     */
+    @Override
+    public Integer getJsonRpcCode(int httpStatusCode) {
+        throw new UnsupportedOperationException("Cannot possibly determine a single JSON RPC from an HTTP status code");


Take into account that a JSON RPC request could be an array of valid JSON RPC requests. Is this case covered in the process of responses/responses code?

ajlopezrsk · 2021-05-06T20:02:59Z

rskj-core/src/main/java/co/rsk/rpc/netty/Web3HttpStatusCodeProvider.java

+    }
+
+    private boolean isBadRequestResultCode(int resultCode) {
+        Optional<JsonError> optionalJsonError = BAD_REQUEST_JSON_ERRORS


Why not a simple contains? Or a map of JSON ERROR to boolean (true == it is a bad request result code)?

I didn't use a contains because it's a list of JsonError and what I receive is an integer (somehow I have to reach the code member variable).
Let me know what you think about the change I just uploaded (instead of using a JsonError list y directly use a collection of the codes, then I just use a contains).

nagarev · 2021-05-07T14:18:26Z

pipeline:run

Vovchyk

lgtm

rskj-core/src/main/java/co/rsk/rpc/netty/Web3HttpStatusCodeProvider.java

rskj-core/src/main/java/co/rsk/rpc/netty/Web3ResultHttpResponseHandler.java

Vovchyk · 2021-05-07T15:02:39Z

rskj-core/src/main/java/co/rsk/rpc/netty/Web3ResultHttpResponseHandler.java

 import static io.netty.handler.codec.http.HttpHeaders.Names.CONTENT_TYPE;
 import static io.netty.handler.codec.http.HttpHeaders.Values.APPLICATION_JSON;

 public class Web3ResultHttpResponseHandler extends SimpleChannelInboundHandler<Web3Result> {

+    private HttpStatusCodeProvider httpStatusCodeProvider;
+
+    public Web3ResultHttpResponseHandler(HttpStatusCodeProvider httpStatusCodeProvider) {


I guess using this 3rd-party interface is perfectly fine here, but as an option it could also be IntFunction<HttpResponseStatus> errorCodeToHttpStatusMapper

I think it's an interesting suggestion, but considering that we already depend on many classes of com.googlecode.jsonrpc4 for JSON-RPC handling, I think it'd be overkill (besides, I guess rskj shouldn't have the responsibility of defining stuff related to this). Is it ok if I leave it like this?

marcciosilva · 2021-05-11T13:43:18Z

pipeline: run

fedejinich

lgtm!
It'd be nice to cover it also in the integration tests

marcciosilva · 2021-05-12T14:29:24Z

pipeline: run

Vovchyk · 2021-06-10T08:15:23Z

pipeline: run

…t mapping logic)

marcciosilva · 2021-06-10T15:36:23Z

pipeline: run

marcciosilva · 2021-06-10T15:36:53Z

pipeline:run

marcciosilva · 2021-06-10T16:23:18Z

pipeline:run

nagarev · 2021-06-10T16:53:39Z

pipeline:run

marcciosilva force-pushed the fix-json-rpc-http-response-status branch from b5a0475 to 382315d Compare May 6, 2021 14:34

marcciosilva marked this pull request as ready for review May 6, 2021 14:34

marcciosilva commented May 6, 2021

View reviewed changes

marcciosilva closed this May 6, 2021

marcciosilva reopened this May 6, 2021

ajlopezrsk reviewed May 6, 2021

View reviewed changes

marcciosilva requested a review from ajlopezrsk May 7, 2021 14:13

Vovchyk previously approved these changes May 7, 2021

View reviewed changes

marcciosilva dismissed Vovchyk’s stale review via fecfb31 May 10, 2021 16:58

marcciosilva requested a review from Vovchyk May 10, 2021 16:58

Vovchyk previously approved these changes May 11, 2021

View reviewed changes

fedejinich previously approved these changes May 11, 2021

View reviewed changes

marcciosilva dismissed stale reviews from fedejinich and Vovchyk via 6e6e16d June 10, 2021 14:21

marcciosilva force-pushed the fix-json-rpc-http-response-status branch from fecfb31 to 6e6e16d Compare June 10, 2021 14:21

Marccio Silva added 3 commits June 10, 2021 12:30

Add custom HttpStatusCodeProvider to adjust to JSON-RPC 2.0 conventions

1b040ed

Address PR suggestion (simplify Web3HttpStatusCodeProvider bad reques…

83eb7a7

…t mapping logic)

Address PR suggestion (minor refactoring)

7a619c1

marcciosilva force-pushed the fix-json-rpc-http-response-status branch from 6e6e16d to 7a619c1 Compare June 10, 2021 15:31

aeidelman approved these changes Jun 10, 2021

View reviewed changes

aeidelman merged commit 33f0f6d into rsksmart:master Jun 10, 2021

marcciosilva deleted the fix-json-rpc-http-response-status branch June 10, 2021 19:57

aeidelman added this to the Iris v3.1.0 milestone Jul 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom HttpStatusCodeProvider to adjust to JSON-RPC 2.0 conventions #1522

Add custom HttpStatusCodeProvider to adjust to JSON-RPC 2.0 conventions #1522

marcciosilva commented May 5, 2021 •

edited

Loading

marcciosilva May 6, 2021

marcciosilva May 6, 2021

ajlopezrsk May 6, 2021

marcciosilva May 7, 2021

marcciosilva commented May 6, 2021

fedejinich commented May 6, 2021

ajlopezrsk May 6, 2021

ajlopezrsk May 6, 2021

marcciosilva May 7, 2021

nagarev commented May 7, 2021

Vovchyk left a comment

Vovchyk May 7, 2021

marcciosilva May 10, 2021

marcciosilva commented May 11, 2021

fedejinich left a comment

marcciosilva commented May 12, 2021

Vovchyk commented Jun 10, 2021

marcciosilva commented Jun 10, 2021

marcciosilva commented Jun 10, 2021

marcciosilva commented Jun 10, 2021

nagarev commented Jun 10, 2021

Add custom HttpStatusCodeProvider to adjust to JSON-RPC 2.0 conventions #1522

Add custom HttpStatusCodeProvider to adjust to JSON-RPC 2.0 conventions #1522

Conversation

marcciosilva commented May 5, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

marcciosilva commented May 6, 2021

fedejinich commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagarev commented May 7, 2021

Vovchyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcciosilva commented May 11, 2021

fedejinich left a comment

Choose a reason for hiding this comment

marcciosilva commented May 12, 2021

Vovchyk commented Jun 10, 2021

marcciosilva commented Jun 10, 2021

marcciosilva commented Jun 10, 2021

marcciosilva commented Jun 10, 2021

nagarev commented Jun 10, 2021

marcciosilva commented May 5, 2021 •

edited

Loading