Move the json check to RPC executor (#676)

Fixes #681

src/webserver/RPCExecutor.h

@@ -58,8 +58,21 @@ public:
      * @param connection The connection
      */
     void
-    operator()(boost::json::object&& req, std::shared_ptr<Server::ConnectionBase> const& connection)
+    operator()(std::string const& reqStr, std::shared_ptr<Server::ConnectionBase> const& connection)
     {
+        auto req = boost::json::object{};
+        try
+        {
+            req = boost::json::parse(reqStr).as_object();
+        }
+        catch (boost::exception const& _)
+        {
+            connection->send(
+                boost::json::serialize(RPC::makeError(RPC::RippledError::rpcBAD_SYNTAX)),
+                boost::beast::http::status::ok);
+            return;
+        }
+
         perfLog_.debug() << connection->tag() << "Adding to work queue";
         // specially handle for http connections
         if (!connection->upgraded)

src/webserver/details/HttpBase.h

@@ -207,21 +207,9 @@ public:
 
         log_.info() << tag() << "Received request from ip = " << clientIp << " - posting to WorkQueue";
 
-        auto request = boost::json::object{};
         try
         {
-            request = boost::json::parse(req_.body()).as_object();
+            (*handler_)(req_.body(), derived().shared_from_this());
-        }
-        catch (boost::exception const& e)
-        {
-            return sender_(httpResponse(
-                http::status::ok,
-                "application/json",
-                boost::json::serialize(RPC::makeError(RPC::RippledError::rpcBAD_SYNTAX))));
-        }
-        try
-        {
-            (*handler_)(std::move(request), derived().shared_from_this());
         }
         catch (std::exception const& e)
         {

src/webserver/details/WsBase.h

@@ -221,12 +221,19 @@ public:
 
         perfLog_.info() << tag() << "Received request from ip = " << this->clientIp;
 
-        auto sendError = [this](auto error, boost::json::value const& request) {
+        auto sendError = [this](auto error, std::string&& requestStr) {
             auto e = RPC::makeError(error);
-
+            try
-            if (request.is_object() && request.as_object().contains("id"))
+            {
-                e["id"] = request.as_object().at("id");
+                auto request = boost::json::parse(requestStr);
-            e["request"] = request;
+                if (request.is_object() && request.as_object().contains("id"))
+                    e["id"] = request.as_object().at("id");
+                e["request"] = std::move(request);
+            }
+            catch (std::exception&)
+            {
+                e["request"] = std::move(requestStr);
+            }
 
             auto responseStr = boost::json::serialize(e);
             log_.trace() << responseStr;
@@ -236,37 +243,21 @@ public:
 
         std::string msg{static_cast<char const*>(buffer_.data().data()), buffer_.size()};
 
-        boost::json::value raw = [](std::string&& msg) {
-            try
-            {
-                return boost::json::parse(msg);
-            }
-            catch (std::exception&)
-            {
-                return boost::json::value{msg};
-            }
-        }(std::move(msg));
-
         // dosGuard served request++ and check ip address
         if (!dosGuard_.get().request(clientIp))
         {
-            sendError(RPC::RippledError::rpcSLOW_DOWN, raw);
+            sendError(RPC::RippledError::rpcSLOW_DOWN, std::move(msg));
-        }
-        else if (!raw.is_object())
-        {
-            sendError(RPC::RippledError::rpcBAD_SYNTAX, raw);
         }
         else
         {
-            auto request = raw.as_object();
             try
             {
-                (*handler_)(std::move(request), shared_from_this());
+                (*handler_)(msg, shared_from_this());
             }
             catch (std::exception const& e)
             {
                 perfLog_.error() << tag() << "Caught exception : " << e.what();
-                sendError(RPC::RippledError::rpcINTERNAL, raw);
+                sendError(RPC::RippledError::rpcINTERNAL, std::move(msg));
             }
         }
 

src/webserver/interface/Concepts.h

@@ -22,7 +22,6 @@
 #include <webserver/interface/ConnectionBase.h>
 
 #include <boost/beast.hpp>
-#include <boost/json.hpp>
 
 #include <memory>
 
@@ -33,9 +32,9 @@ namespace Server {
  */
 // clang-format off
 template <typename T>
-concept ServerHandler = requires(T handler, boost::json::object&& req, std::shared_ptr<ConnectionBase> const& ws, boost::beast::error_code ec) {
+concept ServerHandler = requires(T handler, std::string const& req, std::shared_ptr<ConnectionBase> const& ws, boost::beast::error_code ec) {
     // the callback when server receives a request
-    { handler(std::move(req), ws) };
+    { handler(req, ws) };
     // the callback when there is an error
     { handler(ec, ws) };
 };

unittests/webserver/RPCExecutorTest.cpp

@@ -84,11 +84,10 @@ protected:
 
 TEST_F(WebRPCExecutorTest, HTTPDefaultPath)
 {
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "method": "server_info",
+                                        "method": "server_info",
-                                            "params": [{}]
+                                        "params": [{}]
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -119,11 +118,10 @@ TEST_F(WebRPCExecutorTest, HTTPDefaultPath)
 TEST_F(WebRPCExecutorTest, WsNormalPath)
 {
     session->upgraded = true;
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "command": "server_info",
+                                        "command": "server_info",
-                                            "id": 99
+                                        "id": 99
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -155,11 +153,10 @@ TEST_F(WebRPCExecutorTest, WsNormalPath)
 
 TEST_F(WebRPCExecutorTest, HTTPForwardedPath)
 {
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                         "method": "server_info",
+                                        "method": "server_info",
-                                            "params": [{}]
+                                        "params": [{}]
-                                    })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -197,11 +194,10 @@ TEST_F(WebRPCExecutorTest, HTTPForwardedPath)
 TEST_F(WebRPCExecutorTest, WsForwardedPath)
 {
     session->upgraded = true;
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "command": "server_info",
+                                        "command": "server_info",
-                                            "id": 99
+                                        "id": 99
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -275,14 +271,13 @@ TEST_F(WebRPCExecutorTest, HTTPErrorPath)
                                                 }
                                             ]
                                         })";
-    auto request = boost::json::parse(requestJSON).as_object();
     EXPECT_CALL(*rpcEngine, buildResponse(testing::_))
         .WillOnce(testing::Return(RPC::Status{RPC::RippledError::rpcINVALID_PARAMS, "ledgerIndexMalformed"}));
     EXPECT_CALL(*rpcEngine, notifyErrored("ledger")).Times(1);
 
     EXPECT_CALL(*etl, lastCloseAgeSeconds()).WillOnce(testing::Return(45));
 
-    (*rpcExecutor)(std::move(request), session);
+    (*rpcExecutor)(std::move(requestJSON), session);
     std::this_thread::sleep_for(200ms);
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
@@ -318,25 +313,23 @@ TEST_F(WebRPCExecutorTest, WsErrorPath)
                                             "ledger_index": "xx",
                                             "id": "123"
                                         })";
-    auto request = boost::json::parse(requestJSON).as_object();
     EXPECT_CALL(*rpcEngine, buildResponse(testing::_))
         .WillOnce(testing::Return(RPC::Status{RPC::RippledError::rpcINVALID_PARAMS, "ledgerIndexMalformed"}));
     EXPECT_CALL(*rpcEngine, notifyErrored("ledger")).Times(1);
 
     EXPECT_CALL(*etl, lastCloseAgeSeconds()).WillOnce(testing::Return(45));
 
-    (*rpcExecutor)(std::move(request), session);
+    (*rpcExecutor)(std::move(requestJSON), session);
     std::this_thread::sleep_for(200ms);
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
 
 TEST_F(WebRPCExecutorTest, HTTPNotReady)
 {
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "method": "server_info",
+                                        "method": "server_info",
-                                            "params": [{}]
+                                        "params": [{}]
-                                        })")
+                                    })";
-                       .as_object();
 
     static auto constexpr response = R"({
                                         "result":{
@@ -365,11 +358,10 @@ TEST_F(WebRPCExecutorTest, WsNotReady)
 {
     session->upgraded = true;
 
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "command": "server_info",
+                                        "command": "server_info",
-                                            "id": 99
+                                        "id": 99
-                                        })")
+                                    })";
-                       .as_object();
 
     static auto constexpr response = R"({
                                         "error":"notReady",
