refactor: minor cleanup in RpcClient.cc (#7951)

* refactor: RpcClient::sendNetworkRequest() takes a QByteArray body arg

* refactor: RpcClient::sendLocalRequest() takes a tr_variant const& req arg

* refactor: move RpcClient::getNextTag() into an anonymous namespace

* refactor: merge RpcClient::exec() and RpcClient::sendRequest()

* refactor: remove unnecessary heap allocation in RpcClient::networkRequestFinished()

qt/RpcClient.cc

@@ -4,6 +4,7 @@
 // License text can be found in the licenses/ folder.
 
 #include <string_view>
+#include <utility>
 
 #include <fmt/format.h>
 
@@ -29,9 +30,33 @@ using ::trqt::variant_helpers::variantInit;
 namespace
 {
 
-char constexpr const* const RequestDataPropertyKey{ "requestData" };
+char constexpr const* const RequestBodyKey{ "requestBody" };
 char constexpr const* const RequestFutureinterfacePropertyKey{ "requestReplyFutureInterface" };
 
+[[nodiscard]] int64_t nextTag()
+{
+    static int64_t tag = {};
+    return tag++;
+}
+
+[[nodiscard]] std::pair<tr_variant, int64_t> buildRequest(tr_quark const method, tr_variant* args)
+{
+    auto const tag = nextTag();
+
+    // TODO(ckerr): JSON-RPCize
+    auto req = tr_variant::Map{ 3U };
+    req.try_emplace(TR_KEY_method, tr_variant::unmanaged_string(method));
+    req.try_emplace(TR_KEY_tag, tag);
+    if (args != nullptr)
+    {
+        auto tmp = tr_variant{};
+        std::swap(tmp, *args);
+        req.try_emplace(TR_KEY_arguments, std::move(tmp));
+    }
+
+    return { std::move(req), tag };
+}
+
 TrVariantPtr createVariant()
 {
     return std::make_shared<tr_variant>();
@@ -71,35 +96,31 @@ void RpcClient::start(QUrl const& url)
     request_.reset();
 }
 
-RpcResponseFuture RpcClient::exec(tr_quark const method_key, tr_variant* args)
+RpcResponseFuture RpcClient::exec(tr_quark const method, tr_variant* args)
 {
-    auto const method = tr_quark_get_string_view(method_key);
+    auto const [req, tag] = buildRequest(method, args);
 
-    TrVariantPtr const json = createVariant();
+    auto promise = QFutureInterface<RpcResponse>{};
-    tr_variantInitDict(json.get(), 3);
+    promise.setExpectedResultCount(1);
-    dictAdd(json.get(), TR_KEY_method, method);
+    promise.setProgressRange(0, 1);
+    promise.setProgressValue(0);
+    promise.reportStarted();
 
-    if (args != nullptr) // if args were passed in, use them
+    if (session_ != nullptr)
     {
-        auto* child = tr_variantDictFind(json.get(), TR_KEY_arguments);
+        sendLocalRequest(req, promise, tag);
-
+    }
-        if (child == nullptr)
+    else if (!url_.isEmpty())
-        {
+    {
-            child = tr_variantDictAddDict(json.get(), TR_KEY_arguments, 0);
+        auto const json = tr_variant_serde::json().compact().to_string(req);
-        }
+        auto const body = QByteArray::fromStdString(json);
-
+        sendNetworkRequest(body, promise);
-        std::swap(*child, *args);
     }
 
-    return sendRequest(json);
+    return promise.future();
 }
 
-int64_t RpcClient::getNextTag()
+void RpcClient::sendNetworkRequest(QByteArray const& body, QFutureInterface<RpcResponse> const& promise)
-{
-    return next_tag_++;
-}
-
-void RpcClient::sendNetworkRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise)
 {
     if (!request_)
     {
@@ -117,9 +138,8 @@ void RpcClient::sendNetworkRequest(TrVariantPtr req, QFutureInterface<RpcRespons
         request_ = request;
     }
 
-    auto const json_data = QByteArray::fromStdString(tr_variant_serde::json().compact().to_string(*req));
+    QNetworkReply* reply = networkAccessManager()->post(*request_, body);
-    QNetworkReply* reply = networkAccessManager()->post(*request_, json_data);
+    reply->setProperty(RequestBodyKey, body);
-    reply->setProperty(RequestDataPropertyKey, QVariant::fromValue(req));
     reply->setProperty(RequestFutureinterfacePropertyKey, QVariant::fromValue(promise));
 
     connect(reply, &QNetworkReply::downloadProgress, this, &RpcClient::dataReadProgress);
@@ -135,21 +155,21 @@ void RpcClient::sendNetworkRequest(TrVariantPtr req, QFutureInterface<RpcRespons
         }
 
         qInfo() << "Body:";
-        qInfo() << json_data.constData();
+        qInfo() << body.constData();
     }
 }
 
-void RpcClient::sendLocalRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise, int64_t tag)
+void RpcClient::sendLocalRequest(tr_variant const& req, QFutureInterface<RpcResponse> const& promise, int64_t tag)
 {
     if (verbose_)
     {
-        fmt::print("{:s}:{:d} sending req:\n{:s}\n", __FILE__, __LINE__, tr_variant_serde::json().to_string(*req));
+        fmt::print("{:s}:{:d} sending req:\n{:s}\n", __FILE__, __LINE__, tr_variant_serde::json().to_string(req));
     }
 
     local_requests_.try_emplace(tag, promise);
     tr_rpc_request_exec(
         session_,
-        *req,
+        req,
         [this](tr_session* /*sesson*/, tr_variant&& response)
         {
             if (verbose_)
@@ -166,29 +186,6 @@ void RpcClient::sendLocalRequest(TrVariantPtr req, QFutureInterface<RpcResponse>
         });
 }
 
-RpcResponseFuture RpcClient::sendRequest(TrVariantPtr json)
-{
-    int64_t const tag = getNextTag();
-    dictAdd(json.get(), TR_KEY_tag, tag);
-
-    QFutureInterface<RpcResponse> promise;
-    promise.setExpectedResultCount(1);
-    promise.setProgressRange(0, 1);
-    promise.setProgressValue(0);
-    promise.reportStarted();
-
-    if (session_ != nullptr)
-    {
-        sendLocalRequest(json, promise, tag);
-    }
-    else if (!url_.isEmpty())
-    {
-        sendNetworkRequest(json, promise);
-    }
-
-    return promise.future();
-}
-
 QNetworkAccessManager* RpcClient::networkAccessManager()
 {
     if (nam_ == nullptr)
@@ -230,7 +227,7 @@ void RpcClient::networkRequestFinished(QNetworkReply* reply)
         session_id_ = QString::fromUtf8(reply->rawHeader(TR_RPC_SESSION_ID_HEADER));
         request_.reset();
 
-        sendNetworkRequest(reply->property(RequestDataPropertyKey).value<TrVariantPtr>(), promise);
+        sendNetworkRequest(reply->property(RequestBodyKey).toByteArray(), promise);
         return;
     }
 
@@ -246,18 +243,16 @@ void RpcClient::networkRequestFinished(QNetworkReply* reply)
     }
     else
     {
-        auto const json_data = reply->readAll().trimmed().toStdString();
+        auto const json = reply->readAll().trimmed().toStdString();
-        auto const json = createVariant();
+        auto response = RpcResponse{};
-        auto result = RpcResponse{};
 
-        if (auto top = tr_variant_serde::json().parse(json_data); top)
+        if (auto var = tr_variant_serde::json().parse(json))
         {
-            std::swap(*json, *top);
+            response = parseResponseData(*var);
-            result = parseResponseData(*json);
         }
 
         promise.setProgressValue(1);
-        promise.reportFinished(&result);
+        promise.reportFinished(&response);
     }
 }
 

qt/RpcClient.h

@@ -85,12 +85,10 @@ private slots:
     void localRequestFinished(TrVariantPtr response);
 
 private:
-    RpcResponseFuture sendRequest(TrVariantPtr json);
     QNetworkAccessManager* networkAccessManager();
-    int64_t getNextTag();
 
-    void sendNetworkRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise);
+    void sendNetworkRequest(QByteArray const& body, QFutureInterface<RpcResponse> const& promise);
-    void sendLocalRequest(TrVariantPtr req, QFutureInterface<RpcResponse> const& promise, int64_t tag);
+    void sendLocalRequest(tr_variant const& req, QFutureInterface<RpcResponse> const& promise, int64_t tag);
     [[nodiscard]] int64_t parseResponseTag(tr_variant& response) const;
     [[nodiscard]] RpcResponse parseResponseData(tr_variant& response) const;
 
@@ -101,7 +99,6 @@ private:
     QUrl url_;
     QNetworkAccessManager* nam_ = {};
     std::unordered_map<int64_t, QFutureInterface<RpcResponse>> local_requests_;
-    int64_t next_tag_ = {};
     bool const verbose_ = qEnvironmentVariableIsSet("TR_RPC_VERBOSE");
     bool url_is_loopback_ = false;
 };