Implement request builder.

src/core/NetworkRequest.cpp

@@ -6,6 +6,18 @@
 #include <QtNetwork/QNetworkReply>
 #include <QtNetwork/QNetworkRequest>
 
+namespace {
+    QList<QPair<QString, QString>> createDefaultHeaders() {
+        QList<QPair<QString, QString>> headers;
+        headers.append(QPair{"User-Agent", "KeePassXC"});
+        return headers;
+    }
+}
+
+static constexpr int DEFAULT_MAX_REDIRECTS = 5;
+static constexpr std::chrono::milliseconds DEFAULT_TIMEOUT = std::chrono::seconds(5);
+static QList<QPair<QString, QString>> DEFAULT_HEADERS = createDefaultHeaders();
+
 namespace
 {
     QUrl getRedirectTarget(QNetworkReply* reply)
@@ -51,8 +63,6 @@ void NetworkRequest::fetch(const QUrl& url)
 
     connect(m_reply, &QNetworkReply::finished, this, &NetworkRequest::fetchFinished);
     connect(m_reply, &QIODevice::readyRead, this, &NetworkRequest::fetchReadyRead);
-
-    m_timeout.start();
 }
 
 void NetworkRequest::fetchFinished()
@@ -81,12 +91,10 @@ void NetworkRequest::fetchFinished()
                 redirectTarget = url.resolved(redirectTarget);
             }
             // Request the redirect target
-            qDebug() << "Following redirect to" << redirectTarget;
             m_redirects += 1;
             fetch(redirectTarget);
             return;
         } else {
-            qDebug() << "Too many redirects";
             emit failure();
             return;
         }
@@ -142,32 +150,25 @@ QUrl NetworkRequest::URL() const
     return m_requested_url;
 }
 
-void NetworkRequest::setMaxRedirects(int maxRedirects)
-{
-    m_maxRedirects = std::max(0, maxRedirects);
-}
-
 NetworkRequest::NetworkRequest(
-    QUrl targetURL,
+    QUrl targetURL, bool allowInsecure, unsigned int maxRedirects, std::chrono::milliseconds timeoutDuration, QList<QPair<QString, QString>> headers,
-                                int maxRedirects,
+    QNetworkAccessManager* manager)
-                               std::chrono::milliseconds timeoutDuration,
+    : m_manager(manager)
-                               QList<QPair<QString, QString>> headers,
+    , m_reply(nullptr)
-                               QNetworkAccessManager* manager)
-    : m_reply(nullptr)
     , m_finished(false)
     , m_maxRedirects(maxRedirects)
     , m_redirects(0)
-    , m_timeoutDuration(timeoutDuration)
     , m_headers(headers)
     , m_requested_url(targetURL)
 {
-    m_manager = manager ? manager : getNetMgr();
     connect(&m_timeout, &QTimer::timeout, this, &NetworkRequest::fetchTimeout);
 
-    m_timeout.setInterval(m_timeoutDuration);
+    m_timeout.setInterval(timeoutDuration);
     m_timeout.setSingleShot(true);
 
-    fetch(m_requested_url);
+    if(!allowInsecure) {
+        // TODO
+    }
 }
 
 const QString& NetworkRequest::ContentType() const
@@ -180,11 +181,6 @@ const QHash<QString, QString>& NetworkRequest::ContentTypeParameters() const
     return m_content_type_parameters;
 }
 
-void NetworkRequest::setTimeout(std::chrono::milliseconds timeoutDuration)
-{
-    m_timeoutDuration = timeoutDuration;
-}
-
 void NetworkRequest::fetchTimeout()
 {
     if(m_finished)
@@ -200,16 +196,71 @@ QNetworkReply* NetworkRequest::Reply() const
     return m_reply;
 }
 
