Settings migration: Preserve future settings where possible

See discussion in #6506

src/gui/accountmanager.cpp

@@ -55,6 +55,9 @@ AccountManager *AccountManager::instance()
 
 bool AccountManager::restore()
 {
+    QStringList skipSettingsKeys;
+    backwardMigrationSettingsKeys(&skipSettingsKeys, &skipSettingsKeys);
+
     auto settings = ConfigFile::settingsWithGroup(QLatin1String(accountsC));
     if (settings->status() != QSettings::NoError || !settings->isWritable()) {
         qCWarning(lcAccountManager) << "Could not read settings from" << settings->fileName()
@@ -62,6 +65,12 @@ bool AccountManager::restore()
         return false;
     }
 
+    if (skipSettingsKeys.contains(settings->group())) {
+        // Should not happen: bad container keys should have been deleted
+        qCWarning(lcAccountManager) << "Accounts structure is too new, ignoring";
+        return true;
+    }
+
     // If there are no accounts, check the old format.
     if (settings->childGroups().isEmpty()
         && !settings->contains(QLatin1String(versionC))) {
@@ -71,11 +80,16 @@ bool AccountManager::restore()
 
     for (const auto &accountId : settings->childGroups()) {
         settings->beginGroup(accountId);
-        if (auto acc = loadAccountHelper(*settings)) {
-            acc->_id = accountId;
-            if (auto accState = AccountState::loadFromSettings(acc, *settings)) {
-                addAccountState(accState);
+        if (!skipSettingsKeys.contains(settings->group())) {
+            if (auto acc = loadAccountHelper(*settings)) {
+                acc->_id = accountId;
+                if (auto accState = AccountState::loadFromSettings(acc, *settings)) {
+                    addAccountState(accState);
+                }
             }
+        } else {
+            qCInfo(lcAccountManager) << "Account" << accountId << "is too new, ignoring";
+            _additionalBlockedAccountIds.insert(accountId);
         }
         settings->endGroup();
     }
@@ -83,25 +97,22 @@ bool AccountManager::restore()
     return true;
 }
 
-QStringList AccountManager::backwardMigrationKeys()
+void AccountManager::backwardMigrationSettingsKeys(QStringList *deleteKeys, QStringList *ignoreKeys)
 {
     auto settings = ConfigFile::settingsWithGroup(QLatin1String(accountsC));
-    QStringList badKeys;
-
     const int accountsVersion = settings->value(QLatin1String(versionC)).toInt();
     if (accountsVersion <= maxAccountsVersion) {
         foreach (const auto &accountId, settings->childGroups()) {
             settings->beginGroup(accountId);
             const int accountVersion = settings->value(QLatin1String(versionC), 1).toInt();
             if (accountVersion > maxAccountVersion) {
-                badKeys.append(settings->group());
+                ignoreKeys->append(settings->group());
             }
             settings->endGroup();
         }
     } else {
-        badKeys.append(settings->group());
+        deleteKeys->append(settings->group());
     }
-    return badKeys;
 }
 
 bool AccountManager::restoreFromLegacySettings()
@@ -398,6 +409,9 @@ void AccountManager::shutdown()
 
 bool AccountManager::isAccountIdAvailable(const QString &id) const
 {
+    if (_additionalBlockedAccountIds.contains(id))
+        return false;
+
     return std::none_of(_accounts.cbegin(), _accounts.cend(), [id](const auto &acc) {
         return acc->account()->id() == id;
     });

src/gui/accountmanager.h

@@ -80,7 +80,7 @@ public:
      * Returns the list of settings keys that can't be read because
      * they are from the future.
      */
-    static QStringList backwardMigrationKeys();
+    static void backwardMigrationSettingsKeys(QStringList *deleteKeys, QStringList *ignoreKeys);
 
 private:
     // saving and loading Account to settings
@@ -97,6 +97,8 @@ private:
 
     AccountManager() = default;
     QList<AccountStatePtr> _accounts;
+    /// Account ids from settings that weren't read
+    QSet<QString> _additionalBlockedAccountIds;
 
 public slots:
     /// Saves account data, not including the credentials

src/gui/application.cpp

@@ -102,47 +102,63 @@ namespace {
 
 // ----------------------------------------------------------------------------------
 
-bool Application::configBackwardMigration()
+bool Application::configVersionMigration()
 {
-    auto accountKeys = AccountManager::backwardMigrationKeys();
-    auto folderKeys = FolderMan::backwardMigrationKeys();
+    QStringList deleteKeys, ignoreKeys;
+    AccountManager::backwardMigrationSettingsKeys(&deleteKeys, &ignoreKeys);
+    FolderMan::backwardMigrationSettingsKeys(&deleteKeys, &ignoreKeys);
 
-    bool containsFutureData = !accountKeys.isEmpty() || !folderKeys.isEmpty();
+    ConfigFile configFile;
 
-    // Deal with unreadable accounts
-    if (!containsFutureData)
+    // Did the client version change?
+    // (The client version is adjusted further down)
+    bool versionChanged = configFile.clientVersionString() != MIRALL_VERSION_STRING;
+
+    // We want to message the user either for destructive changes,
+    // or if we're ignoring something and the client version changed.
+    bool warningMessage = !deleteKeys.isEmpty() || (!ignoreKeys.isEmpty() && versionChanged);
+
+    if (!versionChanged && !warningMessage)
         return true;
 
-    const auto backupFile = ConfigFile().backup();
-
-    QMessageBox box(
-        QMessageBox::Warning,
-        APPLICATION_SHORTNAME,
-        tr("Some settings were configured in newer versions of this client and "
-           "use features that are not available in this version.<br>"
-           "<br>"
-           "<b>Continuing will mean losing these settings.</b><br>"
-           "<br>"
-           "The current configuration file was already backed up to <i>%1</i>.")
-            .arg(backupFile));
-    box.addButton(tr("Quit"), QMessageBox::AcceptRole);
-    auto continueBtn = box.addButton(tr("Continue"), QMessageBox::DestructiveRole);
-
-    box.exec();
-    if (box.clickedButton() != continueBtn) {
-        QTimer::singleShot(0, qApp, SLOT(quit()));
-        return false;
-    }
+    const auto backupFile = configFile.backup();
+
+    if (warningMessage) {
+        QString boldMessage;
+        if (!deleteKeys.isEmpty()) {
+            boldMessage = tr("Continuing will mean <b>deleting these settings</b>.");
+        } else {
+            boldMessage = tr("Continuing will mean <b>ignoring these settings</b>.");
+        }
 
-    auto settings = ConfigFile::settingsWithGroup("foo");
-    settings->endGroup();
+        QMessageBox box(
+            QMessageBox::Warning,
+            APPLICATION_SHORTNAME,
+            tr("Some settings were configured in newer versions of this client and "
+               "use features that are not available in this version.<br>"
+               "<br>"
+               "%1<br>"
+               "<br>"
+               "The current configuration file was already backed up to <i>%2</i>.")
+                .arg(boldMessage, backupFile));
+        box.addButton(tr("Quit"), QMessageBox::AcceptRole);
+        auto continueBtn = box.addButton(tr("Continue"), QMessageBox::DestructiveRole);
+
+        box.exec();
+        if (box.clickedButton() != continueBtn) {
+            QTimer::singleShot(0, qApp, SLOT(quit()));
+            return false;
+        }
 
-    // Wipe the keys from the future
-    for (const auto &badKey : accountKeys)
-        settings->remove(badKey);
-    for (const auto &badKey : folderKeys)
-        settings->remove(badKey);
+        auto settings = ConfigFile::settingsWithGroup("foo");
+        settings->endGroup();
+
+        // Wipe confusing keys from the future, ignore the others
+        for (const auto &badKey : deleteKeys)
+            settings->remove(badKey);
+    }
 
+    configFile.setClientVersionString(MIRALL_VERSION_STRING);
     return true;
 }
 
@@ -231,7 +247,7 @@ Application::Application(int &argc, char **argv)
     setupLogging();
     setupTranslations();
 
-    if (!configBackwardMigration()) {
+    if (!configVersionMigration()) {
         return;
     }
 

src/gui/application.h

@@ -107,7 +107,7 @@ private:
      * Maybe a newer version of the client was used with this config file:
      * if so, backup, confirm with user and remove the config that can't be read.
      */
-    bool configBackwardMigration();
+    bool configVersionMigration();
 
     QPointer<ownCloudGui> _gui;
 

src/gui/folderman.cpp

@@ -162,6 +162,9 @@ int FolderMan::setupFolders()
 {
     unloadAndDeleteAllFolders();
 
+    QStringList skipSettingsKeys;
+    backwardMigrationSettingsKeys(&skipSettingsKeys, &skipSettingsKeys);
+
     auto settings = ConfigFile::settingsWithGroup(QLatin1String("Accounts"));
     const auto accountsWithSettings = settings->childGroups();
     if (accountsWithSettings.isEmpty()) {
@@ -181,19 +184,24 @@ int FolderMan::setupFolders()
         }
         settings->beginGroup(id);
 
-        settings->beginGroup(QLatin1String("Folders"));
-        setupFoldersHelper(*settings, account, true);
-        settings->endGroup();
+        // The "backwardsCompatible" flag here is related to migrating old
+        // database locations
+        auto process = [&](const QString &groupName, bool backwardsCompatible = false) {
+            settings->beginGroup(groupName);
+            if (skipSettingsKeys.contains(settings->group())) {
+                // Should not happen: bad container keys should have been deleted
+                qCWarning(lcFolderMan) << "Folder structure" << groupName << "is too new, ignoring";
+            } else {
+                setupFoldersHelper(*settings, account, backwardsCompatible, skipSettingsKeys);
+            }
+            settings->endGroup();
+        };
 
-        // See Folder::saveToSettings for details about why this exists.
-        settings->beginGroup(QLatin1String("Multifolders"));
-        setupFoldersHelper(*settings, account, false);
-        settings->endGroup();
+        process(QStringLiteral("Folders"), true);
 
-        // See Folder::saveToSettings for details about why this exists.
-        settings->beginGroup(QLatin1String("FoldersWithPlaceholders"));
-        setupFoldersHelper(*settings, account, false);
-        settings->endGroup();
+        // See Folder::saveToSettings for details about why these exists.
+        process(QStringLiteral("Multifolders"));
+        process(QStringLiteral("FoldersWithPlaceholders"));
 
         settings->endGroup(); // <account>
     }
@@ -203,9 +211,19 @@ int FolderMan::setupFolders()
     return _folderMap.size();
 }
 
-void FolderMan::setupFoldersHelper(QSettings &settings, AccountStatePtr account, bool backwardsCompatible)
+void FolderMan::setupFoldersHelper(QSettings &settings, AccountStatePtr account, bool backwardsCompatible, const QStringList &ignoreKeys)
 {
     for (const auto &folderAlias : settings.childGroups()) {
+        // Skip folders with too-new version
+        settings.beginGroup(folderAlias);
+        if (ignoreKeys.contains(settings.group())) {
+            qCInfo(lcFolderMan) << "Folder" << folderAlias << "is too new, ignoring";
+            _additionalBlockedFolderAliases.insert(folderAlias);
+            settings.endGroup();
+            continue;
+        }
+        settings.endGroup();
+
         FolderDefinition folderDefinition;
         if (FolderDefinition::load(settings, folderAlias, &folderDefinition)) {
             auto defaultJournalPath = folderDefinition.defaultJournalPath(account->account());
@@ -306,9 +324,8 @@ int FolderMan::setupFoldersMigration()
     return _folderMap.size();
 }
 
-QStringList FolderMan::backwardMigrationKeys()
+void FolderMan::backwardMigrationSettingsKeys(QStringList *deleteKeys, QStringList *ignoreKeys)
 {
-    QStringList badKeys;
     auto settings = ConfigFile::settingsWithGroup(QLatin1String("Accounts"));
 
     auto processSubgroup = [&](const QString &name) {
@@ -319,12 +336,12 @@ QStringList FolderMan::backwardMigrationKeys()
                 settings->beginGroup(folderAlias);
                 const int folderVersion = settings->value(QLatin1String(versionC), 1).toInt();
                 if (folderVersion > FolderDefinition::maxSettingsVersion()) {
-                    badKeys.append(settings->group());
+                    ignoreKeys->append(settings->group());
                 }
                 settings->endGroup();
             }
         } else {
-            badKeys.append(settings->group());
+            deleteKeys->append(settings->group());
         }
         settings->endGroup();
     };
@@ -336,7 +353,6 @@ QStringList FolderMan::backwardMigrationKeys()
         processSubgroup("FoldersWithPlaceholders");
         settings->endGroup();
     }
-    return badKeys;
 }
 
 bool FolderMan::ensureJournalGone(const QString &journalDbFile)
@@ -977,7 +993,9 @@ Folder *FolderMan::addFolderInternal(FolderDefinition folderDefinition,
 {
     auto alias = folderDefinition.alias;
     int count = 0;
-    while (folderDefinition.alias.isEmpty() || _folderMap.contains(folderDefinition.alias)) {
+    while (folderDefinition.alias.isEmpty()
+        || _folderMap.contains(folderDefinition.alias)
+        || _additionalBlockedFolderAliases.contains(folderDefinition.alias)) {
         // There is already a folder configured with this name and folder names need to be unique
         folderDefinition.alias = alias + QString::number(++count);
     }

src/gui/folderman.h

@@ -72,7 +72,7 @@ public:
      * Returns a list of keys that can't be read because they are from
      * future versions.
      */
-    static QStringList backwardMigrationKeys();
+    static void backwardMigrationSettingsKeys(QStringList *deleteKeys, QStringList *ignoreKeys);
 
     OCC::Folder::Map map();
 
@@ -305,7 +305,7 @@ private:
     // restarts the application (Linux only)
     void restartApplication();
 
-    void setupFoldersHelper(QSettings &settings, AccountStatePtr account, bool backwardsCompatible);
+    void setupFoldersHelper(QSettings &settings, AccountStatePtr account, bool backwardsCompatible, const QStringList &ignoreKeys);
 
     QSet<Folder *> _disabledFolders;
     Folder::Map _folderMap;
@@ -314,6 +314,9 @@ private:
     QPointer<Folder> _lastSyncFolder;
     bool _syncEnabled = true;
 
+    /// Folder aliases from the settings that weren't read
+    QSet<QString> _additionalBlockedFolderAliases;
+
     /// Starts regular etag query jobs
     QTimer _etagPollTimer;
     /// The currently running etag query

src/libsync/configfile.cpp

@@ -16,6 +16,7 @@
 
 #include "configfile.h"
 #include "theme.h"
+#include "version.h"
 #include "common/utility.h"
 #include "common/asserts.h"
 #include "version.h"
@@ -81,6 +82,7 @@ static const char logDebugC[] = "logDebug";
 static const char logExpireC[] = "logExpire";
 static const char logFlushC[] = "logFlush";
 static const char showExperimentalOptionsC[] = "showExperimentalOptions";
+static const char clientVersionC[] = "clientVersion";
 
 static const char proxyHostC[] = "Proxy/host";
 static const char proxyTypeC[] = "Proxy/type";
@@ -414,7 +416,14 @@ QString ConfigFile::excludeFileFromSystem()
 QString ConfigFile::backup() const
 {
     QString baseFile = configFile();
-    QString backupFile = QString("%1.backup_%2").arg(baseFile, QDateTime::currentDateTime().toString("yyyyMMdd_HHmmss"));
+    auto versionString = clientVersionString();
+    if (!versionString.isEmpty())
+        versionString.prepend('_');
+    QString backupFile =
+        QString("%1.backup_%2%3")
+            .arg(baseFile)
+            .arg(QDateTime::currentDateTime().toString("yyyyMMdd_HHmmss"))
+            .arg(versionString);
 
     // If this exact file already exists it's most likely that a backup was
     // already done. (two backup calls directly after each other, potentially
@@ -1018,6 +1027,18 @@ void ConfigFile::setCertificatePasswd(const QString &cPasswd)
     settings.sync();
 }
 
+QString ConfigFile::clientVersionString() const
+{
+    QSettings settings(configFile(), QSettings::IniFormat);
+    return settings.value(QLatin1String(clientVersionC), QString()).toString();
+}
+
+void ConfigFile::setClientVersionString(const QString &version)
+{
+    QSettings settings(configFile(), QSettings::IniFormat);
+    settings.setValue(QLatin1String(clientVersionC), version);
+}
+
 Q_GLOBAL_STATIC(QString, g_configFileName)
 
 std::unique_ptr<QSettings> ConfigFile::settingsWithGroup(const QString &group, QObject *parent)

src/libsync/configfile.h

@@ -192,6 +192,11 @@ public:
     QString certificatePasswd() const;
     void setCertificatePasswd(const QString &cPasswd);
 
+    /** The client version that last used this settings file.
+        Updated by configVersionMigration() at client startup. */
+    QString clientVersionString() const;
+    void setClientVersionString(const QString &version);
+
     /**  Returns a new settings pre-set in a specific group.  The Settings will be created
          with the given parent. If no parent is specified, the caller must destroy the settings */
     static std::unique_ptr<QSettings> settingsWithGroup(const QString &group, QObject *parent = nullptr);