From 591808f25e300583f7bbc574471990b1b734afd6 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Fri, 24 May 2024 21:04:15 +0200 Subject: [PATCH 1/6] Check if file is already in database before processing rename. - The rename check was not happening since it was hitting error "Move without permission to rename base file" - The file was then processed as a new file - Also check #6535. Possible workaround for #6549. Signed-off-by: Camila Ayres --- src/libsync/discovery.cpp | 142 +++++++++++++++++++++----------------- 1 file changed, 78 insertions(+), 64 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index dfcb37d29001a..23a0b716067ec 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1348,6 +1348,7 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( const auto originalPath = base.path(); // Function to gradually check conditions for accepting a move-candidate + const auto isExternalStorage = base._remotePerm.hasPermission(RemotePermissions::IsMounted); auto moveCheck = [&]() { if (!base.isValid()) { qCInfo(lcDisco) << "Not a move, no item in db with inode" << localEntry.inode; @@ -1396,6 +1397,19 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( return true; }; + + auto isExternalStorageRename = [&] { + OCC::SyncJournalFileRecord dbRecord; + const auto validRecord = _discoveryData->_statedb->getFileRecordByInode(localEntry.inode, &dbRecord); + qCInfo(lcDisco) << "File is already saved in DB?" << validRecord; + if (validRecord && isExternalStorage) { + qCInfo(lcDisco) << "File is already saved in DB with inode" << dbRecord._inode; + qCInfo(lcDisco) << "Try to process rename for" << dbRecord._path; + return true; + } + + return false; + }; const auto isMove = moveCheck(); const auto isE2eeMove = isMove && (base.isE2eEncrypted() || isInsideEncryptedTree()); const auto isCfApiVfsMode = _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() == Vfs::WindowsCfApi; @@ -1409,77 +1423,77 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( item->_errorString = tr("Moved to invalid target, restoring"); } - // - // If it's not a move it's just a local-NEW - if (!isMove || (isE2eeMove && !isE2eeMoveOnlineOnlyItemWithCfApi)) { - if (base.isE2eEncrypted()) { - // renaming the encrypted folder is done via remove + re-upload hence we need to mark the newly created folder as encrypted - // base is a record in the SyncJournal database that contains the data about the being-renamed folder with it's old name and encryption information - item->_e2eEncryptionStatus = EncryptionStatusEnums::fromDbEncryptionStatus(base._e2eEncryptionStatus); - item->_e2eEncryptionServerCapability = EncryptionStatusEnums::fromEndToEndEncryptionApiVersion(_discoveryData->_account->capabilities().clientSideEncryptionVersion()); - } - postProcessLocalNew(); - /*if (item->isDirectory() && item->_instruction == CSYNC_INSTRUCTION_NEW && item->_direction == SyncFileItem::Up - && _discoveryData->_account->capabilities().clientSideEncryptionVersion() >= 2.0) { - OCC::SyncJournalFileRecord rec; - _discoveryData->_statedb->findEncryptedAncestorForRecord(item->_file, &rec); - if (rec.isValid() && rec._e2eEncryptionStatus >= OCC::SyncJournalFileRecord::EncryptionStatus::EncryptedMigratedV2_0) { - qCDebug(lcDisco) << "Attempting to create a subfolder in top-level E2EE V2 folder. Ignoring."; - item->_instruction = CSYNC_INSTRUCTION_IGNORE; - item->_direction = SyncFileItem::None; - item->_status = SyncFileItem::NormalError; - item->_errorString = tr("Creating nested encrypted folders is not supported yet."); + // If it's not a move/rename, it's just a local-NEW + if (!isExternalStorageRename()) { + if ((!isMove || (isE2eeMove && !isE2eeMoveOnlineOnlyItemWithCfApi))) { + if (base.isE2eEncrypted()) { + // renaming the encrypted folder is done via remove + re-upload hence we need to mark the newly created folder as encrypted + // base is a record in the SyncJournal database that contains the data about the being-renamed folder with it's old name and encryption information + item->_e2eEncryptionStatus = EncryptionStatusEnums::fromDbEncryptionStatus(base._e2eEncryptionStatus); + item->_e2eEncryptionServerCapability = EncryptionStatusEnums::fromEndToEndEncryptionApiVersion(_discoveryData->_account->capabilities().clientSideEncryptionVersion()); } - }*/ - - finalize(); - return; - } - - // Check local permission if we are allowed to put move the file here - // Technically we should use the permissions from the server, but we'll assume it is the same - const auto serverHasMountRootProperty = _discoveryData->_account->serverHasMountRootProperty(); - const auto isExternalStorage = base._remotePerm.hasPermission(RemotePermissions::IsMounted); - const auto movePerms = checkMovePermissions(base._remotePerm, originalPath, item->isDirectory()); - if (!movePerms.sourceOk || !movePerms.destinationOk || (serverHasMountRootProperty && isExternalStorage) || isE2eeMoveOnlineOnlyItemWithCfApi) { - qCInfo(lcDisco) << "Move without permission to rename base file, " - << "source:" << movePerms.sourceOk - << ", target:" << movePerms.destinationOk - << ", targetNew:" << movePerms.destinationNewOk - << ", isExternalStorage:" << isExternalStorage - << ", serverHasMountRootProperty:" << serverHasMountRootProperty - << ", base._remotePerm:" << base._remotePerm.toString() - << ", base.path():" << base.path(); - - // If we can create the destination, do that. - // Permission errors on the destination will be handled by checkPermissions later. - postProcessLocalNew(); - finalize(); + postProcessLocalNew(); + /*if (item->isDirectory() && item->_instruction == CSYNC_INSTRUCTION_NEW && item->_direction == SyncFileItem::Up + && _discoveryData->_account->capabilities().clientSideEncryptionVersion() >= 2.0) { + OCC::SyncJournalFileRecord rec; + _discoveryData->_statedb->findEncryptedAncestorForRecord(item->_file, &rec); + if (rec.isValid() && rec._e2eEncryptionStatus >= OCC::SyncJournalFileRecord::EncryptionStatus::EncryptedMigratedV2_0) { + qCDebug(lcDisco) << "Attempting to create a subfolder in top-level E2EE V2 folder. Ignoring."; + item->_instruction = CSYNC_INSTRUCTION_IGNORE; + item->_direction = SyncFileItem::None; + item->_status = SyncFileItem::NormalError; + item->_errorString = tr("Creating nested encrypted folders is not supported yet."); + } + }*/ - // If the destination upload will work, we're fine with the source deletion. - // If the source deletion can't work, checkPermissions will error. - // In case of external storage mounted folders we are never allowed to move/delete them - if (movePerms.destinationNewOk && !isExternalStorage && !isE2eeMoveOnlineOnlyItemWithCfApi) { + finalize(); return; } - // Here we know the new location can't be uploaded: must prevent the source delete. - // Two cases: either the source item was already processed or not. - auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); - if (wasDeletedOnClient.first) { - // More complicated. The REMOVE is canceled. Restore will happen next sync. - qCInfo(lcDisco) << "Undid remove instruction on source" << originalPath; - if (!_discoveryData->_statedb->deleteFileRecord(originalPath, true)) { - qCWarning(lcDisco) << "Failed to delete a file record from the local DB" << originalPath; + // Check local permission if we are allowed to put move the file here + // Technically we should use the permissions from the server, but we'll assume it is the same + const auto serverHasMountRootProperty = _discoveryData->_account->serverHasMountRootProperty(); + const auto movePerms = checkMovePermissions(base._remotePerm, originalPath, item->isDirectory()); + if (!movePerms.sourceOk || !movePerms.destinationOk || (serverHasMountRootProperty && isExternalStorage) || isE2eeMoveOnlineOnlyItemWithCfApi) { + qCInfo(lcDisco) << "Move without permission to rename base file, " + << "source:" << movePerms.sourceOk + << ", target:" << movePerms.destinationOk + << ", targetNew:" << movePerms.destinationNewOk + << ", isExternalStorage:" << isExternalStorage + << ", serverHasMountRootProperty:" << serverHasMountRootProperty + << ", base._remotePerm:" << base._remotePerm.toString() + << ", base.path():" << base.path(); + + // If we can create the destination, do that. + // Permission errors on the destination will be handled by checkPermissions later. + postProcessLocalNew(); + finalize(); + + // If the destination upload will work, we're fine with the source deletion. + // If the source deletion can't work, checkPermissions will error. + // In case of external storage mounted folders we are never allowed to move/delete them + if (movePerms.destinationNewOk && !isExternalStorage && !isE2eeMoveOnlineOnlyItemWithCfApi) { + return; } - _discoveryData->_statedb->schedulePathForRemoteDiscovery(originalPath); - _discoveryData->_anotherSyncNeeded = true; - } else { - // Signal to future checkPermissions() to forbid the REMOVE and set to restore instead - qCInfo(lcDisco) << "Preventing future remove on source" << originalPath; - _discoveryData->_forbiddenDeletes[originalPath + '/'] = true; + + // Here we know the new location can't be uploaded: must prevent the source delete. + // Two cases: either the source item was already processed or not. + auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); + if (wasDeletedOnClient.first) { + // More complicated. The REMOVE is canceled. Restore will happen next sync. + qCInfo(lcDisco) << "Undid remove instruction on source" << originalPath; + if (!_discoveryData->_statedb->deleteFileRecord(originalPath, true)) { + qCWarning(lcDisco) << "Failed to delete a file record from the local DB" << originalPath; + } + _discoveryData->_statedb->schedulePathForRemoteDiscovery(originalPath); + _discoveryData->_anotherSyncNeeded = true; + } else { + // Signal to future checkPermissions() to forbid the REMOVE and set to restore instead + qCInfo(lcDisco) << "Preventing future remove on source" << originalPath; + _discoveryData->_forbiddenDeletes[originalPath + '/'] = true; + } + return; } - return; } auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); From 4afc033ee0493a4d4d0772fcc3f35a6716f4399d Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Wed, 5 Jun 2024 20:31:41 +0200 Subject: [PATCH 2/6] Add test to detect file renames in external storage. Signed-off-by: Camila Ayres --- test/testsyncmove.cpp | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 6a99af7c777eb..9c5ca18559d23 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -320,6 +320,49 @@ private slots: QCOMPARE(printDbData(fakeFolder.dbState()), printDbData(remoteInfo)); } + void testLocalExternalStorageRenameDetection() + { + FakeFolder fakeFolder { FileInfo { QString(), { FileInfo { QStringLiteral("external") } } }}; + + int nPUT = 0; + int nDELETE = 0; + fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &, QIODevice *) { + if (op == QNetworkAccessManager::PutOperation) { + ++nPUT; + } + + if (op == QNetworkAccessManager::DeleteOperation) { + ++nDELETE; + } + return nullptr; + }); + + fakeFolder.remoteModifier().permissions.setPermission(OCC::RemotePermissions::IsMounted); + fakeFolder.remoteModifier().permissions.setPermission(OCC::RemotePermissions::CanMove); + fakeFolder.remoteModifier().permissions.setPermission(OCC::RemotePermissions::CanWrite); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.remoteModifier()); + + fakeFolder.localModifier().insert("external/file", 100); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(printDbData(fakeFolder.dbState()), printDbData(fakeFolder.remoteModifier())); + QCOMPARE(nPUT, 1); + + fakeFolder.localModifier().rename("external/file", "external/file2"); + fakeFolder.localModifier().rename("external/file2", "external/file3"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(printDbData(fakeFolder.dbState()), printDbData(fakeFolder.remoteModifier())); + QCOMPARE(fakeFolder.remoteModifier().children.size(), 1); + + QCOMPARE(fakeFolder.remoteModifier().children.size(), 1); + QCOMPARE(fakeFolder.remoteModifier().children.value("external").children.size(), 1); + QCOMPARE(fakeFolder.remoteModifier().children.value("external").children.value("file3").name, QStringLiteral("file3")); + QCOMPARE(nPUT, 1); + QCOMPARE(nDELETE, 0); + } + void testDuplicateFileId_data() { QTest::addColumn("prefix"); From 9b2780c99ce97a90fe76a160fe5af1291a6f1d4e Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Wed, 5 Jun 2024 20:35:39 +0200 Subject: [PATCH 3/6] Improve logs for rename check. Signed-off-by: Camila Ayres --- src/libsync/discovery.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 23a0b716067ec..f2e90b58ee3e8 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1401,10 +1401,10 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( auto isExternalStorageRename = [&] { OCC::SyncJournalFileRecord dbRecord; const auto validRecord = _discoveryData->_statedb->getFileRecordByInode(localEntry.inode, &dbRecord); - qCInfo(lcDisco) << "File is already saved in DB?" << validRecord; + qCInfo(lcDisco) << "File is saved in DB:" << validRecord; + qCInfo(lcDisco) << "File is in external storage:" << isExternalStorage; if (validRecord && isExternalStorage) { - qCInfo(lcDisco) << "File is already saved in DB with inode" << dbRecord._inode; - qCInfo(lcDisco) << "Try to process rename for" << dbRecord._path; + qCInfo(lcDisco) << "Try to process rename for path" << dbRecord._path << "and inode" << dbRecord._inode; return true; } From 0965deaa2f15aeec9e54c86632af035ccd0b9957 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Thu, 6 Jun 2024 11:57:05 +0200 Subject: [PATCH 4/6] Remove commented out code. Signed-off-by: Camila Ayres --- src/libsync/discovery.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index f2e90b58ee3e8..a799dd7a5a976 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1433,19 +1433,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( item->_e2eEncryptionServerCapability = EncryptionStatusEnums::fromEndToEndEncryptionApiVersion(_discoveryData->_account->capabilities().clientSideEncryptionVersion()); } postProcessLocalNew(); - /*if (item->isDirectory() && item->_instruction == CSYNC_INSTRUCTION_NEW && item->_direction == SyncFileItem::Up - && _discoveryData->_account->capabilities().clientSideEncryptionVersion() >= 2.0) { - OCC::SyncJournalFileRecord rec; - _discoveryData->_statedb->findEncryptedAncestorForRecord(item->_file, &rec); - if (rec.isValid() && rec._e2eEncryptionStatus >= OCC::SyncJournalFileRecord::EncryptionStatus::EncryptedMigratedV2_0) { - qCDebug(lcDisco) << "Attempting to create a subfolder in top-level E2EE V2 folder. Ignoring."; - item->_instruction = CSYNC_INSTRUCTION_IGNORE; - item->_direction = SyncFileItem::None; - item->_status = SyncFileItem::NormalError; - item->_errorString = tr("Creating nested encrypted folders is not supported yet."); - } - }*/ - finalize(); return; } From e38a9f181aecf561e7c000e6b55240398f8aaa85 Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Mon, 24 Jun 2024 18:58:02 +0200 Subject: [PATCH 5/6] Only check for external storage permissions when it is a folder. Signed-off-by: Camila Ayres --- src/libsync/discovery.cpp | 119 +++++++++++++++++--------------------- 1 file changed, 52 insertions(+), 67 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index a799dd7a5a976..23b743a46c6c2 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -1348,7 +1348,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( const auto originalPath = base.path(); // Function to gradually check conditions for accepting a move-candidate - const auto isExternalStorage = base._remotePerm.hasPermission(RemotePermissions::IsMounted); auto moveCheck = [&]() { if (!base.isValid()) { qCInfo(lcDisco) << "Not a move, no item in db with inode" << localEntry.inode; @@ -1397,19 +1396,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( return true; }; - - auto isExternalStorageRename = [&] { - OCC::SyncJournalFileRecord dbRecord; - const auto validRecord = _discoveryData->_statedb->getFileRecordByInode(localEntry.inode, &dbRecord); - qCInfo(lcDisco) << "File is saved in DB:" << validRecord; - qCInfo(lcDisco) << "File is in external storage:" << isExternalStorage; - if (validRecord && isExternalStorage) { - qCInfo(lcDisco) << "Try to process rename for path" << dbRecord._path << "and inode" << dbRecord._inode; - return true; - } - - return false; - }; const auto isMove = moveCheck(); const auto isE2eeMove = isMove && (base.isE2eEncrypted() || isInsideEncryptedTree()); const auto isCfApiVfsMode = _discoveryData->_syncOptions._vfs && _discoveryData->_syncOptions._vfs->mode() == Vfs::WindowsCfApi; @@ -1423,64 +1409,63 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( item->_errorString = tr("Moved to invalid target, restoring"); } - // If it's not a move/rename, it's just a local-NEW - if (!isExternalStorageRename()) { - if ((!isMove || (isE2eeMove && !isE2eeMoveOnlineOnlyItemWithCfApi))) { - if (base.isE2eEncrypted()) { - // renaming the encrypted folder is done via remove + re-upload hence we need to mark the newly created folder as encrypted - // base is a record in the SyncJournal database that contains the data about the being-renamed folder with it's old name and encryption information - item->_e2eEncryptionStatus = EncryptionStatusEnums::fromDbEncryptionStatus(base._e2eEncryptionStatus); - item->_e2eEncryptionServerCapability = EncryptionStatusEnums::fromEndToEndEncryptionApiVersion(_discoveryData->_account->capabilities().clientSideEncryptionVersion()); - } - postProcessLocalNew(); - finalize(); - return; + // If it's not a move it's just a local-NEW + if (!isMove || (isE2eeMove && !isE2eeMoveOnlineOnlyItemWithCfApi)) { + if (base.isE2eEncrypted()) { + // renaming the encrypted folder is done via remove + re-upload hence we need to mark the newly created folder as encrypted + // base is a record in the SyncJournal database that contains the data about the being-renamed folder with it's old name and encryption information + item->_e2eEncryptionStatus = EncryptionStatusEnums::fromDbEncryptionStatus(base._e2eEncryptionStatus); + item->_e2eEncryptionServerCapability = EncryptionStatusEnums::fromEndToEndEncryptionApiVersion(_discoveryData->_account->capabilities().clientSideEncryptionVersion()); } + postProcessLocalNew(); + finalize(); + return; + } - // Check local permission if we are allowed to put move the file here - // Technically we should use the permissions from the server, but we'll assume it is the same - const auto serverHasMountRootProperty = _discoveryData->_account->serverHasMountRootProperty(); - const auto movePerms = checkMovePermissions(base._remotePerm, originalPath, item->isDirectory()); - if (!movePerms.sourceOk || !movePerms.destinationOk || (serverHasMountRootProperty && isExternalStorage) || isE2eeMoveOnlineOnlyItemWithCfApi) { - qCInfo(lcDisco) << "Move without permission to rename base file, " - << "source:" << movePerms.sourceOk - << ", target:" << movePerms.destinationOk - << ", targetNew:" << movePerms.destinationNewOk - << ", isExternalStorage:" << isExternalStorage - << ", serverHasMountRootProperty:" << serverHasMountRootProperty - << ", base._remotePerm:" << base._remotePerm.toString() - << ", base.path():" << base.path(); - - // If we can create the destination, do that. - // Permission errors on the destination will be handled by checkPermissions later. - postProcessLocalNew(); - finalize(); - - // If the destination upload will work, we're fine with the source deletion. - // If the source deletion can't work, checkPermissions will error. - // In case of external storage mounted folders we are never allowed to move/delete them - if (movePerms.destinationNewOk && !isExternalStorage && !isE2eeMoveOnlineOnlyItemWithCfApi) { - return; - } + // Check local permission if we are allowed to put move the file here + // Technically we should use the permissions from the server, but we'll assume it is the same + const auto serverHasMountRootProperty = _discoveryData->_account->serverHasMountRootProperty(); + const auto isExternalStorage = base._remotePerm.hasPermission(RemotePermissions::IsMounted) && base.isDirectory(); + const auto movePerms = checkMovePermissions(base._remotePerm, originalPath, item->isDirectory()); + if (!movePerms.sourceOk || !movePerms.destinationOk || (serverHasMountRootProperty && isExternalStorage) || isE2eeMoveOnlineOnlyItemWithCfApi) { + qCInfo(lcDisco) << "Move without permission to rename base file, " + << "source:" << movePerms.sourceOk + << ", target:" << movePerms.destinationOk + << ", targetNew:" << movePerms.destinationNewOk + << ", isExternalStorage:" << isExternalStorage + << ", serverHasMountRootProperty:" << serverHasMountRootProperty + << ", base._remotePerm:" << base._remotePerm.toString() + << ", base.path():" << base.path(); + + // If we can create the destination, do that. + // Permission errors on the destination will be handled by checkPermissions later. + postProcessLocalNew(); + finalize(); - // Here we know the new location can't be uploaded: must prevent the source delete. - // Two cases: either the source item was already processed or not. - auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); - if (wasDeletedOnClient.first) { - // More complicated. The REMOVE is canceled. Restore will happen next sync. - qCInfo(lcDisco) << "Undid remove instruction on source" << originalPath; - if (!_discoveryData->_statedb->deleteFileRecord(originalPath, true)) { - qCWarning(lcDisco) << "Failed to delete a file record from the local DB" << originalPath; - } - _discoveryData->_statedb->schedulePathForRemoteDiscovery(originalPath); - _discoveryData->_anotherSyncNeeded = true; - } else { - // Signal to future checkPermissions() to forbid the REMOVE and set to restore instead - qCInfo(lcDisco) << "Preventing future remove on source" << originalPath; - _discoveryData->_forbiddenDeletes[originalPath + '/'] = true; - } + // If the destination upload will work, we're fine with the source deletion. + // If the source deletion can't work, checkPermissions will error. + // In case of external storage mounted folders we are never allowed to move/delete them + if (movePerms.destinationNewOk && !isExternalStorage && !isE2eeMoveOnlineOnlyItemWithCfApi) { return; } + + // Here we know the new location can't be uploaded: must prevent the source delete. + // Two cases: either the source item was already processed or not. + auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); + if (wasDeletedOnClient.first) { + // More complicated. The REMOVE is canceled. Restore will happen next sync. + qCInfo(lcDisco) << "Undid remove instruction on source" << originalPath; + if (!_discoveryData->_statedb->deleteFileRecord(originalPath, true)) { + qCWarning(lcDisco) << "Failed to delete a file record from the local DB" << originalPath; + } + _discoveryData->_statedb->schedulePathForRemoteDiscovery(originalPath); + _discoveryData->_anotherSyncNeeded = true; + } else { + // Signal to future checkPermissions() to forbid the REMOVE and set to restore instead + qCInfo(lcDisco) << "Preventing future remove on source" << originalPath; + _discoveryData->_forbiddenDeletes[originalPath + '/'] = true; + } + return; } auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath); From 0160cec256ed72736a3ef83228aaea7918d82e2f Mon Sep 17 00:00:00 2001 From: Camila Ayres Date: Tue, 25 Jun 2024 16:40:02 +0200 Subject: [PATCH 6/6] Fix renaming tests for files under group folders. Signed-off-by: Camila Ayres --- test/testsyncmove.cpp | 54 ++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index 9c5ca18559d23..7dda06235400c 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -322,45 +322,41 @@ private slots: void testLocalExternalStorageRenameDetection() { - FakeFolder fakeFolder { FileInfo { QString(), { FileInfo { QStringLiteral("external") } } }}; - - int nPUT = 0; - int nDELETE = 0; - fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &, QIODevice *) { - if (op == QNetworkAccessManager::PutOperation) { - ++nPUT; - } - - if (op == QNetworkAccessManager::DeleteOperation) { - ++nDELETE; - } - return nullptr; - }); - - fakeFolder.remoteModifier().permissions.setPermission(OCC::RemotePermissions::IsMounted); - fakeFolder.remoteModifier().permissions.setPermission(OCC::RemotePermissions::CanMove); - fakeFolder.remoteModifier().permissions.setPermission(OCC::RemotePermissions::CanWrite); + FakeFolder fakeFolder{{}}; + fakeFolder.remoteModifier().mkdir("external-storage"); + auto externalStorage = fakeFolder.remoteModifier().find("external-storage"); + externalStorage->extraDavProperties = "true"; + setAllPerm(externalStorage, RemotePermissions::fromServerString("WDNVCKRM")); QVERIFY(fakeFolder.syncOnce()); - QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.remoteModifier()); - fakeFolder.localModifier().insert("external/file", 100); + OperationCounter operationCounter; + fakeFolder.setServerOverride(operationCounter.functor()); + + fakeFolder.localModifier().insert("external-storage/file", 100); QVERIFY(fakeFolder.syncOnce()); QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); QCOMPARE(printDbData(fakeFolder.dbState()), printDbData(fakeFolder.remoteModifier())); - QCOMPARE(nPUT, 1); + QCOMPARE(operationCounter.nPUT, 1); + + const auto firstFileId = fakeFolder.remoteModifier().find("external-storage/file")->fileId; - fakeFolder.localModifier().rename("external/file", "external/file2"); - fakeFolder.localModifier().rename("external/file2", "external/file3"); + fakeFolder.localModifier().rename("external-storage/file", "external-storage/file2"); QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(operationCounter.nMOVE, 1); + + fakeFolder.localModifier().rename("external-storage/file2", "external-storage/file3"); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(operationCounter.nMOVE, 2); + + const auto renamedFileId = fakeFolder.remoteModifier().find("external-storage/file3")->fileId; + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); QCOMPARE(printDbData(fakeFolder.dbState()), printDbData(fakeFolder.remoteModifier())); - QCOMPARE(fakeFolder.remoteModifier().children.size(), 1); - QCOMPARE(fakeFolder.remoteModifier().children.size(), 1); - QCOMPARE(fakeFolder.remoteModifier().children.value("external").children.size(), 1); - QCOMPARE(fakeFolder.remoteModifier().children.value("external").children.value("file3").name, QStringLiteral("file3")); - QCOMPARE(nPUT, 1); - QCOMPARE(nDELETE, 0); + QCOMPARE(fakeFolder.remoteModifier().find("external-storage/file"), nullptr); + QCOMPARE(fakeFolder.remoteModifier().find("external-storage/file2"), nullptr); + QVERIFY(fakeFolder.remoteModifier().find("external-storage/file3")); + QCOMPARE(firstFileId, renamedFileId); } void testDuplicateFileId_data()