-NetworkRequest createRequest(QUrl target,int maxRedirects,
+void NetworkRequest::fetch()
-                             std::chrono::milliseconds timeoutDuration,
-                             QList<QPair<QString, QString>> additionalHeaders,
-                             QNetworkAccessManager* manager)
 {
-    // Append user agent unless given
+    m_timeout.start();
-    if (std::none_of(additionalHeaders.begin(), additionalHeaders.end(), [](const auto& pair) {
+    fetch(m_requested_url);
-            return pair.first == "User-Agent";
+}
-        })) {
+
-        additionalHeaders.append(QPair{"User-Agent", "KeePassXC"});
+NetworkRequestBuilder::NetworkRequestBuilder()
-    }
+{
-    return NetworkRequest(std::move(target), maxRedirects, timeoutDuration, additionalHeaders, manager);
+    this->setAllowInsecure(false);
+    this->setMaxRedirects(DEFAULT_MAX_REDIRECTS);
+    this->setTimeout(DEFAULT_TIMEOUT);
+    this->setHeaders(DEFAULT_HEADERS);
+    this->setManager(nullptr);
+}
+
+NetworkRequestBuilder::NetworkRequestBuilder(QUrl url) : NetworkRequestBuilder()
+{
+    this->setUrl(url);
+}
+
+NetworkRequestBuilder& NetworkRequestBuilder::setManager(QNetworkAccessManager* manager)
+{
+    m_manager = manager ? manager : getNetMgr();
+    return *this;
+}
+
+NetworkRequest NetworkRequestBuilder::build()
+{
+    return NetworkRequest(m_url, m_allowInsecure, m_maxRedirects, m_timeoutDuration, m_headers, m_manager);
+}
+
+NetworkRequestBuilder& NetworkRequestBuilder::setHeaders(QList<QPair<QString, QString>> headers)
+{
+    m_headers = headers;
+
+    // Append user agent unless given
+    if (std::none_of(m_headers.begin(), m_headers.end(), [](const auto& pair) {
+        return pair.first == "User-Agent";
+    })) {
+        m_headers.append(QPair{"User-Agent", "KeePassXC"});
+    }
+
+    return *this;
+}
+
+NetworkRequestBuilder& NetworkRequestBuilder::setTimeout(std::chrono::milliseconds timeoutDuration)
+{
+    m_timeoutDuration = timeoutDuration;
+    return *this;
+}
+
+NetworkRequestBuilder& NetworkRequestBuilder::setUrl(QUrl url)
+{
+    m_url = url;
+    return *this;
+}
+
+NetworkRequestBuilder& NetworkRequestBuilder::setAllowInsecure(bool allowInsecure)
+{
+    m_allowInsecure = allowInsecure;
+    return *this;
+}
+
+NetworkRequestBuilder& NetworkRequestBuilder::setMaxRedirects(unsigned int maxRedirects)
+{
+    m_maxRedirects = maxRedirects;
+    return *this;
 }

src/core/NetworkRequest.h

@@ -47,31 +47,19 @@ class NetworkRequest : public QObject
 
     // Request parameters
     QTimer m_timeout;
-    int m_maxRedirects;
+    unsigned int m_maxRedirects;
-    int m_redirects;
+    unsigned int m_redirects;
-    std::chrono::milliseconds m_timeoutDuration;
     QList<QPair<QString, QString>> m_headers;
     QUrl m_requested_url;
 
 public:
     // TODO Disallow insecure connections by default?
-    explicit NetworkRequest(QUrl targetURL, int maxRedirects,
+    explicit NetworkRequest(QUrl targetURL, bool allowInsecure, unsigned int maxRedirects,
                             std::chrono::milliseconds timeoutDuration,
                             QList<QPair<QString, QString>> headers,
                             QNetworkAccessManager* manager = nullptr);
-    ~NetworkRequest() override;
 
-    /**
+    void fetch();
-     * Sets the maximum number of redirects to follow.
-     * @param maxRedirects The maximum number of redirects to follow. Must be >= 0.
-     */
-    void setMaxRedirects(int maxRedirects);
-
-    /**
-     * Sets the timeout duration for the request. This is the maximum time the request may take, including redirects.
-     * @param timeoutDuration The duration of the timeout in milliseconds.
-     */
-    void setTimeout(std::chrono::milliseconds timeoutDuration);
 
     void cancel();
 
@@ -96,6 +84,7 @@ public:
      */
     const QHash<QString, QString>& ContentTypeParameters() const;
 
+    ~NetworkRequest() override;
 private:
     void reset();
     void fetch(const QUrl& url);
@@ -109,17 +98,58 @@ signals:
     void failure();
 };
 
