refactor: rpc: use convenience fn to auto parse non-string parameters

stickies-v commented at 4:10 pm on November 15, 2022: contributor

Minimizes code duplication and improves function naming by having a single (overloaded) convenience function ParseIfNonString that both checks if the parameter is a non-string parameter and automatically parses the value if so.

DrahtBot commented at 0:10 am on November 16, 2022: 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	aureleoules
Stale ACK	ryanofsky

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

Conflicts

No conflicts as of last run.

glozow added the label Refactoring on Nov 16, 2022

luke-jr commented at 9:08 pm on November 19, 2022: member

What is “non-RFC JSON values”?

stickies-v commented at 12:59 pm on November 21, 2022: contributor

What is “non-RFC JSON values”?

The documentation/naming quite confused me too, as it seems to imply that the string contains a notation that is not JSON RFC compliant, e.g. parse “TRUE” to true. That is not true - ParseNonRFCJSONValue() fails for non-JSON RFC compliant strings. See e.g. this set of tests that passes:

0    BOOST_CHECK(UniValue{true}.isBool());
1    BOOST_CHECK(UniValue{"true"}.isStr());
2    BOOST_CHECK(ParseNonRFCJSONValue("true").isBool());
3    BOOST_CHECK_THROW(ParseNonRFCJSONValue("TRUE"), std::runtime_error);

Perhaps I could rename ParseNonRFCJSONValue() to ParseStringToJSON() and convert_if_non_rfc_json() to ParseToJSONIfNecessary(), and update the documentation accordingly? (will update the camelcased convert_if_non_rfc_json() either way - that was not intentional)

DrahtBot added the label Needs rebase on Nov 30, 2022

stickies-v renamed this:
~~[WIP] refactor: rpc: use convenience fn to auto convert non-RFC JSON values~~
refactor: rpc: use convenience fn to auto convert non-RFC JSON values
on Nov 30, 2022

stickies-v renamed this:
~~refactor: rpc: use convenience fn to auto convert non-RFC JSON values~~
refactor: rpc: use convenience fn to auto parse non-string parameters
on Nov 30, 2022

stickies-v force-pushed on Nov 30, 2022

in src/rpc/client.cpp:288 in bd1685ca6a outdated

282+            positional_args.push_back(rpcCvtTable.ParseIfNonString(s, strMethod, positional_args.size()));
283             continue;
284         }
285 
286         std::string name = s.substr(0, pos);
287         std::string value = s.substr(pos+1);

maflcko commented at 11:48 am on November 30, 2022:

unrelated: I wonder if avoiding a copy (maybe via string_view) would improve performance when large values are passed (like a full hex block)

stickies-v commented at 3:46 pm on November 30, 2022:

ParseIfNonString passes by reference, and in this case we push_back an rvalue already anyway so I don’t think we’re copying anything?

(I know you marked this as resolved already but I’m not sure if that means you changed your mind or think it’s too unrelated?)

maflcko commented at 3:54 pm on November 30, 2022:

I meant that substr creates a copy of the string. It may be possible to avoid this by using a string_view, which can then be passed to ParseIfNonString

stickies-v commented at 6:23 pm on November 30, 2022:

Makes sense, I thought you were talking about the highlighted lines - thanks. Proposed in #26612 as I think it’s pretty orthogonal to this PR but I like the idea (and I’d been considering simplifying ParseNonRFCJSONValue already, which ties in nicely).

stickies-v force-pushed on Nov 30, 2022

DrahtBot removed the label Needs rebase on Nov 30, 2022

stickies-v marked this as ready for review on Nov 30, 2022

stickies-v commented at 3:05 pm on November 30, 2022: contributor

With #19762 now merged, PR is rebased and ready for review.

in src/rpc/client.cpp:229 in 32bfd59c28 outdated

