Skip to content

Commit fa92912

Browse files
author
MarcoFalke
committed
rpc: Use RPCHelpMan for check-rpc-mappings linter
1 parent faf8356 commit fa92912

File tree

7 files changed

+106
-164
lines changed

7 files changed

+106
-164
lines changed

ci/lint/06_script.sh

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@ test/lint/git-subtree-check.sh src/univalue
2121
test/lint/git-subtree-check.sh src/leveldb
2222
test/lint/git-subtree-check.sh src/crc32c
2323
test/lint/check-doc.py
24-
test/lint/check-rpc-mappings.py .
2524
test/lint/lint-all.sh
2625

2726
if [ "$CIRRUS_REPO_FULL_NAME" = "bitcoin/bitcoin" ] && [ -n "$CIRRUS_CRON" ]; then

src/rpc/server.cpp

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,13 @@ static RPCHelpMan help()
144144
[&](const RPCHelpMan& self, const JSONRPCRequest& jsonRequest) -> UniValue
145145
{
146146
std::string strCommand;
147-
if (jsonRequest.params.size() > 0)
147+
if (jsonRequest.params.size() > 0) {
148148
strCommand = jsonRequest.params[0].get_str();
149+
}
150+
if (strCommand == "dump_all_command_conversions") {
151+
// Used for testing only, undocumented
152+
return tableRPC.dumpArgMap();
153+
}
149154

150155
return tableRPC.help(strCommand, jsonRequest);
151156
},
@@ -479,6 +484,18 @@ std::vector<std::string> CRPCTable::listCommands() const
479484
return commandList;
480485
}
481486

487+
UniValue CRPCTable::dumpArgMap() const
488+
{
489+
UniValue ret{UniValue::VARR};
490+
for (const auto& cmd : mapCommands) {
491+
for (const auto& c : cmd.second) {
492+
const auto help = RpcMethodFnType(c->unique_id)();
493+
help.AppendArgMap(ret);
494+
}
495+
}
496+
return ret;
497+
}
498+
482499
void RPCSetTimerInterfaceIfUnset(RPCTimerInterface *iface)
483500
{
484501
if (!timerInterface)

src/rpc/server.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,10 @@ class CRPCTable
147147
*/
148148
std::vector<std::string> listCommands() const;
149149

150+
/**
151+
* Return all named arguments that need to be converted by the client from string to another JSON type
152+
*/
153+
UniValue dumpArgMap() const;
150154

151155
/**
152156
* Appends a CRPCCommand to the dispatch table.

src/rpc/util.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -549,6 +549,24 @@ std::string RPCHelpMan::ToString() const
549549
return ret;
550550
}
551551

552+
void RPCHelpMan::AppendArgMap(UniValue& arr) const
553+
{
554+
for (int i{0}; i < int(m_args.size()); ++i) {
555+
const auto& arg = m_args.at(i);
556+
std::vector<std::string> arg_names;
557+
boost::split(arg_names, arg.m_names, boost::is_any_of("|"));
558+
for (const auto& arg_name : arg_names) {
559+
UniValue map{UniValue::VARR};
560+
map.push_back(m_name);
561+
map.push_back(i);
562+
map.push_back(arg_name);
563+
map.push_back(arg.m_type == RPCArg::Type::STR ||
564+
arg.m_type == RPCArg::Type::STR_HEX);
565+
arr.push_back(map);
566+
}
567+
}
568+
}
569+
552570
std::string RPCArg::GetFirstName() const
553571
{
554572
return m_names.substr(0, m_names.find("|"));

src/rpc/util.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -336,6 +336,8 @@ class RPCHelpMan
336336
RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
337337

338338
std::string ToString() const;
339+
/** Append the named args that need to be converted from string to another JSON type */
340+
void AppendArgMap(UniValue& arr) const;
339341
UniValue HandleRequest(const JSONRPCRequest& request)
340342
{
341343
Check(request);

test/functional/rpc_help.py

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,39 @@
77
from test_framework.test_framework import BitcoinTestFramework
88
from test_framework.util import assert_equal, assert_raises_rpc_error
99

10+
from collections import defaultdict
1011
import os
12+
import re
13+
14+
15+
def parse_string(s):
16+
assert s[0] == '"'
17+
assert s[-1] == '"'
18+
return s[1:-1]
19+
20+
21+
def process_mapping(fname):
22+
"""Find and parse conversion table in implementation file `fname`."""
23+
cmds = []
24+
in_rpcs = False
25+
with open(fname, "r", encoding="utf8") as f:
26+
for line in f:
27+
line = line.rstrip()
28+
if not in_rpcs:
29+
if line == 'static const CRPCConvertParam vRPCConvertParams[] =':
30+
in_rpcs = True
31+
else:
32+
if line.startswith('};'):
33+
in_rpcs = False
34+
elif '{' in line and '"' in line:
35+
m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
36+
assert m, 'No match to table expression: %s' % line
37+
name = parse_string(m.group(1))
38+
idx = int(m.group(2))
39+
argname = parse_string(m.group(3))
40+
cmds.append((name, idx, argname))
41+
assert not in_rpcs and cmds
42+
return cmds
1143

1244

1345
class HelpRpcTest(BitcoinTestFramework):
@@ -16,11 +48,43 @@ def set_test_params(self):
1648
self.supports_cli = False
1749

1850
def run_test(self):
51+
self.test_client_conversion_table()
1952
self.test_categories()
2053
self.dump_help()
2154
if self.is_wallet_compiled():
2255
self.wallet_help()
2356

57+
def test_client_conversion_table(self):
58+
file_conversion_table = os.path.join(self.config["environment"]["SRCDIR"], 'src', 'rpc', 'client.cpp')
59+
mapping_client = process_mapping(file_conversion_table)
60+
# Ignore echojson in client table
61+
mapping_client = [m for m in mapping_client if m[0] != 'echojson']
62+
63+
mapping_server = self.nodes[0].help("dump_all_command_conversions")
64+
# Filter all RPCs whether they need conversion
65+
mapping_server_conversion = [tuple(m[:3]) for m in mapping_server if not m[3]]
66+
67+
# Only check if all RPC methods have been compiled (i.e. wallet is enabled)
68+
if self.is_wallet_compiled() and sorted(mapping_client) != sorted(mapping_server_conversion):
69+
raise AssertionError("RPC client conversion table ({}) and RPC server named arguments mismatch!\n{}".format(
70+
file_conversion_table,
71+
set(mapping_client).symmetric_difference(mapping_server_conversion),
72+
))
73+
74+
# Check for conversion difference by argument name.
75+
# It is preferable for API consistency that arguments with the same name
76+
# have the same conversion, so bin by argument name.
77+
all_methods_by_argname = defaultdict(list)
78+
converts_by_argname = defaultdict(list)
79+
for m in mapping_server:
80+
all_methods_by_argname[m[2]].append(m[0])
81+
converts_by_argname[m[2]].append(m[3])
82+
83+
for argname, convert in converts_by_argname.items():
84+
if all(convert) != any(convert):
85+
# Only allow dummy to fail consistency check
86+
assert argname == 'dummy', ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
87+
2488
def test_categories(self):
2589
node = self.nodes[0]
2690

test/lint/check-rpc-mappings.py

Lines changed: 0 additions & 162 deletions
This file was deleted.

0 commit comments

Comments
 (0)