Cache redirects properly by implementing our own QSettings cache for urls. (#5186)

* Cache redirects properly by implementing our own QSettings cache for urls.

* Load and store redirects properly.

* Set the maximum network cache size from settings value on PictureLoaderWorker instantiation.

* Address comments.

* Lint.

* Adjust debug statements to be in line with existing ones.

* Minor Tweaks

* Make redirect cache ttl a user-adjustable setting.

* Fix Build

* Minor Cleanup

* Minor Cleanup

* Build Fix

---------

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
Co-authored-by: ZeldaZach <zahalpern+github@gmail.com>

cockatrice/src/client/ui/picture_loader.cpp

@@ -1,17 +1,14 @@
 #include "picture_loader.h"
 
-#include "../../game/cards/card_database.h"
 #include "../../game/cards/card_database_manager.h"
 #include "../../settings/cache_settings.h"
-#include "theme_manager.h"
 
 #include <QApplication>
 #include <QBuffer>
 #include <QCryptographicHash>
 #include <QDebug>
-#include <QDir>
 #include <QDirIterator>
-#include <QFile>
+#include <QFileInfo>
 #include <QImageReader>
 #include <QMovie>
 #include <QNetworkAccessManager>
@@ -23,7 +20,6 @@
 #include <QRegularExpression>
 #include <QScreen>
 #include <QSet>
-#include <QSvgRenderer>
 #include <QThread>
 #include <QUrl>
 #include <algorithm>
@@ -136,6 +132,8 @@ PictureLoaderWorker::PictureLoaderWorker()
 #endif
     auto cache = new QNetworkDiskCache(this);
     cache->setCacheDirectory(SettingsCache::instance().getNetworkCachePath());
+    cache->setMaximumCacheSize(1024L * 1024L *
+                               static_cast<qint64>(SettingsCache::instance().getNetworkCacheSizeInMB()));
     // Note: the settings is in MB, but QNetworkDiskCache uses bytes
     connect(&SettingsCache::instance(), &SettingsCache::networkCacheSizeChanged, cache,
             [cache](int newSizeInMB) { cache->setMaximumCacheSize(1024L * 1024L * static_cast<qint64>(newSizeInMB)); });
@@ -145,6 +143,13 @@ PictureLoaderWorker::PictureLoaderWorker()
     networkManager->setRedirectPolicy(QNetworkRequest::ManualRedirectPolicy);
     connect(networkManager, SIGNAL(finished(QNetworkReply *)), this, SLOT(picDownloadFinished(QNetworkReply *)));
 
+    cacheFilePath = SettingsCache::instance().getRedirectCachePath() + REDIRECT_CACHE_FILENAME;
+    loadRedirectCache();
+    cleanStaleEntries();
+
+    connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this,
+            &PictureLoaderWorker::saveRedirectCache);
+
     pictureLoaderThread = new QThread;
     pictureLoaderThread->start(QThread::LowPriority);
     moveToThread(pictureLoaderThread);
