Fix http params handling discrepancy (#913)

Fixes #909

Author: godexsoft

src/rpc/RPCEngine.h

@@ -150,11 +150,9 @@ class RPCEngineBase
 
             if (v)
                 return v->as_object();
-            else
-            {
-                notifyErrored(ctx.method);
-                return Status{v.error()};
-            }
+
+            notifyErrored(ctx.method);
+            return Status{v.error()};
         }
         catch (data::DatabaseTimeout const& t)
         {

src/web/RPCServerHandler.h

@@ -89,8 +89,8 @@ class RPCServerHandler
             auto req = boost::json::parse(request).as_object();
             LOG(perfLog_.debug()) << connection->tag() << "Adding to work queue";
 
-            if (not connection->upgraded and not req.contains("params"))
-                req["params"] = boost::json::array({boost::json::object{}});
+            if (not connection->upgraded and shouldReplaceParams(req))
+                req[JS(params)] = boost::json::array({boost::json::object{}});
 
             if (!rpcEngine_->post(
                     [this, request = std::move(req), connection](boost::asio::yield_context yield) mutable {
@@ -267,6 +267,27 @@ class RPCServerHandler
             return web::detail::ErrorHelper(connection, request).sendInternalError();
         }
     }
+
+    bool
+    shouldReplaceParams(boost::json::object const& req) const
+    {
+        auto const hasParams = req.contains(JS(params));
+        auto const paramsIsArray = hasParams and req.at(JS(params)).is_array();
+        auto const paramsIsEmptyString =
+            hasParams and req.at(JS(params)).is_string() and req.at(JS(params)).as_string().empty();
+        auto const paramsIsEmptyObject =
+            hasParams and req.at(JS(params)).is_object() and req.at(JS(params)).as_object().empty();
+        auto const paramsIsNull = hasParams and req.at(JS(params)).is_null();
+        auto const arrayIsEmpty = paramsIsArray and req.at(JS(params)).as_array().empty();
+        auto const arrayIsNotEmpty = paramsIsArray and not req.at(JS(params)).as_array().empty();
+        auto const firstArgIsNull = arrayIsNotEmpty and req.at(JS(params)).as_array().at(0).is_null();
+        auto const firstArgIsEmptyString = arrayIsNotEmpty and req.at(JS(params)).as_array().at(0).is_string() and
+            req.at(JS(params)).as_array().at(0).as_string().empty();
+
+        // Note: all this compatibility dance is to match `rippled` as close as possible
+        return not hasParams or paramsIsEmptyString or paramsIsNull or paramsIsEmptyObject or arrayIsEmpty or
+            firstArgIsEmptyString or firstArgIsNull;
+    }
 };
 
 }  // namespace web

src/web/impl/HttpBase.h

@@ -180,10 +180,10 @@ class HttpBase : public ConnectionBase
 
         if (boost::beast::websocket::is_upgrade(req_))
         {
-            upgraded = true;
-            // Disable the timeout.
-            // The websocket::stream uses its own timeout settings.
+            // Disable the timeout. The websocket::stream uses its own timeout settings.
             boost::beast::get_lowest_layer(derived().stream()).expires_never();
+
+            upgraded = true;
             return derived().upgrade();
         }
 

unittests/web/RPCServerHandlerTests.cpp

@@ -549,7 +549,7 @@ TEST_F(WebRPCServerHandlerTest, HTTPParamsUnparseableNotArray)
     EXPECT_EQ(session->lastStatus, boost::beast::http::status::bad_request);
 }
 
-TEST_F(WebRPCServerHandlerTest, HTTPParamsUnparseableEmptyArray)
+TEST_F(WebRPCServerHandlerTest, HTTPParamsUnparseableArrayWithDigit)
 {
     static auto constexpr response = "params unparseable";
 
@@ -558,7 +558,7 @@ TEST_F(WebRPCServerHandlerTest, HTTPParamsUnparseableEmptyArray)
 
     static auto constexpr requestJSON = R"({
                                             "method": "ledger",
-                                            "params": []
+                                            "params": [1]
                                         })";
 
     EXPECT_CALL(*rpcEngine, notifyBadSyntax).Times(1);