Skip to content

Feat: forward migration at accounts config(issue #9037) #9070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ok-developer wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Feat: forward migration at accounts config(issue #9037) #9070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 88 additions & 13 deletions src/gui/accountmanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
/*
* SPDX-FileCopyrightText: 2018 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2015 ownCloud GmbH
Expand Down Expand Up @@ -72,10 +72,6 @@
constexpr auto unbrandedRelativeConfigLocationC = "/Nextcloud/nextcloud.cfg";
constexpr auto unbrandedCfgFileNameC = "nextcloud.cfg";

// The maximum versions that this client can read
constexpr auto maxAccountsVersion = 13;
constexpr auto maxAccountVersion = 13;

constexpr auto serverHasValidSubscriptionC = "serverHasValidSubscription";

constexpr auto generalC = "General";
Expand All @@ -84,7 +80,18 @@

namespace OCC {

namespace {
template<typename VersionType>
inline VersionType VersionFromSetting(QSettings *settings) {

Check warning on line 85 in src/gui/accountmanager.cpp

View workflow job for this annotation

GitHub Actions / build

src/gui/accountmanager.cpp:85:24 [modernize-use-trailing-return-type] 

use a trailing return type for this function
return settings->value(QLatin1String(versionC), int(VersionType::Min)).value<VersionType>();
}
template<typename VersionType>
inline void VersionToSetting(QSettings *settings, VersionType version) {
settings->setValue(QLatin1String(versionC), int(version));
}
}

Q_LOGGING_CATEGORY(lcAccountManager, "nextcloud.gui.account.manager", QtInfoMsg)

Check warning on line 94 in src/gui/accountmanager.cpp

View workflow job for this annotation

GitHub Actions / build

src/gui/accountmanager.cpp:94:1 [cppcoreguidelines-avoid-non-const-global-variables] 

variable 'Q_LOGGING_CATEGORY' is non-const and globally accessible, consider making it const

AccountManager *AccountManager::instance()
{
Expand Down Expand Up @@ -149,35 +156,50 @@
}

ConfigFile().cleanupGlobalNetworkConfiguration();
ClientProxy().cleanupGlobalNetworkConfiguration();
ClientProxy().cleanupGlobalNetworkConfiguration();

return result;
}

void AccountManager::backwardMigrationSettingsKeys(QStringList *deleteKeys, QStringList *ignoreKeys)
{
const auto settings = ConfigFile::settingsWithGroup(QLatin1String(accountsC));
const auto accountsVersion = settings->value(QLatin1String(versionC)).toInt();
const auto accountsVersion = VersionFromSetting<AccountsVersion>(settings.get());

qCInfo(lcAccountManager) << "Checking for accounts versions.";
qCInfo(lcAccountManager) << "Config accounts version:" << accountsVersion;
qCInfo(lcAccountManager) << "Max accounts Version is set to:" << maxAccountsVersion;
if (accountsVersion <= maxAccountsVersion) {
qCInfo(lcAccountManager) << "Max accounts Version is set to:" << AccountsVersion::Max;
if (accountsVersion <= AccountsVersion::Max) {
const auto settingsChildGroups = settings->childGroups();
for (const auto &accountId : settingsChildGroups) {
settings->beginGroup(accountId);
const auto accountVersion = settings->value(QLatin1String(versionC), 1).toInt();
const auto accountVersion = VersionFromSetting<AccountVersion>(settings.get());

if (accountVersion > maxAccountVersion) {
if (accountVersion > AccountVersion::Max) {
ignoreKeys->append(settings->group());
qCInfo(lcAccountManager) << "Ignoring account" << accountId << "because of version" << accountVersion;
}

settings->endGroup();
}
} else {
deleteKeys->append(settings->group());
}
}

void AccountManager::migrateToActualVersion()
{
const auto settings = ConfigFile::settingsWithGroup(QLatin1String(accountsC));

migrateAccountsSettings(settings);

for (const auto &accountId : settings->childGroups()) {
settings->beginGroup(accountId);
migrateAccountSettings(settings);
settings->endGroup();
}
}

#if !DISABLE_ACCOUNT_MIGRATION
bool AccountManager::restoreFromLegacySettings()
{
Expand Down Expand Up @@ -317,7 +339,7 @@
moveNetworkSettingsFromGlobalToAccount(acc);
}
configFile.cleanupGlobalNetworkConfiguration();
ClientProxy().cleanupGlobalNetworkConfiguration();
ClientProxy().cleanupGlobalNetworkConfiguration();
return true;
}

Expand All @@ -339,7 +361,9 @@
void AccountManager::save(bool saveCredentials)
{
const auto settings = ConfigFile::settingsWithGroup(QLatin1String(accountsC));
settings->setValue(QLatin1String(versionC), maxAccountsVersion);

VersionToSetting(settings.get(), AccountsVersion::Max);

for (const auto &acc : std::as_const(_accounts)) {
settings->beginGroup(acc->account()->id());
saveAccountHelper(acc->account(), *settings, saveCredentials);
Expand Down Expand Up @@ -376,7 +400,7 @@
void AccountManager::saveAccountHelper(const AccountPtr &account, QSettings &settings, bool saveCredentials)
{
qCDebug(lcAccountManager) << "Saving settings to" << settings.fileName();
settings.setValue(QLatin1String(versionC), maxAccountVersion);
VersionToSetting(&settings, AccountVersion::Max);
if (account->isPublicShareLink()) {
settings.setValue(QLatin1String(urlC), account->publicShareLinkUrl().toString());
} else {
Expand Down Expand Up @@ -798,4 +822,55 @@
_forceLegacyImport = forceLegacyImport;
Q_EMIT forceLegacyImportChanged();
}

void AccountManager::migrateAccountsSettings(const std::unique_ptr<QSettings> &settings)
{
const auto accountsVersion = VersionFromSetting<AccountsVersion>(settings.get());

switch (accountsVersion) {
// Nothing here for now
default:
VersionToSetting(settings.get(), AccountsVersion::Max);
break;
}
}
Comment on lines +826 to +836
Copy link
Collaborator

@mgallien mgallien Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this method is now empty, I fail to see the point of creating it
sounds way too much complicated
current design is that the current version is supposed to find out from past settings the config keys it knows about and can migrate
what are you trying to achieve ?

Copy link
Author

@ok-developer ok-developer Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just code for future usability. If a new version appears, it will just be necessary to add its own “case” section.


void AccountManager::migrateAccountSettings(const std::unique_ptr<QSettings> &settings)
{
const auto accountVersion = VersionFromSetting<AccountVersion>(settings.get());

switch (accountVersion)
{
// No previous statements
case AccountVersion::V13: {
// Related to issue #9037
const auto networkLimitSettingFix = [&settings](const QString &key) {
using NetworkLimitSetting = Account::AccountNetworkTransferLimitSetting;

const auto limitSetting = settings->value(key).value<NetworkLimitSetting>();

switch (limitSetting) {
case NetworkLimitSetting::LegacyGlobalLimit:
[[fallthrough]];
case NetworkLimitSetting::AutoLimit:
settings->setValue(key, int(NetworkLimitSetting::NoLimit));
break;
default:
break;
}
};

qCInfo(lcAccountManager) << "Migrating account" << settings->group()
<< "settings from version" << accountVersion
<< "to" << AccountVersion::V14;

networkLimitSettingFix(networkDownloadLimitSettingC);
networkLimitSettingFix(networkUploadLimitSettingC);
} [[fallthrough]];
// Tip: add new migration rules here
default:
VersionToSetting(settings.get(), AccountVersion::Max);
break;
}
}
}
24 changes: 24 additions & 0 deletions src/gui/accountmanager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
/*
* SPDX-FileCopyrightText: 2019 Nextcloud GmbH and Nextcloud contributors
* SPDX-FileCopyrightText: 2015 ownCloud GmbH
Expand All @@ -6,7 +6,7 @@

#pragma once

#include "account.h"

Check failure on line 9 in src/gui/accountmanager.h

View workflow job for this annotation

GitHub Actions / build

src/gui/accountmanager.h:9:10 [clang-diagnostic-error] 

'account.h' file not found
#include "accountstate.h"

namespace OCC {
Expand All @@ -31,6 +31,21 @@
};
Q_ENUM (AccountsRestoreResult);

enum class AccountsVersion {
V13 = 13,
Min = V13,
Max = V13
Comment on lines +36 to +37
Copy link
Collaborator

@mgallien mgallien Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really not a fan of those special enum values

Suggested change
Min = V13,
Max = V13
Min = V13,
Max = V13,

you no longer can rely on static code checker to see if you handle all existing values
what is their purpose ?

Copy link
Collaborator

@Aiiaiiio Aiiaiiio Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I too think these enum classes don't have much benefit. A struct could be used better for this. I have 2 issues with this solution:

  1. Hard to understand. What is AccountVersion and AccountsVersion? I'm not sure I see the purpose.
  2. Brings with it the template functions to set and get values from the config (VersionFromSetting() and VersionToSetting()). I think those are not necessary. We're setting int values regardless of the enum class type. No need for helpers.

Copy link
Author

@ok-developer ok-developer Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is their purpose ?

It's shortcuts for more readablity and incapsulation of changes. With new version we need to change values only here, not over every depending logic. For example, in "switch-default" section of migration function. Or in other possible cases with checks.

Copy link
Collaborator

@mgallien mgallien Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need special handling for Max ?
I could see why some special older configuration may need some special handling (was never the case before)
I really fail to see why we need to care about the minimum or maximum values (especially when you set the minimum to an obviously wrong value)
currently the client manages to migrate account configuration settings from version 1 up to 13
the enum definition could lead a reader to think otherwise
I therefore have a strong objection to the readability argument

Copy link
Author

@ok-developer ok-developer Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really didn't find any notice of versions 1 till 12 in the code. The only thing mentioned is 13. I set it as the first known for the new workflow.
Max is used as an alias for the highest known version for setting or checking with the existing value. If it weren't there, we would have to remember to change the value in the switch's default. It could be renamed to Actual.
Min is like the default value. In the getter function, for example.

};
Q_ENUM (AccountsVersion);

enum class AccountVersion {
V13 = 13,
V14,
Min = V13,
Max = V14
};
Comment on lines +41 to +46
Copy link
Collaborator

@mgallien mgallien Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Suggested change
enum class AccountVersion {
V13 = 13,
V14,
Min = V13,
Max = V14
};
enum class AccountVersion {
V13 = 13,
V14,
Min = V13,
Max = V14,
};

Q_ENUM (AccountVersion);

static AccountManager *instance();
~AccountManager() override = default;

Expand Down Expand Up @@ -83,6 +98,12 @@
*/
static void backwardMigrationSettingsKeys(QStringList *deleteKeys, QStringList *ignoreKeys);

/**
* Checks the versions of account groups and each account individually
* then attempts a soft migration.
*/
static void migrateToActualVersion();

public slots:
/// Saves account data when adding user, when updating e.g. dav user, not including the credentials
void saveAccount(const OCC::AccountPtr &newAccountData);
Expand Down Expand Up @@ -110,6 +131,9 @@
void capabilitiesChanged();

private:
static void migrateAccountsSettings(const std::unique_ptr<QSettings> &settings);
static void migrateAccountSettings(const std::unique_ptr<QSettings> &settings);

// saving and loading Account to settings
void saveAccountHelper(const AccountPtr &account, QSettings &settings, bool saveCredentials = true);
AccountPtr loadAccountHelper(QSettings &settings);
Expand Down
36 changes: 21 additions & 15 deletions src/gui/application.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,14 +141,6 @@ bool Application::configVersionMigration()
return true;
}

