refactor: migration. by camilasan · Pull Request #9191 · nextcloud/desktop

camilasan · 2025-12-04T21:10:17Z

Replaces work in #8154.

It moves code around and add some very basics tests. More work is needed to be able to fully cover the logic with tests.

desktop/src/libsync/settings/migration.h

Lines 48 to 59 in 2539f95

    
               /** 
        
                * => Application::configVersionMigration: check existing config 
        
                *  => no upgrade/downgrade/migration: Migration::Phase::Done 
        
                *  => need to set config file: Migration::Phase::SetupConfigFile 
        
                * => Application::setupAccountsAndFolders: Migration::Phase::SetupUsers 
        
                *   => Application::restoreLegacyAccount 
        
                *     => AccountManager::restore 
        
                *       => AccountManager::restoreFromLegacySettings: 
        
                *         => Migration::legacyData: find legacy config and read its settings using 
        
                *     => FolderMan::setupFolders: Migration::Phase::SetupFolders 
        
                * => AccountState::slotCredentialsFetched: Migration::Phase::Done 
        
                */

sonarqubecloud · 2025-12-04T21:41:33Z

Quality Gate failed

Failed conditions
28.4% Coverage on New Code (required ≥ 80%)
127 New Code Smells (required ≤ 0)
D Security Rating on New Code (required ≥ A)
C Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

It should cover upgrade, downgrade as well as upgrade from unbranded to branded, from legacy to branded and from legacy to unbranded. Signed-off-by: Camila Ayres <hello@camilasan.com>

Signed-off-by: Camila Ayres <hello@camilasan.com>

If no legacy file is found, it means it might be upgrade only. Signed-off-by: Camila Ayres <hello@camilasan.com>

…to ConfigFile. Signed-off-by: Camila Ayres <hello@camilasan.com>

…n class. - Make all Migration class members static. - Rename Migration class members. Signed-off-by: Camila Ayres <hello@camilasan.com>

Signed-off-by: Camila Ayres <hello@camilasan.com>

Copilot

Pull request overview

This PR refactors client migration logic by moving migration state/decision-making out of ConfigFile into a new OCC::Migration helper, and wires it through GUI/libsync code paths. It also adds a new unit test target for migration behavior.

Changes:

