From a15d37b134dcb07e49a33c4ecdf80a24d146acfa Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Fri, 17 Nov 2023 12:54:23 +0000 Subject: [PATCH] Squashed PR #4820 --- src/ripple/app/ledger/impl/LedgerToJson.cpp | 25 ++- src/ripple/app/misc/NetworkOPs.cpp | 47 +++--- src/ripple/json/MultivarJson.h | 64 +++++-- src/test/json/MultivarJson_test.cpp | 176 +++++++++++++++----- 4 files changed, 231 insertions(+), 81 deletions(-) diff --git a/src/ripple/app/ledger/impl/LedgerToJson.cpp b/src/ripple/app/ledger/impl/LedgerToJson.cpp index d22cc7cd487..9cef4e65b56 100644 --- a/src/ripple/app/ledger/impl/LedgerToJson.cpp +++ b/src/ripple/app/ledger/impl/LedgerToJson.cpp @@ -27,6 +27,7 @@ #include #include #include +#include namespace ripple { @@ -52,10 +53,17 @@ isBinary(LedgerFill const& fill) template void -fillJson(Object& json, bool closed, LedgerInfo const& info, bool bFull) +fillJson( + Object& json, + bool closed, + LedgerInfo const& info, + bool bFull, + unsigned apiVersion) { json[jss::parent_hash] = to_string(info.parentHash); - json[jss::ledger_index] = to_string(info.seq); + json[jss::ledger_index] = (apiVersion > 1) + ? Json::Value(info.seq) + : Json::Value(std::to_string(info.seq)); if (closed) { @@ -159,7 +167,10 @@ fillJsonTx( txJson[jss::validated] = validated; if (validated) { - txJson[jss::ledger_index] = to_string(fill.ledger.seq()); + auto const seq = fill.ledger.seq(); + txJson[jss::ledger_index] = (fill.context->apiVersion > 1) + ? Json::Value(seq) + : Json::Value(std::to_string(seq)); if (fill.closeTime) txJson[jss::close_time_iso] = to_string_iso(*fill.closeTime); } @@ -315,7 +326,13 @@ fillJson(Object& json, LedgerFill const& fill) if (isBinary(fill)) fillJsonBinary(json, !fill.ledger.open(), fill.ledger.info()); else - fillJson(json, !fill.ledger.open(), fill.ledger.info(), bFull); + fillJson( + json, + !fill.ledger.open(), + fill.ledger.info(), + bFull, + (fill.context ? fill.context->apiVersion + : RPC::apiMaximumSupportedVersion)); if (bFull || fill.options & LedgerFill::dumpTxrp) fillJsonTx(json, fill); diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index abb4acf029e..1c16e63f0a1 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -2181,8 +2181,10 @@ NetworkOPsImp::pubValidation(std::shared_ptr const& val) if (masterKey != signerPublic) jvObj[jss::master_key] = toBase58(TokenType::NodePublic, masterKey); + // NOTE *seq is a number, but old API versions used string. We replace + // number with a string using MultiApiJson near end of this function if (auto const seq = (*val)[~sfLedgerSequence]) - jvObj[jss::ledger_index] = to_string(*seq); + jvObj[jss::ledger_index] = *seq; if (val->isFieldPresent(sfAmendments)) { @@ -2220,12 +2222,27 @@ NetworkOPsImp::pubValidation(std::shared_ptr const& val) reserveIncXRP && reserveIncXRP->native()) jvObj[jss::reserve_inc] = reserveIncXRP->xrp().jsonClipped(); + // TODO Replace multiObj with jvObj when API versions 1 is retired + MultiApiJson multiObj{jvObj}; + visit( + multiObj, // + [](Json::Value& jvTx, unsigned int apiVersion) { + // Type conversion for older API versions to string + if (jvTx.isMember(jss::ledger_index) && apiVersion < 2) + { + jvTx[jss::ledger_index] = + std::to_string(jvTx[jss::ledger_index].asUInt()); + } + }); + for (auto i = mStreamMaps[sValidations].begin(); i != mStreamMaps[sValidations].end();) { if (auto p = i->second.lock()) { - p->send(jvObj, true); + p->send( + multiObj.select(apiVersionSelector(p->getApiVersion())), + true); ++i; } else @@ -3159,25 +3176,10 @@ NetworkOPsImp::transJson( } std::string const hash = to_string(transaction->getTransactionID()); - MultiApiJson multiObj({jvObj, jvObj}); - // Minimum supported API version must match index 0 in MultiApiJson - static_assert(apiVersionSelector(RPC::apiMinimumSupportedVersion)() == 0); - // Last valid (possibly beta) API ver. must match last index in MultiApiJson - static_assert( - apiVersionSelector(RPC::apiMaximumValidVersion)() + 1 // - == MultiApiJson::size); - for (unsigned apiVersion = RPC::apiMinimumSupportedVersion, - lastIndex = MultiApiJson::size; - apiVersion <= RPC::apiMaximumValidVersion; - ++apiVersion) - { - unsigned const index = apiVersionSelector(apiVersion)(); - assert(index < MultiApiJson::size); - if (index != lastIndex) - { - lastIndex = index; - - Json::Value& jvTx = multiObj.val[index]; + MultiApiJson multiObj{jvObj}; + visit( + multiObj, // + [&](Json::Value& jvTx, unsigned int apiVersion) { RPC::insertDeliverMax( jvTx[jss::transaction], transaction->getTxnType(), apiVersion); @@ -3190,8 +3192,7 @@ NetworkOPsImp::transJson( { jvTx[jss::transaction][jss::hash] = hash; } - } - } + }); return multiObj; } diff --git a/src/ripple/json/MultivarJson.h b/src/ripple/json/MultivarJson.h index 94d0090edc4..d946bc6c034 100644 --- a/src/ripple/json/MultivarJson.h +++ b/src/ripple/json/MultivarJson.h @@ -26,14 +26,24 @@ #include #include #include +#include +#include namespace ripple { template struct MultivarJson { - std::array val; + std::array val = {}; constexpr static std::size_t size = Size; + explicit MultivarJson(Json::Value const& init = {}) + { + if (init == Json::Value{}) + return; // All elements are already default-initialized + for (auto& v : val) + v = init; + } + Json::Value const& select(auto&& selector) const requires std::same_as @@ -68,7 +78,7 @@ struct MultivarJson }; // Wrapper for Json for all supported API versions. -using MultiApiJson = MultivarJson<2>; +using MultiApiJson = MultivarJson<3>; /* @@ -78,21 +88,17 @@ If a future API version change adds another possible format, change the size of `MultiApiJson`, and update `apiVersionSelector()` to return the appropriate selection value for the new `apiVersion` and higher. -e.g. There are 2 formats now, the first, for version one, the second for -versions > 1. Hypothetically, if API version 4 adds a new format, `MultiApiJson` -would be MultivarJson<3>, and `apiVersionSelector` would return -`static_cast(apiVersion < 2 ? 0u : (apiVersion < 4 ? 1u : 2u))` - -NOTE: - The more different JSON formats we support, the more CPU cycles we need to -pre-build JSON for different API versions e.g. when publishing streams to +prepare JSON for different API versions e.g. when publishing streams to `subscribe` clients. Hence it is desirable to keep MultiApiJson small and instead fully deprecate and remove support for old API versions. For example, if we removed support for API version 1 and added a different format for API version 3, the `apiVersionSelector` would change to `static_cast(apiVersion > 2)` +Such hypothetical change should correspond with change in RPCHelpers.h +`apiMinimumSupportedVersion = 2;` + */ // Helper to create appropriate selector for indexing MultiApiJson by apiVersion @@ -101,12 +107,44 @@ apiVersionSelector(unsigned int apiVersion) noexcept { return [apiVersion]() constexpr { - // apiVersion <= 1 returns 0 - // apiVersion > 1 returns 1 - return static_cast(apiVersion > 1); + return static_cast( + apiVersion <= 1 // + ? 0 // + : (apiVersion <= 2 // + ? 1 // + : 2)); }; } +// Helper to execute a callback for every version. Want both min and max version +// provided explicitly, so user will know to do update `size` when they change +template < + unsigned int minVer, + unsigned int maxVer, + std::size_t size, + typename Fn> + requires // + (maxVer >= minVer) && // + (size == maxVer + 1 - minVer) && // + (apiVersionSelector(minVer)() == 0) && // + (apiVersionSelector(maxVer)() + 1 == size) && // + requires(Json::Value& json, Fn fn) +{ + fn(json, static_cast(1)); +} +void +visit(MultivarJson& json, Fn fn) +{ + [&](std::index_sequence) + { + static_assert(((apiVersionSelector(minVer + offset)() >= 0) && ...)); + static_assert(((apiVersionSelector(minVer + offset)() < size) && ...)); + (fn(json.val[apiVersionSelector(minVer + offset)()], minVer + offset), + ...); + } + (std::make_index_sequence{}); +} + } // namespace ripple #endif diff --git a/src/test/json/MultivarJson_test.cpp b/src/test/json/MultivarJson_test.cpp index 8cb3a49aff2..63b4f8ee394 100644 --- a/src/test/json/MultivarJson_test.cpp +++ b/src/test/json/MultivarJson_test.cpp @@ -33,21 +33,23 @@ namespace test { struct MultivarJson_test : beast::unit_test::suite { + static auto + makeJson(const char* key, int val) + { + Json::Value obj1(Json::objectValue); + obj1[key] = val; + return obj1; + }; + void run() override { - constexpr static Json::StaticString string1("string1"); - static Json::Value const str1{string1}; - - static Json::Value const obj1{[]() { - Json::Value obj1(Json::objectValue); - obj1["one"] = 1; - return obj1; - }()}; - - static Json::Value const jsonNull{}; + Json::Value const obj1 = makeJson("value", 1); + Json::Value const obj2 = makeJson("value", 2); + Json::Value const obj3 = makeJson("value", 3); + Json::Value const jsonNull{}; - MultivarJson<3> const subject({str1, obj1}); + MultivarJson<3> subject{}; static_assert(sizeof(subject) == sizeof(subject.val)); static_assert(subject.size == subject.val.size()); static_assert( @@ -55,13 +57,11 @@ struct MultivarJson_test : beast::unit_test::suite BEAST_EXPECT(subject.val.size() == 3); BEAST_EXPECT( - (subject.val == std::array{str1, obj1, jsonNull})); - BEAST_EXPECT( - (MultivarJson<3>({obj1, str1}).val == - std::array{obj1, str1, jsonNull})); - BEAST_EXPECT( - (MultivarJson<3>({jsonNull, obj1, str1}).val == - std::array{jsonNull, obj1, str1})); + (subject.val == + std::array{jsonNull, jsonNull, jsonNull})); + + subject.val[0] = obj1; + subject.val[1] = obj2; { testcase("default copy construction / assignment"); @@ -96,9 +96,9 @@ struct MultivarJson_test : beast::unit_test::suite testcase("select"); BEAST_EXPECT( - subject.select([]() -> std::size_t { return 0; }) == str1); + subject.select([]() -> std::size_t { return 0; }) == obj1); BEAST_EXPECT( - subject.select([]() -> std::size_t { return 1; }) == obj1); + subject.select([]() -> std::size_t { return 1; }) == obj2); BEAST_EXPECT( subject.select([]() -> std::size_t { return 2; }) == jsonNull); @@ -144,12 +144,9 @@ struct MultivarJson_test : beast::unit_test::suite } { - struct foo_t final - { - }; testcase("set"); - auto x = MultivarJson<2>{{Json::objectValue, Json::objectValue}}; + auto x = MultivarJson<2>{Json::objectValue}; x.set("name1", 42); BEAST_EXPECT(x.val[0].isMember("name1")); BEAST_EXPECT(x.val[1].isMember("name1")); @@ -193,6 +190,9 @@ struct MultivarJson_test : beast::unit_test::suite }(x)); // Tests of requires clause - these are expected NOT to match + struct foo_t final + { + }; static_assert([](auto&& v) { return !requires { @@ -213,32 +213,29 @@ struct MultivarJson_test : beast::unit_test::suite // Well defined behaviour even if we have different types of members BEAST_EXPECT(subject.isMember("foo") == decltype(subject)::none); - auto const makeJson = [](const char* key, int val) { - Json::Value obj1(Json::objectValue); - obj1[key] = val; - return obj1; - }; - { // All variants have element "One", none have element "Two" - MultivarJson<2> const s1{ - {makeJson("One", 12), makeJson("One", 42)}}; + MultivarJson<2> s1{}; + s1.val[0] = makeJson("One", 12); + s1.val[1] = makeJson("One", 42); BEAST_EXPECT(s1.isMember("One") == decltype(s1)::all); BEAST_EXPECT(s1.isMember("Two") == decltype(s1)::none); } { // Some variants have element "One" and some have "Two" - MultivarJson<2> const s2{ - {makeJson("One", 12), makeJson("Two", 42)}}; + MultivarJson<2> s2{}; + s2.val[0] = makeJson("One", 12); + s2.val[1] = makeJson("Two", 42); BEAST_EXPECT(s2.isMember("One") == decltype(s2)::some); BEAST_EXPECT(s2.isMember("Two") == decltype(s2)::some); } { // Not all variants have element "One", because last one is null - MultivarJson<3> const s3{ - {makeJson("One", 12), makeJson("One", 42), {}}}; + MultivarJson<3> s3{}; + s3.val[0] = makeJson("One", 12); + s3.val[1] = makeJson("One", 42); BEAST_EXPECT(s3.isMember("One") == decltype(s3)::some); BEAST_EXPECT(s3.isMember("Two") == decltype(s3)::none); } @@ -248,8 +245,10 @@ struct MultivarJson_test : beast::unit_test::suite // NOTE It's fine to change this test when we change API versions testcase("apiVersionSelector"); - static_assert(MultiApiJson::size == 2); - static MultiApiJson x{{obj1, str1}}; + static_assert(MultiApiJson::size == 3); + static MultiApiJson x{obj1}; + x.val[1] = obj2; + x.val[2] = obj3; static_assert( std::is_same_v); @@ -261,15 +260,16 @@ struct MultivarJson_test : beast::unit_test::suite }(x)); BEAST_EXPECT(x.select(apiVersionSelector(0)) == obj1); - BEAST_EXPECT(x.select(apiVersionSelector(2)) == str1); + BEAST_EXPECT(x.select(apiVersionSelector(2)) == obj2); static_assert(apiVersionSelector(0)() == 0); static_assert(apiVersionSelector(1)() == 0); static_assert(apiVersionSelector(2)() == 1); - static_assert(apiVersionSelector(3)() == 1); + static_assert(apiVersionSelector(3)() == 2); + static_assert(apiVersionSelector(4)() == 2); static_assert( apiVersionSelector( - std::numeric_limits::max())() == 1); + std::numeric_limits::max())() == 2); } { @@ -284,6 +284,100 @@ struct MultivarJson_test : beast::unit_test::suite BEAST_EXPECT(MultiApiJson::size >= 1); } + + { + testcase("visit"); + + MultivarJson<3> s1{}; + s1.val[0] = makeJson("value", 2); + s1.val[1] = makeJson("value", 3); + s1.val[2] = makeJson("value", 5); + + int result = 1; + ripple::visit<1, 3>( + s1, [&](Json::Value& json, unsigned int i) -> void { + if (BEAST_EXPECT(json.isObject() && json.isMember("value"))) + { + auto const value = json["value"].asInt(); + BEAST_EXPECT( + (value == 2 && i == 1) || // + (value == 3 && i == 2) || // + (value == 5 && i == 3)); + result *= value; + } + }); + BEAST_EXPECT(result == 30); + + // Can use fn with constexpr functor + static_assert([](auto&& v) { + return requires + { + ripple::visit<1, 3>( + v, [](Json::Value&, unsigned int) constexpr {}); + }; + }(s1)); + + // Can use fn with deduction over all parameters + static_assert([](auto&& v) { + return requires + { + ripple::visit<1, 3>(v, [](auto&, auto) constexpr {}); + }; + }(s1)); + + // Can use fn with conversion of version parameter + static_assert([](auto&& v) { + return requires + { + ripple::visit<1, 3>(v, [](auto&, std::size_t) constexpr {}); + }; + }(s1)); + + // Cannot use fn with const parameter + static_assert([](auto&& v) { + return !requires + { + ripple::visit<1, 3>( + v, [](Json::Value const&, auto) constexpr {}); + }; + }(const_cast const&>(s1))); + + // Cannot call visit with size mismatch + static_assert([](auto&& v) { + return !requires + { + ripple::visit<1, 2>( + v, [](Json::Value&, unsigned int) constexpr {}); + }; + }(s1)); + + // Cannot call visit with version offset + static_assert([](auto&& v) { + return !requires + { + ripple::visit<0, 2>( + v, [](Json::Value&, unsigned int) constexpr {}); + }; + }(s1)); + + // Cannot call visit with size mismatch + static_assert([](auto&& v) { + return !requires + { + ripple::visit<1, 4>( + v, [](Json::Value&, unsigned int) constexpr {}); + }; + }(s1)); + + // Cannot call visit with wrong order of versions + static_assert([](auto&& v) { + return !requires + { + ripple::visit<3, 1>( + v, [](Json::Value&, unsigned int) constexpr {}); + }; + }(s1)); + } } };