@@ -447,11 +452,104 @@ bool PictureLoaderWorker::imageIsBlackListed(const QByteArray &picData)
 
 QNetworkReply *PictureLoaderWorker::makeRequest(const QUrl &url)
 {
+    // Check if the redirect is cached
+    QUrl cachedRedirect = getCachedRedirect(url);
+    if (!cachedRedirect.isEmpty()) {
+        qDebug().nospace() << "PictureLoader: [card: " << cardBeingDownloaded.getCard()->getCorrectedName()
+                           << " set: " << cardBeingDownloaded.getSetName() << "]: Using cached redirect for "
+                           << url.toDisplayString() << " to " << cachedRedirect.toDisplayString();
+        return makeRequest(cachedRedirect); // Use the cached redirect
+    }
+
     QNetworkRequest req(url);
+
     if (!picDownload) {
         req.setAttribute(QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::AlwaysCache);
     }
-    return networkManager->get(req);
+
+    QNetworkReply *reply = networkManager->get(req);
+
+    connect(reply, &QNetworkReply::finished, this, [this, reply, url]() {
+        QVariant redirectTarget = reply->attribute(QNetworkRequest::RedirectionTargetAttribute);
+
+        if (redirectTarget.isValid()) {
+            QUrl redirectUrl = redirectTarget.toUrl();
+            if (redirectUrl.isRelative()) {
+                redirectUrl = url.resolved(redirectUrl);
+            }
+
+            cacheRedirect(url, redirectUrl);
+            qDebug().nospace() << "PictureLoader: [card: " << cardBeingDownloaded.getCard()->getCorrectedName()
+                               << " set: " << cardBeingDownloaded.getSetName() << "]: Caching redirect from "
+                               << url.toDisplayString() << " to " << redirectUrl.toDisplayString();
+        }
+
+        reply->deleteLater();
+    });
+
+    return reply;
+}
+
+void PictureLoaderWorker::cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl)
+{
+    redirectCache[originalUrl] = qMakePair(redirectUrl, QDateTime::currentDateTimeUtc());
+    saveRedirectCache();
+}
+
+QUrl PictureLoaderWorker::getCachedRedirect(const QUrl &originalUrl) const
+{
+    if (redirectCache.contains(originalUrl)) {
+        return redirectCache[originalUrl].first;
+    }
+    return {};
+}
+
+void PictureLoaderWorker::loadRedirectCache()
+{
+    QSettings settings(cacheFilePath, QSettings::IniFormat);
+
+    redirectCache.clear();
+    int size = settings.beginReadArray(REDIRECT_HEADER_NAME);
+    for (int i = 0; i < size; ++i) {
+        settings.setArrayIndex(i);
+        QUrl originalUrl = settings.value(REDIRECT_ORIGINAL_URL).toUrl();
+        QUrl redirectUrl = settings.value(REDIRECT_URL).toUrl();
+        QDateTime timestamp = settings.value(REDIRECT_TIMESTAMP).toDateTime();
+
+        if (originalUrl.isValid() && redirectUrl.isValid()) {
+            redirectCache[originalUrl] = qMakePair(redirectUrl, timestamp);
+        }
+    }
+    settings.endArray();
+}
+
+void PictureLoaderWorker::saveRedirectCache() const
+{
+    QSettings settings(cacheFilePath, QSettings::IniFormat);
+
+    settings.beginWriteArray(REDIRECT_HEADER_NAME, static_cast<int>(redirectCache.size()));
+    int index = 0;
+    for (auto it = redirectCache.cbegin(); it != redirectCache.cend(); ++it) {
+        settings.setArrayIndex(index++);
+        settings.setValue(REDIRECT_ORIGINAL_URL, it.key());
+        settings.setValue(REDIRECT_URL, it.value().first);
+        settings.setValue(REDIRECT_TIMESTAMP, it.value().second);
+    }
+    settings.endArray();
+}
+
+void PictureLoaderWorker::cleanStaleEntries()
+{
+    QDateTime now = QDateTime::currentDateTimeUtc();
+
+    auto it = redirectCache.begin();
+    while (it != redirectCache.end()) {
+        if (it.value().second.addDays(SettingsCache::instance().getRedirectCacheTtl()) < now) {
+            it = redirectCache.erase(it); // Remove stale entry
+        } else {
+            ++it;
+        }
+    }
 }
 
 void PictureLoaderWorker::picDownloadFinished(QNetworkReply *reply)

cockatrice/src/client/ui/picture_loader.h

@@ -11,6 +11,12 @@ class QNetworkAccessManager;
 class QNetworkReply;
 class QThread;
 
