Handle 301/302 redirects automatically in OpdsRequestManager #1388

ghost · 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 #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!

ghost · 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.

ghost · 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.

ghost · 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

A leftover from earlier experiments (unused and undefined)

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);


A leftover from earlier experiments (unused and undefined)

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)

A leftover from earlier experiments (unused and undefined)

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.

kelson42 · 2025-11-03T19:56:30Z

@depuc Do you still plan to complete this PR?

ghost · 2025-11-03T20:04:29Z

@kelson42 Yes I will, I got busy with college will be back on it asap.

kelson42 · 2025-12-02T09:33:11Z

@depuc Any chance to move forward before the release of 2.5.0 in the next few weeks?

ghost · 2025-12-02T16:34:25Z

On it, surely by the end of this week

- Switch default host to `opds.library.kiwix.org` - Update API paths to remove `/catalog` prefix - Enable `RedirectPolicyAttribute` to follow HTTP redirects - Add class documentation Fixes #1374

ghost · 2025-12-03T14:38:41Z

@veloman-yunkan
When you get a chance, could you please review the changes I made?
I want to confirm that everything compiles correctly on your side as well.
If something needs adjustment, just let me know
Thanks!

veloman-yunkan

In its final form (the clean version of) this PR should consist of two commits (or there should be two separate PRs)

Following redirects
Switching from library.kiwix.org to to opds.library.kiwix.org (@kelson42, is there a ticket for that?)

veloman-yunkan · 2025-12-05T12:42:34Z

src/opdsrequestmanager.h

    void getLanguagesFromOpds();
    void getCategoriesFromOpds();
+    QNetworkReply* opdsResponseFromUrl(const QUrl &url);



A leftover from earlier experiments (unused and undefined)

veloman-yunkan · 2025-12-05T12:42:59Z

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);


A leftover from earlier experiments (unused and undefined)

veloman-yunkan · 2025-12-05T12:43:04Z

src/opdsrequestmanager.h

    QNetworkAccessManager m_networkManager;
    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);


A leftover from earlier experiments (unused and undefined)

Actually I removed all of them both forgot to change in the actual branch(newbie mistake sincere apologies), I will make sure that i will remove all these in the next iteration and come up with two commits
one for the re-directs and the other to change the API-URL

veloman-yunkan · 2025-12-05T12:43:10Z

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;


A leftover from earlier experiments (unused and undefined)

veloman-yunkan · 2025-12-05T12:43:16Z

src/opdsrequestmanager.h

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


A leftover from earlier experiments (unused and undefined)

kelson42 · 2025-12-05T18:17:21Z

2. Switching from library.kiwix.org to to opds.library.kiwix.org (@kelson42, is there a ticket for that?)

@veloman-yunkan You mean #1374?

@depuc I have moved the issue #1374 to the current milestone 2.5.0 which will be release very soon. Please revamp your commits like requested by @veloman-yunkan and I will merge.

veloman-yunkan · 2025-12-06T11:22:36Z

Switching from library.kiwix.org to to opds.library.kiwix.org (@kelson42, is there a ticket for that?)

@veloman-yunkan You mean #1374?

@kelson42 #1374 only requests to handle HTTP redirects. Though it mentions the catalog API switch as a reason for why that enhancement is needed, there is no explicit request to change the catalog API URL. I think that those changes shouldn't be combined in one ticket and we better have them in different PRs as well.

ghost · 2025-12-06T13:49:39Z

@kelson42 @veloman-yunkan
apparently I am stuck with a github issue
its not letting me push code because of a failed payment (ts quite annoying that its not accepting my card) so i will be deleting this account and will make a new PR so i will close this here.

kelson42 · 2025-12-06T18:40:20Z

Superseeded by #1429

kelson42 · 2025-12-06T18:41:28Z

Switching from library.kiwix.org to to opds.library.kiwix.org (@kelson42, is there a ticket for that?)

@veloman-yunkan You mean #1374?

@kelson42 #1374 only requests to handle HTTP redirects. Though it mentions the catalog API switch as a reason for why that enhancement is needed, there is no explicit request to change the catalog API URL. I think that those changes shouldn't be combined in one ticket and we better have them in different PRs as well.

I confirm, we should switch to opds.library.kiwix.org. Thx

- 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 #1374

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

veloman-yunkan requested changes Jul 11, 2025

View reviewed changes

ghost 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

rubmyass added 2 commits July 18, 2025 12:34

veloman-yunkan requested changes Jul 21, 2025

View reviewed changes

kelson42 changed the title ~~- Handle 301/302 redirects automatically in OpdsRequestManager~~ Handle 301/302 redirects automatically in OpdsRequestManager Dec 2, 2025

Merge branch 'kiwix:main' into fix-issue-1374

31a19dc

ghost closed this Dec 3, 2025

ghost deleted the fix-issue-1374 branch December 3, 2025 10:22

ghost restored the fix-issue-1374 branch December 3, 2025 10:28

ghost reopened this Dec 3, 2025

Update OPDS host to opds.library.kiwix.org and enable redirects

7327fa4

- Switch default host to `opds.library.kiwix.org` - Update API paths to remove `/catalog` prefix - Enable `RedirectPolicyAttribute` to follow HTTP redirects - Add class documentation Fixes #1374

veloman-yunkan requested changes Dec 5, 2025

View reviewed changes

Merge branch 'kiwix:main' into fix-issue-1374

d81b3f0

ghost closed this Dec 6, 2025

kelson42 mentioned this pull request Dec 6, 2025

Support HTTP redirections for OPDS feed #1429

Merged

This pull request was closed.

Uh oh!

Handle 301/302 redirects automatically in OpdsRequestManager #1388

Handle 301/302 redirects automatically in OpdsRequestManager #1388

Conversation

ghost 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!

ghost 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!

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

Uh oh!

Uh oh!

kelson42 commented Jul 18, 2025

Uh oh!

ghost 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!

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!

kelson42 commented Nov 3, 2025

Uh oh!

ghost commented Nov 3, 2025 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kelson42 commented Dec 2, 2025

Uh oh!

ghost commented Dec 2, 2025

Uh oh!

ghost commented Dec 3, 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!

veloman-yunkan Jul 11, 2025 •

edited

Loading

kelson42 commented Jul 11, 2025 •

edited

Loading

ghost commented Jul 11, 2025 •

edited by ghost

Loading

ghost commented Nov 3, 2025 •

edited by ghost

Loading

veloman-yunkan commented Dec 6, 2025 •

edited

Loading