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

ok-developer · 2025-11-14T18:47:59Z

Issue #9037 requires manipulation with settings only once after app upgrade. I noticed that config migration already exists, but it makes only backward move. In this PR was added forward migration possibility for accounts part of config.

It uses version key which already exists in accounts configuration file, so it's just needed to be used in forward way. Every change in config layout could be controlled in AccountManager::migrateToActualVersion() function.
Backward migration remained unchanged.

How it works?
Firstly it checks app version string from config. If it was changed, app decides is it upgrade or downgrade. If it's upgrade it checks and perform possible migration actions to higher version of config details. If it's downgrade and config versions are not max, than it will be ignoring or erasing some keys(as it was earlier). Before any change it backups config file and, if enabled, shows warning message.

To resolve the main issue was added checks for networkUploadLimitSetting and networkDownloadLimitSetting keys in account v14 migration rule.

- added forward migration possibility to Application::configVersionMigration(); - added version abstruction for accounts and each account settings; - added version 14 to account settings: - forcly removes buggy up/down network limits(nextcloud#9037). Signed-off-by: Oleksandr Khryshchuk <oleksandr.khryshchuk@proton.me>

mgallien

I feel like the implementation is a bit complicated just to handle one value disappearing from a single setting
why not just modify the reading code to detect the now obsolete value and just replace a single value by another one

mgallien · 2025-11-26T08:20:09Z

src/gui/accountmanager.cpp

+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;
+    }
+}


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 ?

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

mgallien · 2025-11-26T08:23:01Z

src/gui/accountmanager.h

+        Min = V13,
+        Max = V13


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 ?

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:

Hard to understand. What is AccountVersion and AccountsVersion? I'm not sure I see the purpose.

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.

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.

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

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.

mgallien · 2025-11-26T08:23:16Z

src/gui/accountmanager.h

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


same

Suggested change

enum class AccountVersion {

V13 = 13,

V14,

Min = V13,

Max = V14

};

enum class AccountVersion {

V13 = 13,

V14,

Min = V13,

Max = V14,

};

mgallien · 2025-11-26T08:26:14Z

src/gui/application.cpp

-            settings->remove(badKey);
-            qCInfo(lcApplication) << "Migration: removed" << badKey << "key from settings.";
+        if (!deleteKeys.isEmpty()) {
+            auto settings = ConfigFile::settingsWithGroup("foo");


what do you mean by "foo" ?

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

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

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.

Aiiaiiio · 2025-11-26T09:08:12Z

src/gui/accountmanager.h

+        Min = V13,
+        Max = V13


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:

Hard to understand. What is AccountVersion and AccountsVersion? I'm not sure I see the purpose.

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.

Aiiaiiio · 2025-11-26T09:09:05Z

src/gui/networksettings.cpp


    if (_ui->downloadLimitRadioButton->isChecked()) {
-        useDownloadLimit = 1;
+        useDownloadLimit = NetworkTransferLimitSetting::ManualLimit;


Nice touch to use the enum values here!

ok-developer · 2025-11-26T09:24:27Z

I feel like the implementation is a bit complicated just to handle one value disappearing from a single setting why not just modify the reading code to detect the now obsolete value and just replace a single value by another one

The main reasons are that this needs to be done once(forcly changed at the post-update stage), and the option itself will not be removed.
In addition, this mechanism can be explicitly used in future config updates.

mgallien · 2025-11-26T10:56:40Z

src/gui/accountmanager.h

+        Min = V13,
+        Max = V13


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

mgallien · 2025-11-26T10:58:58Z

src/gui/application.cpp

-            settings->remove(badKey);
-            qCInfo(lcApplication) << "Migration: removed" << badKey << "key from settings.";
+        if (!deleteKeys.isEmpty()) {
+            auto settings = ConfigFile::settingsWithGroup("foo");


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

mgallien · 2025-11-28T15:27:20Z

I feel like the implementation is a bit complicated just to handle one value disappearing from a single setting why not just modify the reading code to detect the now obsolete value and just replace a single value by another one

The main reasons are that this needs to be done once(forcly changed at the post-update stage), and the option itself will not be removed. In addition, this mechanism can be explicitly used in future config updates.

I guess I can summarize it that way

ideally the account section in configuration was supposed to have a version number and each changes should bump the version
then the client would be able to run and only run the needed migrations
current client code base does it differently
we look for the client version and run a full settings migration each time the version differ between the runtime in the client binaries and the configuration file

if we are to follow the new explicit migration logic, this PR should include a bump of the version
did I miss it ?

I would also want to see what @camilasan think about this

github-actions · 2025-11-29T02:34:10Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

ok-developer · 2025-12-01T17:20:11Z

if we are to follow the new explicit migration logic, this PR should include a bump of the version did I miss it ?

It's in accountmanager.hpp in AccountVersion enum as V14. It will be set as Max in accountmanager.cpp at line 872.

ok-developer force-pushed the feat/acc-config-migration branch from 4b6d659 to b790384 Compare November 17, 2025 16:12

ok-developer marked this pull request as ready for review November 17, 2025 18:47

ok-developer requested review from Aiiaiiio, camilasan, i2h3, mgallien and nilsding as code owners November 17, 2025 18:47

mgallien requested changes Nov 26, 2025

View reviewed changes

Aiiaiiio requested changes Nov 26, 2025

View reviewed changes

mgallien requested changes Nov 26, 2025

View reviewed changes

github-actions bot added the feedback-requested label Nov 29, 2025

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

ok-developer commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgallien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-developer Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-developer Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ok-developer commented Nov 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgallien commented Nov 28, 2025

Uh oh!

github-actions bot commented Nov 29, 2025

Uh oh!

ok-developer commented Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ok-developer commented Nov 14, 2025 •

edited

Loading

ok-developer Nov 26, 2025 •

edited

Loading

ok-developer Nov 26, 2025 •

edited

Loading