// 'Launch on system startup' defaults to true > 3.11.x
const auto theme = Theme::instance();
configFile.setLaunchOnSystemStartup(configFile.launchOnSystemStartup());
Utility::setLaunchOnStartup(theme->appName(), theme->appNameGUI(), configFile.launchOnSystemStartup());

// default is now off to displaying dialog warning user of too many files deletion
configFile.setPromptDeleteFiles(false);

// back up all old config files
QStringList backupFilesList;
QDir configDir(configFile.configPath());
Expand Down Expand Up @@ -190,15 +182,27 @@ bool Application::configVersionMigration()
}
}

if (!deleteKeys.isEmpty()) {
auto settings = ConfigFile::settingsWithGroup("foo");
settings->endGroup();
if (downgrading) {
// 'Launch on system startup' defaults to true > 3.11.x
const auto theme = Theme::instance();
configFile.setLaunchOnSystemStartup(configFile.launchOnSystemStartup());
Utility::setLaunchOnStartup(theme->appName(), theme->appNameGUI(), configFile.launchOnSystemStartup());

// default is now off to displaying dialog warning user of too many files deletion
configFile.setPromptDeleteFiles(false);

// Wipe confusing keys from the future, ignore the others
for (const auto &badKey : std::as_const(deleteKeys)) {
settings->remove(badKey);
qCInfo(lcApplication) << "Migration: removed" << badKey << "key from settings.";
if (!deleteKeys.isEmpty()) {
auto settings = ConfigFile::settingsWithGroup("foo");
Copy link
Collaborator

@mgallien mgallien Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by "foo" ?

Copy link
Author

@ok-developer ok-developer Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't my code. I tried not to change the existing logic.

Copy link
Collaborator

@mgallien mgallien Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that this was copied over from existing code
I fail to get the point of the if (downgrading) { condition
currently the title of the PR is about forward migration and you introduce new code to downgrade the settings
while this may be useful, that was not existing before and is out of the scope of the issue you ant to address

Copy link
Author

@ok-developer ok-developer Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original function(with the common name configVersionMigration) had a single behavior for migration in both directions. I simply used the existing downgrading flag to select directions. In general behavior, I kept the config backup and isolated the rest in the body of the condition. Nothing new has been added to backward migration. The logic is the same, only limited in direction.

settings->endGroup();

// Wipe confusing keys from the future, ignore the others
for (const auto &badKey : std::as_const(deleteKeys)) {
settings->remove(badKey);
qCInfo(lcApplication) << "Migration: removed" << badKey << "key from settings.";
}
}
} else { // upgrading
AccountManager::migrateToActualVersion();
}

configFile.setClientVersionString(MIRALL_VERSION_STRING);
Expand Down Expand Up @@ -323,6 +327,8 @@ Application::Application(int &argc, char **argv)
// only copy the settings and check what should be skipped
if (!configVersionMigration()) {
qCWarning(lcApplication) << "Config version migration was not possible.";
} else {
AccountManager::instance()->save();
}

ConfigFile cfg;
Expand Down
28 changes: 16 additions & 12 deletions src/gui/networksettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,36 +198,40 @@

void NetworkSettings::saveBWLimitSettings()
{
using NetworkTransferLimitSetting = Account::AccountNetworkTransferLimitSetting;

const auto downloadLimit = _ui->downloadSpinBox->value();
const auto uploadLimit = _ui->uploadSpinBox->value();

auto useDownloadLimit = 0;
auto useUploadLimit = 0;
auto useDownloadLimit = NetworkTransferLimitSetting::NoLimit;
auto useUploadLimit = NetworkTransferLimitSetting::NoLimit;

if (_ui->downloadLimitRadioButton->isChecked()) {

Check warning on line 209 in src/gui/networksettings.cpp

View workflow job for this annotation

GitHub Actions / build

src/gui/networksettings.cpp:209:53 [bugprone-branch-clone] 

repeated branch in conditional chain
useDownloadLimit = 1;
useDownloadLimit = NetworkTransferLimitSetting::ManualLimit;
Copy link
Collaborator

@Aiiaiiio Aiiaiiio Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice touch to use the enum values here!

} else if (_ui->noDownloadLimitRadioButton->isChecked()) {
useDownloadLimit = 0;
useDownloadLimit = NetworkTransferLimitSetting::NoLimit;
} else if (_ui->autoDownloadLimitRadioButton->isChecked()) {
useDownloadLimit = -1;
useDownloadLimit = NetworkTransferLimitSetting::AutoLimit;
} else if (_account) {
useDownloadLimit = -2;
// Legacy global. See Account::AccountNetworkTransferLimitSetting::LegacyGlobalLimit
useDownloadLimit = NetworkTransferLimitSetting::NoLimit;
}

if (_ui->uploadLimitRadioButton->isChecked()) {

Check warning on line 220 in src/gui/networksettings.cpp

View workflow job for this annotation

GitHub Actions / build

src/gui/networksettings.cpp:220:51 [bugprone-branch-clone] 

repeated branch in conditional chain
useUploadLimit = 1;
useUploadLimit = NetworkTransferLimitSetting::ManualLimit;
} else if (_ui->noUploadLimitRadioButton->isChecked()) {
useUploadLimit = 0;
useUploadLimit = NetworkTransferLimitSetting::NoLimit;
} else if (_ui->autoUploadLimitRadioButton->isChecked()) {
useUploadLimit = -1;
useUploadLimit = NetworkTransferLimitSetting::AutoLimit;
} else if (_account) {
useUploadLimit = -2;
// Legacy global. See Account::AccountNetworkTransferLimitSetting::LegacyGlobalLimit
useUploadLimit = NetworkTransferLimitSetting::NoLimit;
}

if (_account) {
_account->setDownloadLimitSetting(static_cast<Account::AccountNetworkTransferLimitSetting>(useDownloadLimit));
_account->setDownloadLimitSetting(useDownloadLimit);
_account->setDownloadLimit(downloadLimit);
_account->setUploadLimitSetting(static_cast<Account::AccountNetworkTransferLimitSetting>(useUploadLimit));
_account->setUploadLimitSetting(useUploadLimit);
_account->setUploadLimit(uploadLimit);
AccountManager::instance()->saveAccount(_account);
}
Expand Down
Loading