test: add tests for RpcClient (#8137)

* refactor do not cache style in convert_outgoing_data() -- not testing-friendly

* refactor: pass a QNetworkAccessManager into RpcClient constructor

this way we can inject a fake one in tests

* refactor: add RpcClient tests

* refactor: remove pointless QVERIFY call

* refactor: add api_compat::default_style()

* refactor: add api_compat::set_default_style()

* test: parameterize RpcClient tests for Tr4 and Tr5

libtransmission/api-compat.cc

@@ -1007,8 +1007,22 @@ void convert_jsonrpc(tr_variant::Map& top, State const& state)
         }
     }
 }
+
+// TODO(TR5) change default to Tr5.
+Style default_style_g = tr_env_get_string("TR_SAVE_VERSION_FORMAT", "4") == "5" ? Style::Tr5 : Style::Tr4;
+
 } // namespace
 
+Style default_style()
+{
+    return default_style_g;
+}
+
+void set_default_style(Style const style)
+{
+    default_style_g = style;
+}
+
 void convert(tr_variant& var, Style const tgt_style)
 {
     if (auto* const top = var.get_if<tr_variant::Map>())
@@ -1023,9 +1037,7 @@ void convert(tr_variant& var, Style const tgt_style)
 
 void convert_outgoing_data(tr_variant& var)
 {
-    // TODO: change default to Tr5 in transmission 5.0.0-beta.1
-    static auto const style = tr_env_get_string("TR_SAVE_VERSION_FORMAT", "4") == "5" ? Style::Tr5 : Style::Tr4;
-    convert(var, style);
+    convert(var, default_style());
 }
 
 void convert_incoming_data(tr_variant& var)

libtransmission/api-compat.h

@@ -21,4 +21,7 @@ void convert(tr_variant& var, Style tgt_style);
 void convert_incoming_data(tr_variant& var);
 void convert_outgoing_data(tr_variant& var);
 
+[[nodiscard]] Style default_style();
+void set_default_style(Style style);
+
 } // namespace libtransmission::api_compat

qt/RpcClient.cc

@@ -57,8 +57,9 @@ char constexpr const* const RequestFutureinterfacePropertyKey{ "requestReplyFutu
 }
 } // namespace
 
-RpcClient::RpcClient(QObject* parent)
+RpcClient::RpcClient(QNetworkAccessManager& nam, QObject* parent)
     : QObject{ parent }
+    , nam_{ &nam }
 {
     qRegisterMetaType<TrVariantPtr>("TrVariantPtr");
 }
@@ -68,13 +69,9 @@ void RpcClient::stop()
     session_ = nullptr;
     session_id_.clear();
     url_.clear();
-    network_style_ = DefaultNetworkStyle;
+    network_style_ = libtransmission::api_compat::default_style();
 
-    if (nam_ != nullptr)
-    {
-        nam_->deleteLater();
-        nam_ = nullptr;
-    }
+    QObject::disconnect(nam_, nullptr, this, nullptr);
 }
 
 void RpcClient::start(tr_session* session)
@@ -84,6 +81,7 @@ void RpcClient::start(tr_session* session)
 
 void RpcClient::start(QUrl const& url)
 {
+    connectNetworkAccessManager();
     url_ = url;
     url_is_loopback_ = QHostAddress{ url_.host() }.isLoopback();
 }
@@ -137,7 +135,7 @@ void RpcClient::sendNetworkRequest(QByteArray const& body, QFutureInterface<RpcR
         qInfo() << body.constData();
     }
 
-    if (QNetworkReply* reply = networkAccessManager()->post(req, body))
+    if (QNetworkReply* reply = nam_->post(req, body))
     {
         reply->setProperty(RequestBodyKey, body);
         reply->setProperty(RequestFutureinterfacePropertyKey, QVariant::fromValue(promise));
@@ -176,18 +174,11 @@ void RpcClient::sendLocalRequest(tr_variant& req, QFutureInterface<RpcResponse>
         });
 }
 
-QNetworkAccessManager* RpcClient::networkAccessManager()
+void RpcClient::connectNetworkAccessManager()
 {
-    if (nam_ == nullptr)
-    {
-        nam_ = new QNetworkAccessManager{};
-
-        connect(nam_, &QNetworkAccessManager::finished, this, &RpcClient::networkRequestFinished);
-
-        connect(nam_, &QNetworkAccessManager::authenticationRequired, this, &RpcClient::httpAuthenticationRequired);
-    }
-
-    return nam_;
+    QObject::disconnect(nam_, nullptr, this, nullptr);
+    connect(nam_, &QNetworkAccessManager::finished, this, &RpcClient::networkRequestFinished);
+    connect(nam_, &QNetworkAccessManager::authenticationRequired, this, &RpcClient::httpAuthenticationRequired);
 }
 
 void RpcClient::networkRequestFinished(QNetworkReply* reply)

