Merge pull request #5104 from nextcloud/bugfix/doNotDeleteFilesInSync…

…Database Bugfix/delete folders during propagation even when propagation has errors
nextcloud · Nov 7, 2022 · b02f112 · b02f112
2 parents ad89333 + 402d959
commit b02f112
Show file tree

Hide file tree

Showing 6 changed files with 71 additions and 75 deletions.
diff --git a/src/libsync/capabilities.cpp b/src/libsync/capabilities.cpp
@@ -143,24 +143,27 @@ bool Capabilities::clientSideEncryptionAvailable() const
     }
 
     const auto version = properties.value(QStringLiteral("api-version"), "1.0").toByteArray();
-    qCInfo(lcServerCapabilities) << "E2EE API version:" << version;
     const auto splittedVersion = version.split('.');
 
     bool ok = false;
     const auto major = !splittedVersion.isEmpty() ? splittedVersion.at(0).toInt(&ok) : 0;
     if (!ok) {
-        qCWarning(lcServerCapabilities) << "Didn't understand version scheme (major), E2EE disabled";
+        qCWarning(lcServerCapabilities) << "Didn't understand version scheme (major), E2EE disabled" << version;
         return false;
     }
 
     ok = false;
     const auto minor = splittedVersion.size() > 1 ? splittedVersion.at(1).toInt(&ok) : 0;
     if (!ok) {
-        qCWarning(lcServerCapabilities) << "Didn't understand version scheme (minor), E2EE disabled";
+        qCWarning(lcServerCapabilities) << "Didn't understand version scheme (minor), E2EE disabled" << version;
         return false;
     }
 
-    return major == 1 && minor >= 1;
+    const auto capabilityAvailable = (major == 1 && minor >= 1);
+    if (!capabilityAvailable) {
+        qCInfo(lcServerCapabilities) << "Incompatible E2EE API version:" << version;
+    }
+    return capabilityAvailable;
 }
 
 bool Capabilities::notificationsAvailable() const

diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
@@ -35,7 +35,7 @@
 
 namespace OCC {
 
-Q_LOGGING_CATEGORY(lcDisco, "sync.discovery", QtInfoMsg)
+Q_LOGGING_CATEGORY(lcDisco, "nextcloud.sync.discovery", QtInfoMsg)
 
 void ProcessDirectoryJob::start()
 {
@@ -615,24 +615,6 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
     // Unknown in db: new file on the server
     Q_ASSERT(!dbEntry.isValid());
 
-    if (!serverEntry.renameName.isEmpty()) {
-        item->_renameTarget = _dirItem ? _dirItem->_file + "/" + serverEntry.renameName : serverEntry.renameName;
-        item->_originalFile = path._original;
-        item->_modtime = serverEntry.modtime;
-        item->_size = serverEntry.size;
-        item->_instruction = CSYNC_INSTRUCTION_RENAME;
-        item->_direction = SyncFileItem::Up;
-        item->_fileId = serverEntry.fileId;
-        item->_remotePerm = serverEntry.remotePerm;
-        item->_isShared = serverEntry.remotePerm.hasPermission(RemotePermissions::IsShared);
-        item->_lastShareStateFetchedTimestmap = QDateTime::currentMSecsSinceEpoch();
-        item->_etag = serverEntry.etag;
-        item->_type = serverEntry.isDirectory ? CSyncEnums::ItemTypeDirectory : CSyncEnums::ItemTypeFile;
-
-        processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer);
-        return;
-    }
-
     item->_instruction = CSYNC_INSTRUCTION_NEW;
     item->_direction = SyncFileItem::Down;
     item->_modtime = serverEntry.modtime;
@@ -881,41 +863,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer);
     };
 
