[CardDB] Keep parse-time set filtering and bust the cache on enablement changes (#7448)

The parsers keep dropping printings for disabled sets so that disabling a
set hides them everywhere, not just in deck editor search. The cached parse
output therefore depends on enablement, which the source hash did not cover:
the first parse froze whatever enablement state existed at that moment and
later toggles hit the stale cache.

- Fold a parser schema version and the sorted enabled set names into the
  binary cache source hash so toggling a set or enabling newly discovered
  sets (including custom ones) forces a re-parse on the next load.
- Harden the legacy set migration to only write keys that exist so missing
  legacy values can no longer clobber cardDatabase.ini and silently disable
  sets on upgrade.
- Make CardDatabase::reloadCardDatabasesAndNotify() bypass the binary cache
  so an explicit reload always re-reads the files on disk, even when their
  size/mtime hash still matches.
- Cover cache invalidation end to end: sandboxed loads assert that disabling
  a set, adding a custom database and enabling a fresh set each rewrite the
  cache, that a no-op load keeps it, and that an explicit reload picks up
  edits the source hash cannot see.

Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>

libcockatrice_card/libcockatrice/card/database/card_database.cpp

@@ -78,7 +78,9 @@ void CardDatabase::loadCardDatabases()
 
 void CardDatabase::reloadCardDatabasesAndNotify()
 {
-    loadCardDatabases();
+    // An explicit reload bypasses the binary cache so the files on disk are
+    // always re-read, even when their size/mtime hash still matches the cache.
+    loadStatus = loader->loadCardDatabases(/* ignoreCache */ true);
 
     if (loadStatus == Ok) {
         checkUnknownSets();

libcockatrice_card/libcockatrice/card/database/card_database.h

@@ -177,8 +177,12 @@ public slots:
 
     void addFormat(const FormatRulesPtr &format);
 
-    /** @brief Loads card databases from configured paths. */
+    /** @brief Loads card databases from configured paths, using the binary cache when valid. */
     void loadCardDatabases();
+    /**
+     * @brief Reloads card databases from disk, bypassing the binary cache,
+     *        and notifies observers of the fresh state.
+     */
     void reloadCardDatabasesAndNotify();
 
     /** @brief Saves custom tokens to file.

libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp

@@ -6,6 +6,7 @@
 #include "libcockatrice/card/database/card_database_data.h"
 #include "libcockatrice/card/set/card_set.h"
 #include "libcockatrice/interfaces/interface_card_database_path_provider.h"
+#include "libcockatrice/interfaces/interface_card_set_priority_controller.h"
 #include "parser/card_database_parser.h"
 #include "parser/cockatrice_xml_3.h"
 #include "parser/cockatrice_xml_4.h"
@@ -28,7 +29,6 @@
 #include <utility>
 
 class ICardPreferenceProvider;
-class ICardSetPriorityController;
 
 CardDatabaseLoader::CardDatabaseLoader(QObject *parent,
                                        CardDatabase *db,
@@ -44,8 +44,7 @@ CardDatabaseLoader::CardDatabaseLoader(QObject *parent,
     // the finished snapshot is swapped into the live database on the GUI thread.
 
     // when SettingsCache's path changes, trigger reloads
-    connect(pathProvider, &ICardDatabasePathProvider::cardDatabasePathChanged, this,
-            &CardDatabaseLoader::loadCardDatabases);
+    connect(pathProvider, &ICardDatabasePathProvider::cardDatabasePathChanged, this, [this] { loadCardDatabases(); });
 }
 
 CardDatabaseLoader::~CardDatabaseLoader()
@@ -90,12 +89,12 @@ LoadStatus CardDatabaseLoader::loadCardDatabase(const QString &path, CardDatabas
     return tempLoadStatus;
 }
 
-LoadStatus CardDatabaseLoader::loadCardDatabases()
+LoadStatus CardDatabaseLoader::loadCardDatabases(bool ignoreCache)
 {
-    return doLoadCardDatabases();
+    return doLoadCardDatabases(ignoreCache);
 }
 
-LoadStatus CardDatabaseLoader::doLoadCardDatabases()
+LoadStatus CardDatabaseLoader::doLoadCardDatabases(bool ignoreCache)
 {
     QMutexLocker locker(reloadDatabaseMutex);
 
@@ -113,9 +112,14 @@ LoadStatus CardDatabaseLoader::doLoadCardDatabases()
     LoadStatus loadStatus = NotLoaded;
 
     // Try the binary cache first: a cache hit avoids re-parsing the (large) XML.
+    // An explicit reload skips the lookup so the files on disk are always
+    // re-read, even when their size/mtime hash still matches the cache.
     const QStringList customPaths = collectCustomDatabasePaths();
     const QByteArray sourceHash = computeSourceHash(customPaths);
-    if (loadFromCache(data, sourceHash)) {
+    if (ignoreCache) {
+        qCInfo(CardDatabaseLoadingLog) << "Ignoring binary cache, re-parsing source files";
+    }
+    if (!ignoreCache && loadFromCache(data, sourceHash)) {
         qCInfo(CardDatabaseLoadingLog) << "Loaded card database from binary cache";
         loadStatus = Ok;
     } else {
@@ -180,6 +184,12 @@ QByteArray CardDatabaseLoader::computeSourceHash(const QStringList &customPaths)
     hash.addData(QCoreApplication::applicationVersion().toUtf8());
     hash.addData(QByteArray(1, '\0'));
 
+    // Include parser schema version to invalidate caches when parser behavior changes.
+    // Increment this if the parser logic changes in a way that affects cached data.
+    static const char SCHEMA_VERSION[] = "carddb-parse-schema-v1";
+    hash.addData(QByteArray(SCHEMA_VERSION, sizeof(SCHEMA_VERSION) - 1));
+    hash.addData(QByteArray(1, '\0'));
+
     const QStringList inputs = QStringList()
                                << pathProvider->getCardDatabasePath() << pathProvider->getTokenDatabasePath()
                                << pathProvider->getSpoilerCardDatabasePath() << customPaths;
@@ -194,6 +204,17 @@ QByteArray CardDatabaseLoader::computeSourceHash(const QStringList &customPaths)
             hash.addData(QByteArray(1, '\0'));
         }
     }
+
+    // The parsers drop printings of disabled sets at parse time, so the cached
+    // snapshot depends on enablement: fold the enabled set names in so that
+    // toggling a set invalidates the cache and forces a re-parse.
+    if (priorityController) {
+        const QStringList enabledSets = priorityController->getEnabledSetNames();
+        for (const QString &setName : enabledSets) {
+            hash.addData(setName.toUtf8());
+            hash.addData(QByteArray(1, '\0'));
+        }
+    }
     return hash.result();
 }
 

libcockatrice_card/libcockatrice/card/database/card_database_loader.h

@@ -73,9 +73,13 @@ public slots:
      *
      * Runs synchronously on the calling thread.  The caller should ensure
      * that any signal receivers are already connected before invoking this.
+     * @param ignoreCache Skip the binary cache lookup and re-parse the source
+     *        files from disk, refreshing the cache afterwards. Used for
+     *        explicit reloads so that edits the source hash cannot see are
+     *        still picked up.
      * @return Status of the main database load.
      */
-    LoadStatus loadCardDatabases();
+    LoadStatus loadCardDatabases(bool ignoreCache = false);
 
     /**
      * @brief Loads a single card database file into the given snapshot.
@@ -128,9 +132,10 @@ private:
 
     /**
      * @brief Performs the actual load work synchronously on the calling thread.
+     * @param ignoreCache Skip the binary cache lookup and re-parse from disk.
      * @return Status of the main database load.
      */
-    LoadStatus doLoadCardDatabases();
+    LoadStatus doLoadCardDatabases(bool ignoreCache);
 
     /**
      * @brief Collects custom card database paths recursively.

libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h

@@ -2,6 +2,7 @@
 #define COCKATRICE_INTERFACE_CARD_SET_PRIORITY_CONTROLLER_H
 
 #include <QString>
+#include <QStringList>
 
 class ICardSetPriorityController
 {
@@ -46,6 +47,16 @@ public:
      */
     virtual SetOptions getSetOptions(QString shortName) const = 0;
 
+    /**
+     * @brief Returns the short names of every set currently marked as enabled.
+     *
+     * Callers fold this into the card database cache source hash so that
+     * toggling set enablement invalidates the cache.
+     *
+     * @return The enabled set short names, sorted alphabetically.
+     */
+    virtual QStringList getEnabledSetNames() const = 0;
+
     virtual void saveSets(const QVector<SetSaveData> &data) = 0;
 };
 

libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h

@@ -34,6 +34,11 @@ public:
         return {0, true, true};
     }
 
+    QStringList getEnabledSetNames() const override
+    {
+        return {};
+    }
+
     void saveSets(const QVector<SetSaveData> & /* data */) override
     {
     }

libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp

@@ -88,6 +88,20 @@ ICardSetPriorityController::SetOptions CardDatabaseSettings::getSetOptions(QStri
     return setOptionsCache.value(shortName);
 }
 
+QStringList CardDatabaseSettings::getEnabledSetNames() const
+{
+    QMutexLocker lock(&setOptionsMutex);
+    ensureSetOptionsLoaded();
+    QStringList names;
+    for (auto it = setOptionsCache.constBegin(); it != setOptionsCache.constEnd(); ++it) {
+        if (it.value().enabled) {
+            names << it.key();
+        }
+    }
+    names.sort();
+    return names;
+}
+
 void CardDatabaseSettings::saveSets(const QVector<ICardSetPriorityController::SetSaveData> &data)
 {
     batchWrite([&](QSettings &s) {

libcockatrice_settings/libcockatrice/settings/card_database_settings.h

@@ -35,6 +35,8 @@ public:
 
     SetOptions getSetOptions(QString shortName) const override;
 
+    QStringList getEnabledSetNames() const override;
+
     void saveSets(const QVector<SetSaveData> &data) override;
 
 private:

libcockatrice_settings/libcockatrice/settings/settings_migration.cpp

@@ -570,9 +570,15 @@ static void migrateLegacySets(const QString &settingsPath)
     QSettings cardDbIni(settingsPath + "cardDatabase.ini", QSettings::IniFormat);
     for (const auto &shortName : groups) {
         legacySetting.beginGroup(shortName);
-        cardDbIni.setValue("sets/" + shortName + "/sortKey", legacySetting.value("sortkey"));
-        cardDbIni.setValue("sets/" + shortName + "/enabled", legacySetting.value("enabled"));
-        cardDbIni.setValue("sets/" + shortName + "/isKnown", legacySetting.value("isknown"));
+        if (legacySetting.contains("sortkey")) {
+            cardDbIni.setValue("sets/" + shortName + "/sortKey", legacySetting.value("sortkey"));
+        }
+        if (legacySetting.contains("enabled")) {
+            cardDbIni.setValue("sets/" + shortName + "/enabled", legacySetting.value("enabled"));
+        }
+        if (legacySetting.contains("isknown")) {
+            cardDbIni.setValue("sets/" + shortName + "/isKnown", legacySetting.value("isknown"));
+        }
         legacySetting.endGroup();
     }
     legacySetting.endGroup();

tests/carddatabase/carddatabase_test.cpp

@@ -7,18 +7,26 @@
 
 #include "gtest/gtest.h"
 #include <QDate>
+#include <QDateTime>
+#include <QDir>
 #include <QFile>
+#include <QFileDevice>
+#include <QFileInfo>
 #include <QHash>
 #include <QIODevice>
 #include <QList>
 #include <QSharedPointer>
 #include <QString>
+#include <QStringList>
 #include <QTemporaryDir>
+#include <QVector>
 #include <libcockatrice/card/card_info.h>
 #include <libcockatrice/card/database/card_database_data.h>
 #include <libcockatrice/card/database/parser/cockatrice_xml_4.h>
 #include <libcockatrice/card/lazy_properties_hash.h>
 #include <libcockatrice/card/printing/printing_info.h>
+#include <libcockatrice/interfaces/interface_card_database_path_provider.h>
+#include <libcockatrice/interfaces/interface_card_set_priority_controller.h>
 #include <libcockatrice/interfaces/noop_card_preference_provider.h>
 #include <libcockatrice/interfaces/noop_card_set_priority_controller.h>
 #include <string>
@@ -93,6 +101,280 @@ TEST(CardDatabaseTest, Xml4LocalizedDataRoundTrip)
     ASSERT_EQ(loaded->getLocalizedText("de"), "Blitzschlag fügt 3 Schadenspunkte zu.");
     ASSERT_EQ(loaded->getLocalizedText("fr"), "Deal 3 damage.");
 }
+
+/**
+ * @brief Path provider serving a sandboxed copy of the test card database.
+ */
+class SandboxPathProvider : public ICardDatabasePathProvider
+{
+public:
+    explicit SandboxPathProvider(const QString &_root) : root(_root)
+    {
+    }
+
+    QString getCardDatabasePath() const override
+    {
+        return root + "/cards.xml";
+    }
+
+    QString getCustomCardDatabasePath() const override
+    {
+        return root + "/customsets/";
+    }
+
+    QString getTokenDatabasePath() const override
+    {
+        return root + "/tokens.xml";
+    }
+
+    QString getSpoilerCardDatabasePath() const override
+    {
+        return root + "/spoiler.xml";
+    }
+
+private:
+    QString root;
+};
+
+/**
+ * @brief Set priority controller whose enablement state the test drives directly.
+ *
+ * Seeded short names behave like entries present in cardDatabase.ini (enabled,
+ * known). Anything the parser discovers but the test did not seed falls back to
+ * the default SetOptions, i.e. disabled and unknown, mirroring
+ * CardDatabaseSettings for sets without an ini group.
+ */
+class SeededSetPriorityController : public ICardSetPriorityController
+{
+public:
+    void seed(const QStringList &shortNames)
+    {
+        for (const QString &shortName : shortNames) {
+            SetOptions options;
+            options.enabled = true;
+            options.isKnown = true;
+            setOptions.insert(shortName, options);
+        }
+    }
+
+    void setSortKey(QString shortName, unsigned int sortKey) override
+    {
+        setOptions[shortName].sortKey = sortKey;
+    }
+
+    void setEnabled(QString shortName, bool enabled) override
+    {
+        setOptions[shortName].enabled = enabled;
+    }
+
+    void setIsKnown(QString shortName, bool isknown) override
+    {
+        setOptions[shortName].isKnown = isknown;
+    }
+
+    unsigned int getSortKey(QString shortName) const override
+    {
+        return setOptions.value(shortName).sortKey;
+    }
+
+    bool isEnabled(QString shortName) const override
+    {
+        return setOptions.value(shortName).enabled;
+    }
+
+    bool isKnown(QString shortName) const override
+    {
+        return setOptions.value(shortName).isKnown;
+    }
+
+    SetOptions getSetOptions(QString shortName) const override
+    {
+        return setOptions.value(shortName);
+    }
+
+    QStringList getEnabledSetNames() const override
+    {
+        QStringList names;
+        for (auto it = setOptions.constBegin(); it != setOptions.constEnd(); ++it) {
+            if (it.value().enabled) {
+                names << it.key();
+            }
+        }
+        names.sort();
+        return names;
+    }
+
+    void saveSets(const QVector<SetSaveData> &data) override
+    {
+        for (const SetSaveData &entry : data) {
+            setOptions[entry.shortName].sortKey = entry.sortKey;
+            setOptions[entry.shortName].enabled = entry.enabled;
+        }
+    }
+
+private:
+    QHash<QString, SetOptions> setOptions;
+};
+
+/**
+ * @brief Sandboxed card database plus enablement controller for cache tests.
+ *
+ * Copies the checked-in fixture XML into a temporary directory so each test
+ * owns the files (and the binary cache written next to them).
+ */
+class CardDatabaseCacheInvalidationTest : public ::testing::Test
+{
+protected:
+    void SetUp() override
+    {
+        ASSERT_TRUE(tempDir.isValid());
+        root = tempDir.path();
+        ASSERT_TRUE(QFile::copy(QString(CARDDB_DATADIR) + "cards.xml", root + "/cards.xml"));
+        ASSERT_TRUE(QFile::copy(QString(CARDDB_DATADIR) + "tokens.xml", root + "/tokens.xml"));
+        ASSERT_TRUE(QDir(root).mkpath("customsets"));
+        ASSERT_TRUE(
+            QFile::copy(QString(CARDDB_DATADIR) + "customsets/customset1.xml", root + "/customsets/customset1.xml"));
+        controller.seed({"CAT", "DOG", "WHO", " Not a Card", "BRD"});
+    }
+
+    /**
+     * @brief Contents of the binary cache written next to cards.xml.
+     * @return The cache file bytes, or an empty array if no cache exists.
+     */
+    QByteArray cacheContents() const
+    {
+        QFile file(root + "/cards.xml.cache");
+        if (!file.open(QIODevice::ReadOnly)) {
+            return {};
+        }
+        return file.readAll();
+    }
+
+    QTemporaryDir tempDir;
+    QString root;
+    SeededSetPriorityController controller;
+};
+
+TEST_F(CardDatabaseCacheInvalidationTest, DisablingSetBustsCacheAndDropsPrintings)
+{
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    const QByteArray initialCache = cacheContents();
+    ASSERT_FALSE(initialCache.isEmpty()) << "first load must write the binary cache";
+    ASSERT_FALSE(db.getCardList().value("Cat").isNull());
+    ASSERT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT"));
+
+    // A cache-aware load with unchanged inputs keeps the cache: the parse
+    // result would be identical, so there is nothing to invalidate.
+    db.loadCardDatabases();
+    EXPECT_EQ(initialCache, cacheContents()) << "load with unchanged inputs must keep the cache";
+
+    // Disabling a set changes the source hash, so the next load re-parses and
+    // the set's printings disappear everywhere.
+    controller.setEnabled("CAT", false);
+    db.reloadCardDatabasesAndNotify();
+    EXPECT_NE(initialCache, cacheContents()) << "toggling set enablement must bust the cache";
+    ASSERT_FALSE(db.getCardList().value("Cat").isNull());
+    EXPECT_FALSE(db.getCardList().value("Cat")->getSets().contains("CAT")) << "disabled set printings must be dropped";
+    ASSERT_FALSE(db.getCardList().value("Kitten").isNull());
+    EXPECT_FALSE(db.getCardList().value("Kitten")->getSets().contains("CAT"));
+}
+
+TEST_F(CardDatabaseCacheInvalidationTest, AddingAndEnablingCustomSetBustsCache)
+{
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    const QByteArray initialCache = cacheContents();
+    ASSERT_FALSE(initialCache.isEmpty()) << "first load must write the binary cache";
+
+    // Drop a new custom set in: the input file list changes, so the next load
+    // must re-parse. BRD2 is unknown, so its set starts disabled.
+    QFile customSetFile(root + "/customsets/customset2.xml");
+    ASSERT_TRUE(customSetFile.open(QIODevice::WriteOnly | QIODevice::Truncate));
+    customSetFile.write("<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
+                        "<cockatrice_carddatabase version=\"4\">\n"
+                        "    <cards>\n"
+                        "        <card>\n"
+                        "            <name>Finch</name>\n"
+                        "            <set>BRD2</set>\n"
+                        "            <tablerow>0</tablerow>\n"
+                        "            <text>Chirp!</text>\n"
+                        "        </card>\n"
+                        "    </cards>\n"
+                        "</cockatrice_carddatabase>\n");
+    customSetFile.close();
+
+    db.reloadCardDatabasesAndNotify();
+    const QByteArray afterCustomSetAdded = cacheContents();
+    EXPECT_NE(initialCache, afterCustomSetAdded) << "adding a custom card database must bust the cache";
+    ASSERT_FALSE(db.getCardList().value("Finch").isNull()) << "the custom card must be loaded";
+    EXPECT_TRUE(db.getCardList().value("Finch")->getSets().isEmpty())
+        << "BRD2 starts disabled, so Finch has no printings yet";
+
+    // Enabling the freshly discovered set busts the cache again, so the next
+    // load finally parses its printings (#7455).
+    controller.setEnabled("BRD2", true);
+    db.reloadCardDatabasesAndNotify();
+    EXPECT_NE(afterCustomSetAdded, cacheContents()) << "enabling a set must bust the cache";
+    ASSERT_FALSE(db.getCardList().value("Finch").isNull());
+    EXPECT_TRUE(db.getCardList().value("Finch")->getSets().contains("BRD2"));
+}
+
+TEST_F(CardDatabaseCacheInvalidationTest, ExplicitReloadBypassesCache)
+{
+    NoopCardPreferenceProvider prefs;
+    SandboxPathProvider pathProvider(root);
+    CardDatabase db(nullptr, &prefs, &pathProvider, &controller);
+
+    db.loadCardDatabases();
+    ASSERT_FALSE(db.getCardList().value("Cat").isNull());
+    ASSERT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT"));
+
+    // Move Cat to the DOG set in place, keeping the file size identical and
+    // restoring the modification time afterwards: the source hash (path, size,
+    // mtime) still matches, so the cache stays valid by construction and only
+    // an explicit reload can pick the edit up.
+    const QString cardsPath = root + "/cards.xml";
+    QFile file(cardsPath);
+    ASSERT_TRUE(file.open(QIODevice::ReadOnly));
+    const qint64 stampedSecs = QFileInfo(cardsPath).lastModified().toSecsSinceEpoch();
+    QByteArray contents = file.readAll();
+    file.close();
+    const QByteArray oldSetCode("<set>CAT</set>");
+    const QByteArray newSetCode("<set>DOG</set>");
+    ASSERT_EQ(contents.count(oldSetCode), 1) << "fixture must contain exactly one CAT printing";
+    contents.replace(contents.indexOf(oldSetCode), oldSetCode.size(), newSetCode);
+    ASSERT_TRUE(file.open(QIODevice::ReadWrite));
+    ASSERT_EQ(file.write(contents), contents.size());
+    // Flush the pending write before stamping the time: close() would otherwise
+    // flush it afterwards and bump the modification time again, invalidating the
+    // cache on platforms that buffer writes (e.g. macOS).
+    ASSERT_TRUE(file.flush());
+    ASSERT_TRUE(file.setFileTime(QDateTime::fromSecsSinceEpoch(stampedSecs), QFileDevice::FileModificationTime));
+    file.close();
+    ASSERT_EQ(QFileInfo(cardsPath).lastModified().toSecsSinceEpoch(), stampedSecs)
+        << "the sandbox file's modification time must be restored for the cache to stay valid";
+
+    // The cache is still valid for a regular load, which serves the stale data.
+    db.loadCardDatabases();
+    ASSERT_FALSE(db.getCardList().value("Cat").isNull());
+    EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT")) << "regular load must hit the cache";
+    EXPECT_FALSE(db.getCardList().value("Cat")->getSets().contains("DOG"));
+
+    // The explicit reload ignores the cache and re-reads the file.
+    const QByteArray staleCache = cacheContents();
+    db.reloadCardDatabasesAndNotify();
+    EXPECT_NE(staleCache, cacheContents()) << "explicit reload must rewrite the cache from disk";
+    ASSERT_FALSE(db.getCardList().value("Cat").isNull());
+    EXPECT_FALSE(db.getCardList().value("Cat")->getSets().contains("CAT"));
+    EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("DOG"));
+}
 } // namespace
 
 int main(int argc, char **argv)