diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.cpp b/libcockatrice_card/libcockatrice/card/database/card_database.cpp index fc2c7f18c..469c8d02c 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.cpp +++ b/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(); diff --git a/libcockatrice_card/libcockatrice/card/database/card_database.h b/libcockatrice_card/libcockatrice/card/database/card_database.h index 44d554df7..9f0d67b67 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database.h +++ b/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. diff --git a/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp b/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp index 812760106..194083615 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database_loader.cpp +++ b/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 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(); } diff --git a/libcockatrice_card/libcockatrice/card/database/card_database_loader.h b/libcockatrice_card/libcockatrice/card/database/card_database_loader.h index 5dad152eb..fffd31d2f 100644 --- a/libcockatrice_card/libcockatrice/card/database/card_database_loader.h +++ b/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. diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h index 36e748006..6b3e684d7 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h +++ b/libcockatrice_interfaces/libcockatrice/interfaces/interface_card_set_priority_controller.h @@ -2,6 +2,7 @@ #define COCKATRICE_INTERFACE_CARD_SET_PRIORITY_CONTROLLER_H #include +#include 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 &data) = 0; }; diff --git a/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h b/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h index 8cc2011bd..94e1f0c9a 100644 --- a/libcockatrice_interfaces/libcockatrice/interfaces/noop_card_set_priority_controller.h +++ b/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 & /* data */) override { } diff --git a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp b/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp index 7d41e5165..acac1125f 100644 --- a/libcockatrice_settings/libcockatrice/settings/card_database_settings.cpp +++ b/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 &data) { batchWrite([&](QSettings &s) { diff --git a/libcockatrice_settings/libcockatrice/settings/card_database_settings.h b/libcockatrice_settings/libcockatrice/settings/card_database_settings.h index 07a5788b9..5bdf17aef 100644 --- a/libcockatrice_settings/libcockatrice/settings/card_database_settings.h +++ b/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 &data) override; private: diff --git a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp b/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp index c343ef801..c3f299f40 100644 --- a/libcockatrice_settings/libcockatrice/settings/settings_migration.cpp +++ b/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(); diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index 3f1284d0a..1ac259d9b 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -7,18 +7,26 @@ #include "gtest/gtest.h" #include +#include +#include #include +#include +#include #include #include #include #include #include +#include #include +#include #include #include #include #include #include +#include +#include #include #include #include @@ -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 &data) override + { + for (const SetSaveData &entry : data) { + setOptions[entry.shortName].sortKey = entry.sortKey; + setOptions[entry.shortName].enabled = entry.enabled; + } + } + +private: + QHash 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("\n" + "\n" + " \n" + " \n" + " Finch\n" + " BRD2\n" + " 0\n" + " Chirp!\n" + " \n" + " \n" + "\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("CAT"); + const QByteArray newSetCode("DOG"); + 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)