diff --git a/src/rpc/common/Validators.hpp b/src/rpc/common/Validators.hpp index 06ad0879..14641215 100644 --- a/src/rpc/common/Validators.hpp +++ b/src/rpc/common/Validators.hpp @@ -121,7 +121,7 @@ public: verify(boost::json::value const& value, std::string_view key) { if (value.is_object() and value.as_object().contains(key.data())) - return Error{Status{RippledError::rpcNOT_SUPPORTED, "Not supported field '" + std::string{key}}}; + return Error{Status{RippledError::rpcNOT_SUPPORTED, "Not supported field '" + std::string{key} + '\''}}; return {}; } diff --git a/src/rpc/common/impl/ForwardingProxy.hpp b/src/rpc/common/impl/ForwardingProxy.hpp index d2918fdb..ed39ab28 100644 --- a/src/rpc/common/impl/ForwardingProxy.hpp +++ b/src/rpc/common/impl/ForwardingProxy.hpp @@ -75,11 +75,8 @@ public: }; auto const checkLedgerForward = [&]() { - return ctx.method == "ledger" and - ((request.contains("queue") and request.at("queue").is_bool() and request.at("queue").as_bool()) or - (request.contains("full") and request.at("full").is_bool() and request.at("full").as_bool()) or - (request.contains("accounts") and request.at("accounts").is_bool() and request.at("accounts").as_bool() - )); + return ctx.method == "ledger" and request.contains("queue") and request.at("queue").is_bool() and + request.at("queue").as_bool(); }; return static_cast(checkAccountInfoForward() or checkLedgerForward()); diff --git a/src/rpc/handlers/Ledger.hpp b/src/rpc/handlers/Ledger.hpp index 2a2b1572..84dec817 100644 --- a/src/rpc/handlers/Ledger.hpp +++ b/src/rpc/handlers/Ledger.hpp @@ -62,13 +62,16 @@ public: * @brief A struct to hold the input data for the command * * Clio does not support: - * - accounts - * - full - * - owner_finds * - queue + * + * And the following are deprecated altogether: + * - full + * - accounts + * - ledger * - type * - * Clio will throw an error when any of `accounts`/`full`/`owner_funds`/`queue` are set to `true` + * Clio will throw an error when `queue` is set to `true` + * or if `full` or `accounts` are used. * @see https://github.com/XRPLF/clio/issues/603 */ struct Input { @@ -102,9 +105,9 @@ public: spec([[maybe_unused]] uint32_t apiVersion) { static auto const rpcSpec = RpcSpec{ - {JS(full), validation::Type{}, validation::NotSupported{true}}, + {JS(full), validation::NotSupported{}}, {JS(full), check::Deprecated{}}, - {JS(accounts), validation::Type{}, validation::NotSupported{true}}, + {JS(accounts), validation::NotSupported{}}, {JS(accounts), check::Deprecated{}}, {JS(owner_funds), validation::Type{}}, {JS(queue), validation::Type{}, validation::NotSupported{true}}, diff --git a/unittests/rpc/ForwardingProxyTests.cpp b/unittests/rpc/ForwardingProxyTests.cpp index bcfebbed..5995bf4c 100644 --- a/unittests/rpc/ForwardingProxyTests.cpp +++ b/unittests/rpc/ForwardingProxyTests.cpp @@ -160,66 +160,6 @@ TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsTrueIfAccountInfoWithQueueSpe }); } -TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsTrueIfLedgerWithQueueSpecified) -{ - auto const rawHandlerProviderPtr = handlerProvider.get(); - auto const apiVersion = 2u; - auto const method = "ledger"; - auto const params = json::parse(R"({"queue": true})"); - - ON_CALL(*rawHandlerProviderPtr, isClioOnly(_)).WillByDefault(Return(false)); - EXPECT_CALL(*rawHandlerProviderPtr, isClioOnly(method)).Times(1); - - runSpawn([&](auto yield) { - auto const range = backend->fetchLedgerRange(); - auto const ctx = - web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true); - - auto const res = proxy.shouldForward(ctx); - ASSERT_TRUE(res); - }); -} - -TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsTrueIfLedgerWithFullSpecified) -{ - auto const rawHandlerProviderPtr = handlerProvider.get(); - auto const apiVersion = 2u; - auto const method = "ledger"; - auto const params = json::parse(R"({"full": true})"); - - ON_CALL(*rawHandlerProviderPtr, isClioOnly(_)).WillByDefault(Return(false)); - EXPECT_CALL(*rawHandlerProviderPtr, isClioOnly(method)).Times(1); - - runSpawn([&](auto yield) { - auto const range = backend->fetchLedgerRange(); - auto const ctx = - web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true); - - auto const res = proxy.shouldForward(ctx); - ASSERT_TRUE(res); - }); -} - -TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsTrueIfLedgerWithAccountsSpecified) -{ - auto const rawHandlerProviderPtr = handlerProvider.get(); - auto const apiVersion = 2u; - auto const method = "ledger"; - auto const params = json::parse(R"({"accounts": true})"); - - ON_CALL(*rawHandlerProviderPtr, isClioOnly(_)).WillByDefault(Return(false)); - EXPECT_CALL(*rawHandlerProviderPtr, isClioOnly(method)).Times(1); - - runSpawn([&](auto yield) { - auto const range = backend->fetchLedgerRange(); - auto const ctx = - web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true); - - auto const res = proxy.shouldForward(ctx); - ASSERT_TRUE(res); - }); -} - TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfAccountInfoQueueIsFalse) { auto const rawHandlerProviderPtr = handlerProvider.get(); @@ -240,6 +180,26 @@ TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfAccountInfoQueueIsFals }); } +TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsTrueIfLedgerWithQueueSpecified) +{ + auto const rawHandlerProviderPtr = handlerProvider.get(); + auto const apiVersion = 2u; + auto const method = "ledger"; + auto const params = json::parse(R"({"queue": true})"); + + ON_CALL(*rawHandlerProviderPtr, isClioOnly(_)).WillByDefault(Return(false)); + EXPECT_CALL(*rawHandlerProviderPtr, isClioOnly(method)).Times(1); + + runSpawn([&](auto yield) { + auto const range = backend->fetchLedgerRange(); + auto const ctx = + web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true); + + auto const res = proxy.shouldForward(ctx); + ASSERT_TRUE(res); + }); +} + TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfLedgerQueueIsFalse) { auto const rawHandlerProviderPtr = handlerProvider.get(); @@ -260,46 +220,6 @@ TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfLedgerQueueIsFalse) }); } -TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfLedgerFullIsFalse) -{ - auto const rawHandlerProviderPtr = handlerProvider.get(); - auto const apiVersion = 2u; - auto const method = "ledger"; - auto const params = json::parse(R"({"full": false})"); - - ON_CALL(*rawHandlerProviderPtr, isClioOnly(_)).WillByDefault(Return(false)); - EXPECT_CALL(*rawHandlerProviderPtr, isClioOnly(method)).Times(1); - - runSpawn([&](auto yield) { - auto const range = backend->fetchLedgerRange(); - auto const ctx = - web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true); - - auto const res = proxy.shouldForward(ctx); - ASSERT_FALSE(res); - }); -} - -TEST_F(RPCForwardingProxyTest, ShouldForwardReturnsFalseIfLedgerAccountsIsFalse) -{ - auto const rawHandlerProviderPtr = handlerProvider.get(); - auto const apiVersion = 2u; - auto const method = "ledger"; - auto const params = json::parse(R"({"accounts": false})"); - - ON_CALL(*rawHandlerProviderPtr, isClioOnly(_)).WillByDefault(Return(false)); - EXPECT_CALL(*rawHandlerProviderPtr, isClioOnly(method)).Times(1); - - runSpawn([&](auto yield) { - auto const range = backend->fetchLedgerRange(); - auto const ctx = - web::Context(yield, method, apiVersion, params.as_object(), nullptr, tagFactory, *range, CLIENT_IP, true); - - auto const res = proxy.shouldForward(ctx); - ASSERT_FALSE(res); - }); -} - TEST_F(RPCForwardingProxyTest, ShouldNotForwardReturnsTrueIfAPIVersionIsV1) { auto const apiVersion = 1u; diff --git a/unittests/rpc/handlers/LedgerTests.cpp b/unittests/rpc/handlers/LedgerTests.cpp index 2f6c84d9..67e65c2d 100644 --- a/unittests/rpc/handlers/LedgerTests.cpp +++ b/unittests/rpc/handlers/LedgerTests.cpp @@ -85,28 +85,28 @@ generateTestValuesForParametersTest() { return std::vector{ { - "AccountsNotBool", - R"({"accounts": 123})", - "invalidParams", - "Invalid parameters.", - }, - { - "AccountsInvalid", + "AccountsInvalidBool", R"({"accounts": true})", "notSupported", - "Not supported field 'accounts's value 'true'", + "Not supported field 'accounts'", }, { - "FullExist", + "AccountsInvalidInt", + R"({"accounts": 123})", + "notSupported", + "Not supported field 'accounts'", + }, + { + "FullInvalidBool", R"({"full": true})", "notSupported", - "Not supported field 'full's value 'true'", + "Not supported field 'full'", }, { - "FullNotBool", + "FullInvalidInt", R"({"full": 123})", - "invalidParams", - "Invalid parameters.", + "notSupported", + "Not supported field 'full'", }, { "QueueExist", @@ -301,21 +301,18 @@ TEST_F(RPCLedgerHandlerTest, Default) }); } -// not supported fields can be set to its default value -TEST_F(RPCLedgerHandlerTest, NotSupportedFieldsDefaultValue) +// fields not supported for specific value can be set to its default value +TEST_F(RPCLedgerHandlerTest, ConditionallyNotSupportedFieldsDefaultValue) { backend->setRange(RANGEMIN, RANGEMAX); auto const ledgerinfo = CreateLedgerInfo(LEDGERHASH, RANGEMAX); - EXPECT_CALL(*backend, fetchLedgerBySequence).Times(1); - ON_CALL(*backend, fetchLedgerBySequence(RANGEMAX, _)).WillByDefault(Return(ledgerinfo)); + EXPECT_CALL(*backend, fetchLedgerBySequence(RANGEMAX, _)).WillRepeatedly(Return(ledgerinfo)); runSpawn([&, this](auto yield) { auto const handler = AnyHandler{LedgerHandler{backend}}; auto const req = json::parse( R"({ - "full": false, - "accounts": false, "queue": false })" );