@@ -391,10 +383,7 @@ TEST_F(WebRPCExecutorTest, WsNotReady)
 
 TEST_F(WebRPCExecutorTest, HTTPBadSyntax)
 {
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({"method2": "server_info"})";
-                                            "method2": "server_info"
-                                        })")
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -420,10 +409,7 @@ TEST_F(WebRPCExecutorTest, HTTPBadSyntax)
 
 TEST_F(WebRPCExecutorTest, HTTPBadSyntaxWhenRequestSubscribe)
 {
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({"method": "subscribe"})";
-                                            "method": "subscribe"
-                                        })")
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -450,12 +436,10 @@ TEST_F(WebRPCExecutorTest, HTTPBadSyntaxWhenRequestSubscribe)
 TEST_F(WebRPCExecutorTest, WsBadSyntax)
 {
     session->upgraded = true;
-    auto request = boost::json::parse(R"(
+    static auto constexpr request = R"({
-    {
+                                        "command2": "server_info",
-        "command2": "server_info",
+                                        "id": 99
-        "id": 99
+                                    })";
-    })")
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -509,10 +493,9 @@ TEST_F(WebRPCExecutorTest, HTTPInternalError)
                                                 }
                                             ]
                                         })";
-    auto request = boost::json::parse(requestJSON).as_object();
     EXPECT_CALL(*rpcEngine, buildResponse(testing::_)).Times(1).WillOnce(testing::Throw(std::runtime_error("MyError")));
 