-/**
+class NetworkRequestBuilder {
- * Creates a NetworkRequest with default parameters.
+    QUrl m_url;
- * @param maxRedirects
+    bool m_allowInsecure;
- * @param timeoutDuration
+    unsigned int m_maxRedirects;
- * @param headers
+    std::chrono::milliseconds m_timeoutDuration;
- * @param manager
+    QList<QPair<QString, QString>> m_headers;
- * @return
+    QNetworkAccessManager* m_manager;
- */
+
-NetworkRequest createRequest(QUrl target, int maxRedirects = 5,
+    NetworkRequestBuilder();
-                             std::chrono::milliseconds timeoutDuration = std::chrono::milliseconds(5000),
+public:
-                             QList<QPair<QString, QString>> additionalHeaders = {},
+    explicit NetworkRequestBuilder(QUrl url);
-                             QNetworkAccessManager* manager = nullptr);
+
+    /**
+     * Sets the URL to request.
+     * @param url The URL to request.
+     */
+    NetworkRequestBuilder& setUrl(QUrl url);
+
+    /**
+     * Sets whether insecure connections are allowed.
+     * @param allowInsecure
+     */
+    NetworkRequestBuilder& setAllowInsecure(bool allowInsecure);
+
+    /**
+     * Sets the maximum number of redirects to follow.
+     * @param maxRedirects The maximum number of redirects to follow. Must be >= 0.
+     */
+    NetworkRequestBuilder& setMaxRedirects(unsigned int maxRedirects);
+
+    /**
+     * Sets the timeout duration for the request. This is the maximum time the request may take, including redirects.
+     * @param timeoutDuration The duration of the timeout in milliseconds.
+     */
+    NetworkRequestBuilder& setTimeout(std::chrono::milliseconds timeoutDuration);
+
+    /**
+     * Sets the headers to send with the request.
+     * @param headers
+     */
+    NetworkRequestBuilder& setHeaders(QList<QPair<QString, QString>> headers);
+
+    /**
+     * Sets the QNetworkAccessManager to use for the request.
+     * @param manager
+     */
+    NetworkRequestBuilder& setManager(QNetworkAccessManager* manager);
+
+    /**
+     * Builds a new NetworkRequest based on the configured parameters.
+     */
+    NetworkRequest build();
+};
 
 #endif // KEEPASSXC_NETWORKREQUEST_H

tests/TestNetworkRequest.cpp

@@ -11,6 +11,12 @@ using ContentTypeParameters_t = QHash<QString, QString>;
 Q_DECLARE_METATYPE(ContentTypeParameters_t);
 Q_DECLARE_METATYPE(std::chrono::milliseconds);
 
+namespace {
+    void processRequests() {
+        QTest::qWait(300);
+    }
+}
+
 void TestNetworkRequest::testNetworkRequest()
 {
     QFETCH(QUrl, requestedURL);
@@ -39,7 +45,7 @@ void TestNetworkRequest::testNetworkRequest()
     }
 
     // Create request
-    NetworkRequest request = createRequest(requestedURL, 5, std::chrono::milliseconds{5000}, QList<QPair<QString, QString>>{}, &manager);
+    NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager).build();
 
     QString actualContent;
     bool didError = false, didSucceed = false;
@@ -54,8 +60,8 @@ void TestNetworkRequest::testNetworkRequest()
     QSignalSpy errorSpy(&request, &NetworkRequest::failure);
     connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; });
 
