Refactor requests to QNetworkAccessManager #15

514ckw4r3 · 2026-02-02T08:02:06Z

Переделал запросы в ngstd через QNetworkAccessManager

NGQ-231

Copilot

Pull request overview

This pull request refactors the HTTP request implementation in the ngstd library to use Qt's QNetworkAccessManager instead of GDAL's CPLHTTPFetch functions. This is a significant architectural change that modernizes the HTTP request handling while maintaining the same external API.

Changes:

Replaced GDAL CPL HTTP functions with Qt QNetworkAccessManager for all HTTP operations
Implemented custom retry logic, timeout handling, and SSL error handling using Qt's networking classes
Added helper functions for request execution, JSON parsing, and proxy credential resolution
Migrated OAuth token refresh, file upload, and all GET/POST operations to the new implementation

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 19 comments.

File	Description
src/core/request.h	Added two new public methods: timeout() and retryDelay() to expose configuration values
src/core/request.cpp	Complete refactoring of HTTP request logic from GDAL CPL to QNetworkAccessManager with new helper functions, retry logic, and error handling

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-06T14:32:28Z

src/core/request.cpp

+    const QString payload = QString("grant_type=refresh_token&client_id=%1&refresh_token=%2")
+            .arg(m_clientId, m_updateToken);
+    const int timeoutMs = std::max(1, m_request->timeout()) * 1000;
+    const int maxRetry = std::max(0, m_request->timeout());


The maxRetry parameter is incorrectly set to m_request->timeout() instead of the retry count. This should use a separate retry count setting, not the timeout value. This will cause the number of retries to be 20 (the timeout value) instead of the expected retry count (which should likely be 3 based on m_maxRetry initialization).

Suggested change

const int maxRetry = std::max(0, m_request->timeout());

const int maxRetry = std::max(0, m_request->maxRetry());

Copilot · 2026-02-06T14:32:28Z

src/core/request.cpp

+    static QNetworkAccessManager *manager = nullptr;
+    if(!manager) {
+        manager = new QNetworkAccessManager();
+    }
+    return manager;
+}
+


Memory leak: The QNetworkAccessManager instance created at line 83 is never deleted. Since this is a static pointer in a function, it will persist for the lifetime of the application but won't be properly cleaned up. Consider using a smart pointer or implementing proper cleanup in the NGRequest destructor.

Suggested change

static QNetworkAccessManager *manager = nullptr;

if(!manager) {

manager = new QNetworkAccessManager();

}

return manager;

}

static QNetworkAccessManager manager;

return &manager;

}

}

Copilot · 2026-02-06T14:32:29Z

src/core/request.cpp

+QNetworkAccessManager *networkManager()
+{
+    static QNetworkAccessManager *manager = nullptr;
+    if(!manager) {
+        manager = new QNetworkAccessManager();
+    }
+    return manager;
+}


Thread safety issue: The static QNetworkAccessManager pointer is accessed without synchronization. Multiple threads could potentially initialize or access the manager concurrently, leading to race conditions. Consider using a thread-safe singleton pattern with proper synchronization or QThreadStorage.

You can find example here

https://github.com/nextgis/qgis/blob/nextgisqgis/src/core/network/qgsnetworkaccessmanager.cpp#L198

Copilot · 2026-02-06T14:32:29Z

src/core/request.cpp

+RequestResult requestWithRetries(const QString &url, const QString &method,
+                                 const QByteArray &body, const QByteArray &contentType,
+                                 bool useAuthHeader, QHttpMultiPart *multiPart,
+                                 int timeoutMs, int maxRetry, int retryDelayMs)
+{
+    RequestResult result;
+    for(int attempt = 0; attempt <= maxRetry; ++attempt) {
+        result = executeRequest(url, method, body, contentType, useAuthHeader,
+                                multiPart, timeoutMs);
+        if(result.error == QNetworkReply::NoError && !result.timedOut) {
+            return result;
+        }
+        if(attempt < maxRetry) {
+            waitMs(retryDelayMs);
+        }


The multiPart parameter ownership is ambiguous. When multiPart is passed to executeRequest and then to networkManager()->post(), Qt takes ownership of it according to the documentation. However, requestWithRetries may call executeRequest multiple times in a retry loop, which would cause a crash on the second attempt because multiPart has already been deleted by Qt. File upload retries are currently disabled (maxRetry=0 at line 769), but this is a latent bug.

Copilot · 2026-02-06T14:32:29Z

src/core/request.cpp

+            userpwd = listOfProxies[0].user().toStdString() + ":" +
+                listOfProxies[0].password().toStdString();
+
+        }
+    }
+    else
+    {
+        url = proxyUrl.toStdString() + ":" + std::to_string(proxyPort);
+        userpwd = proxyUser.toStdString() + ":" +
+            proxyPassword.toStdString();


Potential empty credentials: When using system proxy, if the proxy has empty user/password fields, the code still creates a credentials string with just a colon (":"). This could be interpreted incorrectly by GDAL. Consider checking if user and password are empty and returning an empty string for userpwd in that case.

Suggested change

userpwd = listOfProxies[0].user().toStdString() + ":" +

listOfProxies[0].password().toStdString();

}

}

