Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 46 additions & 3 deletions src/opdsrequestmanager.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#include "opdsrequestmanager.h"
#include "kiwixapp.h"
#include <QPointer>

namespace {
constexpr int kMaxRedirects = 5;
}

OpdsRequestManager::OpdsRequestManager()
{
Expand All @@ -21,6 +26,44 @@ int OpdsRequestManager::getCatalogPort()
: 443;
}

// Helper to handle replies and follow redirects
void OpdsRequestManager::handleReply(QNetworkReply* reply, std::function<void(QNetworkReply*)> finalHandler, int redirectCount) {
QVariant redirectAttr = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

reply->deleteLater();
QNetworkRequest newRequest(redirectUrl);
QNetworkReply* newReply = m_networkManager.get(newRequest);
connect(newReply, &QNetworkReply::finished, this, [=]() {
handleReply(newReply, finalHandler, redirectCount + 1);
});
Comment on lines +33 to +44
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

qWarning() << "Redirect limit (" << kMaxRedirects << ") reached. Reporting error.";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete the "Reporting error." part

// Set a custom property to indicate redirect limit reached
reply->setProperty("redirectLimitReached", true);
// Optionally, abort the reply to set an error
reply->abort();
}
if (redirectCount > 0) {
qInfo() << "Completed after" << redirectCount << "redirects";
}
finalHandler(reply);
}

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

Comment on lines +61 to +66
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be unused

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

void OpdsRequestManager::doUpdate(const QString& currentLanguage, const QString& categoryFilter)
{
QUrlQuery query;
Expand All @@ -43,7 +86,7 @@ void OpdsRequestManager::doUpdate(const QString& currentLanguage, const QString&

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

});
}

Expand All @@ -65,15 +108,15 @@ void OpdsRequestManager::getLanguagesFromOpds()
{
auto mp_reply = opdsResponseFromPath("/catalog/v2/languages");
connect(mp_reply, &QNetworkReply::finished, this, [=]() {
receiveLanguages(mp_reply);
handleReply(mp_reply, [this](QNetworkReply* r) { receiveLanguages(r); }, 0);
});
}

void OpdsRequestManager::getCategoriesFromOpds()
{
auto mp_reply = opdsResponseFromPath("/catalog/v2/categories");
connect(mp_reply, &QNetworkReply::finished, this, [=]() {
receiveCategories(mp_reply);
handleReply(mp_reply, [this](QNetworkReply* r) { receiveCategories(r); }, 0);
});
}

Expand Down
7 changes: 7 additions & 0 deletions src/opdsrequestmanager.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,20 @@ class OpdsRequestManager : public QObject
void doUpdate(const QString& currentLanguage, const QString& categoryFilter);
void getLanguagesFromOpds();
void getCategoriesFromOpds();
QNetworkReply* opdsResponseFromUrl(const QUrl &url);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleNetworkReply() is unused (and not defined either)

void handleReply(QNetworkReply* reply, std::function<void(QNetworkReply*)> finalHandler, int redirectCount);
static constexpr int MAX_REDIRECTS = 5;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OpdsRequestManager::MAX_REDIRECTS is unused


signals:
void requestReceived(const QString&);
void languagesReceived(const QString&);
void categoriesReceived(const QString&);
void requestError(const QString& errorMessage);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused


public slots:
void receiveContent(QNetworkReply*);
Expand All @@ -39,3 +44,5 @@ public slots:
};

#endif // OPDSREQUESTMANAGER_H