rpc: Throw more user friendly arg type check error (1.5/2) #26929

maflcko commented at 11:12 AM on January 20, 2023: member

The arg type check error doesn't list which arg (position or name) failed. Fix that.

DrahtBot commented at 11:12 AM on January 20, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label RPC/REST/ZMQ on Jan 20, 2023

fanquake requested review from ajtowns on Jan 20, 2023

fanquake commented at 11:22 AM on January 20, 2023: member

https://github.com/bitcoin/bitcoin/pull/26929/checks?check_run_id=10776272066

/usr/bin/ccache g++ -m32 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config  -fmacro-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -isystem /tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include -I/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/  -fdebug-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection   -Werror    -fno-extended-identifiers -fvisibility=hidden -fPIE -pipe -std=c++17 -O2  -c -o rpc/libbitcoin_common_a-util.o `test -f 'rpc/util.cpp' || echo './'`rpc/util.cpp
rpc/util.cpp: In function ‘std::optional<UniValue::VType> ExpectedType(RPCArg::Type)’:
rpc/util.cpp:717:1: error: control reaches end of non-void function [-Werror=return-type]
 }
 ^

maflcko force-pushed on Jan 20, 2023

in src/rpc/util.h:216 in fa7562ea7c outdated

 213 | -    void MatchesType(const UniValue& request) const;
 214 | +    /**
 215 | +     * Check whether the request JSON type matches.
 216 | +     * Returns true if type matches, or object describing error(s) if not.
 217 | +     */
 218 | +    UniValue MatchesType(const UniValue& request) const;

stickies-v commented at 12:04 PM on January 20, 2023:

Feels like a util::Result return type would be more appropriate here?

maflcko commented at 12:09 PM on January 20, 2023:

Not sure. This would be util::Result<void, UniValue>, which is not yet available. I could also use std::optional<UniValue>, but not sure if it is worth it.

stickies-v commented at 12:34 PM on January 20, 2023:

Yeah it would benefit from #25665 but could do it with today's implementation already as well. I didn't realize we need a UniValue object (instead of just the string) in HandleRequest, though, so that makes this a bit more verbose than I hoped. Just found it a bit confusing to have a UniValue act as a variant, but that's probably personal preference.

No strong view either way, just thinking out loud. Feel free to close.

diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 5f44b82aa..30528bd09 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -11,6 +11,7 @@
 #include <script/signingprovider.h>
 #include <tinyformat.h>
 #include <util/check.h>
+#include <util/result.h>
 #include <util/strencodings.h>
 #include <util/string.h>
 #include <util/system.h>
@@ -559,9 +560,9 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
     UniValue arg_mismatch{UniValue::VOBJ};
     for (size_t i{0}; i < m_args.size(); ++i) {
         const auto& arg{m_args.at(i)};
-        UniValue match{arg.MatchesType(request.params[i])};
-        if (!match.isTrue()) {
-            arg_mismatch.pushKV(strprintf("Position %s (%s)", i, arg.m_names), std::move(match));
+        auto match{arg.MatchesType(request.params[i])};
+        if (!match) {
+            arg_mismatch.pushKV(strprintf("Position %s (%s)", i, arg.m_names), UniValue{UniValue::VType::VSTR, util::ErrorString(match).original});
         }
     }
     if (!arg_mismatch.empty()) {
@@ -717,7 +718,7 @@ static std::optional<UniValue::VType> ExpectedType(RPCArg::Type type)
     NONFATAL_UNREACHABLE();
 }
 
-UniValue RPCArg::MatchesType(const UniValue& request) const
+util::Result<bool> RPCArg::MatchesType(const UniValue& request) const
 {
     if (m_opts.skip_type_check) return true;
     if (IsOptional() && request.isNull()) return true;
@@ -725,7 +726,7 @@ UniValue RPCArg::MatchesType(const UniValue& request) const
     if (!exp_type) return true; // nothing to check
 
     if (*exp_type != request.getType()) {
-        return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type));
+        return util::Error{Untranslated(strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type)))};
     }
     return true;
 }
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 0d60e29fa..c374ee528 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -16,6 +16,7 @@
 #include <script/standard.h>
 #include <univalue.h>
 #include <util/check.h>
+#include <util/result.h>
 
 #include <string>
 #include <variant>
