refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it

stickies-v commented at 2:11 pm on March 14, 2023: contributor

Follow-up to #26612 (comment). As per #26506#pullrequestreview-1211984059, 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 #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 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().

DrahtBot commented at 2:11 pm on March 14, 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	ryanofsky

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 Refactoring on Mar 14, 2023

stickies-v force-pushed on Mar 14, 2023

in src/univalue/test/object.cpp:424 in df460948ac outdated

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 "));

maflcko commented at 10:38 am on March 16, 2023:

missing check of value

stickies-v commented at 11:23 am on March 16, 2023:

Left it out because I didn’t want to test AmountFromValue() here, but I could indeed still just check the double value directly. Fixed, thanks.

maflcko commented at 1:24 pm on April 16, 2024:

You can’t check equality on a floating point value that can not be represented exactly.

Compilers are free to pick whatever precision and rounding they want. They don’t even have to be self-consistent. See https://eel.is/c++draft/expr.const#15

Please either remove the test, or restore it so that this works on integral values via AmountFromValue.

Otherwise, the test may fail on some compilers:

0# ./src/univalue/test/object 
1object: univalue/test/object.cpp:424: void univalue_readwrite(): Assertion `v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8' failed.
2Aborted (core dumped)

maflcko commented at 2:43 pm on April 16, 2024:

Fixed in #29892

stickies-v force-pushed on Mar 16, 2023

in src/univalue/test/object.cpp:424 in 104e482729 outdated

420@@ -421,7 +421,7 @@ void univalue_readwrite()
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 "));
425+    BOOST_CHECK(v.read("0.00000000000000000000000000000000000001e+30 ") && v.get_real() == 1e-8);

maflcko commented at 11:25 am on March 16, 2023:

wrong commit

stickies-v commented at 11:36 am on March 16, 2023:

rush, I must not. fixed - sorry.

stickies-v force-pushed on Mar 16, 2023

fanquake requested review from ryanofsky on Mar 19, 2023

in src/test/rpc_tests.cpp:301 in 6e28dedcd6 outdated

296-    // Valid, with leading or trailing whitespace
297-    BOOST_CHECK_EQUAL(ParseNonRFCJSONValue(" 1.0").get_real(), 1.0);
298-    BOOST_CHECK_EQUAL(ParseNonRFCJSONValue("1.0 ").get_real(), 1.0);
299-
300-    BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error); //should fail, missing leading 0, therefore invalid JSON
301-    BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1);

ryanofsky commented at 3:35 pm on March 23, 2023:

In commit “test: move coverage on ParseNonRFCJSONValue() to UniValue::read()” (6e28dedcd6fe44b84ddfac71f3325305e5219885)

I don’t think you should delete these two lines entirely, because doing that really seems to lose test coverage for the AmountFromValue function despite the claim in the commit description. I would keep this two lines, but move them to the previous test and replace ParseNonRFCJSONValue with ValueFromString.

stickies-v commented at 4:22 pm on March 23, 2023:

BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);

Unless I misunderstand, AmountFromValue is not even called because ParseNonRFCJSONValue already throws first, so I don’t think we lose any coverage here? And the happy path is already tested at the moment:

0    BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19);

BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue(“0.00000000000000000000000000000000000001e+30 “)), 1);

Is there anything in specific that you think these existing three tests don’t capture that we had here?

0    BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.0000000000000000000000000000000000000000000000000000000000000000000000000001e+68")), COIN/100000000);
1    BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("10000000000000000000000000000000000000000000000000000000000000000e-64")), COIN);
2    BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000e64")), COIN);

I don’t mind adding more tests, I just wanted to be thoughtful about keeping them all with clear intent and purpose.

ryanofsky commented at 5:26 pm on March 23, 2023:

re: #27256 (review)

0BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);

Wow, it didn’t occur to me that AmountFromValue call on this line was useless because it was bypassed by the exception. I’m not sure if the person who wrote this test originally knew the AmountFromValue was being skipped, but it definitely looks to any reasonable observer like this line is intending to check whether AmountFromValue throws an exception on .19e-6. I do think this would be a useful thing to check, and since there is no similar test coverage for it, it would be good to add:

0BOOST_CHECK_EXCEPTION(AmountFromValue(".19e-6"), UniValue, HasJSON(R"({"code":-3,"message":"Invalid amount"})"));

to test what it looked like the previous line was testing.

But I do agree that the “0.0…1e+30” test is basically the same as the “0.0…1e+68” test, so probably not worth adding even if it drops some coverage strictly speaking.

stickies-v commented at 6:22 pm on March 23, 2023:

Thanks, you’re probably right that it was intended to be covered like that so I’ve added your test case on .19e-6.

in src/univalue/test/object.cpp:426 in 6e28dedcd6 outdated

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

ryanofsky commented at 3:37 pm on March 23, 2023:

In commit “test: move coverage on ParseNonRFCJSONValue() to UniValue::read()” (6e28dedcd6fe44b84ddfac71f3325305e5219885)

Test order seems to have changed unintentionally. Would be good to keep the 19e-6 test before the 1e+30 for easier comparison.

stickies-v commented at 4:26 pm on March 23, 2023:

The change is intentional, I’m now first doing the valid tests followed by the one invalid test, making it consistent with the docs too. I think this is easier to understand? But I don’t care too much either way, happy to change the order (and add some more docs) if you think that makes it easier.

ryanofsky commented at 5:32 pm on March 23, 2023:

Makes sense, and fine if this is intentional, I just thought it was an accident.

in src/rpc/client.cpp:226 in 860e70713c outdated

221@@ -222,6 +222,14 @@ static const CRPCConvertParam vRPCConvertParams[] =
222 };
223 // clang-format on
224 
225+/** Parse string to UniValue or throw runtime_error if string contains invalid JSON */
226+UniValue Parse(std::string_view raw)

ryanofsky commented at 3:55 pm on March 23, 2023:

In commit “refactor: rpc: remove ParseNonRFCJSONValue() function” (860e70713c2c4a13f9cb108046669ad9fdb02331)

This commit description seems misleading (or maybe it is out of date?). The commit is not removing the ParseNonRFCJSONValue() function, just renaming it and making it private.

I think a better name for the commit would be something like “Hide and rename ParseNonRFCJSONValue()”, and a better name for the PR would be “Remove unnecessary uses of ParseNonRFCJSONValue() and rename it”.

ryanofsky commented at 3:58 pm on March 23, 2023:

In commit “refactor: rpc: remove ParseNonRFCJSONValue() function” (860e70713c2c4a13f9cb108046669ad9fdb02331)

Probably should add static to avoid exporting a linker symbol that could clash with a Parse(string_view) function in another translation unit.

stickies-v commented at 4:36 pm on March 23, 2023:

Thanks, I’ve adopted your phrasing for both commit and PR description.

stickies-v commented at 4:36 pm on March 23, 2023:

Good point especially for such a generic name, made it static, thanks.

ryanofsky approved

ryanofsky commented at 4:03 pm on March 23, 2023: contributor

Code review ACK 860e70713c2c4a13f9cb108046669ad9fdb02331. This looks good, but we will have to find another function to give the most confusing function name award to. I left a few minor review suggestions, but they can be ignored.

stickies-v force-pushed on Mar 23, 2023

stickies-v renamed this:
~~refactor: rpc: remove ParseNonRFCJSONValue()~~
refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it
on Mar 23, 2023

ryanofsky approved

ryanofsky commented at 5:35 pm on March 23, 2023: contributor

Code review ACK 79ee14bf1ea9530922e5d15edd35f9debf661998. Just added static and updated commit description since last review

test: move coverage on ParseNonRFCJSONValue() to UniValue::read()

Preparation to deprecate ParseNonRFCJSONValue() but keep test coverage
on the underlying UniValue::read() unaffected. The test coverage on
AmountFromValue is no longer included, since that is already tested
in the rpc_parse_monetary_values test case.

Fuzzing coverage on ParseNonRFCJSONValue() was duplicated between string.cpp
and parse_univalue.cpp, only the one in parse_univalue.cpp is kept.

6c8bde6d54

refactor: rpc: hide and rename ParseNonRFCJSONValue()

As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059,
this function is no longer necessary and we can use UniValue::read() directly.

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.

cfbc8a623b

stickies-v force-pushed on Mar 23, 2023

stickies-v commented at 6:21 pm on March 23, 2023: contributor

Force pushed to add test a test case on AmountFromValue() that was previously not reached but probably intended.

Addressed all of @ryanofsky’s comments - thank you for the review!

ryanofsky approved

ryanofsky commented at 6:37 pm on March 23, 2023: contributor

Code review ACK cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test

fanquake merged this on Jun 2, 2023

fanquake closed this on Jun 2, 2023

stickies-v deleted the branch on Jun 2, 2023

sidhujag referenced this in commit c782c267b1 on Jun 2, 2023

refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it #27256

Reviews