Introduces src/libsync/settings/migration.{h,cpp} and updates build/test wiring.
Replaces ConfigFile migration helpers with Migration usage across Application/AccountManager/AccountState/FolderMan.
Adds ConfigFile::backupConfigFiles() and a new Migration test.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
test/testmigration.cpp	Adds a new migration-focused test (currently with correctness issues).
test/CMakeLists.txt	Registers the new `Migration` test target.
src/libsync/settings/migration.h	Declares new `Migration` API and static migration state.
src/libsync/settings/migration.cpp	Implements version checks and legacy config discovery.
src/libsync/configfile.h	Removes old migration API surface; adds `backupConfigFiles()`.
src/libsync/configfile.cpp	Switches to `Migration` for branded/unbranded checks; adds `backupConfigFiles()`.
src/libsync/CMakeLists.txt	Adds new migration sources to libsync build.
src/gui/folderman.h	Adds `TestMigration` friend declaration.
src/gui/folderman.cpp	Uses `Migration::discoveredLegacyConfigPath()` for folder migration path selection.
src/gui/application.cpp	Uses `Migration` phases/flags during config migration and setup flow.
src/gui/accountstate.cpp	Uses `Migration` for branded/unbranded migration keychain handling and phase completion.
src/gui/accountmanager.cpp	Refactors legacy-settings restore flow to use `Migration::legacyData()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-03T21:20:25Z

src/gui/application.cpp

        && !AccountManager::instance()->forceLegacyImport()
        && accountsListSize > 0) {


The condition redundantly checks accountsListSize > 0 twice. Drop the duplicate to keep the logic minimal and avoid mistakes when editing this condition later.

Suggested change

&& !AccountManager::instance()->forceLegacyImport()

&& accountsListSize > 0) {

&& !AccountManager::instance()->forceLegacyImport()) {

Copilot · 2026-03-03T21:20:26Z

test/testmigration.cpp

+        folderDefinition.localPath = "/standardAppName";
+        folderDefinition.targetPath = "/";


folderDefinition.localPath is set to the literal "/standardAppName", which won’t exist on most systems. Since Folder::checkLocalPath() requires the directory to exist and be readable, this will make the test fail in CI. Use a path under _temporaryDir.path() (and mkpath it) for the local sync folder.

Suggested change

folderDefinition.localPath = "/standardAppName";

folderDefinition.targetPath = "/";

const auto localPath = _temporaryDir.path() + QLatin1Char('/') + QLatin1String(standardAppName);

QVERIFY(QDir().mkpath(localPath));

folderDefinition.localPath = localPath;

folderDefinition.targetPath = QLatin1String("/");

Copilot · 2026-03-03T21:20:26Z

test/testmigration.cpp

+        "0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\ignoreHiddenFiles=true\n"
+        "0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\journalPath=.sync_journal.db\n"
+        "0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\localPath=/ownCloud/\n"
+        "0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\paused=false\n"


Several lines in legacyAppConfigContent use a single backslash before 2ba... (e.g. "0\\Folders\2ba..."). In C++ string literals \2 is interpreted as an octal escape, so the resulting INI content is corrupted. Escape the backslashes consistently (use \\ where you want a literal \ in the INI).

Suggested change

"0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\ignoreHiddenFiles=true\n"

"0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\journalPath=.sync_journal.db\n"

"0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\localPath=/ownCloud/\n"

"0\\Folders\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\paused=false\n"

"0\\Folders\\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\ignoreHiddenFiles=true\n"

"0\\Folders\\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\journalPath=.sync_journal.db\n"

"0\\Folders\\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\localPath=/ownCloud/\n"

"0\\Folders\\2ba4b09a-1223-aaaa-abcd-c2df238816d8\\paused=false\n"

Copilot · 2026-03-03T21:20:26Z

src/libsync/settings/migration.cpp

+                qCDebug(lcMigration) << "Copy settings" << oCSettings->allKeys().join(", ");
+                Migration migration;
+                migration.setDiscoveredLegacyConfigPath(configFileInfo.canonicalPath());
+                legacyData.reset(oCSettings.get());


legacyData.reset(oCSettings.get()) assigns a raw pointer still owned by std::unique_ptr<QSettings> oCSettings, which will be deleted at scope exit. This results in double-delete / use-after-free. Transfer ownership explicitly (e.g., construct the shared pointer from oCSettings.release()), or avoid mixing unique_ptr and QSharedPointer here.

Suggested change

legacyData.reset(oCSettings.get());

legacyData.reset(oCSettings.release());

Copilot · 2026-03-03T21:20:27Z

test/testmigration.cpp

+        _configFile.setConfDir(standardConfigFolder);
+    }
+
+    void setupStandarConfig(const QString &version)


Function name typo: setupStandarConfig (missing ‘d’) makes the test harder to read/search and looks unintentional. Rename to setupStandardConfig (and update call sites).

Suggested change

void setupStandarConfig(const QString &version)

void setupStandardConfig(const QString &version)

Copilot · 2026-03-03T21:20:27Z

test/testmigration.cpp

+        _configFile.setOverrideLocalDir("A");
+        _configFile.setVfsEnabled(true);
+        _configFile.setProxyType(0);
+        _configFile.setVfsEnabled(true);


Duplicate call: setVfsEnabled(true) is invoked twice (lines 96 and 98). Remove the redundant call to keep the test setup clear.

Suggested change

_configFile.setVfsEnabled(true);

Copilot · 2026-03-03T21:20:28Z

test/testmigration.cpp

+        _configFile.setConfDir(standardConfigFolder);
+    }
+
+    void setupStandarConfig(const QString &version)


Function name typo: setupStandarConfig (missing ‘d’) makes the test harder to read/search and looks unintentional. Rename to setupStandardConfig (and update call sites).

Suggested change

void setupStandarConfig(const QString &version)

void setupStandardConfig(const QString &version)

Copilot · 2026-03-03T21:20:28Z

test/testmigration.cpp

+        "logHttp=false\n"
+        "optionalDesktopNotifications=true\n"
+        "\n"
+        "[Accounts]e\n"


legacyAppConfigContent has a typo in the INI section header: "[Accounts]e" includes an extra e, which will break parsing if this test data is used. Remove the stray character.

Suggested change

"[Accounts]e\n"

"[Accounts]\n"

Copilot · 2026-03-03T21:20:28Z

src/libsync/settings/migration.h

+    };
+
+    using LegacyData = QSharedPointer<QSettings>;
+


Migration::LegacyData is QSharedPointer<QSettings>, but this header doesn’t include <QSharedPointer>. Depending on include order this can fail to compile. Add the missing include (and consider dropping unused includes like <QMap> if not needed).

Copilot · 2026-03-03T21:20:28Z

src/gui/accountmanager.cpp

        }
+
+        settings.reset(oCSettings.get());
    }


settings.reset(oCSettings.get()) makes the std::unique_ptr take ownership of a pointer that is still owned by oCSettings (QSharedPointer). This will lead to double deletion. Use a single ownership model (e.g., keep settings as a shared pointer, or have Migration::legacyData() return a std::unique_ptr<QSettings> and move it into settings) instead of resetting from get().

Signed-off-by: Camila Ayres <hello@camilasan.com>

github-actions · 2026-03-03T21:40:19Z

Artifact containing the AppImage: nextcloud-appimage-pr-9191.zip

Digest: sha256:7e5932761596faa513e52c1b5b515feda9da327a01c23c3505802d2c77d7ae6e

To test this change/fix you can download the above artifact file, unzip it, and run it.

Please make sure to quit your existing Nextcloud app and backup your data.

sonarqubecloud · 2026-03-03T22:06:32Z

Quality Gate failed

Failed conditions
38.1% Coverage on New Code (required ≥ 80%)
C Maintainability Rating on New Code (required ≥ A)
1 New Bugs (required ≤ 0)
C Reliability Rating on New Code (required ≥ A)
D Security Rating on New Code (required ≥ A)
186 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

camilasan requested review from Aiiaiiio, i2h3, mgallien and nilsding as code owners December 4, 2025 21:10

camilasan marked this pull request as draft December 4, 2025 21:19

camilasan force-pushed the enh/migration branch from 82181cc to 8f83bd3 Compare December 22, 2025 17:05

camilasan force-pushed the enh/migration branch from 8f83bd3 to 2a21520 Compare January 9, 2026 10:48

camilasan mentioned this pull request Jan 9, 2026

fix: detect upgrade only scenario. #9296

Merged

camilasan changed the title ~~WIP: refactor: migration~~ refactor: migration. Jan 9, 2026

camilasan added this to the 33.0.0 milestone Jan 21, 2026

camilasan removed this from the 33.0.0 milestone Feb 17, 2026

camilasan added 6 commits March 3, 2026 22:10

refactor(migration): add a class to handle the migration logic.

165568e

It should cover upgrade, downgrade as well as upgrade from unbranded to branded, from legacy to branded and from legacy to unbranded. Signed-off-by: Camila Ayres <hello@camilasan.com>

test(migration): basic class tests.

4c45852

Signed-off-by: Camila Ayres <hello@camilasan.com>

fix(migration): detect upgrade only scenario.

99d2a87

If no legacy file is found, it means it might be upgrade only. Signed-off-by: Camila Ayres <hello@camilasan.com>

refactor(migration): move backup config files logic from Application …

489ace4

…to ConfigFile. Signed-off-by: Camila Ayres <hello@camilasan.com>

refactor(migration): find and readlegacy config files in the Migratio…

66af508

…n class. - Make all Migration class members static. - Rename Migration class members. Signed-off-by: Camila Ayres <hello@camilasan.com>

refactor(migration): change log level, add comments.

0b0dc97

Signed-off-by: Camila Ayres <hello@camilasan.com>

camilasan force-pushed the enh/migration branch from 2a21520 to 0b0dc97 Compare March 3, 2026 21:11

camilasan requested a review from Copilot March 3, 2026 21:11

camilasan marked this pull request as ready for review March 3, 2026 21:11

Copilot AI reviewed Mar 3, 2026

View reviewed changes

camilasan added 2 commits March 3, 2026 22:26

chore(migration): add extended comment to explain the migration phases.

b1cf250

Signed-off-by: Camila Ayres <hello@camilasan.com>

fix(migration): update function change from ConfigFile to Migration.

2539f95

Signed-off-by: Camila Ayres <hello@camilasan.com>

	/**
	* => Application::configVersionMigration: check existing config
	* => no upgrade/downgrade/migration: Migration::Phase::Done
	* => need to set config file: Migration::Phase::SetupConfigFile
	* => Application::setupAccountsAndFolders: Migration::Phase::SetupUsers
	* => Application::restoreLegacyAccount
	* => AccountManager::restore
	* => AccountManager::restoreFromLegacySettings:
	* => Migration::legacyData: find legacy config and read its settings using
	* => FolderMan::setupFolders: Migration::Phase::SetupFolders
	* => AccountState::slotCredentialsFetched: Migration::Phase::Done
	*/

		&& !AccountManager::instance()->forceLegacyImport()
		&& accountsListSize > 0) {

		folderDefinition.localPath = "/standardAppName";
		folderDefinition.targetPath = "/";

-        folderDefinition.localPath = "/standardAppName";
-        folderDefinition.targetPath = "/";
+        const auto localPath = _temporaryDir.path() + QLatin1Char('/') + QLatin1String(standardAppName);
+        QVERIFY(QDir().mkpath(localPath));
+        folderDefinition.localPath = localPath;
+        folderDefinition.targetPath = QLatin1String("/");

	legacyData.reset(oCSettings.get());
	legacyData.reset(oCSettings.release());

	void setupStandarConfig(const QString &version)
	void setupStandardConfig(const QString &version)

Conversation

camilasan commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 4, 2025

Quality Gate failed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 3, 2026

Uh oh!

sonarqubecloud bot commented Mar 3, 2026

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

camilasan commented Dec 4, 2025 •

edited

Loading