Preserve installed card databases when Oracle refresh fails (#7739)

* Preserve installed card databases when Oracle refresh fails

* 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.

---------

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

oracle/src/database_writer.cpp

@@ -0,0 +1,52 @@
+#include "database_writer.h"
+
+#include <QCoreApplication>
+#include <QIODevice>
+#include <QSaveFile>
+#include <QXmlStreamReader>
+
+namespace OracleDatabase
+{
+QString validateXml(const QByteArray &data)
+{
+    QXmlStreamReader xml(data);
+    if (!xml.readNextStartElement() || xml.name() != QLatin1String("cockatrice_carddatabase")) {
+        return QCoreApplication::translate("OracleDatabase", "The download is not a Cockatrice card database.");
+    }
+    const auto version = xml.attributes().value("version");
+    if (version != QLatin1String("3") && version != QLatin1String("4")) {
+        return QCoreApplication::translate("OracleDatabase", "The downloaded card database version is not supported.");
+    }
+    while (!xml.atEnd()) {
+        xml.readNext();
+    }
+    if (xml.hasError()) {
+        return QCoreApplication::translate("OracleDatabase",
+                                           "The downloaded card database is incomplete or invalid: %1")
+            .arg(xml.errorString());
+    }
+    return {};
+}
+
+bool saveXml(const QString &fileName, const QByteArray &data, QString *error)
+{
+    const QString validationError = validateXml(data);
+    if (!validationError.isEmpty()) {
+        if (error) {
+            *error = validationError;
+        }
+        return false;
+    }
+    QSaveFile file(fileName);
+    if (!file.open(QIODevice::WriteOnly) || file.write(data) != data.size() || !file.commit()) {
+        if (error) {
+            *error = file.errorString();
+        }
+        return false;
+    }
+    if (error) {
+        error->clear();
+    }
+    return true;
+}
+} // namespace OracleDatabase

oracle/src/database_writer.h

@@ -0,0 +1,17 @@
+#ifndef ORACLE_DATABASE_WRITER_H
+#define ORACLE_DATABASE_WRITER_H
+
+#include <QByteArray>
+#include <QString>
+
+namespace OracleDatabase
+{
+// Validate the complete downloaded document without allocating a card database.
+// Empty string means valid; otherwise returns a diagnostic suitable for the wizard.
+QString validateXml(const QByteArray &data);
+// Never truncate the installed database: validate, write a sibling temporary file,
+// check the complete write and atomically commit it.
+bool saveXml(const QString &fileName, const QByteArray &data, QString *error = nullptr);
+} // namespace OracleDatabase
+
+#endif

oracle/src/oracleimporter.cpp

@@ -100,6 +100,10 @@ static CardSet::Priority getSetPriority(const QString &setType, const QString &s
 
 bool OracleImporter::readSetsFromByteArray(QByteArray data)
 {
+    importSuccessful = false;
+    importError.clear();
+    allSets.clear();
+    rawSetsData.clear();
     const RawJson::ScanProgressCallback progress =
         progressReporting
             ? [this](
@@ -764,7 +768,15 @@ int OracleImporter::startImport()
 {
     static ICardSetPriorityController *noOpController = new NoopCardSetPriorityController();
 
-    importCancelled.storeRelease(0);
+    importSuccessful = false;
+    importError.clear();
+    sets.clear();
+    cards.clear();
+    localizedEntries.clear();
+    splitLocalizedTexts.clear();
+    if (allSets.isEmpty()) {
+        return failImport(tr("No set has been selected for import."));
+    }
 
     // Pre-allocate the cards hash to avoid rehashing during import. Keys are
     // distinct card names while raw ranges only count printings (AllPrintings
@@ -786,9 +798,7 @@ int OracleImporter::startImport()
 
     for (const SetToDownload &curSetToParse : allSets) {
         if (importCancelled.loadAcquire()) {
-            // The wizard was closed mid-import: stop at the next set boundary so
-            // the caller can wait for this future without processing every set.
-            break;
+            return failImport(tr("The card database import was cancelled."));
         }
 
         CardSetPtr newSet = CardSet::newInstance(noOpController, curSetToParse.getShortName(),
@@ -798,30 +808,20 @@ int OracleImporter::startImport()
         // parse only this set's slice of the raw document so the whole JSON tree is
         // never kept in memory at once
         const RawJson::SetDataRange &rawRange = curSetToParse.getRawRange();
-        const qsizetype rangeEnd = rawRange.start + rawRange.length;
-        if (rawRange.start < 0 || rawRange.length <= 0 || rangeEnd > rawSetsData.size()) {
-            // rawSetsData is cleared by releaseSetData() while SetToDownload copies
-            // taken from getSets() keep their ranges, and nothing else enforces the
-            // pairing — so never index past the buffer on stale/mismatched ranges.
-            qWarning() << "error: out-of-bounds raw range for set" << curSetToParse.getShortName() << "skipping";
-            ++setIndex;
-            emit setIndexChanged(0, setIndex, curSetToParse.getLongName());
-            continue;
+        if (rawRange.start < 0 || rawRange.length <= 0 || rawRange.start > rawSetsData.size() ||
+            rawRange.length > rawSetsData.size() - rawRange.start) {
+            return failImport(tr("The source data for set %1 is unavailable. Load the sets file again.")
+                                  .arg(curSetToParse.getShortName()));
         }
-        // sliced() shares the buffer instead of deep-copying the slice; the largest
-        // sets in AllPrintings are tens of MB, so the copy is worth avoiding here.
         const QByteArray setBytes = rawSetsData.sliced(rawRange.start, rawRange.length);
         QJsonParseError parseError;
         const QJsonDocument setDoc = QJsonDocument::fromJson(setBytes, &parseError);
         if (parseError.error != QJsonParseError::NoError) {
-            qWarning() << "error: parsing card data for set" << curSetToParse.getShortName() << ":"
-                       << parseError.errorString();
-            ++setIndex;
-            // Keep the progress accounting honest: a set that failed to parse
-            // still advanced the index, so report it (with zero imported cards)
-            // rather than letting SaveSetsPage's bar stall per failed set.
-            emit setIndexChanged(0, setIndex, curSetToParse.getLongName());
-            continue;
+            return failImport(
+                tr("Could not import set %1: %2").arg(curSetToParse.getShortName(), parseError.errorString()));
+        }
+        if (!setDoc.isObject() || !setDoc.object().value("cards").isArray()) {
+            return failImport(tr("Set %1 does not contain a card list.").arg(curSetToParse.getShortName()));
         }
 
         // Only add the set to the database once its slice parsed cleanly;
@@ -838,7 +838,14 @@ int OracleImporter::startImport()
         emit setIndexChanged(numCardsInSet, setIndex, curSetToParse.getLongName());
     }
 
+    if (importCancelled.loadAcquire()) {
+        return failImport(tr("The card database import was cancelled."));
+    }
+    if (cards.isEmpty()) {
+        return failImport(tr("No cards were imported. The existing card database has been kept."));
+    }
     applyLocalizedData();
+    importSuccessful = true;
 
     emit setIndexChanged(0, setIndex, QString());
 
@@ -846,8 +853,23 @@ int OracleImporter::startImport()
     return setIndex;
 }
 
+int OracleImporter::failImport(const QString &message)
+{
+    importSuccessful = false;
+    importError = message;
+    sets.clear();
+    cards.clear();
+    localizedEntries.clear();
+    splitLocalizedTexts.clear();
+    qWarning() << message;
+    return 0;
+}
+
 bool OracleImporter::saveToFile(const QString &fileName, const QString &sourceUrl, const QString &sourceVersion)
 {
+    if (!importSuccessful) {
+        return false;
+    }
     CockatriceXml4Parser parser(new NoopCardSetPriorityController());
 
     return parser.saveToFile(createDefaultMagicFormats(), sets, cards, fileName, sourceUrl, sourceVersion);
@@ -861,6 +883,7 @@ void OracleImporter::releaseSetData()
 
 void OracleImporter::clear()
 {
+    prepareImport();
     sets.clear();
     cards.clear();
     allSets.clear();

oracle/src/oracleimporter.h

@@ -198,6 +198,11 @@ private:
      * the whole import (or racing it).
      */
     QAtomicInt importCancelled;
+    bool importSuccessful = false;
+    QString importError;
+
+    // Discard the incomplete in-memory database; the installed file is untouched.
+    int failImport(const QString &message);
 
     /**
      * The ISO-639 language code whose foreignData is imported; "en" by default.
@@ -278,7 +283,24 @@ public:
      * the wizard can hand over its decompressed buffer without copying it.
      */
     bool readSetsFromByteArray(QByteArray data);
+    // Returns the complete selected-set count on success, zero on failure or cancellation.
     int startImport();
+    bool hasSuccessfulImport() const
+    {
+        return importSuccessful;
+    }
+    const QString &getImportError() const
+    {
+        return importError;
+    }
+    // Call on the owning thread before scheduling a worker, so a cancellation
+    // arriving before that worker starts is not lost.
+    void prepareImport()
+    {
+        importCancelled.storeRelease(0);
+        importSuccessful = false;
+        importError.clear();
+    }
     /**
      * @brief Requests an in-flight startImport() to stop at the next set boundary.
      *

oracle/src/oraclewizard.cpp

@@ -1,6 +1,7 @@
 #include "oraclewizard.h"
 
 #include "client/settings/cache_settings.h"
+#include "database_writer.h"
 #include "main.h"
 #include "oracleimporter.h"
 #include "pages.h"
@@ -141,6 +142,13 @@ 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 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(hasSavedDatabase() ? 0 : 1);
+    }
 }
 
 void OracleWizard::runInBackground()
@@ -212,17 +220,10 @@ void OracleWizard::disableButtons()
 
 bool OracleWizard::saveTokensToFile(const QString &fileName)
 {
-    QFile file(fileName);
-    if (!file.open(QIODevice::WriteOnly)) {
-        qDebug() << "File open (w) failed for" << fileName;
+    QString error;
+    if (!OracleDatabase::saveXml(fileName, tokensData, &error)) {
+        qWarning() << error;
         return false;
     }
-
-    if (file.write(tokensData) == -1) {
-        qDebug() << "File write (w) failed for" << fileName;
-        return false;
-    }
-
-    file.close();
     return true;
 }

oracle/src/pages.cpp

@@ -1,6 +1,7 @@
 #include "pages.h"
 
 #include "client/settings/cache_settings.h"
+#include "database_writer.h"
 #include "main.h"
 #include "oracleimporter.h"
 #include "oraclewizard.h"
@@ -43,6 +44,7 @@
 #include <QWizard>
 #include <QtConcurrentRun>
 #include <QtGlobal>
+#include <algorithm>
 #include <cstdio>
 #include <functional>
 #include <libcockatrice/settings/personal_settings.h>
@@ -169,7 +171,16 @@ LoadSetsResult loadSetsData(const QPointer<OracleImporter> &importer, QByteArray
         break;
     }
 
-    if (data.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;
+        }
         result.ok = true;
         result.plainXml = true;
         result.xmlData = std::move(data);
@@ -651,6 +662,10 @@ void LoadSetsPage::importFinished()
         } else if (!result.ok && !result.plainXml) {
             qWarning() << tr("The file was retrieved successfully, but it does not contain any sets data.");
         }
+        if (!result.ok) {
+            wizard()->reject();
+            return;
+        }
         emit readyToContinue();
         return;
     }
@@ -717,7 +732,7 @@ SaveSetsPage::SaveSetsPage(QWidget *parent) : OracleWizardPage(parent)
 
 bool SaveSetsPage::isComplete() const
 {
-    return !importActive;
+    return !importActive && importReady;
 }
 
 void SaveSetsPage::cleanupPage()
@@ -730,8 +745,11 @@ void SaveSetsPage::cleanupPage()
 
 void SaveSetsPage::initializePage()
 {
+    importReady = false;
     retranslateUi();
     if (wizard()->downloadedPlainXml) {
+        importReady = true;
+        emit completeChanged();
         messageLog->hide();
         progressBar->hide();
         if (wizard()->backgroundMode) {
@@ -757,6 +775,9 @@ void SaveSetsPage::initializePage()
     emit completeChanged();
 
     const QPointer<OracleImporter> importer = wizard()->importer;
+    if (auto *activeImporter = importer.data()) {
+        activeImporter->prepareImport();
+    }
     importFuture = QtConcurrent::run([importer] { return importer ? importer->startImport() : 0; });
     importWatcher.setFuture(importFuture);
 }
@@ -788,22 +809,28 @@ void SaveSetsPage::importFinished()
 
     const int setsImported = importWatcher.result();
     const QPointer<OracleImporter> importer = wizard()->importer;
+    importReady = importer && importer->hasSuccessfulImport() && setsImported == totalSets;
+    const QString error = importer ? importer->getImportError() : tr("The card database importer is unavailable.");
     if (importer) {
         importer->releaseSetData();
     }
+    wizard()->button(QWizard::NextButton)->setEnabled(importReady);
+    emit completeChanged();
 
-    if (wizard()->backgroundMode) {
-        if (setsImported == 0) {
-            qWarning() << tr("No set has been imported.");
+    if (!importReady) {
+        const QString message = error.isEmpty() ? tr("No set has been imported.") : error;
+        if (wizard()->backgroundMode) {
+            qWarning() << message;
+            wizard()->reject();
+        } else {
+            QMessageBox::critical(this, tr("Error"), message);
         }
-        emit readyToContinue();
         return;
     }
 
     progressBar->setValue(progressBar->maximum());
-
-    if (setsImported == 0) {
-        QMessageBox::critical(this, tr("Error"), tr("No set has been imported."));
+    if (wizard()->backgroundMode) {
+        emit readyToContinue();
     }
 }
 
@@ -854,6 +881,18 @@ void SaveSetsPage::updateTotalProgress(int cardsImported, int setIndex, const QS
 
 bool SaveSetsPage::validatePage()
 {
+    if (!isComplete()) {
+        return false;
+    }
+    const auto failSave = [this](const QString &message) {
+        qWarning() << message;
+        if (wizard()->backgroundMode) {
+            wizard()->reject();
+        } else {
+            QMessageBox::critical(this, tr("Error"), message);
+        }
+        return false;
+    };
     QString defaultPath = SettingsCache::instance().getCardDatabasePath();
     QString windowName = tr("Save card database");
     QString fileType = tr("XML; card database (*.xml)");
@@ -872,24 +911,21 @@ bool SaveSetsPage::validatePage()
     QFileInfo fi(fileName);
     QDir fileDir(fi.path());
     if (!fileDir.exists() && !fileDir.mkpath(fileDir.absolutePath())) {
-        return false;
+        return failSave(tr("The database directory could not be created: %1").arg(fileDir.absolutePath()));
     }
 
     if (wizard()->downloadedPlainXml) {
-        QFile file(fileName);
-        if (!file.open(QIODevice::WriteOnly)) {
-            qDebug() << "File write (w) failed for" << fileName;
-            return false;
-        }
-        if (file.write(wizard()->xmlData) < 1) {
-            qDebug() << "File write (w) failed for" << fileName;
-            return false;
+        QString error;
+        if (!OracleDatabase::saveXml(fileName, wizard()->xmlData, &error)) {
+            return failSave(tr("The file could not be saved to %1: %2").arg(fileName, error));
         }
         wizard()->xmlData.clear();
     } else if (!wizard()->importer->saveToFile(fileName, wizard()->getCardSourceUrl(),
                                                wizard()->getCardSourceVersion())) {
-        QMessageBox::critical(this, tr("Error"), tr("The file could not be saved to %1").arg(fileName));
-        return false;
+        // 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();

oracle/src/pages.h

@@ -128,6 +128,7 @@ private:
     QFuture<int> importFuture;
     int totalSets = 0;
     bool importActive = false;
+    bool importReady = false;
 
 protected:
     void initializePage() override;

oracle/src/pagetemplates.cpp

@@ -1,5 +1,6 @@
 #include "pagetemplates.h"
 
+#include "database_writer.h"
 #include "oraclewizard.h"
 
 #include <QCheckBox>
@@ -184,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;
     }
 
@@ -218,6 +227,15 @@ void SimpleDownloadFilePage::actDownloadFinished()
 
 bool SimpleDownloadFilePage::saveToFile()
 {
+    const auto failSave = [this](const QString &message) {
+        qWarning() << message;
+        if (wizard()->backgroundMode) {
+            wizard()->reject();
+        } else {
+            QMessageBox::critical(this, tr("Error"), message);
+        }
+        return false;
+    };
     QString defaultPath = getDefaultSavePath();
     QString windowName = getWindowTitle();
     QString fileType = getFileType();
@@ -236,12 +254,11 @@ bool SimpleDownloadFilePage::saveToFile()
     QFileInfo fi(fileName);
     QDir fileDir(fi.path());
     if (!fileDir.exists() && !fileDir.mkpath(fileDir.absolutePath())) {
-        return false;
+        return failSave(tr("The database directory could not be created: %1").arg(fileDir.absolutePath()));
     }
 
     if (!internalSaveToFile(fileName)) {
-        QMessageBox::critical(this, tr("Error"), tr("The file could not be saved to %1").arg(fileName));
-        return false;
+        return failSave(tr("The file could not be saved to %1").arg(fileName));
     }
 
     // clean saved downloadData
@@ -252,17 +269,10 @@ bool SimpleDownloadFilePage::saveToFile()
 
 bool SimpleDownloadFilePage::internalSaveToFile(const QString &fileName)
 {
-    QFile file(fileName);
-    if (!file.open(QIODevice::WriteOnly)) {
-        qDebug() << "File open (w) failed for" << fileName;
+    QString error;
+    if (!OracleDatabase::saveXml(fileName, downloadData, &error)) {
+        qWarning() << error;
         return false;
     }
-
-    if (file.write(downloadData) == -1) {
-        qDebug() << "File write (w) failed for" << fileName;
-        return false;
-    }
-
-    file.close();
     return true;
 }