From d8b12a75dbfdc1d3e62352f0fa815bbbdc685caf Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Tue, 18 Aug 2020 11:43:42 -0400 Subject: [PATCH] rpc: Allow named and positional arguments to be used together It's nice to be able to use named options and positional arguments together. Most shell tools accept both, and python functions combine options and arguments allowing them to be passed with even more flexibility. This change adds support for python's approach so as a motivating example: bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1 Can be shortened to: bitcoin-cli -named createwallet mywallet load_on_startup=1 JSON-RPC standard doesn't have a convention for passing named and positional parameters together, so this implementation makes one up and interprets any unused "args" named parameter as a positional parameter array. --- doc/JSON-RPC-interface.md | 22 ++++++++ doc/release-notes-19762.md | 19 +++++++ src/rpc/client.cpp | 8 ++- src/rpc/server.cpp | 28 +++++++++- src/test/rpc_tests.cpp | 60 +++++++++++++++++++++ test/functional/interface_bitcoin_cli.py | 5 ++ test/functional/rpc_named_arguments.py | 3 ++ test/functional/test_framework/authproxy.py | 6 ++- test/functional/test_framework/test_node.py | 1 - 9 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 doc/release-notes-19762.md diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md index 12807bfb865..ab5db58cdd1 100644 --- a/doc/JSON-RPC-interface.md +++ b/doc/JSON-RPC-interface.md @@ -5,6 +5,28 @@ The headless daemon `bitcoind` has the JSON-RPC API enabled by default, the GUI option. In the GUI it is possible to execute RPC methods in the Debug Console Dialog. +## Parameter passing + +The JSON-RPC server supports both _by-position_ and _by-name_ [parameter +structures](https://www.jsonrpc.org/specification#parameter_structures) +described in the JSON-RPC specification. For extra convenience, to avoid the +need to name every parameter value, all RPC methods accept a named parameter +called `args`, which can be set to an array of initial positional values that +are combined with named values. + +Examples: + +```sh +# "params": ["mywallet", false, false, "", false, false, true] +bitcoin-cli createwallet mywallet false false "" false false true + +# "params": {"wallet_name": "mywallet", "load_on_startup": true} +bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=true + +# "params": {"args": ["mywallet"], "load_on_startup": true} +bitcoin-cli -named createwallet mywallet load_on_startup=true +``` + ## Versioning The RPC interface might change from one major version of Bitcoin Core to the diff --git a/doc/release-notes-19762.md b/doc/release-notes-19762.md new file mode 100644 index 00000000000..4dc45fb2c86 --- /dev/null +++ b/doc/release-notes-19762.md @@ -0,0 +1,19 @@ +JSON-RPC +--- + +All JSON-RPC methods accept a new [named +parameter](JSON-RPC-interface.md#parameter-passing) called `args` that can +contain positional parameter values. This is a convenience to allow some +parameter values to be passed by name without having to name every value. The +python test framework and `bitcoin-cli` tool both take advantage of this, so +for example: + +```sh +bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1 +``` + +Can now be shortened to: + +```sh +bitcoin-cli -named createwallet mywallet load_on_startup=1 +``` diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 8688263ef54..b3434b80c7b 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -277,11 +277,13 @@ UniValue RPCConvertValues(const std::string &strMethod, const std::vector &strParams) { UniValue params(UniValue::VOBJ); + UniValue positional_args{UniValue::VARR}; for (const std::string &s: strParams) { size_t pos = s.find('='); if (pos == std::string::npos) { - throw(std::runtime_error("No '=' in named argument '"+s+"', this needs to be present for every argument (even if it is empty)")); + positional_args.push_back(rpcCvtTable.convert(strMethod, positional_args.size()) ? ParseNonRFCJSONValue(s) : s); + continue; } std::string name = s.substr(0, pos); @@ -296,5 +298,9 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector vargNames = SplitString(argNamePattern, '|'); auto fr = argsIn.end(); @@ -424,6 +432,24 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c argsIn.erase(fr); } else { hole += 1; + if (out.params.empty()) initial_hole_size = hole; + } + } + // If leftover "args" param was found, use it as a source of positional + // arguments and add named arguments after. This is a convenience for + // clients that want to pass a combination of named and positional + // arguments as described in doc/JSON-RPC-interface.md#parameter-passing + auto positional_args{argsIn.extract("args")}; + if (positional_args && positional_args.mapped()->isArray()) { + const bool has_named_arguments{initial_hole_size < (int)argNames.size()}; + if (initial_hole_size < (int)positional_args.mapped()->size() && has_named_arguments) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + argNames[initial_hole_size] + " specified twice both as positional and named argument"); + } + // Assign positional_args to out.params and append named_args after. + UniValue named_args{std::move(out.params)}; + out.params = *positional_args.mapped(); + for (size_t i{out.params.size()}; i < named_args.size(); ++i) { + out.params.push_back(named_args[i]); } } // If there are still arguments in the argsIn map, this is an error. diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index a52530e179c..21ccbe96484 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -17,12 +17,49 @@ #include +static UniValue JSON(std::string_view json) +{ + UniValue value; + BOOST_CHECK(value.read(json.data(), json.size())); + return value; +} + +class HasJSON +{ +public: + explicit HasJSON(std::string json) : m_json(std::move(json)) {} + bool operator()(const UniValue& value) const + { + std::string json{value.write()}; + BOOST_CHECK_EQUAL(json, m_json); + return json == m_json; + }; + +private: + const std::string m_json; +}; + class RPCTestingSetup : public TestingSetup { public: + UniValue TransformParams(const UniValue& params, std::vector arg_names) const; UniValue CallRPC(std::string args); }; +UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector arg_names) const +{ + UniValue transformed_params; + CRPCTable table; + CRPCCommand command{"category", "method", [&](const JSONRPCRequest& request, UniValue&, bool) -> bool { transformed_params = request.params; return true; }, arg_names, /*unique_id=*/0}; + table.appendCommand("method", &command); + JSONRPCRequest request; + request.strMethod = "method"; + request.params = params; + if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished(); + table.execute(request); + return transformed_params; +} + UniValue RPCTestingSetup::CallRPC(std::string args) { std::vector vArgs{SplitString(args, ' ')}; @@ -45,6 +82,29 @@ UniValue RPCTestingSetup::CallRPC(std::string args) BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup) +BOOST_AUTO_TEST_CASE(rpc_namedparams) +{ + const std::vector arg_names{{"arg1", "arg2", "arg3", "arg4", "arg5"}}; + + // Make sure named arguments are transformed into positional arguments in correct places separated by nulls + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg4": 4})"), arg_names).write(), "[null,2,null,4]"); + + // Make sure named and positional arguments can be combined. + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg5": 5, "args": [1, 2], "arg4": 4})"), arg_names).write(), "[1,2,null,4,5]"); + + // Make sure a unknown named argument raises an exception + BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"arg2": 2, "unknown": 6})"), arg_names), UniValue, + HasJSON(R"({"code":-8,"message":"Unknown named parameter unknown"})")); + + // Make sure an overlap between a named argument and positional argument raises an exception + BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"args": [1,2,3], "arg4": 4, "arg2": 2})"), arg_names), UniValue, + HasJSON(R"({"code":-8,"message":"Parameter arg2 specified twice both as positional and named argument"})")); + + // Make sure extra positional arguments can be passed through to the method implemenation, as long as they don't overlap with named arguments. + BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"args": [1,2,3,4,5,6,7,8,9,10]})"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]"); + BOOST_CHECK_EQUAL(TransformParams(JSON(R"([1,2,3,4,5,6,7,8,9,10])"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]"); +} + BOOST_AUTO_TEST_CASE(rpc_rawparams) { // Test raw transaction API argument handling diff --git a/test/functional/interface_bitcoin_cli.py b/test/functional/interface_bitcoin_cli.py index db5564ac50a..9a6366b0835 100755 --- a/test/functional/interface_bitcoin_cli.py +++ b/test/functional/interface_bitcoin_cli.py @@ -84,6 +84,11 @@ class TestBitcoinCli(BitcoinTestFramework): rpc_response = self.nodes[0].getblockchaininfo() assert_equal(cli_response, rpc_response) + self.log.info("Test named arguments") + assert_equal(self.nodes[0].cli.echo(0, 1, arg3=3, arg5=5), ['0', '1', None, '3', None, '5']) + assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, 1, arg1=1) + assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, None, 2, arg1=1) + user, password = get_auth_cookie(self.nodes[0].datadir, self.chain) self.log.info("Test -stdinrpcpass option") diff --git a/test/functional/rpc_named_arguments.py b/test/functional/rpc_named_arguments.py index 41b93129690..cc3ee9efd5b 100755 --- a/test/functional/rpc_named_arguments.py +++ b/test/functional/rpc_named_arguments.py @@ -30,6 +30,9 @@ class NamedArgumentTest(BitcoinTestFramework): assert_equal(node.echo(arg1=1), [None, 1]) assert_equal(node.echo(arg9=None), [None]*10) assert_equal(node.echo(arg0=0,arg3=3,arg9=9), [0] + [None]*2 + [3] + [None]*5 + [9]) + assert_equal(node.echo(0, 1, arg3=3, arg5=5), [0, 1, None, 3, None, 5]) + assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", node.echo, 0, 1, arg1=1) + assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", node.echo, 0, None, 2, arg1=1) if __name__ == '__main__': NamedArgumentTest().main() diff --git a/test/functional/test_framework/authproxy.py b/test/functional/test_framework/authproxy.py index c4ffd1fbf63..dd20b285507 100644 --- a/test/functional/test_framework/authproxy.py +++ b/test/functional/test_framework/authproxy.py @@ -131,10 +131,12 @@ class AuthServiceProxy(): json.dumps(args or argsn, default=EncodeDecimal, ensure_ascii=self.ensure_ascii), )) if args and argsn: - raise ValueError('Cannot handle both named and positional arguments') + params = dict(args=args, **argsn) + else: + params = args or argsn return {'version': '1.1', 'method': self._service_name, - 'params': args or argsn, + 'params': params, 'id': AuthServiceProxy.__id_count} def __call__(self, *args, **argsn): diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 0075fa0996e..04f46a44e00 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -719,7 +719,6 @@ class TestNodeCLI(): """Run bitcoin-cli command. Deserializes returned string as python object.""" pos_args = [arg_to_cli(arg) for arg in args] named_args = [str(key) + "=" + arg_to_cli(value) for (key, value) in kwargs.items()] - assert not (pos_args and named_args), "Cannot use positional arguments and named arguments in the same bitcoin-cli call" p_args = [self.binary, "-datadir=" + self.datadir] + self.options if named_args: p_args += ["-named"]