refactor(jsonrpc): Add structured errors to query method in ViewClient and RPC #3944

khorolets · 2021-02-10T16:52:21Z

add some errors to runtime/runtime/state_viewer
introduce QueryError into chain primitives
refactor runtime query method implementation in neard
refactor view_client handle_query method
remove routing for query method (resolves View query times out for older block #3848)

Still need to implement errors on the RPC and Rosetta side. Work in progress.

I'm creating this draft PR to ask for a review of error structs and error messages.

frol

We are on the right track!

chain/chain/src/lib.rs

chain/jsonrpc-primitives/src/types/query.rs

neard/src/runtime/errors.rs

runtime/runtime/src/state_viewer/errors.rs

runtime/runtime/src/state_viewer/mod.rs

chain/client-primitives/src/types.rs

frol · 2021-02-16T12:54:23Z

@khorolets Another reminder as we discussed last week: Add tests covering the error responses (at least the legal ones, no need to test IOErrors and Unexpected errors); see neard/tests/rpc_nodes.rs

frol

Just a few nits, and we are ready to merge it!

chain/chain-primitives/src/error.rs

chain/rosetta-rpc/src/utils.rs

neard/src/runtime/mod.rs

khorolets · 2021-02-18T08:24:37Z

Backward compatible messages from jsonrpc needs to be done to finish this PR

chain/client-primitives/src/types.rs

chain/chain-primitives/src/error.rs

chain/client/src/view_client.rs

chain/chain-primitives/src/error.rs

chain/jsonrpc-primitives/src/types/receipts.rs

frol · 2021-02-18T14:22:32Z

neard/tests/rpc_nodes.rs

+                actix::spawn(async move {
+                    let client = new_client(&format!("http://{}", rpc_addrs[0]));
+                    let query_response = client
+                        .query(near_jsonrpc_primitives::types::query::RpcQueryRequest {
+                            block_reference: near_primitives::types::BlockReference::Finality(
+                                Finality::Final,
+                            ),
+                            request: near_primitives::views::QueryRequest::ViewAccount {
+                                account_id: "test.near".to_string(),
+                            },
+                        })
+                        .await
+                        .unwrap();
+                    assert_eq!(false, true);
+                    System::current().stop();
+                });


todo for @frol: This is blocked on #3901

runtime/runtime/src/state_viewer/errors.rs

@bowenwang1996

…ppen (#3969) Closes #3951 Doesn't affect #3938 because that particular method (`query`) is being refactored right now in #3944 As for the `query` timeout can appear there because of network routing but in my PR which refactors `query` method we totally removed routing (decision made by @bowenwang1996 and @frol) so it seems like timeout from jsonrpc side wouldn't be possible.

khorolets · 2021-02-19T17:27:17Z

@frol I hope I've finalized error structs according to your suggestions. I'm going to deal with backward now.

chain/client-primitives/src/types.rs

chain/client/src/view_client.rs

chain/jsonrpc/src/lib.rs

neard/src/runtime/errors.rs

@bowenwang1996

…ppen (#3969) Closes #3951 Doesn't affect #3938 because that particular method (`query`) is being refactored right now in #3944 As for the `query` timeout can appear there because of network routing but in my PR which refactors `query` method we totally removed routing (decision made by @bowenwang1996 and @frol) so it seems like timeout from jsonrpc side wouldn't be possible.

frol

It is a massive effort! Given there is only one comment from me, I think it is time to mark it is ready for further review.

frol · 2021-02-24T19:36:53Z

chain/jsonrpc/src/lib.rs

+        // This match is used here to give backward compatible error message for specific
+        // error variants. Should be refactored once structured errors fully shipped


😢

I think our only option going forward would be to return both result and error in the same JSON RPC response. (result for backward compatibility, and error for future sanity)

why couldn't we get rid of QueryResponseKind::Error and emulate it here instead?

@bowenwang1996 good point.

@khorolets Can we get rid of the Error variant and move this handling to a separate backward-compatibility-wrapper called on the process_request level? (serde_json::to_value(query_response) can moved into this wrapper)

frol · 2021-02-24T19:46:37Z

chain/jsonrpc-primitives/src/types/query.rs

+pub struct RpcQueryResponse {
+    #[serde(flatten)]
+    pub query_response: near_primitives::views::QueryResponse,
+}


I feel that the nesting is completely unnecessary here. We should either (1) implement our own RpcQueryResponse or (2) just type-alias the views::QueryResponse. For clear separation of concerns and following our previous style, we should consider (1) and also removing the serde serialization from view::QueryResponse.

bowenwang1996 · 2021-02-26T16:42:10Z

chain/client/src/view_client.rs

-                }
-
-                Ok(None)
+        if self.runtime_adapter.cares_about_shard(


I am sorry but please do not try to change the logic here. What you did is not an equivalent transformation.

@bowenwang1996 can you assist here to transform it equivalent, please?

@bowenwang1996 we removed query-routing here following your take in https://github.com/near/nearcore/pull/3204/files#diff-34fd63bb882408a6cd040297ab48d277c848f434f9f72da6696aa17e86099405R212-R230

@khorolets simply use get_chunk_extra as the old code and do not use cares_about_shard to determine whether this node has the latest information about the shard

chain/jsonrpc/src/lib.rs

bowenwang1996 · 2021-02-26T16:49:52Z

chain/jsonrpc/src/lib.rs

+        // This match is used here to give backward compatible error message for specific
+        // error variants. Should be refactored once structured errors fully shipped


why couldn't we get rid of QueryResponseKind::Error and emulate it here instead?

frol · 2021-02-28T11:25:41Z

chain/jsonrpc/src/lib.rs

+        // This match is used here to give backward compatible error message for specific
+        // error variants. Should be refactored once structured errors fully shipped


@bowenwang1996 good point.

@khorolets Can we get rid of the Error variant and move this handling to a separate backward-compatibility-wrapper called on the process_request level? (serde_json::to_value(query_response) can moved into this wrapper)

frol · 2021-02-28T11:28:40Z

neard/tests/rpc_nodes.rs

+        System::builder()
+            .stop_on_panic(true)
+            .run(move || {


hint: see #4013 on how to use run_actix_until_stop (after the PR is merged)

frol · 2021-02-28T11:39:35Z

chain/client/src/view_client.rs

-                }
-
-                Ok(None)
+        if self.runtime_adapter.cares_about_shard(


@bowenwang1996 we removed query-routing here following your take in https://github.com/near/nearcore/pull/3204/files#diff-34fd63bb882408a6cd040297ab48d277c848f434f9f72da6696aa17e86099405R212-R230

…, view state and access key methods return typical errors instead of QueryError

…rror instead of RpcQueryError

…on JSON-RPC side

@frol

…g to dropped routing in handle_query ViewClient method (#4120) In merged PR #3944 I've dropped routing for the `Query` handler in ViewClient (@frol and @bowenwang1996 discussed it and approved this change). After that test `pytest/tests/sanity/transactions.py` started failing. @frol and I investigated it and found out that the fails related to the attempts to test transactions between shards, all the test relies on the querying jsonrpc `query` method to get accounts from different shards and check their balances, but since routing is not present there anymore it's impossible to do it. In this PR I've made the smallest change introducing only one shard and all the accounts on that shard so the test is not failing anymore, but I'm not sure if it not brakes the main idea of the test. @bowenwang1996 @SkidanovAlex could you assist in refactoring this test so it could still be useful by checking what it supposed to be checking, please?

khorolets requested a review from frol February 10, 2021 16:52

khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query branch 2 times, most recently from e913cef to 4131a6a Compare February 11, 2021 16:18

frol approved these changes Feb 11, 2021

View reviewed changes

frol approved these changes Feb 16, 2021

View reviewed changes

khorolets mentioned this pull request Feb 17, 2021

feat(jsonrpc): Add warn log to jsronrpc methods where time out can happen #3969

Merged

khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query branch from ac2b912 to 1cb0e04 Compare February 17, 2021 18:27

frol requested changes Feb 18, 2021

View reviewed changes

khorolets changed the title ~~refactor (rpc): Add structured errors to query method in ViewClient and RPC~~ refactor(jsonrpc): Add structured errors to query method in ViewClient and RPC Feb 19, 2021

frol requested changes Feb 22, 2021

View reviewed changes

chain/client-primitives/src/types.rs Outdated Show resolved Hide resolved

chain/client/src/view_client.rs Outdated Show resolved Hide resolved

chain/jsonrpc/src/lib.rs Outdated Show resolved Hide resolved

neard/src/runtime/errors.rs Outdated Show resolved Hide resolved

khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query branch 2 times, most recently from 28427f5 to 09744a6 Compare February 24, 2021 19:16

frol requested changes Feb 24, 2021

View reviewed changes

khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query branch 2 times, most recently from 0312acf to e94101f Compare February 26, 2021 09:53

khorolets marked this pull request as ready for review February 26, 2021 11:21

khorolets requested review from bowenwang1996, evgenykuzyakov, mfornet, MaksymZavershynskyi and SkidanovAlex as code owners February 26, 2021 11:21

bowenwang1996 reviewed Feb 26, 2021

View reviewed changes

khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query branch from e94101f to 459c1df Compare February 28, 2021 08:44

frol reviewed Feb 28, 2021

View reviewed changes

frol approved these changes Feb 28, 2021

View reviewed changes

khorolets added 18 commits March 9, 2021 16:54

Refactor new error structs to be more explicit. Runtime call function…

cb778de

…, view state and access key methods return typical errors instead of QueryError

Extend missed error variants to be explicit

a461798

Address review suggestion, create dummy for testing rpc query method

4d54c91

Fix some naming issues

bcf4a04

Adjust error structures

09be16f

Update rpc_query tests

8827cf2

Add block_height and block_hash to error variants where it is possible

bbb6dcc

Address review suggestions. Backward compatible resposne from query

06a3b85

Make backward compatibility in query method via near_client::QueryE…

14693b4

…rror instead of RpcQueryError

Fix some tests

8db2e35

Address review suggestion

7c3fb95

Fix tests according to changes in jsonrpc query method refactoring

feba4c7

Drop QueryResponseKind::Error variant and add backward compatibility …

ad18379

…on JSON-RPC side

Remove redundant commented code

8de65b9

Add tests for query method. Address issue with wrong refactor logic

4ddc300

Clean up and make more strict test check

c602e37

run cargo fmt

1aa7ca3

Mark rpc routing tests as ignore

efde9e0

khorolets force-pushed the refactor/structured-errors-view-client-jsonrpc-query branch from 7efe70c to efde9e0 Compare March 9, 2021 14:55

Fix tests

4816a80

khorolets merged commit f68140e into master Mar 9, 2021

khorolets deleted the refactor/structured-errors-view-client-jsonrpc-query branch March 9, 2021 16:18

mikhailOK mentioned this pull request Mar 9, 2021

state_sync.py: Unreachable error occurred #4063

Closed

khorolets mentioned this pull request Mar 16, 2021

refactor(pytest): Update sanity/transactions.py test to pass according to dropped routing in handle_query ViewClient method #4120

Merged

frol mentioned this pull request May 1, 2021

feat(jsonrpc): Implement backwards compatible structured errors #3517

Closed

2 tasks

frol mentioned this pull request May 8, 2021

[Discussion] Remove RPC routing from the protocol #3148

Closed

This was referenced May 29, 2021

Misleading error message when querying on garbage collected data through JSON RPC #4340

Open

Misleading error message when querying account before it was created through JSON RPC #4341

Open

miraclx mentioned this pull request Mar 4, 2022

Unstructured errors returned from query RPC method #6384

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(jsonrpc): Add structured errors to query method in ViewClient and RPC #3944

refactor(jsonrpc): Add structured errors to query method in ViewClient and RPC #3944

khorolets commented Feb 10, 2021 •

edited by frol

Loading

frol left a comment

frol commented Feb 16, 2021

frol left a comment

khorolets commented Feb 18, 2021

frol Feb 18, 2021

khorolets commented Feb 19, 2021

frol left a comment

frol Feb 24, 2021

bowenwang1996 Feb 26, 2021

frol Feb 28, 2021

frol Feb 24, 2021

bowenwang1996 Feb 26, 2021

khorolets Feb 28, 2021

frol Feb 28, 2021

bowenwang1996 Mar 1, 2021

bowenwang1996 Feb 26, 2021

frol Feb 28, 2021

frol Feb 28, 2021

frol Feb 28, 2021

		// This match is used here to give backward compatible error message for specific
		// error variants. Should be refactored once structured errors fully shipped

refactor(jsonrpc): Add structured errors to query method in ViewClient and RPC #3944

refactor(jsonrpc): Add structured errors to query method in ViewClient and RPC #3944

Conversation

khorolets commented Feb 10, 2021 • edited by frol Loading

frol left a comment

Choose a reason for hiding this comment

frol commented Feb 16, 2021

frol left a comment

Choose a reason for hiding this comment

khorolets commented Feb 18, 2021

Choose a reason for hiding this comment

khorolets commented Feb 19, 2021

frol left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khorolets commented Feb 10, 2021 •

edited by frol

Loading