refactor: Dual path for rpc handlers (#3198)

Author: godexsoft

src/rpc/RPCHelpers.cpp

@@ -27,6 +27,7 @@
 #include <boost/lexical_cast/bad_lexical_cast.hpp>
 #include <fmt/format.h>
 #include <rpcspec/Errors.hpp>
+#include <rpcspec/Ledger.hpp>
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/StringUtilities.h>
 #include <xrpl/basics/base_uint.h>
@@ -547,6 +548,47 @@ getLedgerHeaderFromHashOrSeq(
     return *lgrInfo;
 }
 
+std::expected<xrpl::LedgerHeader, Status>
+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<xrpl::uint256>(resolved.value), yield);
+        if (not maybeLgrInfo.has_value() or maybeLgrInfo->seq > maxSeq)
+            return err;
+
+        return *maybeLgrInfo;
+    }
+
+    if (resolved.isShortcut()) {
+        auto const shortcut = std::get<rpc::spec::LedgerShortcut>(resolved.value);
+        ASSERT(
+            shortcut == rpc::spec::LedgerShortcut::Validated,
+            "current/closed ledgers must be forwarded before dispatch"
+        );
+    }
+
+    auto const ledgerSequence = resolved.isSequence() ? std::get<uint32_t>(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<unsigned char>
 ledgerHeaderToBlob(xrpl::LedgerHeader const& info, bool includeHash)
 {

src/rpc/RPCHelpers.hpp

@@ -22,6 +22,7 @@
 #include <boost/regex.hpp>
 #include <boost/regex/v5/regex_match.hpp>
 #include <fmt/format.h>
+#include <rpcspec/Ledger.hpp>
 #include <xrpl/basics/Number.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/json/json_value.h>
@@ -58,6 +59,7 @@
 #include <memory>
 #include <optional>
 #include <string>
+#include <string_view>
 #include <tuple>
 #include <utility>
 #include <vector>
@@ -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<xrpl::LedgerHeader, Status>
+getLedgerHeaderFromLedgerSpecifier(
+    BackendInterface const& backend,
+    boost::asio::yield_context yield,
+    rpc::spec::LedgerSpecifier const& ledger,
+    uint32_t maxSeq
+);
+
 /**
  * @brief Traverse nodes owned by an account
  *

src/rpc/common/Concepts.hpp

@@ -7,8 +7,12 @@
 #include <boost/json/value.hpp>
 #include <boost/json/value_from.hpp>
 #include <boost/json/value_to.hpp>
+#include <rpcspec/Errors.hpp>
+#include <rpcspec/RpcSpecView.hpp>
 
+#include <concepts>
 #include <cstdint>
+#include <expected>
 #include <optional>
 #include <string>
 
@@ -71,17 +75,46 @@ concept SomeHandlerWithInput = requires(T a, uint32_t version) {
     { a.spec(version) } -> std::same_as<RpcSpec const&>;
 } and SomeContextProcessWithInput<T> and boost::json::has_value_to<typename T::Input>::value;
 
+/**
+ * @brief Specifies what a Handler validated by the shared consteval spec must provide.
+ *
+ * Such a handler inherits @c rpc::spec::HandlerFor<Input> 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 <typename T>
+concept SomeHandlerWithTypedInput = requires(uint32_t version, boost::json::value jv) {
+    typename T::Input;
+    { T::parseInput(jv, version) } -> std::same_as<std::expected<typename T::Input, Status>>;
+    { T::spec(version) } -> std::same_as<rpc::spec::RpcSpecView>;
+} and SomeContextProcessWithInput<T>;
+
 /**
  * @brief Specifies what a Handler without Input must provide.
  */
 template <typename T>
 concept SomeHandlerWithoutInput = SomeContextProcessWithoutInput<T>;
 
+/**
+ * @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 <typename T>
+constexpr bool kIsSingleInputPath = not(SomeHandlerWithInput<T> and SomeHandlerWithTypedInput<T>);
+
 /**
  * @brief Specifies what a Handler type must provide.
  */
 template <typename T>
-concept SomeHandler = (SomeHandlerWithInput<T> or SomeHandlerWithoutInput<T>) and
+concept SomeHandler =
+    (SomeHandlerWithInput<T> or SomeHandlerWithTypedInput<T> or SomeHandlerWithoutInput<T>) and
     boost::json::has_value_from<typename T::Output>::value;
 
 }  // namespace rpc

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 <boost/json/value.hpp>
+#include <rpcspec/WarningsToJson.hpp>
+
+#include <utility>
 
 namespace rpc::impl {
 
@@ -19,37 +21,61 @@ struct DefaultProcessor final {
     {
         using boost::json::value_from;
         using boost::json::value_to;
-        if constexpr (SomeHandlerWithInput<HandlerType>) {
-            // first we run validation against specified API version
 
+        static_assert(
+            kIsSingleInputPath<HandlerType>,
+            "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<HandlerType> or SomeHandlerWithInput<HandlerType> or
+                SomeHandlerWithoutInput<HandlerType>,
+            "handler matches none of the branches below"
+        );
+
+        // New `rpc-spec`-based handler
+        if constexpr (SomeHandlerWithTypedInput<HandlerType>) {
+            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<HandlerType>) {
+            // 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<typename HandlerType::Input>(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<HandlerType>) {
+        }
+
+        if constexpr (SomeHandlerWithoutInput<HandlerType>) {
             // 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<HandlerType>);
         }
     }
 };

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

tests/common/rpc/FakesAndMocks.cpp

@@ -0,0 +1,6 @@
+#include "rpc/FakesAndMocks.hpp"
+
+#include <rpcspec/HandlerFor.hpp>
+#include <rpcspec/HandlerForDefs.hpp>  // IWYU pragma: keep
+
+template struct rpc::spec::HandlerFor<tests::common::typed_fake::TypedInput>;

tests/common/rpc/FakesAndMocks.hpp

@@ -10,6 +10,13 @@
 #include <boost/json/value_from.hpp>
 #include <boost/json/value_to.hpp>
 #include <gmock/gmock.h>
+#include <rpcspec/Aliases.hpp>
+#include <rpcspec/Converters.hpp>
+#include <rpcspec/Errors.hpp>
+#include <rpcspec/FieldSpec.hpp>
+#include <rpcspec/HandlerFor.hpp>
+#include <rpcspec/Typed.hpp>
+#include <rpcspec/VersionedSpec.hpp>
 
 #include <cstdint>
 #include <optional>
@@ -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<uint32_t> limit;
+};
+
+inline constexpr auto kInputSpec = rpc::spec::spec<TypedInput>(
+    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<TypedInput>(kInputSpec);
+
+[[nodiscard]] constexpr auto const&
+specFor(TypedInput const*) noexcept
+{
+    return kSpec;
+}
+
+}  // namespace typed_fake
+
+class TypedHandlerFake : public rpc::spec::HandlerFor<typed_fake::TypedInput> {
+public:
+    using Output = TestOutput;
+    using Result = rpc::HandlerReturnType<Output>;
+
+    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<typed_fake::TypedInput> {
+public:
+    using Output = TestOutput;
+    using Result = rpc::HandlerReturnType<Output>;
+
+    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