rpc: Adjust RPCTypeCheckObj error string

araujo88 commented at 9:40 pm on October 3, 2022: contributor

Unifies the JSON type error strings as mentioned in #26214. Also refer to #25737.

fanquake requested review from maflcko on Oct 3, 2022

fanquake requested review from furszy on Oct 3, 2022

DrahtBot added the label RPC/REST/ZMQ on Oct 3, 2022

in test/functional/wallet_signrawtransactionwithwallet.py:287 in edf89d288a outdated

285                     "vout": 3,
286                     "amount": 1,
287                 }
288             ])
289-            assert_raises_rpc_error(-3, "Missing scriptPubKey", self.nodes[0].signrawtransactionwithwallet, rawtx, [
290+            assert_raises_rpc_error(-3, "JSON value of type null is not of expected type string", self.nodes[0].signrawtransactionwithwallet, rawtx, [

maflcko commented at 7:04 am on October 4, 2022:

I don’t think this needs to change. The previous error message seems clearer and more direct

in test/functional/rpc_mempool_info.py:87 in edf89d288a outdated

84         self.log.info("Invalid vout provided")
85         assert_raises_rpc_error(-8, "Invalid parameter, vout cannot be negative", self.nodes[0].gettxspendingprevout, [{'txid' : txidA, 'vout' : -1}])
86 
87         self.log.info("Invalid txid provided")
88-        assert_raises_rpc_error(-3, "Expected type string for txid, got number", self.nodes[0].gettxspendingprevout, [{'txid' : 42, 'vout' : 0}])
89+

maflcko commented at 7:04 am on October 4, 2022:

why a newline?

araujo88 commented at 12:53 pm on October 4, 2022:

Accidentally committed. I will correct it in the next review commit.

in test/functional/wallet_send.py:349 in edf89d288a outdated

345@@ -346,7 +346,7 @@ def run_test(self):
346 
347         assert_raises_rpc_error(-8, "Use fee_rate (sat/vB) instead of feeRate", w0.send, {w1.getnewaddress(): 1}, 6, "conservative", 1, {"feeRate": 0.01})
348 
349-        assert_raises_rpc_error(-3, "Unexpected key totalFee", w0.send, {w1.getnewaddress(): 1}, 6, "conservative", 1, {"totalFee": 0.01})
350+        assert_raises_rpc_error(-3, "JSON value of type string is not of expected type bool", w0.send, {w1.getnewaddress(): 1}, 6, "conservative", 1, {"totalFee": 0.01})

maflcko commented at 7:06 am on October 4, 2022:

The new error message is simply wrong and unrelated??

araujo88 commented at 12:55 pm on October 4, 2022:

The opened issue referred to the unification of the error string. I might have understood it incorrectly, but from my interpreration the idea is to use the same error message for all JSON format errors.

maflcko commented at 7:07 am on October 4, 2022: member

I don’t follow the changes. The changes shouldn’t change any logic, but only adjust a format string

araujo88 commented at 12:56 pm on October 4, 2022: contributor

I don’t follow the changes. The changes shouldn’t change any logic, but only adjust a format string

I might have misunderstood the issue, but it mentions the unification of the error string.

in src/rpc/util.cpp:69 in edf89d288a outdated

67-
68+        if (!fAllowNull && v.isNull()) {
69+            RPCTypeCheckArgument(v, t.second);
70+        }
71         if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {
72-            std::string err = strprintf("Expected type %s for %s, got %s",

maflcko commented at 1:03 pm on October 4, 2022:

Only this string needs to be adjusted, per the created issue. Other strings should stay untouched.

maflcko commented at 1:09 pm on October 4, 2022:

Sorry if this was unclear, but only the type error string should be adjusted, not the error string for missing fields or unexpected fields, or other error strings.

in src/rpc/util.cpp:67 in edf89d288a outdated

61@@ -62,24 +62,20 @@ void RPCTypeCheckObj(const UniValue& o,
62 {
63     for (const auto& t : typesExpected) {
64         const UniValue& v = find_value(o, t.first);
65-        if (!fAllowNull && v.isNull())
66-            throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing %s", t.first));
67-
68+        if (!fAllowNull && v.isNull()) {
69+            RPCTypeCheckArgument(v, t.second);
70+        }

furszy commented at 1:28 pm on October 4, 2022:

just need to revert this lines. The other one is ok.

araujo88 force-pushed on Oct 4, 2022

in test/functional/rpc_fundrawtransaction.py:796 in 8a495b9473 outdated

792@@ -793,7 +793,7 @@ def test_option_feerate(self):
793 
794         self.log.info("Test fundrawtxn with invalid estimate_mode settings")
795         for k, v in {"number": 42, "object": {"foo": "bar"}}.items():
796-            assert_raises_rpc_error(-3, "Expected type string for estimate_mode, got {}".format(k),
797+            assert_raises_rpc_error(-3, "JSON value of type {} is not of expected type string".format(k),

maflcko commented at 5:00 pm on October 4, 2022:

Could update to f-strings while touching?

araujo88 commented at 5:09 pm on October 4, 2022:

Wouldn’t be better if this is refactored in another issue/branch? There are several test files which don’t use f-strings.

maflcko commented at 5:16 pm on October 4, 2022:

No, I was only suggesting to use f-strings for the lines that you are touching anyway. The others can be kept as-is

maflcko approved

maflcko commented at 5:00 pm on October 4, 2022: member

Thanks. Lgtm, feel free to ignore the nit

maflcko commented at 5:59 pm on October 4, 2022: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

araujo88 force-pushed on Oct 4, 2022

in src/rpc/util.cpp:69 in 1640a5e5a0 outdated

65@@ -66,9 +66,7 @@ void RPCTypeCheckObj(const UniValue& o,
66             throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing %s", t.first));
67 
68         if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {
69-            std::string err = strprintf("Expected type %s for %s, got %s",
70-                uvTypeName(t.second.type), t.first, uvTypeName(v.type()));
71-            throw JSONRPCError(RPC_TYPE_ERROR, err);
72+            RPCTypeCheckArgument(v, t.second);

furszy commented at 1:23 am on October 5, 2022:

The check is duplicated in this way:

First statement

0if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {

Then you call RPCTypeCheckArgument which contains another:

0if (!typeExpected.typeAny && value.type() != typeExpected.type) {

araujo88 force-pushed on Oct 5, 2022

in src/rpc/util.cpp:69 in db218795e3 outdated

64@@ -65,10 +65,8 @@ void RPCTypeCheckObj(const UniValue& o,
65         if (!fAllowNull && v.isNull())
66             throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing %s", t.first));
67 
68-        if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {
69-            std::string err = strprintf("Expected type %s for %s, got %s",
70-                uvTypeName(t.second.type), t.first, uvTypeName(v.type()));
71-            throw JSONRPCError(RPC_TYPE_ERROR, err);
72+        if (!(fAllowNull && v.isNull())) {
73+            RPCTypeCheckArgument(v, t.second);

furszy commented at 9:41 pm on October 6, 2022:

nit: maybe now that we are here, could write it a bit easier to read.

0if (fAllowNull && v.isNull()) continue;
1RPCTypeCheckArgument(v, t.second);

furszy commented at 10:03 pm on October 6, 2022: member

Code ACK db218795

Need to cleanup the commit message, it’s currently saying “rpc: Adjust RPCTypeCheckObj error string” three times.

araujo88 force-pushed on Oct 7, 2022

furszy commented at 1:58 pm on October 7, 2022: member

Something that #26214 isn’t stating is that, with this change, we will remove the name of the invalid arg in the returned error description.

e.g. the user will receive a general “JSON value of type string is not of expected type number”. When before, was receiving the name of the invalid parameter: “Expected type number for conf_target, got string”.

I’m more inclined to add the invalid parameter name into the RPCTypeCheckArgument error string rather than remove it from RPCTypeCheckObj. It’s useful to quickly detect which arg type is wrong when you type a long command.

This refactor could be done in a follow-up PR, but if we are agree over this point, then instead of calling RPCTypeCheckArgument inside RPCTypeCheckObj, we can just change the returned string to be:

0strprintf("JSON value of type %s for arg %s is not of expected type %s", uvTypeName(v.type()),  t.first, uvTypeName(t.second.type)));

araujo88 commented at 3:00 pm on October 7, 2022: contributor

Something that #26214 isn’t stating is that, with this change, we will remove the name of the invalid arg in the returned error description.

e.g. the user will receive a general “JSON value of type string is not of expected type number”. When before, was receiving the name of the invalid parameter: “Expected type number for conf_target, got string”.

I’m more inclined to add the invalid parameter name into the RPCTypeCheckArgument error string rather than remove it from RPCTypeCheckObj. It’s useful to quickly detect which arg type is wrong when you type a long command.

This refactor could be done in a follow-up PR, but if we are agree over this point, then instead of calling RPCTypeCheckArgument inside RPCTypeCheckObj, we can just change the returned string to be:
0strprintf("JSON value of type %s for arg %s is not of expected type %s", uvTypeName(v.type()),  t.first, uvTypeName(t.second.type)));

This modification would modify the signature of the function void RPCTypeCheckArgument(const UniValue& value, const UniValueType& typeExpected) to void RPCTypeCheckArgument(const UniValue& value, const std::pair<const std::string, const UniValueType> & typeExpected) and this would conflict with its use in RPCTypeCheck. I can visualize two ways of handling this: either by modifying RPCTypeCheckArgument to accept different argument types and handle them internally or modifying the RPCTypeCheck function itself. The latter would involve bigger changes in the code.

maflcko commented at 3:43 pm on October 7, 2022: member

I’d say to just leave the code logic as it was before and only change the string

araujo88 force-pushed on Oct 8, 2022

araujo88 commented at 3:18 am on October 8, 2022: contributor

The file feature_backwards_compatibility.py raised the following error while running the ci test. I’m not sure about how to handle and interpret it. Locally on my machine all tests have passed.

 0131/252 - feature_backwards_compatibility.py --descriptors
 1 failed, Duration: 15 s
 2stdout:
 32022-10-08T01:49:20.923000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-build/ci/scratch/test_runner/test_runner_₿_🏃_20221008_014101/feature_backwards_compatibility_117
 42022-10-08T01:49:26.955000Z TestFramework (INFO): Test wallet backwards compatibility...
 52022-10-08T01:49:32.888000Z TestFramework (INFO): Test wallet upgrade path...
 62022-10-08T01:49:33.602000Z TestFramework (INFO): Stopping nodes
 7stderr:
 8Traceback (most recent call last):
 9  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/feature_backwards_compatibility.py", line 329, in <module>
10    BackwardsCompatibilityTest().main()
11  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 156, in main
12    exit_code = self.shutdown()
13  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 311, in shutdown
14    self.stop_nodes()
15  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 571, in stop_nodes
16    node.wait_until_stopped()
17  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 384, in wait_until_stopped
18    wait_until_helper(self.is_node_stopped, timeout=timeout, timeout_factor=self.timeout_factor)
19  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 270, in wait_until_helper
20    if predicate():
21  File "/tmp/cirrus-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 375, in is_node_stopped
22    "Node returned non-zero exit code (%d) when stopping" % return_code)
23AssertionError: [node 9] Node returned non-zero exit code (-6) when stopping

in src/rpc/util.cpp:68 in e0a2cbe40c outdated

64@@ -65,11 +65,8 @@ void RPCTypeCheckObj(const UniValue& o,
65         if (!fAllowNull && v.isNull())
66             throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing %s", t.first));
67 
68-        if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull()))) {

maflcko commented at 7:42 am on October 10, 2022:

Any reason to remove the {}? According to the dev notes we want them

aureleoules commented at 8:08 am on October 10, 2022:

Any reason to remove the {}? According to the dev notes we want them

Yes, this would also reduce the diff.

in src/rpc/util.cpp:69 in e0a2cbe40c outdated

69-            std::string err = strprintf("Expected type %s for %s, got %s",
70-                uvTypeName(t.second.type), t.first, uvTypeName(v.type()));
71-            throw JSONRPCError(RPC_TYPE_ERROR, err);
72-        }
73+        if (!(t.second.typeAny || v.type() == t.second.type || (fAllowNull && v.isNull())))
74+            throw JSONRPCError(RPC_TYPE_ERROR, strprintf("JSON value of type %s for arg %s is not of expected type %s", uvTypeName(v.type()),  t.first, uvTypeName(t.second.type)));

maflcko commented at 7:43 am on October 10, 2022:

Instead of arg, it might be better to use “field” or “key”, as JSON dictionary entries are not arguments, are they?

maflcko approved

aureleoules approved

aureleoules commented at 8:10 am on October 10, 2022: member

ACK e0a2cbe40ce51079472593330f8de39c78397a23 LGTM

Adjust RPCTypeCheckObj error string 2dede9f675

araujo88 force-pushed on Oct 10, 2022

furszy approved

furszy commented at 1:38 pm on October 11, 2022: member

ACK 2dede9f6

DrahtBot commented at 5:20 am on November 12, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26485 (RPC: Add support for name-only parameters by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko merged this on Nov 14, 2022

maflcko closed this on Nov 14, 2022

sidhujag referenced this in commit f165af6302 on Nov 14, 2022

bitcoin locked this on Nov 14, 2023

araujo88 deleted the branch on Jan 22, 2024

rpc: Adjust RPCTypeCheckObj error string #26240

Conflicts