+#define REDIRECT_HEADER_NAME "redirects"
+#define REDIRECT_ORIGINAL_URL "original"
+#define REDIRECT_URL "redirect"
+#define REDIRECT_TIMESTAMP "timestamp"
+#define REDIRECT_CACHE_FILENAME "cache.ini"
+
 class PictureToLoad
 {
 private:
@@ -83,6 +89,9 @@ private:
     QList<PictureToLoad> loadQueue;
     QMutex mutex;
     QNetworkAccessManager *networkManager;
+    QHash<QUrl, QPair<QUrl, QDateTime>> redirectCache; // Stores redirect and timestamp
+    QString cacheFilePath;                             // Path to persistent storage
+    static constexpr int CacheTTLInDays = 30;          // TODO: Make user configurable
     QList<PictureToLoad> cardsToDownload;
     PictureToLoad cardBeingLoaded;
     PictureToLoad cardBeingDownloaded;
@@ -91,6 +100,12 @@ private:
     bool cardImageExistsOnDisk(QString &setName, QString &correctedCardName);
     bool imageIsBlackListed(const QByteArray &);
     QNetworkReply *makeRequest(const QUrl &url);
+    void cacheRedirect(const QUrl &originalUrl, const QUrl &redirectUrl);
+    QUrl getCachedRedirect(const QUrl &originalUrl) const;
+    void loadRedirectCache();
+    void saveRedirectCache() const;
+    void cleanStaleEntries();
+
 private slots:
     void picDownloadFinished(QNetworkReply *reply);
     void picDownloadFailed();

cockatrice/src/dialogs/dlg_settings.cpp

@@ -629,11 +629,22 @@ DeckEditorSettingsPage::DeckEditorSettingsPage()
     networkCacheEdit.setValue(SettingsCache::instance().getNetworkCacheSizeInMB());
     networkCacheEdit.setSuffix(" MB");
 
+    networkRedirectCacheTtlEdit.setMinimum(NETWORK_REDIRECT_CACHE_TTL_MIN);
+    networkRedirectCacheTtlEdit.setMaximum(NETWORK_REDIRECT_CACHE_TTL_MAX);
+    networkRedirectCacheTtlEdit.setSingleStep(1);
+    networkRedirectCacheTtlEdit.setValue(SettingsCache::instance().getRedirectCacheTtl());
+    networkRedirectCacheTtlEdit.setSuffix(" " + tr("Day(s)"));
+
     auto networkCacheLayout = new QHBoxLayout;
     networkCacheLayout->addStretch();
     networkCacheLayout->addWidget(&networkCacheLabel);
     networkCacheLayout->addWidget(&networkCacheEdit);
 
+    auto networkRedirectCacheLayout = new QHBoxLayout;
+    networkRedirectCacheLayout->addStretch();
+    networkRedirectCacheLayout->addWidget(&networkRedirectCacheTtlLabel);
+    networkRedirectCacheLayout->addWidget(&networkRedirectCacheTtlEdit);
+
     auto pixmapCacheLayout = new QHBoxLayout;
     pixmapCacheLayout->addStretch();
     pixmapCacheLayout->addWidget(&pixmapCacheLabel);
@@ -645,8 +656,9 @@ DeckEditorSettingsPage::DeckEditorSettingsPage()
     lpGeneralGrid->addLayout(messageListLayout, 1, 0, 1, 2);
     lpGeneralGrid->addLayout(networkCacheLayout, 2, 0, 1, 2);
     lpGeneralGrid->addLayout(pixmapCacheLayout, 3, 0, 1, 2);
-    lpGeneralGrid->addWidget(&urlLinkLabel, 4, 0);
+    lpGeneralGrid->addLayout(networkRedirectCacheLayout, 4, 0, 1, 2);
-    lpGeneralGrid->addWidget(&clearDownloadedPicsButton, 4, 1);
+    lpGeneralGrid->addWidget(&urlLinkLabel, 5, 0);
+    lpGeneralGrid->addWidget(&clearDownloadedPicsButton, 5, 1);
 
     // Spoiler Layout
     lpSpoilerGrid->addWidget(&mcDownloadSpoilersCheckBox, 0, 0);
@@ -657,13 +669,15 @@ DeckEditorSettingsPage::DeckEditorSettingsPage()
     lpSpoilerGrid->addWidget(updateNowButton, 2, 1);
     lpSpoilerGrid->addWidget(&infoOnSpoilersLabel, 3, 0, 1, 3, Qt::AlignTop);
 
-    // On a change to the check box, hide/unhide the other fields
+    // On a change to the checkbox, hide/un-hide the other fields
     connect(&mcDownloadSpoilersCheckBox, SIGNAL(toggled(bool)), &SettingsCache::instance(),
             SLOT(setDownloadSpoilerStatus(bool)));
     connect(&mcDownloadSpoilersCheckBox, SIGNAL(toggled(bool)), this, SLOT(setSpoilersEnabled(bool)));
     connect(&pixmapCacheEdit, SIGNAL(valueChanged(int)), &SettingsCache::instance(), SLOT(setPixmapCacheSize(int)));
     connect(&networkCacheEdit, SIGNAL(valueChanged(int)), &SettingsCache::instance(),
             SLOT(setNetworkCacheSizeInMB(int)));
+    connect(&networkRedirectCacheTtlEdit, SIGNAL(valueChanged(int)), &SettingsCache::instance(),
+            SLOT(setNetworkRedirectCacheTtl(int)));
 
     mpGeneralGroupBox = new QGroupBox;
     mpGeneralGroupBox->setLayout(lpGeneralGrid);
@@ -844,9 +858,11 @@ void DeckEditorSettingsPage::retranslateUi()
     urlLinkLabel.setText(QString("<a href='%1'>%2</a>").arg(WIKI_CUSTOM_PIC_URL).arg(tr("How to add a custom URL")));
     clearDownloadedPicsButton.setText(tr("Delete Downloaded Images"));
     resetDownloadURLs.setText(tr("Reset Download URLs"));
-    networkCacheLabel.setText(tr("Downloaded images directory size:"));
+    networkCacheLabel.setText(tr("Network Cache Size:"));
     networkCacheEdit.setToolTip(tr("On-disk cache for downloaded pictures"));
-    pixmapCacheLabel.setText(tr("Picture cache size:"));
+    networkRedirectCacheTtlLabel.setText(tr("Redirect Cache TTL:"));
+    networkRedirectCacheTtlEdit.setToolTip(tr("How long cached redirects for urls are valid for."));
+    pixmapCacheLabel.setText(tr("Picture Cache Size:"));
     pixmapCacheEdit.setToolTip(tr("In-memory cache for pictures not currently on screen"));
 }
 

