Skip to content

Commit 436c185

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#27256: refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it
cfbc8a6 refactor: rpc: hide and rename ParseNonRFCJSONValue() (stickies-v) 6c8bde6 test: move coverage on ParseNonRFCJSONValue() to UniValue::read() (stickies-v) Pull request description: Follow-up to bitcoin/bitcoin#26612 (comment). As per bitcoin/bitcoin#26506 (review), `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly: > IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. laanwj pointed this out in bitcoin/bitcoin#9028 (comment) The implementation of `ParseNonRFCJSONValue()` was already [simplified in #26612](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-84c7a7f36362b9724c31e5dec9879b2f81eae0d0addbc9c0933c3558c577de65R259-R263) and [test coverage updated](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-fc0f86b6c3bb23b0e983bcf79d7546d1c9eaa15d6e4d8a7b03b5b85955f585abR292-R312) to ensure behaviour didn't change. To avoid code duplication, we keep the function to throw on invalid input data but rename it to `Parse()` and remove it from the header. The existing test coverage we had on `ParseNonRFCJSONValue()` is moved over to `UniValue::read()`. ACKs for top commit: ryanofsky: Code review ACK cfbc8a6. Only change since last review is adding a new test Tree-SHA512: 89be959d2353af7ace0c1348ba1600b9ac1d3c7b3cf7f0b59f6e005b7fb9d695ce3e8720e1be3cf77fe7e318a4017c880df108928e7179ec50447583d13bc781
2 parents b22408d + cfbc8a6 commit 436c185

File tree

6 files changed

+41
-57
lines changed

6 files changed

+41
-57
lines changed

‎src/rpc/client.cpp‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,14 @@ static const CRPCConvertParam vRPCConvertParams[] =
297297
};
298298
// clang-format on
299299