-    (*rpcExecutor)(std::move(request), session);
+    (*rpcExecutor)(std::move(requestJSON), session);
     std::this_thread::sleep_for(200ms);
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
@@ -541,21 +524,19 @@ TEST_F(WebRPCExecutorTest, WsInternalError)
                                             "command": "ledger",
                                             "id": "123"
                                         })";
-    auto request = boost::json::parse(requestJSON).as_object();
     EXPECT_CALL(*rpcEngine, buildResponse(testing::_)).Times(1).WillOnce(testing::Throw(std::runtime_error("MyError")));
 
-    (*rpcExecutor)(std::move(request), session);
+    (*rpcExecutor)(std::move(requestJSON), session);
     std::this_thread::sleep_for(200ms);
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
 
 TEST_F(WebRPCExecutorTest, HTTPOutDated)
 {
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "method": "server_info",
+                                        "method": "server_info",
-                                            "params": [{}]
+                                        "params": [{}]
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -591,11 +572,10 @@ TEST_F(WebRPCExecutorTest, WsOutdated)
 {
     session->upgraded = true;
 
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "command": "server_info",
+                                        "command": "server_info",
-                                            "id": 99
+                                        "id": 99
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
@@ -636,17 +616,22 @@ TEST_F(WebRPCExecutorTest, WsTooBusy)
     auto rpcEngine2 = std::make_shared<MockRPCEngine>();
     auto rpcExecutor2 =
         std::make_shared<RPCExecutor<MockRPCEngine, MockETLService>>(cfg, mockBackendPtr, rpcEngine2, etl, subManager);
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "command": "server_info",
+                                        "command": "server_info",
-                                            "id": 99
+                                        "id": 99
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
 
     static auto constexpr response =
-        R"({"error":"tooBusy","error_code":9,"error_message":"The server is too busy to help you now.","status":"error","type":"response"})";
+        R"({
+            "error":"tooBusy",
+            "error_code":9,
+            "error_message":"The server is too busy to help you now.",
+            "status":"error",
+            "type":"response"
+        })";
     EXPECT_CALL(*rpcEngine2, post).WillOnce(testing::Return(false));
     (*rpcExecutor2)(std::move(request), session);
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
@@ -657,19 +642,57 @@ TEST_F(WebRPCExecutorTest, HTTPTooBusy)
     auto rpcEngine2 = std::make_shared<MockRPCEngine>();
     auto rpcExecutor2 =
         std::make_shared<RPCExecutor<MockRPCEngine, MockETLService>>(cfg, mockBackendPtr, rpcEngine2, etl, subManager);
-    auto request = boost::json::parse(R"({
+    static auto constexpr request = R"({
-                                            "method": "server_info",
+                                        "method": "server_info",
-                                            "params": [{}]
+                                        "params": [{}]
-                                        })")
+                                    })";
-                       .as_object();
 
     mockBackendPtr->updateRange(MINSEQ);  // min
     mockBackendPtr->updateRange(MAXSEQ);  // max
 
     static auto constexpr response =
-        R"({"error":"tooBusy","error_code":9,"error_message":"The server is too busy to help you now.","status":"error","type":"response"})";
+        R"({
+            "error":"tooBusy",
+            "error_code":9,
+            "error_message":"The server is too busy to help you now.",
+            "status":"error",
+            "type":"response"
+        })";
 
     EXPECT_CALL(*rpcEngine2, post).WillOnce(testing::Return(false));
     (*rpcExecutor2)(std::move(request), session);
     EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
 }
+
+TEST_F(WebRPCExecutorTest, HTTPRequestNotJson)
+{
+    static auto constexpr request = "not json";
+    static auto constexpr response =
+        R"({
+            "error":"badSyntax",
+            "error_code":1,
+            "error_message":"Syntax error.",
+            "status":"error",
+            "type":"response"
+        })";
+
+    (*rpcExecutor)(std::move(request), session);
+    EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
+}
+
+TEST_F(WebRPCExecutorTest, WsRequestNotJson)
+{
+    session->upgraded = true;
+    static auto constexpr request = "not json";
+    static auto constexpr response =
+        R"({
+            "error":"badSyntax",
+            "error_code":1,
+            "error_message":"Syntax error.",
+            "status":"error",
+            "type":"response"
+        })";
+
+    (*rpcExecutor)(std::move(request), session);
+    EXPECT_EQ(boost::json::parse(session->message), boost::json::parse(response));
+}

