Merge pull request #6665 from nextcloud/backport/6655/stable-3.12

[stable-3.12] Bugfix/folder conflict disappear
nextcloud · Apr 23, 2024 · 846df3a · 846df3a
2 parents ed3035f + 49c6c3e
commit 846df3a
Show file tree

Hide file tree

Showing 8 changed files with 205 additions and 10 deletions.
diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
@@ -404,6 +404,11 @@ bool ProcessDirectoryJob::handleExcluded(const QString &path, const Entries &ent
         }
     }
 
+    if (_dirItem) {
+        _dirItem->_isAnyInvalidCharChild = _dirItem->_isAnyInvalidCharChild || item->_status == SyncFileItem::FileNameInvalid;
+        _dirItem->_isAnyCaseClashChild = _dirItem->_isAnyCaseClashChild || item->_status == SyncFileItem::FileNameClash;
+    }
+
     _childIgnored = true;
     emit _discoveryData->itemDiscovered(item);
     return true;

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -1290,6 +1290,19 @@ void PropagatorCompositeJob::slotSubJobFinished(SyncFileItem::Status status)
 {
     auto *subJob = dynamic_cast<PropagatorJob *>(sender());
     ASSERT(subJob);
+
+    if (!_isAnyInvalidCharChild || !_isAnyCaseClashChild) {
+        SyncFileItemPtr childDirItem;
+        if (const auto propagateDirectoryjob = qobject_cast<PropagateDirectory *>(subJob)) {
+            childDirItem = propagateDirectoryjob->_item;
+        } else if (const auto propagateIgnoreJob = qobject_cast<PropagateIgnoreJob *>(subJob)) {
+            childDirItem = propagateIgnoreJob->_item;
+        }
+        if (childDirItem) {
+            _isAnyCaseClashChild = _isAnyCaseClashChild || childDirItem->_status == SyncFileItem::FileNameClash || childDirItem->_isAnyCaseClashChild;
+            _isAnyInvalidCharChild = _isAnyInvalidCharChild || childDirItem->_status == SyncFileItem::FileNameInvalid || childDirItem->_isAnyInvalidCharChild;
+        }
+    }
 
     // Delete the job and remove it from our list of jobs.
     subJob->deleteLater();
@@ -1407,6 +1420,8 @@ void PropagateDirectory::slotFirstJobFinished(SyncFileItem::Status status)
 void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
 {
     if (!_item->isEmpty() && status == SyncFileItem::Success) {
+        _item->_isAnyCaseClashChild = _item->_isAnyCaseClashChild || _subJobs._isAnyCaseClashChild;
+        _item->_isAnyInvalidCharChild = _item->_isAnyInvalidCharChild || _subJobs._isAnyInvalidCharChild;
         // If a directory is renamed, recursively delete any stale items
         // that may still exist below the old path.
         if (_item->_instruction == CSYNC_INSTRUCTION_RENAME && _item->_originalFile != _item->_renameTarget) {
@@ -1442,14 +1457,16 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status)
         if (_item->_instruction == CSYNC_INSTRUCTION_RENAME
             || _item->_instruction == CSYNC_INSTRUCTION_NEW
             || _item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA) {
-            const auto result = propagator()->updateMetadata(*_item);
-            if (!result) {
-                status = _item->_status = SyncFileItem::FatalError;
-                _item->_errorString = tr("Error updating metadata: %1").arg(result.error());
-                qCWarning(lcDirectory) << "Error writing to the database for file" << _item->_file << "with" << result.error();
-            } else if (*result == Vfs::ConvertToPlaceholderResult::Locked) {
-                _item->_status = SyncFileItem::SoftError;
-                _item->_errorString = tr("File is currently in use");
+            if (!_item->_isAnyCaseClashChild && !_item->_isAnyInvalidCharChild) {
+                const auto result = propagator()->updateMetadata(*_item);
+                if (!result) {
+                    status = _item->_status = SyncFileItem::FatalError;
+                    _item->_errorString = tr("Error updating metadata: %1").arg(result.error());
+                    qCWarning(lcDirectory) << "Error writing to the database for file" << _item->_file << "with" << result.error();
+                } else if (*result == Vfs::ConvertToPlaceholderResult::Locked) {
+                    _item->_status = SyncFileItem::SoftError;
+                    _item->_errorString = tr("File is currently in use");
+                }
             }
         }
     }
@@ -1693,4 +1710,4 @@ void PropagateIgnoreJob::start()
     done(status, _item->_errorString, ErrorCategory::NoError);
 }
 
-}
+}
diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h
@@ -246,6 +246,8 @@ class PropagatorCompositeJob : public PropagatorJob
     QVector<PropagatorJob *> _runningJobs;
     SyncFileItem::Status _hasError = SyncFileItem::NoStatus; // NoStatus,  or NormalError / SoftError if there was an error
     quint64 _abortsCount = 0;