@@ -213,7 +214,7 @@ struct RPCArg {
      * Check whether the request JSON type matches.
      * Returns true if type matches, or object describing error(s) if not.
      */
-    UniValue MatchesType(const UniValue& request) const;
+    util::Result<bool> MatchesType(const UniValue& request) const;
 
     /** Return the first of all aliases */
     std::string GetFirstName() const;

</details>

stickies-v commented at 12:37 PM on January 20, 2023:

Although I think with #25665 merged I'd definitely prefer having the util::Result<void, UniValue> return type, just for readability.

maflcko commented at 12:44 PM on January 20, 2023:

Your diff doesn't work, because the return type is util::Result<bool>, but the function needs to return an UniValue object somehow. (To make the calling code less verbose and because it is needed for follow up 2/2)

stickies-v commented at 2:03 PM on January 20, 2023:

I think my diff does work? I'm not sure what the follow up and its requirements are, but the diff compiles, and seems to work fine e.g. for this call. But I agree that making the calling code more verbose is not ideal, so probably best to drop this approach.

<details> <summary>./src/bitcoin-cli -signet getmempoolancestors tx 5</summary>

error code: -3
error message:
Wrong type passed:
{
    "Position 1 (verbose)": "JSON value of type number is not of expected type bool"
}

</details>

I don't mean to bikeshed, but here's another approach without util::Result that may be more appropriate by returning std::pair<bool, UniValue>. Still slightly more verbose than currently, but could be worth it if compatible with 2/2? I'll give up after this :-D

diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 5f44b82aa..e0c6aec67 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -559,9 +559,9 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
     UniValue arg_mismatch{UniValue::VOBJ};
     for (size_t i{0}; i < m_args.size(); ++i) {
         const auto& arg{m_args.at(i)};
-        UniValue match{arg.MatchesType(request.params[i])};
-        if (!match.isTrue()) {
-            arg_mismatch.pushKV(strprintf("Position %s (%s)", i, arg.m_names), std::move(match));
+        auto [match, error] = arg.MatchesType(request.params[i]);
+        if (!match) {
+            arg_mismatch.pushKV(strprintf("Position %s (%s)", i, arg.m_names), std::move(error.value()));
         }
     }
     if (!arg_mismatch.empty()) {
@@ -717,17 +717,17 @@ static std::optional<UniValue::VType> ExpectedType(RPCArg::Type type)
     NONFATAL_UNREACHABLE();
 }
 
-UniValue RPCArg::MatchesType(const UniValue& request) const
+std::pair<bool, std::optional<UniValue>> RPCArg::MatchesType(const UniValue& request) const
 {
-    if (m_opts.skip_type_check) return true;
-    if (IsOptional() && request.isNull()) return true;
+    if (m_opts.skip_type_check) return {true, std::nullopt};
+    if (IsOptional() && request.isNull()) return {true, std::nullopt};
     const auto exp_type{ExpectedType(m_type)};
-    if (!exp_type) return true; // nothing to check
+    if (!exp_type) return {true, std::nullopt}; // nothing to check
 
     if (*exp_type != request.getType()) {
-        return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type));
+        return {false, strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type))};
     }
-    return true;
+    return {true, std::nullopt};
 }
 
 std::string RPCArg::GetFirstName() const
diff --git a/src/rpc/util.h b/src/rpc/util.h
index 0d60e29fa..2d5f882fe 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -213,7 +213,7 @@ struct RPCArg {
      * Check whether the request JSON type matches.
      * Returns true if type matches, or object describing error(s) if not.
      */
-    UniValue MatchesType(const UniValue& request) const;
+    std::pair<bool, std::optional<UniValue>> MatchesType(const UniValue& request) const;
 
     /** Return the first of all aliases */
     std::string GetFirstName() const;

</details>

maflcko commented at 2:13 PM on January 20, 2023:

The return value in 2/2 needs to be of a type that can hold the nested types of a JSON argument, so UniValue seems a good fit.

std::pair<bool, UniValue>

yes, that works, but seems worse than just std::optional<UniValue>.

stickies-v commented at 2:25 PM on January 20, 2023:

but seems worse than just std::optional<UniValue>

From a code brevity perspective, yes, but from a readability perspective I don't think so, because then boolean evaluation of MatchesType is the negation of what the name implies.

maflcko commented at 2:45 PM on January 20, 2023:

Ok, but then I'd prefer UniValue over std::pair<bool, UniValue>

stickies-v commented at 12:05 PM on January 20, 2023: contributor

Concept ACK for better error feedback

maflcko force-pushed on Jan 20, 2023

rpc: Throw more user friendly arg type check error fafeddfe0e

maflcko force-pushed on Jan 20, 2023

in test/functional/rpc_blockchain.py:443 in fafeddfe0e

 438 | +                "Position 1 (nblocks)": "JSON value of type string is not of expected type number",
 439 | +                "Position 2 (height)": "JSON value of type array is not of expected type number"
 440 | +            }
 441 | +            """).strip(),
 442 | +            lambda: self.nodes[0].getnetworkhashps("a", []),
 443 | +        )

stickies-v commented at 1:46 PM on January 25, 2023:

I don't think this is an appropriate place for this test?

E.g. renaming rpc_named_arguments.py -> rpc_arguments.py and adding it there seems more logical to me.

maflcko commented at 2:04 PM on January 25, 2023:

Why not? Every other RPC that has missing-arg or wrong-type checks is also called in the rpc-specific test file?

Examples:

git grep 'is not of expected type' test

For example in this file:

test/functional/rpc_blockchain.py:        assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", self.nodes[0].getchaintxstats, '')
test/functional/rpc_blockchain.py:        assert_raises_rpc_error(-3, "JSON value of type number is not of expected type string", self.nodes[0].getchaintxstats, blockhash=0)
test/functional/rpc_blockchain.py:        assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", node.getblock, blockhash, "2")

stickies-v commented at 2:16 PM on January 25, 2023:

Oh yes, I'm not arguing against checking for an RPC's specific argument types within the RPC-specific test file in general, that can help with regressions etc.

Since this PR is about type checking in general (and not about getnetworkhashps), I think it'd be good to have a test that checks the correct error message is raised - and that should be a test dedicated to just that? If for some unrelated reason in the future _test_getnetworkhashps() gets deleted because the method is deprecated, we'd unintentionally remove test coverage on this PR too.

maflcko commented at 2:25 PM on January 25, 2023:

Ok, will add more tests in the follow-up

stickies-v approved

stickies-v commented at 1:52 PM on January 25, 2023: contributor

ACK fafeddfe0e6748e9769ad3dd526a6c0eaf6f4aac - although I think the functional test isn't in a logical place (but not blocking)

fanquake merged this on Jan 25, 2023

fanquake closed this on Jan 25, 2023

sidhujag referenced this in commit a2bc1cf413 on Jan 25, 2023

maflcko deleted the branch on Jan 26, 2023

bitcoin locked this on Jan 26, 2024