-    auto handleInvalidSpaceRename = [&] (SyncFileItem::Direction direction) {
-        if (_dirItem) {
-            path._target = _dirItem->_file + "/" + localEntry.renameName;
-        } else {
-            path._target = localEntry.renameName;
-        }
-        OCC::SyncJournalFileRecord base;
-        if (!_discoveryData->_statedb->getFileRecordByInode(localEntry.inode, &base)) {
-            dbError();
-            return;
-        }
-        const auto originalPath = base.path();
-        const auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down);
-        _discoveryData->_renamedItemsLocal.insert(originalPath, path._target);
-        item->_renameTarget = path._target;
-        path._server = adjustedOriginalPath;
-        if (_dirItem) {
-            item->_file = _dirItem->_file + "/" + localEntry.name;
-        } else {
-            item->_file = localEntry.name;
-        }
-        path._original = originalPath;
-        item->_originalFile = path._original;
-        item->_modtime = base.isValid() ? base._modtime : localEntry.modtime;
-        item->_inode = base.isValid() ? base._inode : localEntry.inode;
-        item->_instruction = CSYNC_INSTRUCTION_RENAME;
-        item->_direction = direction;
-        item->_fileId = base.isValid() ? base._fileId : QByteArray{};
-        item->_remotePerm = base.isValid() ? base._remotePerm : RemotePermissions{};
-        item->_etag = base.isValid() ? base._etag : QByteArray{};
-        item->_type = base.isValid() ? base._type : localEntry.type;
-        item->_isShared = base.isValid() ? base._isShared : false;
-        item->_lastShareStateFetchedTimestmap = base.isValid() ? base._lastShareStateFetchedTimestmap : 0;
-    };
-
     if (!localEntry.isValid()) {
         if (_queryLocal == ParentNotChanged && dbEntry.isValid()) {
             // Not modified locally (ParentNotChanged)
@@ -1000,8 +947,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
                     || _discoveryData->_syncOptions._vfs->needsMetadataUpdate(*item))) {
                 item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
                 item->_direction = SyncFileItem::Down;
-            } else if (!localEntry.renameName.isEmpty()) {
-                handleInvalidSpaceRename(SyncFileItem::Up);
             }
         } else if (!typeChange && isVfsWithSuffix()
             && dbEntry.isVirtualFile() && !localEntry.isVirtualFile
@@ -1097,16 +1042,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
         return;
     }
 
-    if (!localEntry.renameName.isEmpty()) {
-        handleInvalidSpaceRename(SyncFileItem::Down);
-        item->_instruction = CSYNC_INSTRUCTION_NEW;
-        item->_direction = SyncFileItem::Up;
-        item->_originalFile = item->_file;
-        item->_file = item->_renameTarget;
-        finalize();
-        return;
-    }
-
     // New local file or rename
     item->_instruction = CSYNC_INSTRUCTION_NEW;
     item->_direction = SyncFileItem::Up;

diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h
@@ -49,7 +49,6 @@ struct RemoteInfo
 {
     /** FileName of the entry (this does not contains any directory or path, just the plain name */
     QString name;
-    QString renameName;
     QByteArray etag;
     QByteArray fileId;
     QByteArray checksumHeader;
@@ -79,7 +78,6 @@ struct LocalInfo
 {
     /** FileName of the entry (this does not contains any directory or path, just the plain name */
     QString name;
-    QString renameName;
     time_t modtime = 0;
     int64_t size = 0;
     uint64_t inode = 0;

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
@@ -594,7 +594,7 @@ void OwncloudPropagator::start(SyncFileItemVector &&items)
     }
 
     foreach (PropagatorJob *it, directoriesToRemove) {
-        _rootJob->_dirDeletionJobs.appendJob(it);
+        _rootJob->appendDirDeletionJob(it);
     }
 
     connect(_rootJob.data(), &PropagatorJob::finished, this, &OwncloudPropagator::emitFinished);
@@ -1292,6 +1292,11 @@ qint64 PropagateRootDirectory::committedDiskSpace() const
     return _subJobs.committedDiskSpace() + _dirDeletionJobs.committedDiskSpace();
 }
 
