diff --git a/src/rpc/RPCHelpers.cpp b/src/rpc/RPCHelpers.cpp index 2c95a64e1..d9720f585 100644 --- a/src/rpc/RPCHelpers.cpp +++ b/src/rpc/RPCHelpers.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -547,6 +548,47 @@ getLedgerHeaderFromHashOrSeq( return *lgrInfo; } +std::expected +getLedgerHeaderFromLedgerSpecifier( + BackendInterface const& backend, + boost::asio::yield_context yield, + rpc::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 maybeLgrInfo = + backend.fetchLedgerByHash(std::get(resolved.value), yield); + if (not maybeLgrInfo.has_value() or maybeLgrInfo->seq > maxSeq) + return err; + + return *maybeLgrInfo; + } + + if (resolved.isShortcut()) { + auto const shortcut = std::get(resolved.value); + ASSERT( + shortcut == rpc::spec::LedgerShortcut::Validated, + "current/closed ledgers must be forwarded before dispatch" + ); + } + + auto const ledgerSequence = resolved.isSequence() ? std::get(resolved.value) : maxSeq; + + // return without hitting the db + if (ledgerSequence > maxSeq) + return err; + + auto const maybeLgrInfo = backend.fetchLedgerBySequence(ledgerSequence, yield); + if (not maybeLgrInfo.has_value()) + return err; + + return *maybeLgrInfo; +} + std::vector ledgerHeaderToBlob(xrpl::LedgerHeader const& info, bool includeHash) { diff --git a/src/rpc/RPCHelpers.hpp b/src/rpc/RPCHelpers.hpp index e649809b9..0e10dfbf0 100644 --- a/src/rpc/RPCHelpers.hpp +++ b/src/rpc/RPCHelpers.hpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -58,6 +59,7 @@ #include #include #include +#include #include #include #include @@ -299,6 +301,35 @@ 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. + * + * A @c validated shortcut resolves to @p maxSeq, which is what it means for a server that + * only serves validated data. An unspecified ledger resolves via + * @c LedgerSpecifier::resolved(), which the spec library fixes to @c validated for Clio. + * + * @c current and @c closed cannot reach here: @ref specifiesCurrentOrClosedLedger forwards + * those upstream before dispatch. + * + * @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, + rpc::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..df7be4d52 100644 --- a/src/rpc/common/Concepts.hpp +++ b/src/rpc/common/Concepts.hpp @@ -7,8 +7,12 @@ #include #include #include +#include +#include +#include #include +#include #include #include @@ -71,17 +75,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 @c RpcSpecView. Presence of @c parseInput is what + * selects this path over @c 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. @c 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 @c 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..1604af1d2 100644 --- a/src/rpc/common/impl/Processors.hpp +++ b/src/rpc/common/impl/Processors.hpp @@ -2,9 +2,11 @@ #include "rpc/common/Concepts.hpp" #include "rpc/common/Types.hpp" -#include "util/UnsupportedType.hpp" #include +#include + +#include namespace rpc::impl { @@ -19,37 +21,61 @@ struct DefaultProcessor final { { using boost::json::value_from; using boost::json::value_to; - if constexpr (SomeHandlerWithInput) { - // first we run validation against specified API version + 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" + ); + static_assert( + SomeHandlerWithTypedInput or SomeHandlerWithInput or + SomeHandlerWithoutInput, + "handler matches none of the branches below" + ); + + // New `rpc-spec`-based handler + if constexpr (SomeHandlerWithTypedInput) { + auto input = HandlerType::parseInput(value, ctx.apiVersion); + auto warnings = rpc::spec::toJsonArray(HandlerType::spec(ctx.apiVersion).check(value)); + + if (not input.has_value()) + return ReturnType{Error{std::move(input).error()}, std::move(warnings)}; + + auto ret = handler.process(*input, ctx); + + if (not ret.has_value()) + return ReturnType{Error{std::move(ret).error()}, std::move(warnings)}; + + return ReturnType{value_from(std::move(ret).value()), std::move(warnings)}; + } + + if constexpr (SomeHandlerWithInput) { + // Old spec-based handler: first we run validation against specified API version + // TODO: This will be eventually removed once fully migraded to new rpc-spec system. auto const spec = handler.spec(ctx.apiVersion); auto warnings = spec.check(value); auto input = value; // copy here, spec require mutable data - if (auto const ret = spec.process(input); not ret) + if (auto const ret = spec.process(input); not ret.has_value()) return ReturnType{Error{ret.error()}, std::move(warnings)}; // forward Status auto const inData = value_to(input); auto ret = handler.process(inData, ctx); // real handler is given expected Input, not json - if (!ret) { - return ReturnType{ - Error{std::move(ret).error()}, std::move(warnings) - }; // forward Status - } + if (not ret.has_value()) + return ReturnType{Error{std::move(ret).error()}, std::move(warnings)}; + return ReturnType{value_from(std::move(ret).value()), std::move(warnings)}; - } else if constexpr (SomeHandlerWithoutInput) { + } + + if constexpr (SomeHandlerWithoutInput) { // no input to pass, ignore the value auto const ret = handler.process(ctx); - if (not ret) { + if (not ret.has_value()) return ReturnType{Error{ret.error()}}; // forward Status - } + return ReturnType{value_from(ret.value())}; - } else { - // when concept SomeHandlerWithInput and SomeHandlerWithoutInput not cover all Handler - // case - static_assert(util::Unsupported); } } }; diff --git a/tests/common/CMakeLists.txt b/tests/common/CMakeLists.txt index 51b41e6a6..6c093112a 100644 --- a/tests/common/CMakeLists.txt +++ b/tests/common/CMakeLists.txt @@ -3,6 +3,7 @@ add_library(clio_testing_common) target_sources( clio_testing_common PRIVATE + rpc/FakesAndMocks.cpp util/AssignRandomPort.cpp util/BinaryTestObject.cpp util/CallWithTimeout.cpp diff --git a/tests/common/rpc/FakesAndMocks.cpp b/tests/common/rpc/FakesAndMocks.cpp new file mode 100644 index 000000000..62a824527 --- /dev/null +++ b/tests/common/rpc/FakesAndMocks.cpp @@ -0,0 +1,6 @@ +#include "rpc/FakesAndMocks.hpp" + +#include +#include // IWYU pragma: keep + +template struct rpc::spec::HandlerFor; diff --git a/tests/common/rpc/FakesAndMocks.hpp b/tests/common/rpc/FakesAndMocks.hpp index e277b68d6..62a0765b2 100644 --- a/tests/common/rpc/FakesAndMocks.hpp +++ b/tests/common/rpc/FakesAndMocks.hpp @@ -10,6 +10,13 @@ #include #include #include +#include +#include +#include +#include +#include +#include +#include #include #include @@ -153,4 +160,54 @@ 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); + +[[nodiscard]] constexpr auto const& +specFor(TypedInput const*) noexcept +{ + return kSpec; +} + +} // namespace typed_fake + +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))}; + } +}; + +class FailingTypedHandlerFake : public rpc::spec::HandlerFor { +public: + using Output = TestOutput; + using Result = rpc::HandlerReturnType; + + static Result + process([[maybe_unused]] Input const& input, [[maybe_unused]] rpc::Context const& ctx) + { + return rpc::Error{rpc::Status{"Very custom error"}}; + } +}; + } // namespace tests::common diff --git a/tests/unit/rpc/RPCHelpersTests.cpp b/tests/unit/rpc/RPCHelpersTests.cpp index 55fce5e97..990e00286 100644 --- a/tests/unit/rpc/RPCHelpersTests.cpp +++ b/tests/unit/rpc/RPCHelpersTests.cpp @@ -7,6 +7,7 @@ #include "util/AsioContextTestFixture.hpp" #include "util/LoggerFixtures.hpp" #include "util/MockAmendmentCenter.hpp" +#include "util/MockAssert.hpp" #include "util/MockBackendTestFixture.hpp" #include "util/MockPrometheus.hpp" #include "util/NameGenerator.hpp" @@ -26,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -2058,3 +2060,153 @@ INSTANTIATE_TEST_SUITE_P( ), tests::util::kNameGenerator ); + +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, rpc::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, rpc::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, rpc::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, rpc::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, + rpc::spec::LedgerSpecifier{uint32_t{kSpecifierRangeMax + 1}}, + kSpecifierRangeMax + ); + ASSERT_FALSE(res.has_value()); + EXPECT_EQ(res.error().message, "ledgerNotFound"); + }); +} + +TEST_F(RPCHelpersTest, LedgerHeaderFromSpecifierValidatedUsesMaxSeq) +{ + EXPECT_CALL(*backend_, fetchLedgerBySequence(kSpecifierRangeMax, _)) + .WillOnce(Return(createLedgerHeader(kIndex1, kSpecifierRangeMax))); + + runSpawn([&, this](auto yield) { + auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, + yield, + rpc::spec::LedgerSpecifier{rpc::spec::LedgerShortcut::Validated}, + kSpecifierRangeMax + ); + ASSERT_TRUE(res.has_value()); + EXPECT_EQ(res->seq, kSpecifierRangeMax); + }); +} + +struct RPCHelpersAssertTest : RPCHelpersTest, common::util::WithMockAssert {}; + +TEST_F(RPCHelpersAssertTest, LedgerHeaderFromSpecifierCurrentAsserts) +{ + EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(0); + + runSpawn([&, this](auto yield) { + EXPECT_CLIO_ASSERT_FAIL_WITH_MESSAGE( + { + [[maybe_unused]] auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, + yield, + rpc::spec::LedgerSpecifier{rpc::spec::LedgerShortcut::Current}, + kSpecifierRangeMax + ); + }, + "must be forwarded before dispatch" + ); + }); +} + +TEST_F(RPCHelpersAssertTest, LedgerHeaderFromSpecifierClosedAsserts) +{ + EXPECT_CALL(*backend_, fetchLedgerBySequence).Times(0); + + runSpawn([&, this](auto yield) { + EXPECT_CLIO_ASSERT_FAIL_WITH_MESSAGE( + { + [[maybe_unused]] auto const res = getLedgerHeaderFromLedgerSpecifier( + *backend_, + yield, + rpc::spec::LedgerSpecifier{rpc::spec::LedgerShortcut::Closed}, + kSpecifierRangeMax + ); + }, + "must be forwarded before dispatch" + ); + }); +} + +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, rpc::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..5c0db6296 100644 --- a/tests/unit/rpc/handlers/DefaultProcessorTests.cpp +++ b/tests/unit/rpc/handlers/DefaultProcessorTests.cpp @@ -1,4 +1,6 @@ +#include "rpc/Errors.hpp" #include "rpc/FakesAndMocks.hpp" +#include "rpc/common/Concepts.hpp" #include "rpc/common/Specs.hpp" #include "rpc/common/Types.hpp" #include "rpc/common/Validators.hpp" @@ -67,3 +69,101 @@ TEST_F(RPCDefaultProcessorTest, InvalidInput) EXPECT_TRUE(ret.warnings.empty()); }); } + +static_assert(SomeHandlerWithTypedInput); +static_assert(not SomeHandlerWithInput); +static_assert(SomeHandlerWithTypedInput); +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_HandlerReturnsError_ForwardsError) +{ + runSpawn([](auto yield) { + FailingTypedHandlerFake 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_FALSE(ret); + EXPECT_EQ(rpc::makeError(ret.result.error()).at("error").as_string(), "Very custom error"); + EXPECT_TRUE(ret.warnings.empty()); + }); +} + +TEST_F(RPCDefaultProcessorTest, NewSpecHandler_HandlerReturnsError_StillForwardsWarnings) +{ + runSpawn([](auto yield) { + FailingTypedHandlerFake 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_FALSE(ret); + EXPECT_EQ(rpc::makeError(ret.result.error()).at("error").as_string(), "Very custom error"); + 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()); + }); +}