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

stickies-v · 2022-11-15T16:10:13Z

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 · 2022-11-16T00:10:10Z

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.

luke-jr · 2022-11-19T21:08:15Z

What is "non-RFC JSON values"?

stickies-v · 2022-11-21T12:59:24Z

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:

    BOOST_CHECK(UniValue{true}.isBool());
    BOOST_CHECK(UniValue{"true"}.isStr());
    BOOST_CHECK(ParseNonRFCJSONValue("true").isBool());
    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)

src/rpc/client.cpp

stickies-v · 2022-11-30T15:05:23Z

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

ryanofsky

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

re: #26506 (comment)

Perhaps I could rename ParseNonRFCJSONValue()

I believe since jgarzik/univalue#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)

src/rpc/client.cpp

stickies-v · 2022-12-11T08:30:18Z

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.

stickies-v · 2022-12-14T10:20:38Z

Force pushed to:

fix merge conflict introduced by RPC: Reject RPC requests with same named parameter specified multiple times #26628
rename ParseIfNonString()->ParseIfNonStringParam()

maflcko

left a comment

src/rpc/client.cpp

maflcko

lgtm, left some nits

src/rpc/client.cpp

stickies-v

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

src/rpc/client.cpp

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.

aureleoules

ACK 6d0ab07

Looks good and simplifies the code.

stickies-v · 2023-01-18T17:20:09Z

Reviewers of this PR might be interested in the follow-up: #26612

…non-string parameters 6d0ab07 refactor: use convenience fn to auto parse non-string parameters (stickies-v) Pull request description: 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. ACKs for top commit: aureleoules: ACK 6d0ab07 Tree-SHA512: 8cbf68a17cfbdce1e31a19916f447a2965c139fdef00c19e32a9b679f4a4015dfe69ceea0bbe1723711e1c5033ea8d4005d1f4485dfbeea22226140f8cbe8aa3

… as string_view 545ff92 refactor: use string_view for RPC named argument values (stickies-v) 7727603 refactor: reduce unnecessary complexity in ParseNonRFCJSONValue (stickies-v) 1d02e59 test: add cases to JSON parsing (stickies-v) Pull request description: Inspired by MarcoFalke's [comment](bitcoin/bitcoin#26506 (comment)). Main purpose of this PR is to minimize copying (potentially large) RPC named arguments when calling `.substr()` by using `std::string_view` instead of `std::string`. Furthermore, cleans up the code by removing unnecessary complexity in `ParseNonRFCJSONValue()` (done first to avoid refactoring required to concatenate `string` and `string_view`), updates some naming and adds a few test cases. Should not introduce any behaviour change. ## Questions - ~Was there actually any merit to `ParseNonRFCJSONValue()` surrounding the value with brackets and then parsing it as an array? I don't see it, and the new approach doesn't fail any tests. Still a bit suspicious about it though.~ - Cleared up by bitcoin/bitcoin#26506 (review) - If there are no objections to 7727603, I think we should follow up with a PR to rename `ParseNonRFCJSONValue()` to a local `Parse()` helper function (that throws if invalid), remove it from `client.h` and merge the test coverage we currently have on `ParseNonRFCJSONValue()` with the coverage we have on `UniValue::read()`. ACKs for top commit: ryanofsky: Code review ACK 545ff92 MarcoFalke: review ACK 545ff92 📻 Tree-SHA512: b1c89fb010ac9c3054b023cac1acbba2a539a09cf39a7baffbd7f7571ee268d5a6d98701c7ac10d68a814526e8fd0fe96ac1d1fb072f272033e415b753f64a5c

As per bitcoin#26506 (review), this function is no longer necessary and we can use UniValue::read() directly. To avoid code duplication, a new local Parse() function is introduced that throws on invalid JSON input, but is not in the header file.

As per bitcoin#26506 (review), 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.

…f 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

As per bitcoin/bitcoin#26506 (review), 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.

glozow added the Refactoring label Nov 16, 2022

This was referenced Nov 16, 2022

cli: Parse and allow hash value #19949

Closed

rpc: Allow named and positional arguments to be used together #19762

Merged

RPC: Accept options as named-only parameters #26485

Merged

DrahtBot added the Needs rebase label Nov 30, 2022

stickies-v changed the title ~~[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 Nov 30, 2022

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

stickies-v force-pushed the rpc-convert-if-necessary branch from f4cdeaf to bd1685c Compare November 30, 2022 11:44

maflcko reviewed Nov 30, 2022

View reviewed changes

src/rpc/client.cpp Outdated Show resolved Hide resolved

stickies-v force-pushed the rpc-convert-if-necessary branch 2 times, most recently from 4334cae to 32bfd59 Compare November 30, 2022 12:04

DrahtBot removed the Needs rebase label Nov 30, 2022

stickies-v marked this pull request as ready for review November 30, 2022 15:05

stickies-v mentioned this pull request Nov 30, 2022

refactor: RPC: pass named argument value as string_view #26612

Merged

ryanofsky approved these changes Dec 9, 2022

View reviewed changes

src/rpc/client.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 10, 2022

RPC: Reject RPC requests with same named parameter specified multiple times #26628

Merged

DrahtBot added the Needs rebase label Dec 13, 2022

stickies-v force-pushed the rpc-convert-if-necessary branch from 32bfd59 to 135e832 Compare December 13, 2022 17:47

DrahtBot removed the Needs rebase label Dec 13, 2022

maflcko reviewed Dec 14, 2022

View reviewed changes

src/rpc/client.cpp Show resolved Hide resolved

maflcko reviewed Dec 14, 2022

View reviewed changes

src/rpc/client.cpp Outdated Show resolved Hide resolved

src/rpc/client.cpp Outdated Show resolved Hide resolved

stickies-v force-pushed the rpc-convert-if-necessary branch from 135e832 to ae7c013 Compare December 14, 2022 12:37

stickies-v commented Dec 14, 2022

View reviewed changes

src/rpc/client.cpp Outdated Show resolved Hide resolved

src/rpc/client.cpp Outdated Show resolved Hide resolved

src/rpc/client.cpp Show resolved Hide resolved

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

6d0ab07

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.

stickies-v force-pushed the rpc-convert-if-necessary branch from ae7c013 to 6d0ab07 Compare January 4, 2023 16:06

aureleoules approved these changes Jan 18, 2023

View reviewed changes

maflcko merged commit 78c3081 into bitcoin:master Jan 18, 2023

stickies-v deleted the rpc-convert-if-necessary branch January 18, 2023 12:16

stickies-v mentioned this pull request Mar 14, 2023

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

Merged

bitcoin locked and limited conversation to collaborators Jan 18, 2024

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

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

Conversation

stickies-v commented Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

luke-jr commented Nov 19, 2022

Uh oh!

stickies-v commented Nov 21, 2022

Uh oh!

Uh oh!

stickies-v commented Nov 30, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v commented Dec 11, 2022

Uh oh!

stickies-v commented Dec 14, 2022

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Jan 18, 2023

Uh oh!

Uh oh!

stickies-v commented Nov 15, 2022 •

edited

Loading

DrahtBot commented Nov 16, 2022 •

edited

Loading