+void PropagateRootDirectory::appendDirDeletionJob(PropagatorJob *job)
+{
+    _dirDeletionJobs.appendJob(job);
+}
+
 bool PropagateRootDirectory::scheduleSelfOrChild()
 {
     qCInfo(lcRootDirectory()) << "scheduleSelfOrChild" << _state << "pending uploads" << propagator()->delayedTasks().size() << "subjobs state" << _subJobs._state;
@@ -1327,6 +1332,7 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status)
 
     if (status != SyncFileItem::Success
         && status != SyncFileItem::Restoration
+        && status != SyncFileItem::BlacklistedError
         && status != SyncFileItem::Conflict) {
         if (_state != Finished) {
             // Synchronously abort
@@ -1338,11 +1344,37 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status)
         return;
     }
 
+    if (_errorStatus == SyncFileItem::NoStatus) {
+        switch (status) {
+        case SyncFileItem::NoStatus:
+        case SyncFileItem::FatalError:
+        case SyncFileItem::NormalError:
+        case SyncFileItem::SoftError:
+        case SyncFileItem::Conflict:
+        case SyncFileItem::FileIgnored:
+        case SyncFileItem::FileLocked:
+        case SyncFileItem::Restoration:
+        case SyncFileItem::FileNameInvalid:
+        case SyncFileItem::FileNameClash:
+        case SyncFileItem::DetailError:
+        case SyncFileItem::Success:
+            break;
+        case SyncFileItem::BlacklistedError:
+            _errorStatus = SyncFileItem::BlacklistedError;
+            break;
+        }
+    }
+
     propagator()->scheduleNextJob();
 }
 
 void PropagateRootDirectory::slotDirDeletionJobsFinished(SyncFileItem::Status status)
 {
+    if (_errorStatus != SyncFileItem::NoStatus && status == SyncFileItem::Success) {
+        qCInfo(lcPropagator) << "PropagateRootDirectory::slotDirDeletionJobsFinished" << "reporting previous error" << _errorStatus;
+        status = _errorStatus;
+    }
+
     _state = Finished;
     qCInfo(lcPropagator) << "PropagateRootDirectory::slotDirDeletionJobsFinished" << "emit finished" << status;
     emit finished(status);

diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h
@@ -365,8 +365,6 @@ class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory
 {
     Q_OBJECT
 public:
-    PropagatorCompositeJob _dirDeletionJobs;
-
     explicit PropagateRootDirectory(OwncloudPropagator *propagator);
 
     bool scheduleSelfOrChild() override;
@@ -375,13 +373,20 @@ class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory
 
     [[nodiscard]] qint64 committedDiskSpace() const override;
 
+public slots:
+    void appendDirDeletionJob(PropagatorJob *job);
+
 private slots:
     void slotSubJobsFinished(SyncFileItem::Status status) override;
     void slotDirDeletionJobsFinished(SyncFileItem::Status status);
 
 private:
 
     bool scheduleDelayedJobs();
+
+    PropagatorCompositeJob _dirDeletionJobs;
+
+    SyncFileItem::Status _errorStatus = SyncFileItem::Status::NoStatus;
 };
 
 /**

diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
@@ -890,6 +890,29 @@ private slots:
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
 
+    void testRenameParallelismWithBlacklist()
+    {
+        constexpr auto testFileName = "blackListFile";
+        FakeFolder fakeFolder{ FileInfo{} };
+        fakeFolder.remoteModifier().mkdir("A");
+        fakeFolder.remoteModifier().insert("A/file");
+
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        fakeFolder.remoteModifier().insert(testFileName);
+        fakeFolder.serverErrorPaths().append(testFileName, 500); // will be blacklisted
+        QVERIFY(!fakeFolder.syncOnce());
+
+        fakeFolder.remoteModifier().mkdir("B");
+        fakeFolder.remoteModifier().rename("A/file", "B/file");
+        fakeFolder.remoteModifier().remove("A");
+
+        QVERIFY(!fakeFolder.syncOnce());
+        auto folderA = fakeFolder.currentLocalState().find("A");
+        QCOMPARE(folderA, nullptr);
+    }
+
     void testMovedWithError_data()
     {
         QTest::addColumn<Vfs::Mode>("vfsMode");