unittests/webserver/ServerTest.cpp

@@ -162,9 +162,9 @@ class EchoExecutor
 {
 public:
     void
-    operator()(boost::json::object&& req, std::shared_ptr<Server::ConnectionBase> const& ws)
+    operator()(std::string const& reqStr, std::shared_ptr<Server::ConnectionBase> const& ws)
     {
-        ws->send(boost::json::serialize(req), http::status::ok);
+        ws->send(std::string(reqStr), http::status::ok);
     }
 
     void
@@ -177,7 +177,7 @@ class ExceptionExecutor
 {
 public:
     void
-    operator()(boost::json::object&& req, std::shared_ptr<Server::ConnectionBase> const& ws)
+    operator()(std::string const& req, std::shared_ptr<Server::ConnectionBase> const& ws)
     {
         throw std::runtime_error("MyError");
     }
@@ -207,52 +207,6 @@ TEST_F(WebServerTest, Ws)
     wsClient.disconnect();
 }
 
-TEST_F(WebServerTest, HttpBodyNotJsonValue)
-{
-    auto e = std::make_shared<EchoExecutor>();
-    auto const server = Server::make_HttpServer(cfg, ctx, std::nullopt, dosGuard, e);
-    auto const res = HttpSyncClient::syncPost("localhost", "8888", R"({)");
-    EXPECT_EQ(
-        res,
-        R"({"error":"badSyntax","error_code":1,"error_message":"Syntax error.","status":"error","type":"response"})");
-}
-
-TEST_F(WebServerTest, HttpBodyNotJsonObject)
-{
-    auto e = std::make_shared<EchoExecutor>();
-    auto const server = Server::make_HttpServer(cfg, ctx, std::nullopt, dosGuard, e);
-    auto const res = HttpSyncClient::syncPost("localhost", "8888", R"("123")");
-    EXPECT_EQ(
-        res,
-        R"({"error":"badSyntax","error_code":1,"error_message":"Syntax error.","status":"error","type":"response"})");
-}
-
-TEST_F(WebServerTest, WsBodyNotJsonValue)
-{
-    auto e = std::make_shared<EchoExecutor>();
-    WebSocketSyncClient wsClient;
-    auto const server = Server::make_HttpServer(cfg, ctx, std::nullopt, dosGuard, e);
-    wsClient.connect("localhost", "8888");
-    auto const res = wsClient.syncPost(R"({)");
-    wsClient.disconnect();
-    EXPECT_EQ(
-        res,
-        R"({"error":"badSyntax","error_code":1,"error_message":"Syntax error.","status":"error","type":"response","request":["{"]})");
-}
-
-TEST_F(WebServerTest, WsBodyNotJsonObject)
-{
-    auto e = std::make_shared<EchoExecutor>();
-    auto const server = Server::make_HttpServer(cfg, ctx, std::nullopt, dosGuard, e);
-    WebSocketSyncClient wsClient;
-    wsClient.connect("localhost", "8888");
-    auto const res = wsClient.syncPost(R"("Hello")");
-    wsClient.disconnect();
-    EXPECT_EQ(
-        res,
-        R"({"error":"badSyntax","error_code":1,"error_message":"Syntax error.","status":"error","type":"response","request":"Hello"})");
-}
-
 TEST_F(WebServerTest, HttpInternalError)
 {
     auto e = std::make_shared<ExceptionExecutor>();
@@ -269,11 +223,24 @@ TEST_F(WebServerTest, WsInternalError)
     auto const server = Server::make_HttpServer(cfg, ctx, std::nullopt, dosGuard, e);
     WebSocketSyncClient wsClient;
     wsClient.connect("localhost", "8888");
-    auto const res = wsClient.syncPost(R"({})");
+    auto const res = wsClient.syncPost(R"({"id":"id1"})");
     wsClient.disconnect();
     EXPECT_EQ(
         res,
-        R"({"error":"internal","error_code":73,"error_message":"Internal error.","status":"error","type":"response","request":{}})");
+        R"({"error":"internal","error_code":73,"error_message":"Internal error.","status":"error","type":"response","id":"id1","request":{"id":"id1"}})");
+}
+
+TEST_F(WebServerTest, WsInternalErrorNotJson)
+{
+    auto e = std::make_shared<ExceptionExecutor>();
+    auto const server = Server::make_HttpServer(cfg, ctx, std::nullopt, dosGuard, e);
+    WebSocketSyncClient wsClient;
+    wsClient.connect("localhost", "8888");
+    auto const res = wsClient.syncPost("not json");
+    wsClient.disconnect();
+    EXPECT_EQ(
+        res,
+        R"({"error":"internal","error_code":73,"error_message":"Internal error.","status":"error","type":"response","request":"not json"})");
 }
 
 TEST_F(WebServerTest, Https)