From d93a0ae7d57a5ce5d0562be3987e5fbb1685bd05 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Thu, 19 May 2022 09:58:51 -0700 Subject: [PATCH] [FOLD] Additional HTTP status codes suggested during review --- src/ripple/net/impl/RPCCall.cpp | 11 +------ src/ripple/protocol/ErrorCodes.h | 2 +- src/ripple/protocol/impl/ErrorCodes.cpp | 40 +++++++++++++++---------- src/ripple/server/impl/JSONRPCUtil.cpp | 17 ++++++++++- src/test/rpc/LedgerRPC_test.cpp | 2 +- 5 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/ripple/net/impl/RPCCall.cpp b/src/ripple/net/impl/RPCCall.cpp index 820f25ddfc2..98274b60ffd 100644 --- a/src/ripple/net/impl/RPCCall.cpp +++ b/src/ripple/net/impl/RPCCall.cpp @@ -1398,16 +1398,7 @@ struct RPCCallImp // callbackFuncP. // Receive reply - if (iStatus == 401) - Throw( - "incorrect rpcuser or rpcpassword (authorization failed)"); - else if ( - (iStatus >= 400) && (iStatus != 400) && (iStatus != 404) && - (iStatus != 500)) // ? - Throw( - std::string("server returned HTTP error ") + - std::to_string(iStatus)); - else if (strData.empty()) + if (strData.empty()) Throw("no response from server"); // Parse reply diff --git a/src/ripple/protocol/ErrorCodes.h b/src/ripple/protocol/ErrorCodes.h index bbebeb94410..ee33eee0604 100644 --- a/src/ripple/protocol/ErrorCodes.h +++ b/src/ripple/protocol/ErrorCodes.h @@ -163,7 +163,7 @@ enum warning_code_i { namespace RPC { -/** Maps an rpc error code to its token and default message. */ +/** Maps an rpc error code to its token, default message, and HTTP status. */ struct ErrorInfo { // Default ctor needed to produce an empty std::array during constexpr eval. diff --git a/src/ripple/protocol/impl/ErrorCodes.cpp b/src/ripple/protocol/impl/ErrorCodes.cpp index a66da5231b7..30719b1b59c 100644 --- a/src/ripple/protocol/impl/ErrorCodes.cpp +++ b/src/ripple/protocol/impl/ErrorCodes.cpp @@ -32,6 +32,16 @@ namespace detail { // // This array will be omitted from the object file; only the sorted version // will remain in the object file. But the string literals will remain. +// +// There's a certain amount of tension in determining the correct HTTP +// status to associate with a given RPC error. Initially all RPC errors +// returned 200 (OK). And that's the default behavior if no HTTP status code +// is specified below. +// +// The codes currently selected target the load balancer fail-over use case. +// If a query fails on one node but is likely to have a positive outcome +// on a different node, then the failure should return a 4xx/5xx range +// status code. // clang-format off constexpr static ErrorInfo unorderedErrorInfos[]{ @@ -39,11 +49,11 @@ constexpr static ErrorInfo unorderedErrorInfos[]{ {rpcACT_NOT_FOUND, "actNotFound", "Account not found."}, {rpcALREADY_MULTISIG, "alreadyMultisig", "Already multisigned."}, {rpcALREADY_SINGLE_SIG, "alreadySingleSig", "Already single-signed."}, - {rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade."}, - {rpcEXPIRED_VALIDATOR_LIST, "unlBlocked", "Validator list expired."}, + {rpcAMENDMENT_BLOCKED, "amendmentBlocked", "Amendment blocked, need upgrade.", 503}, + {rpcEXPIRED_VALIDATOR_LIST, "unlBlocked", "Validator list expired.", 503}, {rpcATX_DEPRECATED, "deprecated", "Use the new API or specify a ledger range."}, {rpcBAD_KEY_TYPE, "badKeyType", "Bad key type."}, - {rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid."}, + {rpcBAD_FEATURE, "badFeature", "Feature unknown or invalid.", 500}, {rpcBAD_ISSUER, "badIssuer", "Issuer account malformed."}, {rpcBAD_MARKET, "badMarket", "No such market."}, {rpcBAD_SECRET, "badSecret", "Secret does not match account."}, @@ -60,28 +70,28 @@ constexpr static ErrorInfo unorderedErrorInfos[]{ {rpcDST_AMT_MISSING, "dstAmtMissing", "Destination amount/currency/issuer is missing."}, {rpcDST_ISR_MALFORMED, "dstIsrMalformed", "Destination issuer is malformed."}, {rpcEXCESSIVE_LGR_RANGE, "excessiveLgrRange", "Ledger range exceeds 1000."}, - {rpcFORBIDDEN, "forbidden", "Bad credentials."}, - {rpcFAILED_TO_FORWARD, "failedToForward", "Failed to forward request to p2p node"}, + {rpcFORBIDDEN, "forbidden", "Bad credentials.", 403}, + {rpcFAILED_TO_FORWARD, "failedToForward", "Failed to forward request to p2p node", 503}, {rpcHIGH_FEE, "highFee", "Current transaction fee exceeds your limit."}, {rpcINTERNAL, "internal", "Internal error.", 500}, {rpcINVALID_LGR_RANGE, "invalidLgrRange", "Ledger range is invalid."}, {rpcINVALID_PARAMS, "invalidParams", "Invalid parameters.", 400}, - {rpcJSON_RPC, "json_rpc", "JSON-RPC transport error."}, + {rpcJSON_RPC, "json_rpc", "JSON-RPC transport error.", 500}, {rpcLGR_IDXS_INVALID, "lgrIdxsInvalid", "Ledger indexes invalid."}, {rpcLGR_IDX_MALFORMED, "lgrIdxMalformed", "Ledger index malformed."}, {rpcLGR_NOT_FOUND, "lgrNotFound", "Ledger not found."}, - {rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated."}, + {rpcLGR_NOT_VALIDATED, "lgrNotValidated", "Ledger not validated.", 202}, {rpcMASTER_DISABLED, "masterDisabled", "Master key is disabled."}, - {rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration."}, + {rpcNOT_ENABLED, "notEnabled", "Not enabled in configuration.", 501}, {rpcNOT_IMPL, "notImpl", "Not implemented.", 501}, - {rpcNOT_READY, "notReady", "Not ready to handle this request."}, + {rpcNOT_READY, "notReady", "Not ready to handle this request.", 202}, {rpcNOT_SUPPORTED, "notSupported", "Operation not supported.", 501}, - {rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable."}, - {rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable."}, - {rpcNOT_SYNCED, "notSynced", "Not synced to the network."}, + {rpcNO_CLOSED, "noClosed", "Closed ledger is unavailable.", 503}, + {rpcNO_CURRENT, "noCurrent", "Current ledger is unavailable.", 503}, + {rpcNOT_SYNCED, "notSynced", "Not synced to the network.", 503}, {rpcNO_EVENTS, "noEvents", "Current transport does not support events."}, - {rpcNO_NETWORK, "noNetwork", "Not synced to the network."}, - {rpcNO_PERMISSION, "noPermission", "You don't have permission for this command."}, + {rpcNO_NETWORK, "noNetwork", "Not synced to the network.", 503}, + {rpcNO_PERMISSION, "noPermission", "You don't have permission for this command.", 401}, {rpcNO_PF_REQUEST, "noPathRequest", "No pathfinding request in progress."}, {rpcOBJECT_NOT_FOUND, "objectNotFound", "The requested object was not found."}, {rpcPUBLIC_MALFORMED, "publicMalformed", "Public key is malformed."}, @@ -97,7 +107,7 @@ constexpr static ErrorInfo unorderedErrorInfos[]{ {rpcSTREAM_MALFORMED, "malformedStream", "Stream malformed."}, {rpcTOO_BUSY, "tooBusy", "The server is too busy to help you now.", 503}, {rpcTXN_NOT_FOUND, "txnNotFound", "Transaction not found."}, - {rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method."}}; + {rpcUNKNOWN_COMMAND, "unknownCmd", "Unknown method.", 405}}; // clang-format on // Sort and validate unorderedErrorInfos at compile time. Should be diff --git a/src/ripple/server/impl/JSONRPCUtil.cpp b/src/ripple/server/impl/JSONRPCUtil.cpp index f5bb815a959..12d12829ca9 100644 --- a/src/ripple/server/impl/JSONRPCUtil.cpp +++ b/src/ripple/server/impl/JSONRPCUtil.cpp @@ -61,7 +61,7 @@ HTTPReply( { JLOG(j.trace()) << "HTTP Reply " << nStatus << " " << content; - if (nStatus == 401) + if (content.empty() && nStatus == 401) { output("HTTP/1.0 401 Authorization Required\r\n"); output(getHTTPHeaderTimestamp()); @@ -100,18 +100,33 @@ HTTPReply( case 200: output("HTTP/1.1 200 OK\r\n"); break; + case 202: + output("HTTP/1.1 202 Accepted\r\n"); + break; case 400: output("HTTP/1.1 400 Bad Request\r\n"); break; + case 401: + output("HTTP/1.1 401 Authorization Required\r\n"); + break; case 403: output("HTTP/1.1 403 Forbidden\r\n"); break; case 404: output("HTTP/1.1 404 Not Found\r\n"); break; + case 405: + output("HTTP/1.1 405 Method Not Allowed\r\n"); + break; + case 429: + output("HTTP/1.1 429 Too Many Requests\r\n"); + break; case 500: output("HTTP/1.1 500 Internal Server Error\r\n"); break; + case 501: + output("HTTP/1.1 501 Not Implemented\r\n"); + break; case 503: output("HTTP/1.1 503 Server is overloaded\r\n"); break; diff --git a/src/test/rpc/LedgerRPC_test.cpp b/src/test/rpc/LedgerRPC_test.cpp index 1692b980673..3d2230287b0 100644 --- a/src/test/rpc/LedgerRPC_test.cpp +++ b/src/test/rpc/LedgerRPC_test.cpp @@ -1678,7 +1678,7 @@ class LedgerRPC_test : public beast::unit_test::suite void testLedgerAccountsOption() { - testcase("Ledger Request, Accounts Option"); + testcase("Ledger Request, Accounts Hashes"); using namespace test::jtx; Env env{*this};