- Handle 301/302 redirects automatically in OpdsRequestManager #1388

depuc · 2025-07-10T18:36:51Z

Follow redirects up to 5 times to prevent infinite loops
Add debug logging for redirect chains
Fixes Online library requests should be able to deal with HTTP (301 and 302) redirects #1374

- Follow redirects up to 5 times to prevent infinite loops - Add debug logging for redirect chains - Fixes kiwix#1374

veloman-yunkan

Obviously, the commit is incomplete (the changes in the sibling header file have not been checked in).

veloman-yunkan · 2025-07-11T07:58:33Z

src/opdsrequestmanager.cpp

+    if (redirectAttr.isValid() && redirectCount < kMaxRedirects) {
+        QUrl redirectUrl = redirectAttr.toUrl();
+        // Make absolute if needed
+        if (redirectUrl.isRelative()) {
+            redirectUrl = reply->url().resolved(redirectUrl);
+        }
+        qInfo() << "Following redirect" << redirectCount + 1 << "to:" << redirectUrl.toString();
+        reply->deleteLater();
+        QNetworkRequest newRequest(redirectUrl);
+        QNetworkReply* newReply = m_networkManager.get(newRequest);
+        connect(newReply, &QNetworkReply::finished, this, [=]() {
+            handleReply(newReply, finalHandler, redirectCount + 1);
+        });
+        return;
+    }
+    // No redirect, or max redirects reached
+    if (redirectCount > 0) {
+        qInfo() << "Completed after" << redirectCount << "redirects";
+    }
+    finalHandler(reply);


If the limit on the redirect chain length is exceeded then finalHandler() is called. Shouldn't an error somehow be reported instead?

Thank you for pointing this out!
You are correct—previously, if the redirect chain exceeded the maximum allowed, the handler would be called without any indication of an error, which could make it difficult to distinguish this case from a normal response.
I've updated the implementation so that when the redirect limit is reached:
A warning is logged,
A custom property (redirectLimitReached) is set on the QNetworkReply,
And reply->abort() is called to set an error state.
This way, the handler can now detect when the redirect limit has been exceeded and handle it appropriately.
Let me know if you have any further suggestions :)

veloman-yunkan · 2025-07-11T07:59:14Z

src/opdsrequestmanager.cpp

+// New method to allow requests to arbitrary URLs (for redirects)
+QNetworkReply* OpdsRequestManager::opdsResponseFromUrl(const QUrl &url) {
+    QNetworkRequest request(url);
+    return m_networkManager.get(request);
+}
+


Seems to be unused

Thank you for pointing this out!
I added opdsResponseFromUrl to support potential future use cases where we might need to fetch OPDS data from arbitrary URLs, not just the default catalog endpoints.
While it’s not currently used, I thought it might be useful for future extensibility or for unit testing.
If you prefer, I can remove it for now and reintroduce it when needed.

Undoubtedly there is much more stuff that can be added to support potential future use cases, but since kiwix-desktop is an application rather than a library we don't need such unused code.

veloman-yunkan · 2025-07-11T08:08:05Z