qt/RpcClient.h

@@ -53,7 +53,7 @@ class RpcClient : public QObject
     Q_OBJECT
 
 public:
-    explicit RpcClient(QObject* parent = nullptr);
+    explicit RpcClient(QNetworkAccessManager& nam, QObject* parent = nullptr);
     RpcClient(RpcClient&&) = delete;
     RpcClient(RpcClient const&) = delete;
     RpcClient& operator=(RpcClient&&) = delete;
@@ -86,24 +86,23 @@ private slots:
     void localRequestFinished(TrVariantPtr response);
 
 private:
-    QByteArray const SessionIdHeaderName = std::data(TrRpcSessionIdHeader);
-    QByteArray const VersionHeaderName = std::data(TrRpcVersionHeader);
+    QByteArray const SessionIdHeaderName = QByteArray{ TrRpcSessionIdHeader.data(),
+                                                       static_cast<qsizetype>(TrRpcSessionIdHeader.size()) };
+    QByteArray const VersionHeaderName = QByteArray{ TrRpcVersionHeader.data(),
+                                                     static_cast<qsizetype>(TrRpcVersionHeader.size()) };
 
-    QNetworkAccessManager* networkAccessManager();
+    void connectNetworkAccessManager();
 
     void sendNetworkRequest(QByteArray const& body, QFutureInterface<RpcResponse> const& promise);
     void sendLocalRequest(tr_variant& req, QFutureInterface<RpcResponse> const& promise, int64_t id);
     [[nodiscard]] int64_t parseResponseId(tr_variant& response) const;
     [[nodiscard]] RpcResponse parseResponseData(tr_variant& response) const;
 
-    // TODO: change this default in 5.0.0-beta.1
-    static auto constexpr DefaultNetworkStyle = libtransmission::api_compat::Style::Tr4;
-
-    libtransmission::api_compat::Style network_style_ = DefaultNetworkStyle;
+    libtransmission::api_compat::Style network_style_ = libtransmission::api_compat::default_style();
     tr_session* session_ = {};
     QByteArray session_id_;
     QUrl url_;
-    QNetworkAccessManager* nam_ = {};
+    QNetworkAccessManager* const nam_;
     std::unordered_map<int64_t, QFutureInterface<RpcResponse>> local_requests_;
     bool const verbose_ = qEnvironmentVariableIsSet("TR_RPC_VERBOSE");
     bool url_is_loopback_ = false;

qt/Session.cc

@@ -295,6 +295,7 @@ void Session::updatePref(int key)
 Session::Session(QString config_dir, Prefs& prefs)
     : config_dir_{ std::move(config_dir) }
     , prefs_{ prefs }
+    , rpc_{ nam_ }
 {
     connect(&prefs_, &Prefs::changed, this, &Session::updatePref);
     connect(&rpc_, &RpcClient::httpAuthenticationRequired, this, &Session::httpAuthenticationRequired);

qt/Session.h

@@ -16,6 +16,7 @@
 #include <QString>
 #include <QStringList>
 #include <QTimer>
+#include <QNetworkAccessManager>
 
 #include <libtransmission/transmission.h>
 #include <libtransmission/quark.h>
@@ -192,6 +193,7 @@ private:
     QString session_version_;
     QString session_id_;
     bool is_definitely_local_session_ = true;
+    QNetworkAccessManager nam_;
     RpcClient rpc_;
     torrent_ids_t const RecentlyActiveIDs = { -1 };
 

tests/qt/CMakeLists.txt

@@ -17,10 +17,12 @@ function(add_trqt_test source)
 endfunction()
 
 add_trqt_test(prefs-test.cc)
+add_trqt_test(rpcclient-test.cc)
 
 add_custom_target(qt-tests
     DEPENDS
-        qt-test-prefs)
+        qt-test-prefs
+        qt-test-rpcclient)
 
 set_property(
     TARGET qt-tests

tests/qt/rpcclient-test.cc

@@ -0,0 +1,277 @@
+// This file Copyright © Mnemosyne LLC.
+// It may be used under GPLv2 (SPDX: GPL-2.0-only), GPLv3 (SPDX: GPL-3.0-only),
+// or any future license endorsed by Mnemosyne LLC.
+// License text can be found in the licenses/ folder.
+
+#include <QApplication>
+#include <QNetworkAccessManager>
+#include <QNetworkReply>
+#include <QNetworkRequest>
+#include <QRegularExpression>
+#include <QTest>
+#include <QUrl>
+
+#include <libtransmission/api-compat.h>
+#include <libtransmission/rpcimpl.h>
+#include <libtransmission/quark.h>
+
+#include "RpcClient.h"
+
+namespace api_compat = libtransmission::api_compat;
+using Style = api_compat::Style;
+
+Q_DECLARE_METATYPE(Style)
+
+#if QT_VERSION < QT_VERSION_CHECK(6, 3, 0)
+#define QCOMPARE_EQ(actual, expected) QCOMPARE(actual, expected)
+#define QCOMPARE_NE(actual, expected) QVERIFY((actual) != (expected))
+#endif
+
+namespace
+{
+template<typename String>
+[[nodiscard]] QByteArray toQBA(String const& str)
+{
+    auto const sv = std::string_view{ str };
+    return { sv.data(), static_cast<qsizetype>(sv.size()) };
+}
+
+class FakeReply final : public QNetworkReply
+{
+    Q_OBJECT
+
+public:
+    [[nodiscard]] static FakeReply* newPostReply(QUrl const& url, QObject* parent = nullptr)
+    {
+        return newPostReply(QNetworkRequest{ url }, parent);
+    }
+
+    [[nodiscard]] static FakeReply* newPostReply(QNetworkRequest const& req, QObject* parent = nullptr)
+    {
+        auto reply = new FakeReply{ QNetworkAccessManager::PostOperation, req, parent };
+
+        // networkRequestFinished expects these properties to exist.
+        auto promise = QFutureInterface<RpcResponse>{};
+        promise.reportStarted();
+        reply->setProperty("requestReplyFutureInterface", QVariant::fromValue(promise));
+        reply->setProperty("requestBody", QByteArray{ "{}" });
+
+        return reply;
+    }
+
+    explicit FakeReply(QNetworkAccessManager::Operation op, QNetworkRequest const& req, QObject* parent = nullptr)
+        : QNetworkReply{ parent }
+    {
+        setOperation(op);
+        setRequest(req);
+        setUrl(req.url());
+        open(QIODevice::ReadOnly);
+    }
+
+    void setHttpStatus(int const code)
+    {
+        setAttribute(QNetworkRequest::HttpStatusCodeAttribute, code);
+    }
+
+    template<typename StringA, typename StringB>
+    void addRawHeader(StringA const& name, StringB const& value)
+    {
+        setRawHeader(toQBA(name), toQBA(value));
+    }
+
+    void abort() override
+    {
+    }
+
+protected:
+    qint64 readData(char* /*data*/, qint64 /*maxSize*/) override
+    {
+        return 0;
+    }
+};
+
+class FakeNetworkAccessManager final : public QNetworkAccessManager
+{
+    Q_OBJECT
+
+public:
+    int create_count = 0;
+    QNetworkAccessManager::Operation last_operation = QNetworkAccessManager::UnknownOperation;
+    QNetworkRequest last_request;
+    QByteArray last_body;
+
+protected:
+    QNetworkReply* createRequest(Operation op, QNetworkRequest const& req, QIODevice* outgoing_data) override
+    {
+        ++create_count;
+        last_operation = op;
+        last_request = req;
+
+        if (outgoing_data != nullptr)
+        {
+            last_body = outgoing_data->readAll();
+            outgoing_data->seek(0);
+        }
+
+        return new FakeReply{ op, req, this };
+    }
+};
+
+auto const tr4_session_get_payload_re = QRegularExpression{ R"(^\{"method":"session-get","tag":[0-9]+\}$)" };
+auto const tr5_session_get_payload_re = QRegularExpression{ R"(^\{"id":[0-9]+,"jsonrpc":"2\.0","method":"session_get"\}$)" };
+
+} // namespace
+
+class RpcClientTest : public QObject
+{
+    Q_OBJECT
+
+    static void QVERIFY_re_matches(QRegularExpression const& re, QByteArray const& bytes)
+    {
+        auto const str = QString::fromUtf8(bytes);
+        QVERIFY2(re.match(str).hasMatch(), bytes.constData());
+    }
+
+    static void QVERIFY_is_session_get_request(Style style, QByteArray const& bytes)
+    {
+        auto const payload_re = style == Style::Tr4 ? tr4_session_get_payload_re : tr5_session_get_payload_re;
+        QVERIFY_re_matches(payload_re, bytes);
+    }
+
+    static void invoke_network_finished(RpcClient& client, QNetworkReply* reply)
+    {
+        QMetaObject::invokeMethod(&client, "networkRequestFinished", Qt::DirectConnection, Q_ARG(QNetworkReply*, reply));
+    }
+
+    static void add_style_data()
+    {
+        QTest::addColumn<Style>("initial_style");
+        QTest::newRow("Tr4") << Style::Tr4;
+        QTest::newRow("Tr5") << Style::Tr5;
+    }
+
+private slots:
+    void init()
+    {
+        api_compat::set_default_style(initial_style_);
+    }
+
+    static void first_post_is_in_default_style_data()
+    {
+        add_style_data();
+    }
+    static void first_post_is_in_default_style()
+    {
+        // setup: set style
+        QFETCH(Style const, initial_style);
+        api_compat::set_default_style(initial_style);
+
+        // setup: create & start `client`
+        auto nam = FakeNetworkAccessManager{};
+        auto client = RpcClient{ nam };
+        auto const url = QUrl{ "http://example.invalid:9091/transmission/rpc" };
+        client.start(url);
+        QCOMPARE_EQ(client.url(), url);
+
+        client.exec(TR_KEY_session_get, nullptr);
+
+        // verify that a request to `url` was posted
+        QVERIFY(nam.create_count >= 1);
+        QCOMPARE_EQ(nam.last_operation, QNetworkAccessManager::PostOperation);
+        QCOMPARE_EQ(nam.last_request.url(), url);
+
+        // verify that the request's headers look right
+        QCOMPARE_NE(nam.last_request.rawHeader("Content-Type"), QByteArray{});
+        QVERIFY(nam.last_request.rawHeader("Content-Type").contains("application/json"));
+        QVERIFY(nam.last_request.rawHeader("User-Agent").startsWith("Transmission/"));
+
+        // verify that the request's payload looks right
+        QVERIFY_is_session_get_request(initial_style, nam.last_body);
+    }
+
+    static void exec_posts_tr5_after_409_sets_style_data()
+    {
+        add_style_data();
+    }
+    static void exec_posts_tr5_after_409_sets_style()
+    {
+        // setup: set style
+        QFETCH(Style const, initial_style);
+        api_compat::set_default_style(initial_style);
+
+        // setup: create & start `client`
+        auto const url = QUrl{ "http://example.invalid:9091/transmission/rpc" };
+        auto nam = FakeNetworkAccessManager{};
+        auto client = RpcClient{ nam };
+        client.start(url);
+
+        // setup: post initial request
+        client.exec(TR_KEY_session_get, nullptr);
+        QVERIFY_is_session_get_request(initial_style, nam.last_body);
+
+        // setup: make a 409 response that includes Session-Id *and* RPC version.
+        auto* reply = FakeReply::newPostReply(url, &nam);
+        reply->setHttpStatus(409);
+        reply->addRawHeader(TrRpcSessionIdHeader, "fake-session-id");
+        reply->addRawHeader(TrRpcVersionHeader, TrRpcVersionSemver);
+        invoke_network_finished(client, reply);
+
+        // action: make another request after receiving the 409
+        auto const n_created = nam.create_count;
+        client.exec(TR_KEY_session_get, nullptr);
+
+        // verify subsequent request used Style::Tr5
+        QVERIFY(nam.create_count > n_created);
+        QVERIFY_is_session_get_request(Style::Tr5, nam.last_body);
+    }
+
+    static void exec_post_tr4_after_409_without_rpc_version_header_data()
+    {
+        add_style_data();
+    }
+    static void exec_post_tr4_after_409_without_rpc_version_header()
+    {
+        // setup: set style
+        QFETCH(Style const, initial_style);
+        api_compat::set_default_style(initial_style);
+
+        // setup: create & start `client`
+        auto const url = QUrl{ "http://example.invalid:9091/transmission/rpc" };
+        auto nam = FakeNetworkAccessManager{};
+        auto client = RpcClient{ nam };
+        client.start(QUrl{ "http://example.invalid:9091/transmission/rpc" });
+
+        // setup: post initial request
+        client.exec(TR_KEY_session_get, nullptr);
+        QVERIFY_is_session_get_request(initial_style, nam.last_body);
+
+        // setup: make a 409 response with Session-Id but *not* RPC version.
+        auto* reply = FakeReply::newPostReply(url, &nam);
+        reply->setHttpStatus(409);
+        reply->addRawHeader(TrRpcSessionIdHeader, "fake-session-id");
+        invoke_network_finished(client, reply);
+
+        // action: make another request after receiving the 409
+        auto const n_created = nam.create_count;
+        client.exec(TR_KEY_session_get, nullptr);
+
+        // verify subsequent request used Style::Tr4
+        QVERIFY(nam.create_count > n_created);
+        QVERIFY_is_session_get_request(Style::Tr4, nam.last_body);
+    }
+
+    // previous declaration was `private slots:`
+    // NOLINTNEXTLINE(readability-redundant-access-specifiers)
+private:
+    Style const initial_style_ = api_compat::default_style();
+};
+
+int main(int argc, char** argv)
+{
+    auto const app = QApplication{ argc, argv };
+
+    auto test = RpcClientTest{};
+    return QTest::qExec(&test, argc, argv);
+}
+
+#include "rpcclient-test.moc"