224@@ -225,11 +225,14 @@ class CRPCConvertTable
225 public:
226     CRPCConvertTable();
227 
228-    bool convert(const std::string& method, int idx) {
229-        return (members.count(std::make_pair(method, idx)) > 0);
230+    /* Return value as UniValue, and first parse it if it is a non-string parameter */
231+    UniValue ParseIfNonString(const std::string& value, const std::string& method, int idx) {

ryanofsky commented at 5:25 pm on December 9, 2022:

In commit “refactor: use convenience fn to auto parse non-string parameters” (32bfd59c28c61dab46fc204145aa52f8db2b7860)

The name ParseIfNonString sounds a little nonsensical to me because it reads like “parse value if value is not a string”, but it really means “parse value if parameter type is non-string”. Probably hard to choose a better name, but I might go with something like “ParseJsonOrStringParam” or “MaybeParseJson” or “ArgToUniValue” if anything like that sounds better.

stickies-v commented at 8:34 am on December 11, 2022:

I agree there’s room for misunderstanding here. I think ParseJsonOrStringParam doesn’t help with clarity, and in general I don’t like Maybe functions if alternatively the condition(s) can realistically be described in the function name (which I think is possible here). I would be okay with ArgToUniValue - or what about ParseIfNonStringParam?

stickies-v commented at 5:48 pm on December 13, 2022:

~~Renamed to ParseIfNonStringParam() - happy to further discuss a better name though. Marking as resolved until then.~~

Edit: now renamed to ArgToUniValue()

ryanofsky approved

ryanofsky commented at 5:35 pm on December 9, 2022: contributor

Code review ACK 32bfd59c28c61dab46fc204145aa52f8db2b7860. This makes client code less repetitive and messy.

re: #26506 (comment)

Perhaps I could rename ParseNonRFCJSONValue()

I believe since https://github.com/jgarzik/univalue/pull/31 the ParseNonRFCJSONValue() function is obsolete and you can just call UniValue::read directly instead. 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)

stickies-v commented at 8:30 am on December 11, 2022: contributor

I believe since jgarzik/univalue#31 the ParseNonRFCJSONValue() function is obsolete and you can just call UniValue::read directly instead.

I didn’t know that background - thanks for sharing. I already thought it was unnecessary so I’ve addressed it in #26612 b143564. I think there is merit in keeping the function to maintain the error throwing behaviour that UniValue::read does not have, but probably I’ll rename the function to just Parse() instead of ParseNonRFCJSONValue(), unless you have better ideas? Unless you’d still prefer removing the function altogether, I’d suggest we move that discussion to #26612 since that would be out of scope for this PR imo.

DrahtBot added the label Needs rebase on Dec 13, 2022

stickies-v force-pushed on Dec 13, 2022

DrahtBot removed the label Needs rebase on Dec 13, 2022

stickies-v commented at 10:20 am on December 14, 2022: contributor

Force pushed to:

fix merge conflict introduced by #26628
rename ParseIfNonString()->ParseIfNonStringParam()

in src/rpc/client.cpp:297 in 135e8326ad outdated

287@@ -292,13 +288,7 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
288         // Intentionally overwrite earlier named values with later ones as a
289         // convenience for scripts and command line users that want to merge
290         // options.
291-        if (!rpcCvtTable.convert(strMethod, name)) {
292-            // insert string value directly
293-            params.pushKV(name, value);

maflcko commented at 11:38 am on December 14, 2022:

Previously it would have been possible to add a std::move here to avoid a copy. With your new method this is no longer possible.

maflcko commented at 11:40 am on December 14, 2022:

I guess this is fine, if move ahead with string_views from #26612 and assume strParams to remain const.

stickies-v commented at 12:33 pm on December 14, 2022:

if move ahead with string_views from #26612 and assume

I’ll pick that up as soon as this is merged, marking as resolved then?

maflcko commented at 11:38 am on December 14, 2022: member

left a comment

in src/rpc/client.cpp:229 in 135e8326ad outdated

224@@ -225,11 +225,14 @@ class CRPCConvertTable
225 public:
226     CRPCConvertTable();
227 
228-    bool convert(const std::string& method, int idx) {
229-        return (members.count(std::make_pair(method, idx)) > 0);
230+    /* Return value as UniValue, and first parse it if it is a non-string parameter */
231+    UniValue ParseIfNonStringParam(const std::string& value, const std::string& method, int idx) {

maflcko commented at 11:41 am on December 14, 2022:

clang format new code?

maflcko commented at 11:45 am on December 14, 2022:

Also, could rename the function to just UniValue Convert(..., given that you removed bool convert(... and the caller doesn’t care about the implementation details here, only that this turns a value into UniValue.

stickies-v commented at 12:27 pm on December 14, 2022:

clang format new code?

done

Also, could rename the function to just UniValue Convert(…

Agreed with the caller not caring about implementation details, but I’ve now adopted @ryanofsky’s suggested ArgToUniValue since it’s more clear than Convert() wrt what kind of conversion we’re doing (and also slightly improved function parameter naming)

in src/rpc/client.cpp:230 in 135e8326ad outdated

224@@ -225,11 +225,14 @@ class CRPCConvertTable
225 public:
226     CRPCConvertTable();
227 
228-    bool convert(const std::string& method, int idx) {
229-        return (members.count(std::make_pair(method, idx)) > 0);
230+    /* Return value as UniValue, and first parse it if it is a non-string parameter */
231+    UniValue ParseIfNonStringParam(const std::string& value, const std::string& method, int idx) {
232+        return (members.count(std::make_pair(method, idx))) > 0 ? ParseNonRFCJSONValue(value) : value;

maflcko commented at 11:41 am on December 14, 2022:

0        return members.count(std::make_pair(method, idx)) > 0 ? ParseNonRFCJSONValue(value) : value;

nit

stickies-v commented at 12:23 pm on December 14, 2022:

fixed

maflcko commented at 11:45 am on December 14, 2022: member

lgtm, left some nits

stickies-v force-pushed on Dec 14, 2022

stickies-v commented at 12:37 pm on December 14, 2022: contributor

Force pushed to address review comments from MarcoFalke:

renamed ParseIfNonStringParam()->ArgToUniValue()
improved parameter naming of ArgToUniValue()
tidy and fixed clang-format warnings in new code

refactor: use convenience fn to auto parse non-string parameters

Minimizes code duplication and improves function naming by having
a single (overloaded) convenience function that both checks if
the parameter is a non-string parameter and automatically parses the
value if so.

6d0ab07e81

stickies-v force-pushed on Jan 4, 2023

aureleoules approved

aureleoules commented at 10:24 am on January 18, 2023: member

ACK 6d0ab07e817725369df699791b9c5b2fae204990

Looks good and simplifies the code.

maflcko merged this on Jan 18, 2023

maflcko closed this on Jan 18, 2023

stickies-v deleted the branch on Jan 18, 2023

stickies-v commented at 5:20 pm on January 18, 2023: contributor

Reviewers of this PR might be interested in the follow-up: https://github.com/bitcoin/bitcoin/pull/26612

sidhujag referenced this in commit 854f873405 on Jan 18, 2023

fanquake referenced this in commit 3b88c85025 on Mar 3, 2023

stickies-v referenced this in commit a1bb11bbee on Mar 14, 2023

stickies-v referenced this in commit 0f224565e5 on Mar 14, 2023

stickies-v referenced this in commit f43b98b26e on Mar 14, 2023

stickies-v referenced this in commit 104e482729 on Mar 16, 2023

stickies-v referenced this in commit 860e70713c on Mar 16, 2023

stickies-v referenced this in commit 79ee14bf1e on Mar 23, 2023

stickies-v referenced this in commit cfbc8a623b on Mar 23, 2023

fanquake referenced this in commit 436c185b05 on Jun 2, 2023

janus referenced this in commit 676f64fe98 on Sep 3, 2023

bitcoin locked this on Jan 18, 2024

refactor: rpc: use convenience fn to auto parse non-string parameters #26506

Reviews

Conflicts