else

{

url = proxyUrl.toStdString() + ":" + std::to_string(proxyPort);

userpwd = proxyUser.toStdString() + ":" +

proxyPassword.toStdString();

const QString proxyUserQ = listOfProxies[0].user();

const QString proxyPasswordQ = listOfProxies[0].password();

if (proxyUserQ.isEmpty() && proxyPasswordQ.isEmpty()) {

userpwd.clear();

} else {

userpwd = proxyUserQ.toStdString() + ":" +

proxyPasswordQ.toStdString();

}

}

}

else

{

url = proxyUrl.toStdString() + ":" + std::to_string(proxyPort);

if (proxyUser.isEmpty() && proxyPassword.isEmpty()) {

userpwd.clear();

} else {

userpwd = proxyUser.toStdString() + ":" +

proxyPassword.toStdString();

}

Copilot · 2026-02-06T14:32:32Z

src/core/request.cpp

            QString postPayload = QString("grant_type=authorization_code&code=%1&redirect_uri=%2&client_id=%3")
                    .arg(options["code"])
                    .arg(options["redirectUri"])
                    .arg(clientId);
            if(!verify.isEmpty()) {
                postPayload += "&code_verifier=" + verify;
            }


Missing URL encoding: The authorization code payload construction concatenates OAuth parameters without URL encoding. Values like code, redirectUri, clientId, and codeVerifier may contain special characters that need to be percent-encoded for proper form-urlencoded format.

Copilot · 2026-02-06T14:32:32Z

src/core/request.cpp

+    RequestResult result = requestWithRetries(url, "GET", QByteArray(),
+                                              QByteArray(), true, nullptr,
+                                              timeoutMs, maxRetry, retryDelayMs);
+    if(result.error == QNetworkReply::NoError && !result.timedOut) {


Inconsistent error checking: Throughout the code, network errors are checked with 'result.error != QNetworkReply::NoError || result.timedOut', but HTTP status codes are not validated. A 200 OK with NoError but a 4xx/5xx status code from the server would be treated as success. Consider checking HTTP status codes for a more robust error detection.

Copilot · 2026-02-06T14:32:33Z

src/core/request.cpp

+               type != QSslError::SelfSignedCertificateInChain &&
+               type != QSslError::CertificateUntrusted) {


Inconsistent SSL error handling: The code only ignores SSL errors for self-signed certificates, but the type CertificateUntrusted is too broad and could ignore legitimate security issues. CertificateUntrusted can occur for reasons other than self-signed certificates. Consider removing CertificateUntrusted from the whitelist or being more specific about which certificate errors to ignore.

Suggested change

type != QSslError::SelfSignedCertificateInChain &&

type != QSslError::CertificateUntrusted) {

type != QSslError::SelfSignedCertificateInChain) {

Copilot · 2026-02-06T14:32:33Z

src/core/request.cpp

    if(useProxy) {
-        std::string url;
-        std::string userpwd;
        if(useSystemProxy) {
-            QNetworkProxyQuery npq(QUrl("http://www.google.com"));
-            QList<QNetworkProxy> listOfProxies =
-                    QNetworkProxyFactory::systemProxyForQuery(npq);
-            // Get first proxy if any.
-            if(!listOfProxies.isEmpty()) {
-                url = listOfProxies[0].hostName().toStdString() + ":" +
-                        std::to_string(listOfProxies[0].port());
-                userpwd = listOfProxies[0].user().toStdString() + ":" +
-                        listOfProxies[0].password().toStdString();
-
-            }
+            QNetworkProxyFactory::setUseSystemConfiguration(true);
+            networkManager()->setProxyFactory(nullptr);
        }
        else {
-            url = proxyUrl.toStdString() + ":" + std::to_string(porxyPort);
-            userpwd = proxyUser.toStdString() + ":" +
-                    proxyPassword.toStdString();
+            QNetworkProxy proxy(QNetworkProxy::HttpProxy, proxyUrl, proxyPort,
+                                proxyUser, proxyPassword);
+            networkManager()->setProxyFactory(nullptr);
+            networkManager()->setProxy(proxy);


Proxy settings not applied to existing requests: When setProxy is called, it updates the QNetworkAccessManager's proxy settings, but any ongoing requests may continue using the old proxy settings. Consider documenting this behavior or ensuring that proxy changes take effect for all subsequent requests.

Copilot · 2026-02-06T14:32:33Z

src/core/request.cpp

-            return QString("Authorization: Bearer %1").arg(m_accessToken);
-        }
+    const QString payload = QString("grant_type=refresh_token&client_id=%1&refresh_token=%2")
+            .arg(m_clientId, m_updateToken);


Missing URL encoding: The payload for OAuth token requests (lines 370-371, 659-660) concatenates values directly without URL encoding. If clientId, updateToken, or other OAuth parameters contain special characters (like &, =, or +), the request will be malformed. Consider using QUrl::toPercentEncoding() to properly encode these values.

Suggested change

.arg(m_clientId, m_updateToken);

.arg(QString::fromUtf8(QUrl::toPercentEncoding(m_clientId)),

QString::fromUtf8(QUrl::toPercentEncoding(m_updateToken)));

Refactor requests to QNetworkAccessManager

ca00715

NGQ-231

514ckw4r3 requested a review from ivanbarsukov February 2, 2026 08:02

514ckw4r3 self-assigned this Feb 2, 2026

514ckw4r3 added the enhancement New feature or request label Feb 2, 2026

Ignore self-signed SSL errors

3eb6c80

ivanbarsukov requested a review from Copilot February 6, 2026 14:25

Copilot started reviewing on behalf of ivanbarsukov February 6, 2026 14:26 View session

Copilot AI reviewed Feb 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor requests to QNetworkAccessManager #15

Refactor requests to QNetworkAccessManager #15

Uh oh!

514ckw4r3 commented Feb 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

ivanbarsukov Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Copilot AI Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	const int maxRetry = std::max(0, m_request->timeout());
	const int maxRetry = std::max(0, m_request->maxRetry());

-            userpwd = listOfProxies[0].user().toStdString() + ":" +
-                listOfProxies[0].password().toStdString();
-        }
-    }
-    else
-    {
-        url = proxyUrl.toStdString() + ":" + std::to_string(proxyPort);
-        userpwd = proxyUser.toStdString() + ":" +
-            proxyPassword.toStdString();
+            const QString proxyUserQ = listOfProxies[0].user();
+            const QString proxyPasswordQ = listOfProxies[0].password();
+            if (proxyUserQ.isEmpty() && proxyPasswordQ.isEmpty()) {
+                userpwd.clear();
+            } else {
+                userpwd = proxyUserQ.toStdString() + ":" +
+                    proxyPasswordQ.toStdString();
+            }
+        }
+    }
+    else
+    {
+        url = proxyUrl.toStdString() + ":" + std::to_string(proxyPort);
+        if (proxyUser.isEmpty() && proxyPassword.isEmpty()) {
+            userpwd.clear();
+        } else {
+            userpwd = proxyUser.toStdString() + ":" +
+                proxyPassword.toStdString();
+        }

		type != QSslError::SelfSignedCertificateInChain &&
		type != QSslError::CertificateUntrusted) {

	.arg(m_clientId, m_updateToken);
	.arg(QString::fromUtf8(QUrl::toPercentEncoding(m_clientId)),
	QString::fromUtf8(QUrl::toPercentEncoding(m_updateToken)));

Refactor requests to QNetworkAccessManager #15

Are you sure you want to change the base?

Refactor requests to QNetworkAccessManager #15

Uh oh!

Conversation

514ckw4r3 commented Feb 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

ivanbarsukov Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants