From 7f4652751e5ae4eff215d1187b7f0a4c19ab877b Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Tue, 1 Sep 2026 14:43:30 +0100 Subject: [PATCH] Accept spec-library handlers alongside legacy ones --- src/rpc/RPCHelpers.cpp | 37 ++++++ src/rpc/RPCHelpers.hpp | 29 +++++ src/rpc/common/Concepts.hpp | 33 ++++- src/rpc/common/impl/Processors.hpp | 28 ++++- tests/common/rpc/FakesAndMocks.hpp | 48 ++++++++ tests/unit/rpc/RPCHelpersTests.cpp | 114 ++++++++++++++++++ .../rpc/handlers/DefaultProcessorTests.cpp | 70 +++++++++++ 7 files changed, 357 insertions(+), 2 deletions(-) diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 2c95a64e1..e3ccc9528 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -82,6 +83,7 @@ #include #include #include +#include #include namespace rpc { @@ -547,6 +549,41 @@ getLedgerHeaderFromHashOrSeq( return *lgrInfo; } +std::expected +getLedgerHeaderFromLedgerSpecifier( + BackendInterface const& backend, + boost::asio::yield_context yield, + spec::LedgerSpecifier const& ledger, + uint32_t maxSeq +) +{ + auto const err = std::unexpected{Status{RippledError::RpcLgrNotFound, "ledgerNotFound"}}; + auto const resolved = ledger.resolved(); + + if (resolved.isHash()) { + auto const lgrInfo = + backend.fetchLedgerByHash(std::get(resolved.value), yield); + if (!lgrInfo || lgrInfo->seq > maxSeq) + return err; + + return *lgrInfo; + } + + // A shortcut means the latest validated ledger; see the declaration for why that holds + // for all three of them. + auto const ledgerSequence = resolved.isSequence() ? std::get(resolved.value) : maxSeq; + + // return without hitting the db + if (ledgerSequence > maxSeq) + return err; + + auto const lgrInfo = backend.fetchLedgerBySequence(ledgerSequence, yield); + if (!lgrInfo) + return err; + + return *lgrInfo; +} + std::vector ledgerHeaderToBlob(xrpl::LedgerHeader const& info, bool includeHash) { diff --git a/src/rpc/RPCHelpers.hpp b/src/rpc/RPCHelpers.hpp index e649809b9..a9a77fa8f 100644 --- a/src/rpc/RPCHelpers.hpp +++ b/src/rpc/RPCHelpers.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -299,6 +300,34 @@ getLedgerHeaderFromHashOrSeq( uint32_t maxSeq ); +/** + * @brief Get ledger header from a spec-library ledger specifier. + * + * The strong-typed counterpart of @ref getLedgerHeaderFromHashOrSeq, for handlers whose + * spec produces a @c LedgerSpecifier instead of a ledger_hash / ledger_index pair. + * Behaviour matches that overload: a hash or sequence beyond @p maxSeq, or one absent from + * the backend, yields @c ledgerNotFound. + * + * All three shortcuts resolve to @p maxSeq. Clio only serves validated data, so + * @c validated is the latest validated sequence by definition, and @c current / @c closed + * never arrive here — @ref specifiesCurrentOrClosedLedger forwards those upstream before + * dispatch. An unspecified ledger resolves via @c LedgerSpecifier::resolved(), which the + * spec library fixes to @c validated for Clio. + * + * @param backend The backend to use + * @param yield The coroutine context + * @param ledger The ledger the request selected + * @param maxSeq The maximum sequence to search + * @return The ledger header or an error status + */ +std::expected +getLedgerHeaderFromLedgerSpecifier( + BackendInterface const& backend, + boost::asio::yield_context yield, + spec::LedgerSpecifier const& ledger, + uint32_t maxSeq +); + /** * @brief Traverse nodes owned by an account * diff --git a/src/rpc/common/Concepts.hpp b/src/rpc/common/Concepts.hpp index c455b2ded..330b905ce 100644 --- a/src/rpc/common/Concepts.hpp +++ b/src/rpc/common/Concepts.hpp @@ -7,8 +7,10 @@ #include #include #include +#include #include +#include #include #include @@ -71,17 +73,46 @@ concept SomeHandlerWithInput = requires(T a, uint32_t version) { { a.spec(version) } -> std::same_as; } and SomeContextProcessWithInput and boost::json::has_value_to::value; +/** + * @brief Specifies what a Handler validated by the shared consteval spec must provide. + * + * Such a handler inherits @c rpc::spec::HandlerFor from the spec library, which + * supplies a static @c parseInput (validate and deserialise in one pass) and a static + * @c spec returning a type-erased @ref rpc::spec::RpcSpecView. Presence of @c parseInput + * is what selects this path over @ref SomeHandlerWithInput. + * + * The two input paths are mutually exclusive by construction: a legacy handler returns + * @c RpcSpec @c const& from a non-static @c spec and needs a @c value_to for its Input, + * neither of which holds here. @ref kIsSingleInputPath asserts that below. + */ +template +concept SomeHandlerWithTypedInput = requires(uint32_t version, boost::json::value jv) { + typename T::Input; + { T::parseInput(jv, version) } -> std::same_as>; + { T::spec(version) } -> std::same_as; +} and SomeContextProcessWithInput; + /** * @brief Specifies what a Handler without Input must provide. */ template concept SomeHandlerWithoutInput = SomeContextProcessWithoutInput; +/** + * @brief True when @p T does not straddle the legacy and typed input paths. + * + * Guards the @c if @c constexpr chain in @ref rpc::impl::DefaultProcessor: were a handler + * to satisfy both, the dispatch order alone would silently decide which spec ran. + */ +template +constexpr bool kIsSingleInputPath = not(SomeHandlerWithInput and SomeHandlerWithTypedInput); + /** * @brief Specifies what a Handler type must provide. */ template -concept SomeHandler = (SomeHandlerWithInput or SomeHandlerWithoutInput) and +concept SomeHandler = + (SomeHandlerWithInput or SomeHandlerWithTypedInput or SomeHandlerWithoutInput) and boost::json::has_value_from::value; } // namespace rpc diff --git a/src/rpc/common/impl/Processors.hpp b/src/rpc/common/impl/Processors.hpp index 1242ded03..dc56157ec 100644 --- a/src/rpc/common/impl/Processors.hpp +++ b/src/rpc/common/impl/Processors.hpp @@ -5,6 +5,9 @@ #include "util/UnsupportedType.hpp" #include +#include + +#include namespace rpc::impl { @@ -19,7 +22,30 @@ struct DefaultProcessor final { { using boost::json::value_from; using boost::json::value_to; - if constexpr (SomeHandlerWithInput) { + + static_assert( + kIsSingleInputPath, + "handler satisfies both the legacy and the typed input path; dispatch would be " + "decided by the order of the branches below rather than by the handler" + ); + + if constexpr (SomeHandlerWithTypedInput) { + // The shared consteval spec validates and deserializes in a single pass, so there + // is no separate process() step here: RpcSpecView::process() is a no-op for a + // TypedSpec. check() still runs separately because warnings are collected against + // the request as sent, and must be forwarded even when parsing then fails. + auto warnings = spec::toJsonArray(HandlerType::spec(ctx.apiVersion).check(value)); + + auto input = HandlerType::parseInput(value, ctx.apiVersion); + if (not input) + return ReturnType{Error{std::move(input).error()}, std::move(warnings)}; + + auto ret = handler.process(*input, ctx); + if (not ret) + return ReturnType{Error{std::move(ret).error()}, std::move(warnings)}; + + return ReturnType{value_from(std::move(ret).value()), std::move(warnings)}; + } else if constexpr (SomeHandlerWithInput) { // first we run validation against specified API version auto const spec = handler.spec(ctx.apiVersion); diff --git a/tests/common/rpc/FakesAndMocks.hpp b/tests/common/rpc/FakesAndMocks.hpp index e277b68d6..f44781feb 100644 --- a/tests/common/rpc/FakesAndMocks.hpp +++ b/tests/common/rpc/FakesAndMocks.hpp @@ -10,6 +10,12 @@ #include #include #include +#include +#include +#include +#include +#include +#include #include #include @@ -153,4 +159,46 @@ struct HandlerWithoutInputMock { MOCK_METHOD(Result, process, (rpc::Context const&), (const)); }; +// The shared consteval spec resolves a handler's spec from its Input type via an ADL +// `specFor` hook, so the fake Input below needs its own namespace to host that hook. +namespace typed_fake { + +// input data for TypedHandlerFake; mirrors TestInput so the two paths stay comparable +struct TypedInput { + std::string hello; + std::optional limit; +}; + +inline constexpr auto kInputSpec = rpc::spec::spec( + rpc::spec::field("hello", &TypedInput::hello, rpc::spec::required, rpc::spec::asString), + rpc::spec::field("limit", &TypedInput::limit, rpc::spec::asUint32), + rpc::spec::field("old_field", rpc::spec::deprecated) +); + +inline constexpr auto kSpec = rpc::spec::versioned(kInputSpec); + +/** @brief ADL hook: resolve the versioned spec from the Input type. */ +[[nodiscard]] constexpr auto const& +specFor(TypedInput const*) noexcept +{ + return kSpec; +} + +} // namespace typed_fake + +// example handler validated by the shared consteval spec rather than by rpc::RpcSpec. +// Note it declares no spec() and no Input of its own: both come from HandlerFor, and there +// is no tag_invoke for TypedInput, which is what keeps it off the legacy path. +class TypedHandlerFake : public rpc::spec::HandlerFor { +public: + using Output = TestOutput; + using Result = rpc::HandlerReturnType; + + static Result + process(Input const& input, [[maybe_unused]] rpc::Context const& ctx) + { + return Output{input.hello + '_' + std::to_string(input.limit.value_or(0))}; + } +}; + } // namespace tests::common diff --git a/tests/unit/rpc/RPCHelpersTests.cpp b/tests/unit/rpc/RPCHelpersTests.cpp index 55fce5e97..48ca1b794 100644 --- a/tests/unit/rpc/RPCHelpersTests.cpp +++ b/tests/unit/rpc/RPCHelpersTests.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -2058,3 +2059,116 @@ INSTANTIATE_TEST_SUITE_P( ), tests::util::kNameGenerator ); + +// getLedgerHeaderFromLedgerSpecifier — the strong-typed counterpart of +// getLedgerHeaderFromHashOrSeq. The fixture's range is [10, 300], so kRangeMax below is 300. + +namespace { +constexpr auto kSpecifierRangeMax = 300u; +} // namespace + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierByHash) +{ + auto const expected = createLedgerHeader(kIndex1, 30); + EXPECT_CALL(*backend_, fetchLedgerByHash(xrpl::uint256{kIndex1}, _)).WillOnce(Return(expected)); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, yield, spec::LedgerSpecifier{xrpl::uint256{kIndex1}}, kSpecifierRangeMax + ); + ASSERT_TRUE(res.has_value()); + EXPECT_EQ(res->seq, 30); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierByHashNotFound) +{ + EXPECT_CALL(*backend_, fetchLedgerByHash(xrpl::uint256{kIndex1}, _)) + .WillOnce(Return(std::nullopt)); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, yield, spec::LedgerSpecifier{xrpl::uint256{kIndex1}}, kSpecifierRangeMax + ); + ASSERT_FALSE(res.has_value()); + EXPECT_EQ(res.error().message, "ledgerNotFound"); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierByHashBeyondMaxSeq) +{ + // present in the backend, but newer than the range the caller may serve + EXPECT_CALL(*backend_, fetchLedgerByHash(xrpl::uint256{kIndex1}, _)) + .WillOnce(Return(createLedgerHeader(kIndex1, kSpecifierRangeMax + 1))); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, yield, spec::LedgerSpecifier{xrpl::uint256{kIndex1}}, kSpecifierRangeMax + ); + ASSERT_FALSE(res.has_value()); + EXPECT_EQ(res.error().message, "ledgerNotFound"); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierBySequence) +{ + EXPECT_CALL(*backend_, fetchLedgerBySequence(30, _)) + .WillOnce(Return(createLedgerHeader(kIndex1, 30))); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, yield, spec::LedgerSpecifier{uint32_t{30}}, kSpecifierRangeMax + ); + ASSERT_TRUE(res.has_value()); + EXPECT_EQ(res->seq, 30); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierBySequenceBeyondMaxSeqSkipsBackend) +{ + EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(0); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, + yield, + spec::LedgerSpecifier{uint32_t{kSpecifierRangeMax + 1}}, + kSpecifierRangeMax + ); + ASSERT_FALSE(res.has_value()); + EXPECT_EQ(res.error().message, "ledgerNotFound"); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierShortcutUsesMaxSeq) +{ + EXPECT_CALL(*backend_, fetchLedgerBySequence(kSpecifierRangeMax, _)) + .WillOnce(Return(createLedgerHeader(kIndex1, kSpecifierRangeMax))); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, + yield, + spec::LedgerSpecifier{spec::LedgerShortcut::Validated}, + kSpecifierRangeMax + ); + ASSERT_TRUE(res.has_value()); + EXPECT_EQ(res->seq, kSpecifierRangeMax); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierUnspecifiedResolvesToMaxSeq) +{ + // an unspecified ledger resolves via LedgerSpecifier::resolved(), which the spec library + // fixes to `validated` under RPCSPEC_IS_CLIO + EXPECT_CALL(*backend_, fetchLedgerBySequence(kSpecifierRangeMax, _)) + .WillOnce(Return(createLedgerHeader(kIndex1, kSpecifierRangeMax))); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, yield, spec::LedgerSpecifier{}, kSpecifierRangeMax + ); + ASSERT_TRUE(res.has_value()); + EXPECT_EQ(res->seq, kSpecifierRangeMax); + }); +} diff --git a/tests/unit/rpc/handlers/DefaultProcessorTests.cpp b/tests/unit/rpc/handlers/DefaultProcessorTests.cpp index cb4066ec0..9b900c6d5 100644 --- a/tests/unit/rpc/handlers/DefaultProcessorTests.cpp +++ b/tests/unit/rpc/handlers/DefaultProcessorTests.cpp @@ -67,3 +67,73 @@ TEST_F(RPCDefaultProcessorTest, InvalidInput) EXPECT_TRUE(ret.warnings.empty()); }); } + +// Pin which path each fake takes. Without this, a change that made a typed handler also +// satisfy SomeHandlerWithInput would silently reroute it through the legacy validators and +// every test below would still pass. +static_assert(SomeHandlerWithTypedInput); +static_assert(not SomeHandlerWithInput); +static_assert(SomeHandlerWithInput); +static_assert(not SomeHandlerWithTypedInput); + +// The four tests below exercise the typed path — a handler whose spec, validation and +// deserialization all come from the shared consteval spec via HandlerFor. They run +// against the same DefaultProcessor as the legacy tests above, which is the point: the +// dual path is a dispatch detail, not a second processor. + +TEST_F(RPCDefaultProcessorTest, NewSpecHandler_HappyPath) +{ + runSpawn([](auto yield) { + TypedHandlerFake const handler; + rpc::impl::DefaultProcessor const processor; + + auto const input = boost::json::parse(R"JSON({ "hello": "world", "limit": 42 })JSON"); + + auto const ret = processor(handler, input, Context{yield}); + ASSERT_TRUE(ret); + EXPECT_TRUE(ret.warnings.empty()); + EXPECT_EQ(ret.result.value().at("computed").as_string(), "world_42"); + }); +} + +TEST_F(RPCDefaultProcessorTest, NewSpecHandler_MissingRequiredField_ReturnsError) +{ + runSpawn([](auto yield) { + TypedHandlerFake const handler; + rpc::impl::DefaultProcessor const processor; + + auto const input = boost::json::parse(R"JSON({ "limit": 42 })JSON"); + + auto const ret = processor(handler, input, Context{yield}); + ASSERT_FALSE(ret); + EXPECT_TRUE(ret.warnings.empty()); + }); +} + +TEST_F(RPCDefaultProcessorTest, NewSpecHandler_DeprecatedField_WarningsForwarded) +{ + runSpawn([](auto yield) { + TypedHandlerFake const handler; + rpc::impl::DefaultProcessor const processor; + + auto const input = boost::json::parse(R"JSON({ "hello": "world", "old_field": true })JSON"); + + auto const ret = processor(handler, input, Context{yield}); + ASSERT_TRUE(ret); + EXPECT_EQ(ret.warnings.size(), 1); + }); +} + +TEST_F(RPCDefaultProcessorTest, NewSpecHandler_DeprecatedFieldAbsent_NoWarnings) +{ + runSpawn([](auto yield) { + TypedHandlerFake const handler; + rpc::impl::DefaultProcessor const processor; + + auto const input = boost::json::parse(R"JSON({ "hello": "world" })JSON"); + + auto const ret = processor(handler, input, Context{yield}); + ASSERT_TRUE(ret); + EXPECT_TRUE(ret.warnings.empty()); + }); +}