+    bool _isAnyCaseClashChild = false;
+    bool _isAnyInvalidCharChild = false;
 
     explicit PropagatorCompositeJob(OwncloudPropagator *propagator)
         : PropagatorJob(propagator)

diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
@@ -247,7 +247,17 @@ void PropagateLocalRename::start()
         if (QString::compare(_item->_file, _item->_renameTarget, Qt::CaseInsensitive) != 0
             && propagator()->localFileNameClash(_item->_renameTarget)) {
 
-            qCInfo(lcPropagateLocalRename) << "renaming a case clashed file" << _item->_file << _item->_renameTarget;
+            qCInfo(lcPropagateLocalRename) << "renaming a case clashed item" << _item->_file << _item->_renameTarget;
+            if (_item->isDirectory()) {
+                // #HotFix
+                // fix a crash (we can not create a conflicted copy for folders)
+                // right now, the conflict resolution will not even work for this scenario with folders,
+                // but, let's fix it step by step, this will be a second stage
+                done(SyncFileItem::FileNameClash,
+                     tr("Folder %1 cannot be renamed because of a local file or folder name clash!").arg(_item->_file),
+                     ErrorCategory::GenericError);
+                return;
+            }
             const auto caseClashConflictResult = propagator()->createCaseClashConflict(_item, existingFile);
             if (caseClashConflictResult) {
                 done(SyncFileItem::SoftError, *caseClashConflictResult, ErrorCategory::GenericError);

diff --git a/src/libsync/syncfileitem.h b/src/libsync/syncfileitem.h
@@ -335,6 +335,9 @@ class OWNCLOUDSYNC_EXPORT SyncFileItem
     bool _isFileDropDetected = false;
 
     bool _isEncryptedMetadataNeedUpdate = false;
+
+    bool _isAnyInvalidCharChild = false;
+    bool _isAnyCaseClashChild = false;
 };
 
 inline bool operator<(const SyncFileItemPtr &item1, const SyncFileItemPtr &item2)

diff --git a/test/syncenginetestutils.cpp b/test/syncenginetestutils.cpp
@@ -252,6 +252,21 @@ FileInfo *FileInfo::find(PathComponents pathComponents, const bool invalidateEta
     return nullptr;
 }
 
+FileInfo FileInfo::findRecursive(PathComponents pathComponents, const bool invalidateEtags)
+{
+    auto result = find({pathComponents.takeFirst()}, invalidateEtags);
+    if (!result) {
+        return *result;
+    }
+    for (const auto &pathComponent : pathComponents) {
+        if (!result) {
+            break;
+        }
+        result = result->find({pathComponent});
+    }
+    return *result;
+}
+
 FileInfo *FileInfo::createDir(const QString &relativePath)
 {
     const PathComponents pathComponents { relativePath };

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
@@ -147,6 +147,7 @@ class FileInfo : public FileModifier
     void setE2EE(const QString &relativepath, const bool enabled) override;
 
     FileInfo *find(PathComponents pathComponents, const bool invalidateEtags = false);
+    FileInfo findRecursive(PathComponents pathComponents, const bool invalidateEtags = false);
 
     FileInfo *createDir(const QString &relativePath);
 

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
@@ -1690,6 +1690,148 @@ private slots:
         QVERIFY(fakeFolder.syncOnce());
     }
 
+    void testServer_caseClash_createDiverseConflictsInsideOneFolderAndSolveThem()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        const QStringList conflictsFolderPathComponents = {"Documents", "DiverseConflicts"};
+
+        QString diverseConflictsFolderPath;
+        for (const auto &conflictsFolderPathComponent : conflictsFolderPathComponents) {
+            if (diverseConflictsFolderPath.isEmpty()) {
+                diverseConflictsFolderPath += conflictsFolderPathComponent;
+            } else {
+                diverseConflictsFolderPath += "/" + conflictsFolderPathComponent;
+            }
+            fakeFolder.remoteModifier().mkdir(diverseConflictsFolderPath);
+        }
+
+        constexpr auto testLowerCaseFile = "testfile";
+        constexpr auto testUpperCaseFile = "TESTFILE";
+
+        constexpr auto testLowerCaseFolder = "testfolder";
+        constexpr auto testUpperCaseFolder = "TESTFOLDER";
+
+        constexpr auto testInvalidCharFolder = "Really?";
+
+        fakeFolder.remoteModifier().insert(diverseConflictsFolderPath + "/" + testLowerCaseFile);
+        fakeFolder.remoteModifier().insert(diverseConflictsFolderPath + "/" + testUpperCaseFile);
+
+        fakeFolder.remoteModifier().mkdir(diverseConflictsFolderPath + "/" + testLowerCaseFolder);
+        fakeFolder.remoteModifier().mkdir(diverseConflictsFolderPath + "/" + testUpperCaseFolder);
+
+        fakeFolder.remoteModifier().mkdir(diverseConflictsFolderPath + "/" + testInvalidCharFolder);
+
+#if defined Q_OS_LINUX
+        constexpr auto shouldHaveCaseClashConflict = false;
+#else
+        constexpr auto shouldHaveCaseClashConflict = true;
+#endif
+        if (shouldHaveCaseClashConflict) {
+            ItemCompletedSpy completeSpy(fakeFolder);
+
+            fakeFolder.syncEngine().setLocalDiscoveryOptions(OCC::LocalDiscoveryStyle::DatabaseAndFilesystem);
+            QVERIFY(fakeFolder.syncOnce());
+
+            // verify the parent of a folder where caseclash and invalidchar conflicts were found, has corresponding flags set (conflict info must get
+            // propagated to the very top)
+            const auto diverseConflictsFolderParent = completeSpy.findItem(conflictsFolderPathComponents.first());
+            QVERIFY(diverseConflictsFolderParent);
+            QVERIFY(diverseConflictsFolderParent->_isAnyCaseClashChild);
+            QVERIFY(diverseConflictsFolderParent->_isAnyInvalidCharChild);
+            completeSpy.clear();
+
+            auto diverseConflictsFolderInfo = fakeFolder.currentLocalState().findRecursive(diverseConflictsFolderPath);
+            QVERIFY(!diverseConflictsFolderInfo.name.isEmpty());
+
+            auto conflictsFile = findCaseClashConflicts(diverseConflictsFolderInfo);
+            QCOMPARE(conflictsFile.size(), shouldHaveCaseClashConflict ? 1 : 0);
+            const auto hasFileCaseClashConflict = expectConflict(diverseConflictsFolderInfo, testLowerCaseFile);
+            QCOMPARE(hasFileCaseClashConflict, shouldHaveCaseClashConflict ? true : false);
+
+            fakeFolder.syncEngine().setLocalDiscoveryOptions(OCC::LocalDiscoveryStyle::DatabaseAndFilesystem);
+            QVERIFY(fakeFolder.syncOnce());
+
+            diverseConflictsFolderInfo = fakeFolder.currentLocalState().findRecursive(diverseConflictsFolderPath);
+            QVERIFY(!diverseConflictsFolderInfo.name.isEmpty());
+
+            conflictsFile = findCaseClashConflicts(diverseConflictsFolderInfo);
+            QCOMPARE(conflictsFile.size(), shouldHaveCaseClashConflict ? 1 : 0);
+
+            const auto conflictFileName = QString{conflictsFile.constFirst()};
+            qDebug() << conflictFileName;
+            CaseClashConflictSolver conflictSolver(fakeFolder.localPath() + diverseConflictsFolderPath + "/" + testLowerCaseFile,
+                                                   fakeFolder.localPath() + conflictFileName,
+                                                   QStringLiteral("/"),
+                                                   fakeFolder.localPath(),
+                                                   fakeFolder.account(),
+                                                   &fakeFolder.syncJournal());
+
+            QSignalSpy conflictSolverDone(&conflictSolver, &CaseClashConflictSolver::done);
+
+            conflictSolver.solveConflict("testfile2");
+
+            QVERIFY(conflictSolverDone.wait());
+
+            QVERIFY(fakeFolder.syncOnce());
+
+            diverseConflictsFolderInfo = fakeFolder.currentLocalState().findRecursive(diverseConflictsFolderPath);
+            QVERIFY(!diverseConflictsFolderInfo.name.isEmpty());
+
+            conflictsFile = findCaseClashConflicts(diverseConflictsFolderInfo);
+            QCOMPARE(conflictsFile.size(), 0);
+
+            fakeFolder.syncEngine().setLocalDiscoveryOptions(OCC::LocalDiscoveryStyle::DatabaseAndFilesystem);
+            QVERIFY(fakeFolder.syncOnce());
+
+            // After solving file conflict, verify that we did not lose conflict detection of caseclash and invalidchar for folders
+            for (auto it = completeSpy.begin(); it != completeSpy.end(); ++it) {
+                auto item = (*it).first().value<OCC::SyncFileItemPtr>();
+                item = nullptr;
+            }
+            auto invalidFilenameConflictFolderItem = completeSpy.findItem(diverseConflictsFolderPath + "/" + testInvalidCharFolder);
+            QVERIFY(invalidFilenameConflictFolderItem);
+            QVERIFY(invalidFilenameConflictFolderItem->_status == SyncFileItem::FileNameInvalid);
+
+            auto caseClashConflictFolderItemLower = completeSpy.findItem(diverseConflictsFolderPath + "/" + testLowerCaseFolder);
+            auto caseClashConflictFolderItemUpper = completeSpy.findItem(diverseConflictsFolderPath + "/" + testUpperCaseFolder);
+            completeSpy.clear();
+
+            // we always create UPPERCASE folder in current syncengine logic for now and then create a conflict for a lowercase folder, but this may change, so
+            // keep this check more future proof
+            const auto upperOrLowerCaseFolderCaseClashFound =
+                (caseClashConflictFolderItemLower && caseClashConflictFolderItemLower->_status == SyncFileItem::FileNameClash)
+                || (caseClashConflictFolderItemUpper && caseClashConflictFolderItemUpper->_status == SyncFileItem::FileNameClash);
+
+            QVERIFY(upperOrLowerCaseFolderCaseClashFound);
+
+            // solve case clash folders conflict
+            CaseClashConflictSolver conflictSolverCaseClashForFolder(fakeFolder.localPath() + diverseConflictsFolderPath + "/" + testLowerCaseFolder,
+                                                                     fakeFolder.localPath() + diverseConflictsFolderPath + "/" + testUpperCaseFolder,
+                                                                     QStringLiteral("/"),
+                                                                     fakeFolder.localPath(),
+                                                                     fakeFolder.account(),
+                                                                     &fakeFolder.syncJournal());
+
+            QSignalSpy conflictSolverCaseClashForFolderDone(&conflictSolverCaseClashForFolder, &CaseClashConflictSolver::done);
+            conflictSolverCaseClashForFolder.solveConflict("testfolder1");
+            QVERIFY(conflictSolverCaseClashForFolderDone.wait());
+            QVERIFY(fakeFolder.syncOnce());
+
+            // verify no case clash conflicts folder items are found
+            caseClashConflictFolderItemLower = completeSpy.findItem(diverseConflictsFolderPath + "/" + testLowerCaseFolder);
+            caseClashConflictFolderItemUpper = completeSpy.findItem(diverseConflictsFolderPath + "/" + testUpperCaseFolder);
+            QVERIFY((!caseClashConflictFolderItemLower || caseClashConflictFolderItemLower->_file.isEmpty())
+                    && (!caseClashConflictFolderItemUpper || caseClashConflictFolderItemUpper->_file.isEmpty()));
+
+            // veriy invalid filename conflict folder item is still present
+            invalidFilenameConflictFolderItem = completeSpy.findItem(diverseConflictsFolderPath + "/" + testInvalidCharFolder);
+            completeSpy.clear();
+            QVERIFY(invalidFilenameConflictFolderItem);
+            QVERIFY(invalidFilenameConflictFolderItem->_status == SyncFileItem::FileNameInvalid);
+        }
+    }
+
     void testExistingFolderBecameBig()
     {
         constexpr auto testFolder = "folder";