From 46bd67a9ecb4ac068cae9d74b941842d25f829ac Mon Sep 17 00:00:00 2001 From: Sergey Kuznetsov Date: Tue, 9 Jul 2024 15:22:01 +0100 Subject: [PATCH] feat: More verbose forwarding errors (#1523) Fixes #1516. --- src/etl/ETLState.hpp | 8 ++- src/etl/LoadBalancer.cpp | 22 ++++-- src/etl/LoadBalancer.hpp | 6 +- src/etl/Source.hpp | 6 +- src/etl/impl/ForwardingSource.cpp | 21 ++++-- src/etl/impl/ForwardingSource.hpp | 6 +- src/etl/impl/SourceImpl.hpp | 7 +- src/rpc/Errors.cpp | 5 ++ src/rpc/Errors.hpp | 8 +++ src/rpc/common/impl/ForwardingProxy.hpp | 2 +- src/web/impl/ErrorHandling.hpp | 4 ++ tests/common/util/MockLoadBalancer.hpp | 6 +- tests/common/util/MockSource.hpp | 9 ++- tests/unit/etl/ETLStateTests.cpp | 3 +- tests/unit/etl/ForwardingSourceTests.cpp | 19 +++-- tests/unit/etl/LoadBalancerTests.cpp | 80 ++++++++++++++++++--- tests/unit/etl/SourceImplTests.cpp | 3 +- tests/unit/rpc/ForwardingProxyTests.cpp | 6 +- tests/unit/rpc/handlers/ServerInfoTests.cpp | 8 +-- 19 files changed, 176 insertions(+), 53 deletions(-) diff --git a/src/etl/ETLState.hpp b/src/etl/ETLState.hpp index 91004e40a..d9183f24b 100644 --- a/src/etl/ETLState.hpp +++ b/src/etl/ETLState.hpp @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -46,8 +47,11 @@ struct ETLState { static std::optional fetchETLStateFromSource(Forward& source) noexcept { - auto const serverInfoRippled = data::synchronous([&source](auto yield) { - return source.forwardToRippled({{"command", "server_info"}}, std::nullopt, {}, yield); + auto const serverInfoRippled = data::synchronous([&source](auto yield) -> std::optional { + if (auto result = source.forwardToRippled({{"command", "server_info"}}, std::nullopt, {}, yield)) { + return std::move(result).value(); + } + return std::nullopt; }); if (serverInfoRippled) diff --git a/src/etl/LoadBalancer.cpp b/src/etl/LoadBalancer.cpp index f42a3d1d4..8987cc644 100644 --- a/src/etl/LoadBalancer.cpp +++ b/src/etl/LoadBalancer.cpp @@ -24,6 +24,7 @@ #include "etl/NetworkValidatedLedgersInterface.hpp" #include "etl/Source.hpp" #include "feed/SubscriptionManagerInterface.hpp" +#include "rpc/Errors.hpp" #include "util/Assert.hpp" #include "util/Random.hpp" #include "util/log/Logger.hpp" @@ -39,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -211,7 +213,7 @@ LoadBalancer::fetchLedger( return response; } -std::optional +std::expected LoadBalancer::forwardToRippled( boost::json::object const& request, std::optional const& clientIp, @@ -221,7 +223,7 @@ LoadBalancer::forwardToRippled( { if (forwardingCache_) { if (auto cachedResponse = forwardingCache_->get(request); cachedResponse) { - return cachedResponse; + return std::move(cachedResponse).value(); } } @@ -233,20 +235,26 @@ LoadBalancer::forwardToRippled( auto xUserValue = isAdmin ? ADMIN_FORWARDING_X_USER_VALUE : USER_FORWARDING_X_USER_VALUE; std::optional response; + rpc::ClioError error = rpc::ClioError::etlCONNECTION_ERROR; while (numAttempts < sources_.size()) { - if (auto res = sources_[sourceIdx]->forwardToRippled(request, clientIp, xUserValue, yield)) { - response = std::move(res); + auto res = sources_[sourceIdx]->forwardToRippled(request, clientIp, xUserValue, yield); + if (res) { + response = std::move(res).value(); break; } + error = std::max(error, res.error()); // Choose the best result between all sources sourceIdx = (sourceIdx + 1) % sources_.size(); ++numAttempts; } - if (response and forwardingCache_ and not response->contains("error")) - forwardingCache_->put(request, *response); + if (response) { + if (forwardingCache_ and not response->contains("error")) + forwardingCache_->put(request, *response); + return std::move(response).value(); + } - return response; + return std::unexpected{error}; } boost::json::value diff --git a/src/etl/LoadBalancer.hpp b/src/etl/LoadBalancer.hpp index 064746cbf..e2b86118a 100644 --- a/src/etl/LoadBalancer.hpp +++ b/src/etl/LoadBalancer.hpp @@ -25,6 +25,7 @@ #include "etl/Source.hpp" #include "etl/impl/ForwardingCache.hpp" #include "feed/SubscriptionManagerInterface.hpp" +#include "rpc/Errors.hpp" #include "util/config/Config.hpp" #include "util/log/Logger.hpp" @@ -41,6 +42,7 @@ #include #include #include +#include #include #include #include @@ -181,9 +183,9 @@ class LoadBalancer { * @param clientIp The IP address of the peer, if known * @param isAdmin Whether the request is from an admin * @param yield The coroutine context - * @return Response received from rippled node as JSON object on success; nullopt on failure + * @return Response received from rippled node as JSON object on success or error on failure */ - std::optional + std::expected forwardToRippled( boost::json::object const& request, std::optional const& clientIp, diff --git a/src/etl/Source.hpp b/src/etl/Source.hpp index 70c637b1c..8c2f69e24 100644 --- a/src/etl/Source.hpp +++ b/src/etl/Source.hpp @@ -22,6 +22,7 @@ #include "data/BackendInterface.hpp" #include "etl/NetworkValidatedLedgersInterface.hpp" #include "feed/SubscriptionManagerInterface.hpp" +#include "rpc/Errors.hpp" #include "util/config/Config.hpp" #include "util/log/Logger.hpp" @@ -34,6 +35,7 @@ #include #include +#include #include #include #include @@ -131,9 +133,9 @@ class SourceBase { * @param forwardToRippledClientIp IP of the client forwarding this request if known * @param xUserValue Value of the X-User header * @param yield The coroutine context - * @return Response wrapped in an optional on success; nullopt otherwise + * @return Response on success or error on failure */ - virtual std::optional + virtual std::expected forwardToRippled( boost::json::object const& request, std::optional const& forwardToRippledClientIp, diff --git a/src/etl/impl/ForwardingSource.cpp b/src/etl/impl/ForwardingSource.cpp index bff743433..ebda3a155 100644 --- a/src/etl/impl/ForwardingSource.cpp +++ b/src/etl/impl/ForwardingSource.cpp @@ -19,6 +19,7 @@ #include "etl/impl/ForwardingSource.hpp" +#include "rpc/Errors.hpp" #include "util/log/Logger.hpp" #include @@ -55,7 +56,7 @@ ForwardingSource::ForwardingSource( ); } -std::optional +std::expected ForwardingSource::forwardToRippled( boost::json::object const& request, std::optional const& forwardToRippledClientIp, @@ -74,18 +75,26 @@ ForwardingSource::forwardToRippled( auto expectedConnection = connectionBuilder.connect(yield); if (not expectedConnection) { - return std::nullopt; + LOG(log_.debug()) << "Couldn't connect to rippled to forward request."; + return std::unexpected{rpc::ClioError::etlCONNECTION_ERROR}; } auto& connection = expectedConnection.value(); auto writeError = connection->write(boost::json::serialize(request), yield, forwardingTimeout_); if (writeError) { - return std::nullopt; + LOG(log_.debug()) << "Error sending request to rippled to forward request."; + return std::unexpected{rpc::ClioError::etlREQUEST_ERROR}; } auto response = connection->read(yield, forwardingTimeout_); if (not response) { - return std::nullopt; + if (auto errorCode = response.error().errorCode(); + errorCode.has_value() and errorCode->value() == boost::system::errc::timed_out) { + LOG(log_.debug()) << "Request to rippled timed out"; + return std::unexpected{rpc::ClioError::etlREQUEST_TIMEOUT}; + } + LOG(log_.debug()) << "Error sending request to rippled to forward request."; + return std::unexpected{rpc::ClioError::etlREQUEST_ERROR}; } boost::json::value parsedResponse; @@ -94,8 +103,8 @@ ForwardingSource::forwardToRippled( if (not parsedResponse.is_object()) throw std::runtime_error("response is not an object"); } catch (std::exception const& e) { - LOG(log_.error()) << "Error parsing response from rippled: " << e.what() << ". Response: " << *response; - return std::nullopt; + LOG(log_.debug()) << "Error parsing response from rippled: " << e.what() << ". Response: " << *response; + return std::unexpected{rpc::ClioError::etlINVALID_RESPONSE}; } auto responseObject = parsedResponse.as_object(); diff --git a/src/etl/impl/ForwardingSource.hpp b/src/etl/impl/ForwardingSource.hpp index ee14fa178..efdb425fa 100644 --- a/src/etl/impl/ForwardingSource.hpp +++ b/src/etl/impl/ForwardingSource.hpp @@ -19,6 +19,7 @@ #pragma once +#include "rpc/Errors.hpp" #include "util/log/Logger.hpp" #include "util/requests/WsConnection.hpp" @@ -26,6 +27,7 @@ #include #include +#include #include #include #include @@ -54,9 +56,9 @@ class ForwardingSource { * @param forwardToRippledClientIp IP of the client forwarding this request if known * @param xUserValue Optional value for X-User header * @param yield The coroutine context - * @return Response wrapped in an optional on success; nullopt otherwise + * @return Response on success or error on failure */ - std::optional + std::expected forwardToRippled( boost::json::object const& request, std::optional const& forwardToRippledClientIp, diff --git a/src/etl/impl/SourceImpl.hpp b/src/etl/impl/SourceImpl.hpp index a24ddff5d..7302bfa4c 100644 --- a/src/etl/impl/SourceImpl.hpp +++ b/src/etl/impl/SourceImpl.hpp @@ -23,6 +23,7 @@ #include "etl/impl/ForwardingSource.hpp" #include "etl/impl/GrpcSource.hpp" #include "etl/impl/SubscriptionSource.hpp" +#include "rpc/Errors.hpp" #include #include @@ -31,12 +32,14 @@ #include #include +#include #include #include #include #include #include #include + namespace etl::impl { /** @@ -205,9 +208,9 @@ class SourceImpl : public SourceBase { * @param forwardToRippledClientIp IP of the client forwarding this request if known * @param xUserValue Optional value of the X-User header * @param yield The coroutine context - * @return Response wrapped in an optional on success; nullopt otherwise + * @return Response or ClioError */ - std::optional + std::expected forwardToRippled( boost::json::object const& request, std::optional const& forwardToRippledClientIp, diff --git a/src/rpc/Errors.cpp b/src/rpc/Errors.cpp index e9d21aaf9..5d47b8e0d 100644 --- a/src/rpc/Errors.cpp +++ b/src/rpc/Errors.cpp @@ -99,6 +99,11 @@ getErrorInfo(ClioError code) {ClioError::rpcCOMMAND_NOT_STRING, "commandNotString", "Method is not a string."}, {ClioError::rpcCOMMAND_IS_EMPTY, "emptyCommand", "Method is an empty string."}, {ClioError::rpcPARAMS_UNPARSEABLE, "paramsUnparseable", "Params must be an array holding exactly one object."}, + // etl related errors + {ClioError::etlCONNECTION_ERROR, "connectionError", "Couldn't connect to rippled."}, + {ClioError::etlREQUEST_ERROR, "requestError", "Error sending request to rippled."}, + {ClioError::etlREQUEST_TIMEOUT, "timeout", "Request to rippled timed out."}, + {ClioError::etlINVALID_RESPONSE, "invalidResponse", "Rippled returned an invalid response."} }; auto matchByCode = [code](auto const& info) { return info.code == code; }; diff --git a/src/rpc/Errors.hpp b/src/rpc/Errors.hpp index 0a681395b..7ddbd1479 100644 --- a/src/rpc/Errors.hpp +++ b/src/rpc/Errors.hpp @@ -50,6 +50,14 @@ enum class ClioError { rpcCOMMAND_NOT_STRING = 6002, rpcCOMMAND_IS_EMPTY = 6003, rpcPARAMS_UNPARSEABLE = 6004, + + // TODO: Since it is not only rpc errors here now, we should move it to util + // etl related errors start with 7000 + // Higher value in this errors means better progress in the forwarding + etlCONNECTION_ERROR = 7000, + etlREQUEST_ERROR = 7001, + etlREQUEST_TIMEOUT = 7002, + etlINVALID_RESPONSE = 7003, }; /** @brief Holds info about a particular @ref ClioError. */ diff --git a/src/rpc/common/impl/ForwardingProxy.hpp b/src/rpc/common/impl/ForwardingProxy.hpp index ea989a04d..d6bc4995b 100644 --- a/src/rpc/common/impl/ForwardingProxy.hpp +++ b/src/rpc/common/impl/ForwardingProxy.hpp @@ -96,7 +96,7 @@ class ForwardingProxy { auto res = balancer_->forwardToRippled(toForward, ctx.clientIp, ctx.isAdmin, ctx.yield); if (not res) { notifyFailedToForward(ctx.method); - return Result{Status{RippledError::rpcFAILED_TO_FORWARD}}; + return Result{Status{CombinedError{res.error()}}}; } notifyForwarded(ctx.method); diff --git a/src/web/impl/ErrorHandling.hpp b/src/web/impl/ErrorHandling.hpp index 14411835f..464e9bdc9 100644 --- a/src/web/impl/ErrorHandling.hpp +++ b/src/web/impl/ErrorHandling.hpp @@ -90,6 +90,10 @@ class ErrorHelper { case rpc::ClioError::rpcINVALID_HOT_WALLET: case rpc::ClioError::rpcFIELD_NOT_FOUND_TRANSACTION: case rpc::ClioError::rpcMALFORMED_ORACLE_DOCUMENT_ID: + case rpc::ClioError::etlCONNECTION_ERROR: + case rpc::ClioError::etlREQUEST_ERROR: + case rpc::ClioError::etlREQUEST_TIMEOUT: + case rpc::ClioError::etlINVALID_RESPONSE: ASSERT( false, "Unknown rpc error code {}", static_cast(*clioCode) ); // this should never happen diff --git a/tests/common/util/MockLoadBalancer.hpp b/tests/common/util/MockLoadBalancer.hpp index 60749bb80..c51e25db1 100644 --- a/tests/common/util/MockLoadBalancer.hpp +++ b/tests/common/util/MockLoadBalancer.hpp @@ -19,6 +19,7 @@ #pragma once +#include "rpc/Errors.hpp" #include "util/FakeFetchResponse.hpp" #include @@ -28,6 +29,7 @@ #include #include +#include #include #include @@ -37,8 +39,10 @@ struct MockLoadBalancer { MOCK_METHOD(void, loadInitialLedger, (std::uint32_t, bool), ()); MOCK_METHOD(std::optional, fetchLedger, (uint32_t, bool, bool), ()); MOCK_METHOD(boost::json::value, toJson, (), (const)); + + using ForwardToRippledReturnType = std::expected; MOCK_METHOD( - std::optional, + ForwardToRippledReturnType, forwardToRippled, (boost::json::object const&, std::optional const&, bool, boost::asio::yield_context), (const) diff --git a/tests/common/util/MockSource.hpp b/tests/common/util/MockSource.hpp index ca23b6c70..523d06dc2 100644 --- a/tests/common/util/MockSource.hpp +++ b/tests/common/util/MockSource.hpp @@ -19,10 +19,10 @@ #pragma once #include "data/BackendInterface.hpp" -#include "etl/ETLHelpers.hpp" #include "etl/NetworkValidatedLedgersInterface.hpp" #include "etl/Source.hpp" #include "feed/SubscriptionManagerInterface.hpp" +#include "rpc/Errors.hpp" #include "util/config/Config.hpp" #include @@ -38,6 +38,7 @@ #include #include #include +#include #include #include #include @@ -59,8 +60,10 @@ struct MockSource : etl::SourceBase { (override) ); MOCK_METHOD((std::pair, bool>), loadInitialLedger, (uint32_t, uint32_t, bool), (override)); + + using ForwardToRippledReturnType = std::expected; MOCK_METHOD( - std::optional, + ForwardToRippledReturnType, forwardToRippled, (boost::json::object const&, std::optional const&, std::string_view, boost::asio::yield_context), (const, override) @@ -127,7 +130,7 @@ class MockSourceWrapper : public etl::SourceBase { return mock_->loadInitialLedger(sequence, maxLedger, getObjects); } - std::optional + std::expected forwardToRippled( boost::json::object const& request, std::optional const& forwardToRippledClientIp, diff --git a/tests/unit/etl/ETLStateTests.cpp b/tests/unit/etl/ETLStateTests.cpp index 2eed30376..d189d8bf4 100644 --- a/tests/unit/etl/ETLStateTests.cpp +++ b/tests/unit/etl/ETLStateTests.cpp @@ -18,6 +18,7 @@ //============================================================================== #include "etl/ETLState.hpp" +#include "rpc/Errors.hpp" #include "util/LoggerFixtures.hpp" #include "util/MockSource.hpp" @@ -37,7 +38,7 @@ struct ETLStateTest : public NoLoggerFixture { TEST_F(ETLStateTest, Error) { - EXPECT_CALL(source, forwardToRippled).WillOnce(Return(std::nullopt)); + EXPECT_CALL(source, forwardToRippled).WillOnce(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE})); auto const state = etl::ETLState::fetchETLStateFromSource(source); EXPECT_FALSE(state); } diff --git a/tests/unit/etl/ForwardingSourceTests.cpp b/tests/unit/etl/ForwardingSourceTests.cpp index e622cf27d..c3c7b0db5 100644 --- a/tests/unit/etl/ForwardingSourceTests.cpp +++ b/tests/unit/etl/ForwardingSourceTests.cpp @@ -18,6 +18,7 @@ //============================================================================== #include "etl/impl/ForwardingSource.hpp" +#include "rpc/Errors.hpp" #include "util/AsioContextTestFixture.hpp" #include "util/TestWsServer.hpp" @@ -50,7 +51,8 @@ TEST_F(ForwardingSourceTests, ConnectionFailed) { runSpawn([&](boost::asio::yield_context yield) { auto result = forwardingSource.forwardToRippled({}, {}, {}, yield); - EXPECT_FALSE(result); + ASSERT_FALSE(result); + EXPECT_EQ(result.error(), rpc::ClioError::etlCONNECTION_ERROR); }); } @@ -90,7 +92,8 @@ TEST_F(ForwardingSourceOperationsTests, XUserHeader) runSpawn([&](boost::asio::yield_context yield) { auto result = forwardingSource.forwardToRippled(boost::json::parse(message_).as_object(), {}, xUserValue, yield); - EXPECT_FALSE(result); + ASSERT_FALSE(result); + EXPECT_EQ(result.error(), rpc::ClioError::etlREQUEST_ERROR); }); } @@ -103,7 +106,8 @@ TEST_F(ForwardingSourceOperationsTests, ReadFailed) runSpawn([&](boost::asio::yield_context yield) { auto result = forwardingSource.forwardToRippled(boost::json::parse(message_).as_object(), {}, {}, yield); - EXPECT_FALSE(result); + ASSERT_FALSE(result); + EXPECT_EQ(result.error(), rpc::ClioError::etlREQUEST_ERROR); }); } @@ -116,7 +120,8 @@ TEST_F(ForwardingSourceOperationsTests, ReadTimeout) runSpawn([&](boost::asio::yield_context yield) { auto result = forwardingSource.forwardToRippled(boost::json::parse(message_).as_object(), {}, {}, yield); - EXPECT_FALSE(result); + ASSERT_FALSE(result); + EXPECT_EQ(result.error(), rpc::ClioError::etlREQUEST_TIMEOUT); }); } @@ -137,7 +142,8 @@ TEST_F(ForwardingSourceOperationsTests, ParseFailed) runSpawn([&](boost::asio::yield_context yield) { auto result = forwardingSource.forwardToRippled(boost::json::parse(message_).as_object(), {}, {}, yield); - EXPECT_FALSE(result); + ASSERT_FALSE(result); + EXPECT_EQ(result.error(), rpc::ClioError::etlINVALID_RESPONSE); }); } @@ -159,7 +165,8 @@ TEST_F(ForwardingSourceOperationsTests, GotNotAnObject) runSpawn([&](boost::asio::yield_context yield) { auto result = forwardingSource.forwardToRippled(boost::json::parse(message_).as_object(), {}, {}, yield); - EXPECT_FALSE(result); + ASSERT_FALSE(result); + EXPECT_EQ(result.error(), rpc::ClioError::etlINVALID_RESPONSE); }); } diff --git a/tests/unit/etl/LoadBalancerTests.cpp b/tests/unit/etl/LoadBalancerTests.cpp index 34e057fa8..c160523f6 100644 --- a/tests/unit/etl/LoadBalancerTests.cpp +++ b/tests/unit/etl/LoadBalancerTests.cpp @@ -19,12 +19,14 @@ #include "etl/LoadBalancer.hpp" #include "etl/Source.hpp" +#include "rpc/Errors.hpp" #include "util/AsioContextTestFixture.hpp" #include "util/MockBackendTestFixture.hpp" #include "util/MockNetworkValidatedLedgers.hpp" #include "util/MockPrometheus.hpp" #include "util/MockSource.hpp" #include "util/MockSubscriptionManager.hpp" +#include "util/NameGenerator.hpp" #include "util/Random.hpp" #include "util/config/Config.hpp" @@ -41,10 +43,12 @@ #include #include +#include #include #include #include #include +#include #include #include @@ -111,8 +115,10 @@ TEST_F(LoadBalancerConstructorTests, forwardingTimeoutPassedToSourceFactory) TEST_F(LoadBalancerConstructorTests, fetchETLState_AllSourcesFail) { EXPECT_CALL(sourceFactory_, makeSource).Times(2); - EXPECT_CALL(sourceFactory_.sourceAt(0), forwardToRippled).WillOnce(Return(std::nullopt)); - EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled).WillOnce(Return(std::nullopt)); + EXPECT_CALL(sourceFactory_.sourceAt(0), forwardToRippled) + .WillOnce(Return(std::unexpected{rpc::ClioError::etlCONNECTION_ERROR})); + EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled) + .WillOnce(Return(std::unexpected{rpc::ClioError::etlCONNECTION_ERROR})); EXPECT_THROW({ makeLoadBalancer(); }, std::logic_error); } @@ -130,7 +136,8 @@ TEST_F(LoadBalancerConstructorTests, fetchETLState_Source1Fails0OK) { EXPECT_CALL(sourceFactory_, makeSource).Times(2); EXPECT_CALL(sourceFactory_.sourceAt(0), forwardToRippled).WillOnce(Return(boost::json::object{})); - EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled).WillOnce(Return(std::nullopt)); + EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled) + .WillOnce(Return(std::unexpected{rpc::ClioError::etlCONNECTION_ERROR})); EXPECT_CALL(sourceFactory_.sourceAt(0), run); EXPECT_CALL(sourceFactory_.sourceAt(1), run); makeLoadBalancer(); @@ -139,7 +146,8 @@ TEST_F(LoadBalancerConstructorTests, fetchETLState_Source1Fails0OK) TEST_F(LoadBalancerConstructorTests, fetchETLState_Source0Fails1OK) { EXPECT_CALL(sourceFactory_, makeSource).Times(2); - EXPECT_CALL(sourceFactory_.sourceAt(0), forwardToRippled).WillOnce(Return(std::nullopt)); + EXPECT_CALL(sourceFactory_.sourceAt(0), forwardToRippled) + .WillOnce(Return(std::unexpected{rpc::ClioError::etlCONNECTION_ERROR})); EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled).WillOnce(Return(boost::json::object{})); EXPECT_CALL(sourceFactory_.sourceAt(0), run); EXPECT_CALL(sourceFactory_.sourceAt(1), run); @@ -162,7 +170,8 @@ TEST_F(LoadBalancerConstructorTests, fetchETLState_AllSourcesFailButAllowNoEtlIs EXPECT_CALL(sourceFactory_, makeSource).Times(2); EXPECT_CALL(sourceFactory_.sourceAt(0), forwardToRippled).WillOnce(Return(boost::json::object{})); EXPECT_CALL(sourceFactory_.sourceAt(0), run); - EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled).WillOnce(Return(std::nullopt)); + EXPECT_CALL(sourceFactory_.sourceAt(1), forwardToRippled) + .WillOnce(Return(std::unexpected{rpc::ClioError::etlCONNECTION_ERROR})); EXPECT_CALL(sourceFactory_.sourceAt(1), run); configJson_.as_object()["allow_no_etl"] = true; @@ -566,7 +575,7 @@ TEST_F(LoadBalancerForwardToRippledTests, source0Fails) sourceFactory_.sourceAt(0), forwardToRippled(request_, clientIP_, LoadBalancer::USER_FORWARDING_X_USER_VALUE, testing::_) ) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{rpc::ClioError::etlCONNECTION_ERROR})); EXPECT_CALL( sourceFactory_.sourceAt(1), forwardToRippled(request_, clientIP_, LoadBalancer::USER_FORWARDING_X_USER_VALUE, testing::_) @@ -578,7 +587,56 @@ TEST_F(LoadBalancerForwardToRippledTests, source0Fails) }); } -TEST_F(LoadBalancerForwardToRippledTests, bothSourcesFail) +struct LoadBalancerForwardToRippledErrorTestBundle { + std::string testName; + rpc::ClioError firstSourceError; + rpc::ClioError secondSourceError; + rpc::ClioError responseExpectedError; +}; + +struct LoadBalancerForwardToRippledErrorTests + : LoadBalancerForwardToRippledTests, + testing::WithParamInterface {}; + +INSTANTIATE_TEST_SUITE_P( + LoadBalancerForwardToRippledErrorTests, + LoadBalancerForwardToRippledErrorTests, + testing::Values( + LoadBalancerForwardToRippledErrorTestBundle{ + "ConnectionError_RequestError", + rpc::ClioError::etlCONNECTION_ERROR, + rpc::ClioError::etlREQUEST_ERROR, + rpc::ClioError::etlREQUEST_ERROR + }, + LoadBalancerForwardToRippledErrorTestBundle{ + "RequestError_RequestTimeout", + rpc::ClioError::etlREQUEST_ERROR, + rpc::ClioError::etlREQUEST_TIMEOUT, + rpc::ClioError::etlREQUEST_TIMEOUT + }, + LoadBalancerForwardToRippledErrorTestBundle{ + "RequestTimeout_InvalidResponse", + rpc::ClioError::etlREQUEST_TIMEOUT, + rpc::ClioError::etlINVALID_RESPONSE, + rpc::ClioError::etlINVALID_RESPONSE + }, + LoadBalancerForwardToRippledErrorTestBundle{ + "BothRequestTimeout", + rpc::ClioError::etlREQUEST_TIMEOUT, + rpc::ClioError::etlREQUEST_TIMEOUT, + rpc::ClioError::etlREQUEST_TIMEOUT + }, + LoadBalancerForwardToRippledErrorTestBundle{ + "InvalidResponse_RequestError", + rpc::ClioError::etlINVALID_RESPONSE, + rpc::ClioError::etlREQUEST_ERROR, + rpc::ClioError::etlINVALID_RESPONSE + } + ), + tests::util::NameGenerator +); + +TEST_P(LoadBalancerForwardToRippledErrorTests, bothSourcesFail) { EXPECT_CALL(sourceFactory_, makeSource).Times(2); auto loadBalancer = makeLoadBalancer(); @@ -586,15 +644,17 @@ TEST_F(LoadBalancerForwardToRippledTests, bothSourcesFail) sourceFactory_.sourceAt(0), forwardToRippled(request_, clientIP_, LoadBalancer::USER_FORWARDING_X_USER_VALUE, testing::_) ) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{GetParam().firstSourceError})); EXPECT_CALL( sourceFactory_.sourceAt(1), forwardToRippled(request_, clientIP_, LoadBalancer::USER_FORWARDING_X_USER_VALUE, testing::_) ) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{GetParam().secondSourceError})); runSpawn([&](boost::asio::yield_context yield) { - EXPECT_EQ(loadBalancer->forwardToRippled(request_, clientIP_, false, yield), std::nullopt); + auto const response = loadBalancer->forwardToRippled(request_, clientIP_, false, yield); + ASSERT_FALSE(response); + EXPECT_EQ(response.error(), GetParam().responseExpectedError); }); } diff --git a/tests/unit/etl/SourceImplTests.cpp b/tests/unit/etl/SourceImplTests.cpp index f9535dd46..cc8e6cc95 100644 --- a/tests/unit/etl/SourceImplTests.cpp +++ b/tests/unit/etl/SourceImplTests.cpp @@ -18,6 +18,7 @@ //============================================================================== #include "etl/impl/SourceImpl.hpp" +#include "rpc/Errors.hpp" #include #include @@ -63,7 +64,7 @@ struct SubscriptionSourceMock { struct ForwardingSourceMock { MOCK_METHOD(void, constructor, (std::string const&, std::string const&, std::chrono::steady_clock::duration)); - using ForwardToRippledReturnType = std::optional; + using ForwardToRippledReturnType = std::expected; using ClientIpOpt = std::optional; MOCK_METHOD( ForwardToRippledReturnType, diff --git a/tests/unit/rpc/ForwardingProxyTests.cpp b/tests/unit/rpc/ForwardingProxyTests.cpp index 19721c948..c0b4f6375 100644 --- a/tests/unit/rpc/ForwardingProxyTests.cpp +++ b/tests/unit/rpc/ForwardingProxyTests.cpp @@ -336,7 +336,7 @@ TEST_F(RPCForwardingProxyTest, ForwardCallsBalancerWithCorrectParams) EXPECT_CALL( *rawBalancerPtr, forwardToRippled(forwarded.as_object(), std::make_optional(CLIENT_IP), true, _) ) - .WillOnce(Return(std::make_optional())); + .WillOnce(Return(json::object{})); EXPECT_CALL(*rawHandlerProviderPtr, contains(method)).WillOnce(Return(true)); @@ -366,7 +366,7 @@ TEST_F(RPCForwardingProxyTest, ForwardingFailYieldsErrorStatus) EXPECT_CALL( *rawBalancerPtr, forwardToRippled(forwarded.as_object(), std::make_optional(CLIENT_IP), true, _) ) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE})); EXPECT_CALL(*rawHandlerProviderPtr, contains(method)).WillOnce(Return(true)); @@ -381,6 +381,6 @@ TEST_F(RPCForwardingProxyTest, ForwardingFailYieldsErrorStatus) auto const status = std::get_if(&res.response); EXPECT_TRUE(status != nullptr); - EXPECT_EQ(*status, ripple::rpcFAILED_TO_FORWARD); + EXPECT_EQ(*status, rpc::ClioError::etlINVALID_RESPONSE); }); } diff --git a/tests/unit/rpc/handlers/ServerInfoTests.cpp b/tests/unit/rpc/handlers/ServerInfoTests.cpp index 8e2a04f3c..ae6e80375 100644 --- a/tests/unit/rpc/handlers/ServerInfoTests.cpp +++ b/tests/unit/rpc/handlers/ServerInfoTests.cpp @@ -194,7 +194,7 @@ TEST_F(RPCServerInfoHandlerTest, DefaultOutputIsPresent) EXPECT_CALL(*backend, doFetchLedgerObject).WillOnce(Return(feeBlob)); EXPECT_CALL(*rawBalancerPtr, forwardToRippled(testing::_, testing::Eq(CLIENTIP), false, testing::_)) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE})); EXPECT_CALL(*rawCountersPtr, uptime).WillOnce(Return(std::chrono::seconds{1234})); @@ -231,7 +231,7 @@ TEST_F(RPCServerInfoHandlerTest, AmendmentBlockedIsPresentIfSet) EXPECT_CALL(*backend, doFetchLedgerObject).WillOnce(Return(feeBlob)); EXPECT_CALL(*rawBalancerPtr, forwardToRippled(testing::_, testing::Eq(CLIENTIP), false, testing::_)) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE})); EXPECT_CALL(*rawCountersPtr, uptime).WillOnce(Return(std::chrono::seconds{1234})); @@ -266,7 +266,7 @@ TEST_F(RPCServerInfoHandlerTest, CorruptionDetectedIsPresentIfSet) EXPECT_CALL(*backend, doFetchLedgerObject).WillOnce(Return(feeBlob)); EXPECT_CALL(*rawBalancerPtr, forwardToRippled(testing::_, testing::Eq(CLIENTIP), false, testing::_)) - .WillOnce(Return(std::nullopt)); + .WillOnce(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE})); EXPECT_CALL(*rawCountersPtr, uptime).WillOnce(Return(std::chrono::seconds{1234})); @@ -301,7 +301,7 @@ TEST_F(RPCServerInfoHandlerTest, CacheReportsEnabledFlagCorrectly) EXPECT_CALL(*rawBalancerPtr, forwardToRippled(testing::_, testing::Eq(CLIENTIP), false, testing::_)) .Times(2) - .WillRepeatedly(Return(std::nullopt)); + .WillRepeatedly(Return(std::unexpected{rpc::ClioError::etlINVALID_RESPONSE})); EXPECT_CALL(*rawCountersPtr, uptime).Times(2).WillRepeatedly(Return(std::chrono::seconds{1234}));