src/opdsrequestmanager.cpp

    auto mp_reply = opdsResponseFromPath("/catalog/v2/entries", query);
    connect(mp_reply, &QNetworkReply::finished, this, [=]() {
-        receiveContent(mp_reply);
+        handleReply(mp_reply, [this](QNetworkReply* r) { receiveContent(r); }, 0);


If you give up some generality of handleReply() (by changing the type of the second parameter to a member-function pointer of appropriate type) you can shorten this line to a more readable handleReply(mp_reply, receiveContent, 0). But it's a matter of taste. I am just pointing out the possibility.

Thank you for the suggestion!
I agree that using a member function pointer for the second parameter of handleReply would allow for a more concise call like handleReply(mp_reply, &OpdsRequestManager::receiveContent, 0), which improves readability.
I chose to use std::function and a lambda for the handler to allow for greater flexibility, such as capturing additional context or using different callables if needed in the future. However, for the current use case, switching to a member function pointer would work well and simplify the code.
I’ll consider this refactor for improved clarity, unless there’s a strong reason to keep the extra generality.
Thanks for pointing out the alternative!

depuc · 2025-07-11T08:53:26Z

Obviously, the commit is incomplete (the changes in the sibling header file have not been checked in).

Thank you for the code review! This was actually my first time going through this process, and I realize I was a bit clumsy with the commit — I missed including the changes in the sibling header file. I appreciate your feedback and will definitely look into this.

veloman-yunkan · 2025-07-11T10:52:39Z

If a PR is not approved on first attempt it doesn't mean that it has to be closed. If you plan to pursue this improvement, you can do it in the same PR instead of submitting a new one.

kelson42 · 2025-07-11T13:40:03Z

@depuc Kind of hope you don't give uo, so reopening the PR.

depuc · 2025-07-11T13:49:59Z

Thank you very much.
I'll work through it

Follow redirects up to 5 times to prevent infinite loops Improves reliability of online catalog requests

kelson42 · 2025-07-18T05:26:35Z

@depuc Great. Thx, please give feedback to @veloman-yunkan comments and let us know when you feel ready for a new review pass.

…dd logic to detect when the maximum number of HTTP redirects is reached. - Set a custom property and abort the QNetworkReply to indicate a redirect loop or excessive redirects. - Log a warning when the redirect limit is hit, so the handler can distinguish this error case. - Improves robustness for online library requests with multiple redirects.

…ager - Add missing declarations for handleReply and opdsResponseFromUrl to the header file. - Ensures all used methods are properly declared, resolving build errors. - Keeps header and implementation files in sync.

depuc · 2025-07-18T07:58:54Z

Hi @kelson42,
I've addressed @veloman-yunkan’s comments and pushed the changes.
Feel free to take another look whenever you have time—let me know if anything else needs tweaking.

Thanks again!

veloman-yunkan

For the next iteration, please provide a clean version of the PR (with all changes in a single commit).

veloman-yunkan · 2025-07-21T10:47:58Z

src/opdsrequestmanager.h

    QNetworkReply* opdsResponseFromPath(const QString &path, const QUrlQuery &query = QUrlQuery());
+    void handleNetworkReply(QNetworkReply* reply, void (OpdsRequestManager::*finalHandler)(QNetworkReply*), int redirectCount);
+    void handleReply(QNetworkReply* reply, std::function<void(QNetworkReply*)> finalHandler, int redirectCount);
+    static constexpr int MAX_REDIRECTS = 5;


OpdsRequestManager::MAX_REDIRECTS is unused

veloman-yunkan · 2025-07-21T10:49:02Z

src/opdsrequestmanager.h

    void requestReceived(const QString&);
    void languagesReceived(const QString&);
    void categoriesReceived(const QString&);
+    void requestError(const QString& errorMessage);


veloman-yunkan · 2025-07-21T10:49:58Z

src/opdsrequestmanager.h

 private:
    QNetworkAccessManager m_networkManager;
    QNetworkReply* opdsResponseFromPath(const QString &path, const QUrlQuery &query = QUrlQuery());
+    void handleNetworkReply(QNetworkReply* reply, void (OpdsRequestManager::*finalHandler)(QNetworkReply*), int redirectCount);


handleNetworkReply() is unused (and not defined either)

veloman-yunkan · 2025-07-21T11:04:22Z

src/opdsrequestmanager.cpp

+    }
+    // No redirect, or max redirects reached
+    if (redirectAttr.isValid() && redirectCount >= kMaxRedirects) {
+        qWarning() << "Redirect limit (" << kMaxRedirects << ") reached. Reporting error.";


Please delete the "Reporting error." part

veloman-yunkan · 2025-07-21T11:07:15Z

src/opdsrequestmanager.cpp

+        if (redirectUrl.isRelative()) {
+            redirectUrl = reply->url().resolved(redirectUrl);
+        }
+        qInfo() << "Following redirect" << redirectCount + 1 << "to:" << redirectUrl.toString();


It would be nice to have the URL that was redirected (reply->url()) in this info message.

veloman-yunkan · 2025-07-21T11:09:53Z

src/opdsrequestmanager.cpp

+        QUrl redirectUrl = redirectAttr.toUrl();
+        // Make absolute if needed
+        if (redirectUrl.isRelative()) {
+            redirectUrl = reply->url().resolved(redirectUrl);
+        }
+        qInfo() << "Following redirect" << redirectCount + 1 << "to:" << redirectUrl.toString();
+        reply->deleteLater();
+        QNetworkRequest newRequest(redirectUrl);
+        QNetworkReply* newReply = m_networkManager.get(newRequest);
+        connect(newReply, &QNetworkReply::finished, this, [=]() {
+            handleReply(newReply, finalHandler, redirectCount + 1);
+        });


I would extract this piece of code into a function of its own followRedirect(reply, finalHandler, redirectCount) (in order to keep handleReply() short and readable.

veloman-yunkan · 2025-07-21T11:16:54Z

src/opdsrequestmanager.cpp

+        return;
+    }
+    // No redirect, or max redirects reached
+    if (redirectAttr.isValid() && redirectCount >= kMaxRedirects) {


With current code the redirectCount >= kMaxRedirects check is redundant. However simply deleting it will lead to confusion. Please merge this if statement with the previous one and check the redirect count in a nested if/else statement.

- Handle 301/302 redirects automatically in OpdsRequestManager

2552691

- Follow redirects up to 5 times to prevent infinite loops - Add debug logging for redirect chains - Fixes kiwix#1374

kelson42 requested a review from veloman-yunkan July 11, 2025 04:45

veloman-yunkan requested changes Jul 11, 2025

View reviewed changes

depuc closed this Jul 11, 2025

kelson42 reopened this Jul 11, 2025

Add automatic HTTP 301/302 redirect handling to OpdsRequestManager

d8c3af8

Follow redirects up to 5 times to prevent infinite loops Improves reliability of online catalog requests

depuc added 2 commits July 18, 2025 12:34

veloman-yunkan requested changes Jul 21, 2025

View reviewed changes

Uh oh!

- Handle 301/302 redirects automatically in OpdsRequestManager #1388

Are you sure you want to change the base?

- Handle 301/302 redirects automatically in OpdsRequestManager #1388

Uh oh!

Conversation

depuc commented Jul 10, 2025

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

depuc commented Jul 11, 2025

Uh oh!

veloman-yunkan commented Jul 11, 2025

Uh oh!

kelson42 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

depuc commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kelson42 commented Jul 18, 2025

Uh oh!

depuc commented Jul 18, 2025

Uh oh!

veloman-yunkan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

veloman-yunkan Jul 11, 2025 •

edited

Loading

kelson42 commented Jul 11, 2025 •

edited

Loading

depuc commented Jul 11, 2025 •

edited

Loading