cockatrice/src/dialogs/dlg_settings.h

@@ -174,6 +174,8 @@ private:
     QPushButton *updateNowButton;
     QLabel networkCacheLabel;
     QSpinBox networkCacheEdit;
+    QLabel networkRedirectCacheTtlLabel;
+    QSpinBox networkRedirectCacheTtlEdit;
     QSpinBox pixmapCacheEdit;
     QLabel pixmapCacheLabel;
 };

cockatrice/src/settings/cache_settings.cpp

@@ -221,6 +221,7 @@ SettingsCache::SettingsCache()
         pixmapCacheSize = PIXMAPCACHE_SIZE_DEFAULT;
 
     networkCacheSize = settings->value("personal/networkCacheSize", NETWORK_CACHE_SIZE_DEFAULT).toInt();
+    redirectCacheTtl = settings->value("personal/redirectCacheTtl", NETWORK_REDIRECT_CACHE_TTL_DEFAULT).toInt();
 
     picDownload = settings->value("personal/picturedownload", true).toBool();
 
@@ -657,6 +658,13 @@ void SettingsCache::setNetworkCacheSizeInMB(const int _networkCacheSize)
     emit networkCacheSizeChanged(networkCacheSize);
 }
 
+void SettingsCache::setNetworkRedirectCacheTtl(const int _redirectCacheTtl)
+{
+    redirectCacheTtl = _redirectCacheTtl;
+    settings->setValue("personal/redirectCacheSize", redirectCacheTtl);
+    emit redirectCacheTtlChanged(redirectCacheTtl);
+}
+
 void SettingsCache::setClientID(const QString &_clientID)
 {
     clientID = _clientID;
@@ -1030,6 +1038,7 @@ void SettingsCache::loadPaths()
     replaysPath = getSafeConfigPath("paths/replays", dataPath + "/replays/");
     themesPath = getSafeConfigPath("paths/themes", dataPath + "/themes/");
     picsPath = getSafeConfigPath("paths/pics", dataPath + "/pics/");
+    redirectCachePath = getSafeConfigPath("paths/redirects", getCachePath() + "/redirects/");
     // this has never been exposed as an user-configurable setting
     if (picsPath.endsWith("/")) {
         customPicsPath = getSafeConfigPath("paths/custompics", picsPath + "CUSTOM/");

cockatrice/src/settings/cache_settings.h

@@ -16,16 +16,21 @@
 
 class ReleaseChannel;
 
-// size should be a multiple of 64
+// In MB (Increments of 64)
-#define PIXMAPCACHE_SIZE_DEFAULT 2047
+#define PIXMAPCACHE_SIZE_DEFAULT 2048
 #define PIXMAPCACHE_SIZE_MIN 64
-#define PIXMAPCACHE_SIZE_MAX 2047
+#define PIXMAPCACHE_SIZE_MAX 4096
 
 // In MB
 constexpr int NETWORK_CACHE_SIZE_DEFAULT = 1024 * 4; // 4 GB
 constexpr int NETWORK_CACHE_SIZE_MIN = 1;            // 1 MB
 constexpr int NETWORK_CACHE_SIZE_MAX = 1024 * 1024;  // 1 TB
 
+// In Days
+#define NETWORK_REDIRECT_CACHE_TTL_DEFAULT 30
+#define NETWORK_REDIRECT_CACHE_TTL_MIN 1
+#define NETWORK_REDIRECT_CACHE_TTL_MAX 90
+
 #define DEFAULT_LANG_NAME "English"
 #define CLIENT_INFO_NOT_SET "notset"
 
@@ -53,6 +58,7 @@ signals:
     void ignoreUnregisteredUserMessagesChanged();
     void pixmapCacheSizeChanged(int newSizeInMBs);
     void networkCacheSizeChanged(int newSizeInMBs);
+    void redirectCacheTtlChanged(int newTtl);
     void masterVolumeChanged(int value);
     void chatMentionCompleterChanged();
     void downloadSpoilerTimeIndexChanged();
@@ -73,8 +79,8 @@ private:
     QByteArray tokenDialogGeometry;
     QByteArray setsDialogGeometry;
     QString lang;
-    QString deckPath, replaysPath, picsPath, customPicsPath, cardDatabasePath, customCardDatabasePath, themesPath,
+    QString deckPath, replaysPath, picsPath, redirectCachePath, customPicsPath, cardDatabasePath,
-        spoilerDatabasePath, tokenDatabasePath, themeName;
+        customCardDatabasePath, themesPath, spoilerDatabasePath, tokenDatabasePath, themeName;
     bool notifyAboutUpdates;
     bool notifyAboutNewVersion;
     bool showTipsOnStartup;
@@ -116,6 +122,7 @@ private:
     bool useTearOffMenus;
     int pixmapCacheSize;
     int networkCacheSize;
+    int redirectCacheTtl;
     bool scaleCards;
     int verticalCardOverlapPercent;
     bool showMessagePopups;
@@ -183,6 +190,10 @@ public:
     {
         return picsPath;
     }
+    QString getRedirectCachePath() const
+    {
+        return redirectCachePath;
+    }
     QString getCustomPicsPath() const
     {
         return customPicsPath;
@@ -356,6 +367,10 @@ public:
     {
         return networkCacheSize;
     }
+    int getRedirectCacheTtl() const
+    {
+        return redirectCacheTtl;
+    }
     bool getScaleCards() const
     {
         return scaleCards;
@@ -557,6 +572,7 @@ public slots:
     void setIgnoreUnregisteredUserMessages(QT_STATE_CHANGED_T _ignoreUnregisteredUserMessages);
     void setPixmapCacheSize(const int _pixmapCacheSize);
     void setNetworkCacheSizeInMB(const int _networkCacheSize);
+    void setNetworkRedirectCacheTtl(const int _redirectCacheTtl);
     void setCardScaling(const QT_STATE_CHANGED_T _scaleCards);
     void setStackCardOverlapPercent(const int _verticalCardOverlapPercent);
     void setShowMessagePopups(const QT_STATE_CHANGED_T _showMessagePopups);

dbconverter/src/mocks.cpp

@@ -220,6 +220,9 @@ void SettingsCache::setPixmapCacheSize(const int /* _pixmapCacheSize */)
 void SettingsCache::setNetworkCacheSizeInMB(const int /* _networkCacheSize */)
 {
 }
+void SettingsCache::setNetworkRedirectCacheTtl(const int /* _redirectCacheTtl */)
+{
+}
 void SettingsCache::setClientID(const QString & /* _clientID */)
 {
 }

tests/carddatabase/mocks.cpp

@@ -224,6 +224,9 @@ void SettingsCache::setPixmapCacheSize(const int /* _pixmapCacheSize */)
 void SettingsCache::setNetworkCacheSizeInMB(const int /* _networkCacheSize */)
 {
 }
+void SettingsCache::setNetworkRedirectCacheTtl(const int /* _redirectCacheTtl */)
+{
+}
 void SettingsCache::setClientID(const QString & /* _clientID */)
 {
 }