Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unit test api_version to enable api_version 2 #4785

Merged
merged 5 commits into from
Nov 2, 2023
Merged

Fix unit test api_version to enable api_version 2 #4785

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
737a89f
Fix unit test api_version to enable api_version 2
pwang200 Oct 25, 2023
c15fd32
move unit test specific to src/test/jtx, default apiVersion to max
pwang200 Oct 26, 2023
2d3d91c
clang-format
intelliot Oct 31, 2023
e1d1c27
make journal the last parameter
pwang200 Oct 31, 2023
3e9b260
Merge branch 'develop' into api_version-fix
intelliot Nov 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/ripple/net/RPCCall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,20 @@ fromNetwork(
std::unordered_map<std::string, std::string> headers = {});
} // namespace RPCCall

/** Given a rippled command line, return the corresponding JSON.
/** Given a rippled unit test rpc command, return the corresponding JSON.
*/
Json::Value
cmdLineToJSONRPC(std::vector<std::string> const& args, beast::Journal j);
unitTestCmdToJSONRPC(std::vector<std::string> const& args, beast::Journal j);
pwang200 marked this conversation as resolved.
Show resolved Hide resolved

/** Internal invocation of RPC client.
* Used by both rippled command line as well as rippled unit tests
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this comment ! 👍

*/
std::pair<int, Json::Value>
rpcClient(
std::vector<std::string> const& args,
Config const& config,
Logs& logs,
bool fromCmdLine,
std::unordered_map<std::string, std::string> const& headers = {});

} // namespace ripple
Expand Down
19 changes: 11 additions & 8 deletions src/ripple/net/impl/RPCCall.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1473,10 +1473,11 @@ struct RPCCallImp

