From 49c6c3e36c7a4d5fba86f2e92cf4aae948bbfff4 Mon Sep 17 00:00:00 2001 From: allexzander Date: Tue, 23 Apr 2024 12:18:25 +0200 Subject: [PATCH] Bugfix/folder conflict disappear Signed-off-by: alex-z --- src/libsync/discovery.cpp | 5 + src/libsync/owncloudpropagator.cpp | 35 +++++-- src/libsync/owncloudpropagator.h | 2 + src/libsync/propagatorjobs.cpp | 12 ++- src/libsync/syncfileitem.h | 3 + test/syncenginetestutils.cpp | 15 +++ test/syncenginetestutils.h | 1 + test/testsyncengine.cpp | 142 +++++++++++++++++++++++++++++ 8 files changed, 205 insertions(+), 10 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index ef0644ca1426..387633c73f54 100644 --- 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 index 1e46940b62b1..04792ef31f85 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1290,6 +1290,19 @@ void PropagatorCompositeJob::slotSubJobFinished(SyncFileItem::Status status) { auto *subJob = dynamic_cast(sender()); ASSERT(subJob); + + if (!_isAnyInvalidCharChild || !_isAnyCaseClashChild) { + SyncFileItemPtr childDirItem; + if (const auto propagateDirectoryjob = qobject_cast(subJob)) { + childDirItem = propagateDirectoryjob->_item; + } else if (const auto propagateIgnoreJob = qobject_cast(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); } -} +} \ No newline at end of file diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index 0e4640e22cf8..d8f5f8eec022 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -246,6 +246,8 @@ class PropagatorCompositeJob : public PropagatorJob QVector _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 index 82167f5b9c81..a4e4d026c6f6 100644 --- 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 index d06e6f6d93b2..3f6a52898611 100644 --- 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 index 88ece82c1e7e..2e11ab4dc987 100644 --- 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 index c688eddce044..49cdac710338 100644 --- 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 index 8ef503956740..0574b5b1f0a7 100644 --- 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(); + 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";