-
+    request.fetch();
-    QTest::qWait(3*100);
+    processRequests();
 
     // Ensures that predicates match - i.e., the header was set correctly
     QCOMPARE(manager.matchedRequests().length(), 1);
@@ -134,7 +140,7 @@ void TestNetworkRequest::testNetworkRequestTimeout()
     reply.withFinishDelayUntil(&timer, &QTimer::timeout);
 
     // Create request
-    NetworkRequest request = createRequest(requestedURL, 5, timeout, QList<QPair<QString, QString>>{}, &manager);
+    NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager).setTimeout(timeout).build();
 
     // Start timer
     timer.start();
@@ -149,8 +155,8 @@ void TestNetworkRequest::testNetworkRequestTimeout()
     QSignalSpy errorSpy(&request, &NetworkRequest::failure);
     connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; });
 
-
+    request.fetch();
-    QTest::qWait(3*100);
+    processRequests();
 
     QTEST_ASSERT(didError || didSucceed);
 
@@ -168,8 +174,8 @@ void TestNetworkRequest::testNetworkRequestTimeout_data()
     QTest::addColumn<std::chrono::milliseconds>("delay");
     QTest::addColumn<std::chrono::milliseconds>("timeout");
 
-    QTest::newRow("timeout") << true << std::chrono::milliseconds{200} << std::chrono::milliseconds{100};
+    QTest::newRow("timeout") << true << std::chrono::milliseconds{100} << std::chrono::milliseconds{50};
-    QTest::newRow("no timeout") << false << std::chrono::milliseconds{100} << std::chrono::milliseconds{200};
+    QTest::newRow("no timeout") << false << std::chrono::milliseconds{50} << std::chrono::milliseconds{100};
 }
 
 void TestNetworkRequest::testNetworkRequestRedirects()
@@ -209,7 +215,8 @@ void TestNetworkRequest::testNetworkRequestRedirects()
     reply->withBody(QString{"test-content"}.toUtf8());
 
     // Create request
-    NetworkRequest request = createRequest(requestedURL, maxRedirects, std::chrono::milliseconds{5000}, QList<QPair<QString, QString>>{}, &manager);
+    NetworkRequest request = NetworkRequestBuilder(requestedURL).setManager(&manager)
+                                 .setMaxRedirects(maxRedirects).build();
 
     bool didSucceed = false, didError = false;
     // Check request
@@ -221,8 +228,8 @@ void TestNetworkRequest::testNetworkRequestRedirects()
     QSignalSpy errorSpy(&request, &NetworkRequest::failure);
     connect(&request, &NetworkRequest::failure, [&didError]() { didError = true; });
 
-
+    request.fetch();
-    QTest::qWait(3*100);
+    processRequests();
 
     QTEST_ASSERT(didError || didSucceed);
     // Ensures that predicates match - i.e., the header was set correctly
@@ -239,10 +246,10 @@ void TestNetworkRequest::testNetworkRequestRedirects_data()
     QTest::addColumn<int>("numRedirects");
     QTest::addColumn<int>("maxRedirects");
 
-    QTest::newRow("all good (0)") << 0 << 5;
+    QTest::newRow("fewer redirects than allowed (0)") << 0 << 5;
-    QTest::newRow("all good (1)") << 1 << 5;
+    QTest::newRow("fewer redirects than allowed (1)") << 1 << 5;
-    QTest::newRow("all good (2)") << 2 << 5;
+    QTest::newRow("fewer redirects than allowed (2)") << 2 << 5;
-    QTest::newRow("no good (1, 0)") << 1 << 0;
+    QTest::newRow("more redirects than allowed (1, 0)") << 1 << 0;
-    QTest::newRow("no good (2, 1)") << 2 << 1;
+    QTest::newRow("more redirects than allowed (2, 1)") << 2 << 1;
-    QTest::newRow("no good (3, 2)") << 3 << 2;
+    QTest::newRow("more redirects than allowed (3, 2)") << 3 << 2;
 }