// Used internally by rpcClient.
static Json::Value
rpcCmdLineToJson(
rpcCmdToJson(
std::vector<std::string> const& args,
Json::Value& retParams,
beast::Journal j)
beast::Journal j,
unsigned int apiVersion)
{
Json::Value jvRequest(Json::objectValue);

Expand All @@ -1493,11 +1494,11 @@ rpcCmdLineToJson(

jvRequest = rpParser.parseCommand(args[0], jvRpcParams, true);

auto insert_api_version = [](Json::Value& jr) {
auto insert_api_version = [apiVersion](Json::Value& jr) {
if (jr.isObject() && !jr.isMember(jss::error) &&
!jr.isMember(jss::api_version))
{
jr[jss::api_version] = RPC::apiMaximumSupportedVersion;
jr[jss::api_version] = apiVersion;
}
};

Expand All @@ -1511,10 +1512,10 @@ rpcCmdLineToJson(
}

Json::Value
cmdLineToJSONRPC(std::vector<std::string> const& args, beast::Journal j)
unitTestCmdToJSONRPC(std::vector<std::string> const& args, beast::Journal j)
{
Json::Value jv = Json::Value(Json::objectValue);
auto const paramsObj = rpcCmdLineToJson(args, jv, j);
auto const paramsObj = rpcCmdToJson(args, jv, j, RPC::apiMinimumSupportedVersion);

// Re-use jv to return our formatted result.
jv.clear();
Expand Down Expand Up @@ -1546,6 +1547,7 @@ rpcClient(
std::vector<std::string> const& args,
Config const& config,
Logs& logs,
bool fromCmdLine,
std::unordered_map<std::string, std::string> const& headers)
{
static_assert(
Expand All @@ -1561,7 +1563,8 @@ rpcClient(
try
{
Json::Value jvRpc = Json::Value(Json::objectValue);
jvRequest = rpcCmdLineToJson(args, jvRpc, logs.journal("RPCParser"));
auto apiVersion = fromCmdLine? RPC::apiMaximumSupportedVersion : RPC::apiMinimumSupportedVersion;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather not have this logic hardcoded in this location; it would be easier to understand if rpcClient took explicitly unsigned apiVersion parameter instead, and then fromCommandLine passes RPC::apiMaximumSupportedVersion. That maintains isolation of concerns and also enables unit tests to test different versions (by which, I mean more than two) of command line generated parameters.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you go this way, I would add assert(apiVersion <= RPC::apiBetaVersion);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, changed.

jvRequest = rpcCmdToJson(args, jvRpc, logs.journal("RPCParser"), apiVersion);

if (jvRequest.isMember(jss::error))
{
Expand Down Expand Up @@ -1698,7 +1701,7 @@ fromCommandLine(
const std::vector<std::string>& vCmd,
Logs& logs)
{
auto const result = rpcClient(vCmd, config, logs);
auto const result = rpcClient(vCmd, config, logs, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned above, instead of true pass RPC::apiMaximumSupportedVersion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


std::cout << result.second.toStyledString();

Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/impl/RPCHelpers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,7 +1040,7 @@ chooseLedgerEntryType(Json::Value const& params)

beast::SemanticVersion const firstVersion("1.0.0");
beast::SemanticVersion const goodVersion("1.0.0");
beast::SemanticVersion const lastVersion("1.0.0");
beast::SemanticVersion const lastVersion("2.0.0");

unsigned int
getAPIVersionNumber(Json::Value const& jv, bool betaEnabled)
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/rpc/impl/RPCHelpers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@ extern beast::SemanticVersion const lastVersion;
constexpr unsigned int apiInvalidVersion = 0;
constexpr unsigned int apiVersionIfUnspecified = 1;
constexpr unsigned int apiMinimumSupportedVersion = 1;
constexpr unsigned int apiMaximumSupportedVersion = 1;
constexpr unsigned int apiMaximumSupportedVersion = 2;
constexpr unsigned int apiBetaVersion = 2;
constexpr unsigned int apiMaximumValidVersion = apiBetaVersion;

Expand Down
2 changes: 1 addition & 1 deletion src/test/jtx/impl/Env.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ Env::do_rpc(
std::vector<std::string> const& args,
std::unordered_map<std::string, std::string> const& headers)
{
return rpcClient(args, app().config(), app().logs(), headers).second;
return rpcClient(args, app().config(), app().logs(), false, headers).second;
}

void
Expand Down
2 changes: 1 addition & 1 deletion src/test/rpc/LedgerRequestRPC_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ class LedgerRequestRPC_test : public beast::unit_test::suite
env.timeKeeper().adjustCloseTime(weeks{3});
result = env.rpc("ledger_request", "1")[jss::result];
BEAST_EXPECT(result[jss::status] == "error");
if (RPC::apiMaximumSupportedVersion == 1)
if (RPC::apiMinimumSupportedVersion == 1)
{
BEAST_EXPECT(result[jss::error] == "noCurrent");
BEAST_EXPECT(
Expand Down
4 changes: 2 additions & 2 deletions src/test/rpc/RPCCall_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6403,7 +6403,7 @@ std::string
updateAPIVersionString(const char* const req)
{
static std::string version_str =
std::to_string(RPC::apiMaximumSupportedVersion);
std::to_string(RPC::apiMinimumSupportedVersion);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should minimum version be default or apiVersionIfUnspecified ? Both of them = 1 right now, so it doesn't really matter. But I am just curious.

Copy link
Collaborator

@arihantkothari arihantkothari Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know most of the times (maybe all the times), apiVersionIfUnspecified = apiMinimumSupportedVersion will be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot say no. I.e., probably make sense to use apiVersionIfUnspecified as default.

static auto place_holder = "%MAX_API_VER%";
Copy link
Collaborator

@arihantkothari arihantkothari Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are putting minimum version as version_str. Should this line instead be static auto place_holder = "%MIN_API_VER%"; ? [line 6407]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it back to RPC::apiMaximumSupportedVersion and we will see what we eventually decide to use as default. Could be apiVersionIfUnspecified as you said earlier.

std::string jr(req);
boost::replace_all(jr, place_holder, version_str);
Expand Down Expand Up @@ -6442,7 +6442,7 @@ class RPCCall_test : public beast::unit_test::suite
Json::Value got;
try
{
got = cmdLineToJSONRPC(args, env.journal);
got = unitTestCmdToJSONRPC(args, env.journal);
}
catch (std::bad_cast const&)
{
Expand Down
Loading