300+
/** Parse string to UniValue or throw runtime_error if string contains invalid JSON */
301+
static UniValue Parse(std::string_view raw)
302+
{
303+
UniValue parsed;
304+
if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw));
305+
return parsed;
306+
}
307+
300308
class CRPCConvertTable
301309
{
302310
private:
@@ -309,13 +317,13 @@ class CRPCConvertTable
309317
/** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
310318
UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx)
311319
{
312-
return members.count({method, param_idx}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value;
320+
return members.count({method, param_idx}) > 0 ? Parse(arg_value) : arg_value;
313321
}
314322

315323
/** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
316324
UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, const std::string& param_name)
317325
{
318-
return membersByName.count({method, param_name}) > 0 ? ParseNonRFCJSONValue(arg_value) : arg_value;
326+
return membersByName.count({method, param_name}) > 0 ? Parse(arg_value) : arg_value;
319327
}
320328
};
321329

@@ -329,16 +337,6 @@ CRPCConvertTable::CRPCConvertTable()
329337

330338
static CRPCConvertTable rpcCvtTable;
331339

332-
/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null)
333-
* as well as objects and arrays.
334-
*/
335-
UniValue ParseNonRFCJSONValue(std::string_view raw)
336-
{
337-
UniValue parsed;
338-
if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw));
339-
return parsed;
340-
}
341-
342340
UniValue RPCConvertValues(const std::string &strMethod, const std::vector<std::string> &strParams)
343341
{
344342
UniValue params(UniValue::VARR);

‎src/rpc/client.h‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,4 @@ UniValue RPCConvertValues(const std::string& strMethod, const std::vector<std::s
1717
/** Convert named arguments to command-specific RPC representation */
1818
UniValue RPCConvertNamedValues(const std::string& strMethod, const std::vector<std::string>& strParams);
1919

20-
/** Non-RFC4627 JSON parser, accepts internal values (such as numbers, true, false, null)
21-
* as well as objects and arrays.
22-
*/
23-
UniValue ParseNonRFCJSONValue(std::string_view raw);
24-
2520
#endif // BITCOIN_RPC_CLIENT_H

‎src/test/fuzz/parse_univalue.cpp‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,12 +22,9 @@ FUZZ_TARGET_INIT(parse_univalue, initialize_parse_univalue)
2222
const std::string random_string(buffer.begin(), buffer.end());
2323
bool valid = true;
2424
const UniValue univalue = [&] {
25-
try {
26-
return ParseNonRFCJSONValue(random_string);
27-
} catch (const std::runtime_error&) {
28-
valid = false;
29-
return UniValue{};
30-
}
25+
UniValue uv;
26+
if (!uv.read(random_string)) valid = false;
27+
return valid ? uv : UniValue{};
3128
}();
3229
if (!valid) {
3330
return;

‎src/test/fuzz/string.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,10 +66,6 @@ FUZZ_TARGET(string)
6666
const util::Settings settings;
6767
(void)OnlyHasDefaultSectionSetting(settings, random_string_1, random_string_2);
6868
(void)ParseNetwork(random_string_1);
69-
try {
70-
(void)ParseNonRFCJSONValue(random_string_1);
71-
} catch (const std::runtime_error&) {
72-
}
7369
(void)ParseOutputType(random_string_1);
7470
(void)RemovePrefix(random_string_1, random_string_2);
7571
(void)ResolveErrMsg(random_string_1, random_string_2);

‎src/test/rpc_tests.cpp‎

Lines changed: 1 addition & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -300,43 +300,14 @@ BOOST_AUTO_TEST_CASE(rpc_parse_monetary_values)
300300
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.00000001000000")), 1LL); //should pass, cut trailing 0
301301
BOOST_CHECK_THROW(AmountFromValue(ValueFromString("19e-9")), UniValue); //should fail
302302
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19); //should pass, leading 0 is present
303+
BOOST_CHECK_EXCEPTION(AmountFromValue(".19e-6"), UniValue, HasJSON(R"({"code":-3,"message":"Invalid amount"})")); //should fail, no leading 0
303304

304305
BOOST_CHECK_THROW(AmountFromValue(ValueFromString("92233720368.54775808")), UniValue); //overflow error
305306
BOOST_CHECK_THROW(AmountFromValue(ValueFromString("1e+11")), UniValue); //overflow error
306307
BOOST_CHECK_THROW(AmountFromValue(ValueFromString("1e11")), UniValue); //overflow error signless
307308
BOOST_CHECK_THROW(AmountFromValue(ValueFromString("93e+9")), UniValue); //overflow error
308309
}
309310

310-
BOOST_AUTO_TEST_CASE(json_parse_errors)
311-
{
312-
// Valid
313-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0").get_real(), 1.0);
314-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("true").get_bool(), true);
315-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("[false]")[0].get_bool(), false);
316-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("{\"a\": true}")["a"].get_bool(), true);
317-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("{\"1\": \"true\"}")["1"].get_str(), "true");
318-
// Valid, with leading or trailing whitespace
319-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue(" 1.0").get_real(), 1.0);
320-
BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0 ").get_real(), 1.0);
321-
322-
BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error); //should fail, missing leading 0, therefore invalid JSON
323-
BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1);
324-
// Invalid, initial garbage
325-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("[1.0"), std::runtime_error);
326-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("a1.0"), std::runtime_error);
327-
// Invalid, trailing garbage
328-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("1.0sds"), std::runtime_error);
329-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("1.0]"), std::runtime_error);
330-
// Invalid, keys have to be names
331-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("{1: \"true\"}"), std::runtime_error);
332-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("{true: 1}"), std::runtime_error);
333-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("{[1]: 1}"), std::runtime_error);
334-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("{{\"a\": \"a\"}: 1}"), std::runtime_error);
335-
// BTC addresses should fail parsing
336-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W"), std::runtime_error);
337-
BOOST_CHECK_THROW(ParseNonRFCJSONValue("3J98t1WpEZ73CNmQviecrnyiWrnqRhWNL"), std::runtime_error);
338-
}
339-
340311
BOOST_AUTO_TEST_CASE(rpc_ban)
341312
{
342313
BOOST_CHECK_NO_THROW(CallRPC(std::string("clearbanned")));

‎src/univalue/test/object.cpp‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -412,6 +412,33 @@ void univalue_readwrite()
412412

413413
BOOST_CHECK_EQUAL(strJson1, v.write());
414414

415+
// Valid
416+
BOOST_CHECK(v.read("1.0") && (v.get_real() == 1.0));
417+
BOOST_CHECK(v.read("true") && v.get_bool());
418+
BOOST_CHECK(v.read("[false]") && !v[0].get_bool());
419+
BOOST_CHECK(v.read("{\"a\": true}") && v["a"].get_bool());
420+
BOOST_CHECK(v.read("{\"1\": \"true\"}") && (v["1"].get_str() == "true"));
421+
// Valid, with leading or trailing whitespace
422+
BOOST_CHECK(v.read(" 1.0") && (v.get_real() == 1.0));
423+
BOOST_CHECK(v.read("1.0 ") && (v.get_real() == 1.0));
424+
BOOST_CHECK(v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8);
425+
426+
BOOST_CHECK(!v.read(".19e-6")); //should fail, missing leading 0, therefore invalid JSON
427+
// Invalid, initial garbage
428+
BOOST_CHECK(!v.read("[1.0"));
429+
BOOST_CHECK(!v.read("a1.0"));
430+
// Invalid, trailing garbage
431+
BOOST_CHECK(!v.read("1.0sds"));
432+
BOOST_CHECK(!v.read("1.0]"));
433+
// Invalid, keys have to be names
434+
BOOST_CHECK(!v.read("{1: \"true\"}"));
435+
BOOST_CHECK(!v.read("{true: 1}"));
436+
BOOST_CHECK(!v.read("{[1]: 1}"));
437+
BOOST_CHECK(!v.read("{{\"a\": \"a\"}: 1}"));
438+
// BTC addresses should fail parsing
439+
BOOST_CHECK(!v.read("175tWpb8K1S7NmH4Zx6rewF9WQrcZv245W"));
440+
BOOST_CHECK(!v.read("3J98t1WpEZ73CNmQviecrnyiWrnqRhWNL"));
441+
415442
/* Check for (correctly reporting) a parsing error if the initial
416443
JSON construct is followed by more stuff. Note that whitespace
417444
is, of course, exempt. */

0 commit comments

Comments
 (0)