From 18b2896bf5371b68eedf9bc3efb99e2c8b7fbc50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Wed, 4 Nov 2026 17:27:59 +0100 Subject: [PATCH] Fix the Oracle refresh verdict and cover the parser write path A background refresh that already installed cards.xml exited 1 when the token save afterwards failed, so the client kept its old in-memory database and reported the update as failed. Return the same verdict main() already computes: a reject after the database was written is a successful update. Cover the writer Oracle actually uses: add a short-write death test for CockatriceXml4Parser::saveToFile, which reverted to QFile would truncate an installed database without any test noticing. Route a failed background token download through reject() instead of a modal box on a hidden wizard, which left the client waiting with no timeout and blocked later updates. Test the first non-space byte in place rather than copying the payload, and report an import refusal with the importer's own message. --- oracle/src/oraclewizard.cpp | 6 ++- oracle/src/pages.cpp | 13 ++++- oracle/src/pagetemplates.cpp | 12 ++++- tests/carddatabase/carddatabase_test.cpp | 64 ++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 6 deletions(-) diff --git a/oracle/src/oraclewizard.cpp b/oracle/src/oraclewizard.cpp index 946fd3ed3..dc8c7ecb3 100644 --- a/oracle/src/oraclewizard.cpp +++ b/oracle/src/oraclewizard.cpp @@ -143,9 +143,11 @@ void OracleWizard::reject() QWizard::reject(); // A hidden wizard does not close the application's last visible window. - // Stop background refreshes explicitly so the hosting client receives failure. + // Stop background refreshes explicitly so the hosting client receives a verdict, + // using the same rule as main(): a reject after the database was written is still + // a successful update, only a reject that wrote nothing counts as a failure. if (backgroundMode) { - QCoreApplication::exit(1); + QCoreApplication::exit(hasSavedDatabase() ? 0 : 1); } } diff --git a/oracle/src/pages.cpp b/oracle/src/pages.cpp index a22ef62e0..f5c38be1c 100644 --- a/oracle/src/pages.cpp +++ b/oracle/src/pages.cpp @@ -44,6 +44,7 @@ #include #include #include +#include #include #include #include @@ -170,7 +171,12 @@ LoadSetsResult loadSetsData(const QPointer &importer, QByteArray break; } - if (data.trimmed().startsWith("<")) { + // Test the first non-space byte in place: trimmed() would duplicate the whole + // payload, which on the plain-XML path is the largest download we handle. + const auto firstNonSpace = std::find_if_not(data.cbegin(), data.cend(), + [](char c) { return c == ' ' || c == '\t' || c == '\n' || c == '\r'; }); + const bool looksLikeXml = firstNonSpace != data.cend() && *firstNonSpace == '<'; + if (looksLikeXml) { result.errorMessage = OracleDatabase::validateXml(data); if (!result.errorMessage.isEmpty()) { return result; @@ -916,7 +922,10 @@ bool SaveSetsPage::validatePage() wizard()->xmlData.clear(); } else if (!wizard()->importer->saveToFile(fileName, wizard()->getCardSourceUrl(), wizard()->getCardSourceVersion())) { - return failSave(tr("The file could not be saved to %1").arg(fileName)); + // The importer refuses to write a database whose import did not finish, and + // reports the reason there; do not dress an import failure up as a disk failure. + const QString importError = wizard()->importer->getImportError(); + return failSave(importError.isEmpty() ? tr("The file could not be saved to %1").arg(fileName) : importError); } wizard()->markDatabaseSaved(); diff --git a/oracle/src/pagetemplates.cpp b/oracle/src/pagetemplates.cpp index 3efbf192c..7e5f30bb7 100644 --- a/oracle/src/pagetemplates.cpp +++ b/oracle/src/pagetemplates.cpp @@ -185,9 +185,17 @@ void SimpleDownloadFilePage::actDownloadFinished() } QNetworkReply::NetworkError errorCode = reply->error(); if (errorCode != QNetworkReply::NoError) { - QMessageBox::critical(this, tr("Error"), tr("Network error: %1.").arg(reply->errorString())); - wizard()->enableButtons(); + const QString message = tr("Network error: %1.").arg(reply->errorString()); reply->deleteLater(); + // A hidden wizard cannot show a modal box: without an explicit stop it would sit + // here forever and the hosting client would wait with no timeout. + if (wizard()->backgroundMode) { + qWarning() << message; + wizard()->reject(); + return; + } + QMessageBox::critical(this, tr("Error"), message); + wizard()->enableButtons(); return; } diff --git a/tests/carddatabase/carddatabase_test.cpp b/tests/carddatabase/carddatabase_test.cpp index 22cbe9074..b54205f30 100644 --- a/tests/carddatabase/carddatabase_test.cpp +++ b/tests/carddatabase/carddatabase_test.cpp @@ -36,6 +36,12 @@ #include #include +#ifdef Q_OS_UNIX +#include +#include +#include +#endif + namespace { @@ -204,6 +210,64 @@ TEST(CardDatabaseTest, Xml4LocalizedDataRoundTrip) ASSERT_EQ(printings.first().getProperty("manacost"), "1R"); } +#ifdef Q_OS_UNIX +/** + * @brief A failed write keeps the previously installed card database intact. + * + * This is the writer Oracle uses to install a refreshed database, so the truncation + * bug has to be covered here and not only through OracleDatabase::saveXml. + */ +TEST(CardDatabaseTest, Xml4ShortWriteKeepsInstalledFile) +{ + NoopCardSetPriorityController controller; + CardSetPtr set = + CardSet::newInstance(&controller, "TST", "Test Set", "expansion", QDate(), CardSet::PriorityPrimary); + + QHash props; + props["manacost"] = "1R"; + PrintingInfo printing(set, LazyPropertiesHash(props)); + SetToPrintingsMap setsInfo; + setsInfo["TST"].append(printing); + + CardInfo::UiAttributes attributes = {.tableRow = 1}; + CardInfoPtr card = + CardInfo::newInstance("Lightning Bolt", QString(1024, 'x'), false, {}, {}, {}, setsInfo, attributes); + + SetNameMap sets; + sets.insert("TST", set); + CardNameMap cards; + cards.insert("Lightning Bolt", card); + + QTemporaryDir tempDir; + ASSERT_TRUE(tempDir.isValid()); + const QString fileName = tempDir.filePath("cards.xml"); + + QFile installed(fileName); + ASSERT_TRUE(installed.open(QIODevice::WriteOnly)); + ASSERT_EQ(installed.write("installed database"), 18); + installed.close(); + + // A real filesystem write failure, isolated in the death-test child so the + // process-wide file limit never affects other cases or the parent runner. + ASSERT_EXIT( + { + std::signal(SIGXFSZ, SIG_IGN); + struct rlimit limit; + limit.rlim_cur = 128; + limit.rlim_max = 128; + if (setrlimit(RLIMIT_FSIZE, &limit) != 0) { + _exit(2); + } + CockatriceXml4Parser writer(&controller); + _exit(writer.saveToFile({}, sets, cards, fileName) ? 1 : 0); + }, + ::testing::ExitedWithCode(0), ""); + + ASSERT_TRUE(installed.open(QIODevice::ReadOnly)); + EXPECT_EQ(installed.readAll(), "installed database"); +} +#endif + /** * @brief Path provider serving a sandboxed copy of the test card database. */