Merge pull request #395 from Infomaniak/KDESKTOP-1384-File-are-still-…

…blacklisted Kdesktop 1384 file are still blacklisted
Infomaniak · Nov 18, 2024 · da2d5a8 · da2d5a8
2 parents 0059a03 + 407bfd8
commit da2d5a8
Show file tree

Hide file tree

Showing 10 changed files with 126 additions and 20 deletions.
diff --git a/src/libcommon/utility/types.cpp b/src/libcommon/utility/types.cpp
@@ -70,6 +70,8 @@ std::string toString(const OperationType e) {
             return "Delete";
         case OperationType::Rights:
             return "Rights";
+        case OperationType::MoveOut:
+            return "MoveOut";
         default:
             return noConversionStr;
     }

diff --git a/src/libcommon/utility/types.h b/src/libcommon/utility/types.h
@@ -168,7 +168,7 @@ enum class NodeType {
 };
 std::string toString(NodeType e);
 
-enum class OperationType { None = 0x00, Create = 0x01, Move = 0x02, Edit = 0x04, Delete = 0x08, Rights = 0x10 };
+enum class OperationType { None = 0x00, Create = 0x01, Move = 0x02, Edit = 0x04, Delete = 0x08, Rights = 0x10, MoveOut = 0x20 };
 std::string toString(OperationType e);
 
 enum class ExitCode {

diff --git a/src/libsyncengine/update_detection/file_system_observer/folderwatcher_win.cpp b/src/libsyncengine/update_detection/file_system_observer/folderwatcher_win.cpp
@@ -118,8 +118,7 @@ void FolderWatcher_win::watchChanges() {
         _ready = true;
 
         HANDLE handles[] = {_resultEventHandle, _stopEventHandle};
-        DWORD result = WaitForMultipleObjects(2, handles, false // awake once one of them arrives
-                                              ,
+        DWORD result = WaitForMultipleObjects(2, handles, false, // awake once one of them arrives
                                               INFINITE);
 
         if (result == 1) {
@@ -180,6 +179,8 @@ void FolderWatcher_win::watchChanges() {
                                                       << L" detected on item with " << Utility::formatSyncPath(longfilepath));
                 }
 
+                if (opType == OperationType::MoveOut) opType = OperationType::Move; // "MoveOut" is considered as Move from now on
+
                 changeDetected(longfilepath, opType);
             }
 
@@ -213,20 +214,15 @@ void FolderWatcher_win::closeHandle() {
 OperationType FolderWatcher_win::operationFromAction(DWORD action) {
     switch (action) {
         case FILE_ACTION_RENAMED_OLD_NAME:
-            return OperationType::Move;
-            break;
+            return OperationType::MoveOut;
         case FILE_ACTION_RENAMED_NEW_NAME:
             return OperationType::Move;
-            break;
         case FILE_ACTION_ADDED:
             return OperationType::Create;
-            break;
         case FILE_ACTION_REMOVED:
             return OperationType::Delete;
-            break;
         case FILE_ACTION_MODIFIED:
             return OperationType::Edit;
-            break;
     }
 
     return OperationType::None;

diff --git a/src/libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker.h b/src/libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker.h
@@ -32,7 +32,7 @@ class LocalFileSystemObserverWorker : public FileSystemObserverWorker {
         void start() override;
         void stop() override;
 
-        void changesDetected(const std::list<std::pair<std::filesystem::path, OperationType>> &changes);
+        virtual void changesDetected(const std::list<std::pair<std::filesystem::path, OperationType>> &changes);
         virtual void forceUpdate() override;
 
     protected:

diff --git a/src/libsyncengine/update_detection/file_system_observer/snapshot/snapshot.cpp b/src/libsyncengine/update_detection/file_system_observer/snapshot/snapshot.cpp
@@ -77,6 +77,28 @@ bool Snapshot::updateItem(const SnapshotItem &newItem) {
         return false;
     }
 
+    // Check if `newItem` already exists with the same path but a different Id
+    if (auto itNewParent = _items.find(newItem.parentId()); itNewParent != _items.end()) {
+        for (const NodeId &childId: itNewParent->second.childrenIds()) {
+            auto child = _items.find(childId);
+            if (child == _items.end()) {
+                assert(false && "Child not found in snapshot");
+                LOG_WARN(Log::instance()->getLogger(), "Child " << childId.c_str() << " not found in snapshot");
+                continue;
+            }
+
+            if (child->second.name() == newItem.name() && child->second.id() != newItem.id()) {
+                LOGW_DEBUG(Log::instance()->getLogger(),
+                           L"Item: " << SyncName2WStr(newItem.name()) << L" (" << Utility::s2ws(newItem.id())
+                                     << L") already exists in parent: " << Utility::s2ws(newItem.parentId())
+                                     << L" with a different id. Removing it and adding the new one.");
+                removeItem(childId);
+                break; // There should be (at most) only one item with the same name in a folder 
+            }
+        }
+
+    }
+
     const SnapshotItem &prevItem = _items[newItem.id()];
 
     // Update parent's children lists
@@ -118,7 +140,7 @@ bool Snapshot::updateItem(const SnapshotItem &newItem) {
     return true;
 }
 
-bool Snapshot::removeItem(const NodeId &id) {
+bool Snapshot::removeItem(const NodeId id) {
     const std::scoped_lock lock(_mutex);
 
     if (id.empty()) {

diff --git a/src/libsyncengine/update_detection/file_system_observer/snapshot/snapshot.h b/src/libsyncengine/update_detection/file_system_observer/snapshot/snapshot.h
@@ -40,7 +40,7 @@ class Snapshot : public SharedObject {
         void init();
 
         bool updateItem(const SnapshotItem &newItem);
-        bool removeItem(const NodeId &id);
+        bool removeItem(const NodeId id); // Do not pass by reference to avoid dangling references
 
         NodeId itemId(const SyncPath &path) const;
         NodeId parentId(const NodeId &itemId) const;

diff --git a/...libsyncengine/update_detection/file_system_observer/testlocalfilesystemobserverworker.cpp b/...libsyncengine/update_detection/file_system_observer/testlocalfilesystemobserverworker.cpp
@@ -408,19 +408,53 @@ void TestLocalFileSystemObserverWorker::testLFSOWithSpecialCases2() {
     CPPUNIT_ASSERT(_syncPal->snapshot(ReplicaSide::Local)->name(initItemId) == testFilename);
 }
 
-void TestLocalFileSystemObserverWorker::testLFSOFastMoveDelete() {
+void TestLocalFileSystemObserverWorker::testLFSOFastMoveDeleteMove() { // MS Office test
     LOGW_DEBUG(_logger, L"***** Test fast move/delete *****");
+    _syncPal->_localFSObserverWorker->stop();
+    _syncPal->_localFSObserverWorker.reset();
+
+    // Create a slow observer
+    auto slowObserver = std::make_shared<MockLocalFileSystemObserverWorker>(_syncPal, "Local File System Observer", "LFSO");
+    _syncPal->_localFSObserverWorker = slowObserver;
+    _syncPal->_localFSObserverWorker->start();
+
+    int count = 0;
+    while (!_syncPal->snapshot(ReplicaSide::Local)->isValid()) { // Wait for the snapshot generation
+        Utility::msleep(100);
+        CPPUNIT_ASSERT(count++ < 20); // Do not wait more than 2s
+    }
+    CPPUNIT_ASSERT(_syncPal->snapshot(ReplicaSide::Local)->exists(_testFiles[0].first));
 
     IoError ioError = IoError::Unknown;
     SyncPath destinationPath = _testFiles[0].second.parent_path() / (_testFiles[0].second.filename().string() + "2");
-    CPPUNIT_ASSERT(IoHelper::renameItem(_testFiles[0].second, destinationPath, ioError));
+    CPPUNIT_ASSERT(IoHelper::renameItem(_testFiles[0].second, destinationPath, ioError)); // test0.txt -> test0.txt2
     CPPUNIT_ASSERT_EQUAL(IoError::Success, ioError);
-    CPPUNIT_ASSERT(IoHelper::deleteItem(destinationPath, ioError));
+    CPPUNIT_ASSERT(IoHelper::deleteItem(destinationPath, ioError)); // Delete test0.txt2 (before the previous rename is processed)
+    CPPUNIT_ASSERT_EQUAL(IoError::Success, ioError);
+    CPPUNIT_ASSERT(IoHelper::renameItem(_testFiles[1].second, _testFiles[0].second,
+                                        ioError)); // test1.txt -> test0.txt (before the previous rename and delete is processed)
     CPPUNIT_ASSERT_EQUAL(IoError::Success, ioError);
 
-    Utility::msleep(1000); // Wait 1sec
+    CPPUNIT_ASSERT_MESSAGE("No update detected in the expected time.", slowObserver->waitForUpdate());
+
+    FileStat fileStat;
+    CPPUNIT_ASSERT(IoHelper::getFileStat(_testFiles[0].second, &fileStat, ioError));
+    CPPUNIT_ASSERT_EQUAL(IoError::Success, ioError);
 
     CPPUNIT_ASSERT(!_syncPal->snapshot(ReplicaSide::Local)->exists(_testFiles[0].first));
+    CPPUNIT_ASSERT(_syncPal->snapshot(ReplicaSide::Local)->exists(std::to_string(fileStat.inode)));
+}
+
+bool MockLocalFileSystemObserverWorker::waitForUpdate(uint64_t timeoutMs) const {
+    using namespace std::chrono;
+    auto start = system_clock::now();
+    while (!_updating && duration_cast<milliseconds>(system_clock::now() - start).count() < timeoutMs) {
+        Utility::msleep(10);
+    }
+    while (_updating && duration_cast<milliseconds>(system_clock::now() - start).count() < timeoutMs) {
+        Utility::msleep(10);
+    }
+    return duration_cast<milliseconds>(system_clock::now() - start).count() < timeoutMs;
 }
 
 } // namespace KDC
diff --git a/test/libsyncengine/update_detection/file_system_observer/testlocalfilesystemobserverworker.h b/test/libsyncengine/update_detection/file_system_observer/testlocalfilesystemobserverworker.h
@@ -20,16 +20,47 @@
 
 #include "testincludes.h"
 #include "test_utility/localtemporarydirectory.h"
-
 #include "syncpal/syncpal.h"
-
+#if defined(_WIN32)
+#include "libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker_win.h"
+#else
+#include "libsyncengine/update_detection/file_system_observer/localfilesystemobserverworker_unix.h"
+#endif
 #include <log4cplus/logger.h>
 
 using namespace CppUnit;
 
 namespace KDC {
 
 class LocalFileSystemObserverWorker;
+#if defined(_WIN32)
+class MockLocalFileSystemObserverWorker : public LocalFileSystemObserverWorker_win {
+    public:
+        MockLocalFileSystemObserverWorker(std::shared_ptr<SyncPal> syncPal, const std::string &name,
+                                          const std::string &shortName) :
+            LocalFileSystemObserverWorker_win(syncPal, name, shortName) {}
+
+        void changesDetected(const std::list<std::pair<std::filesystem::path, OperationType>> &changes) final {
+            Utility::msleep(200);
+            LocalFileSystemObserverWorker_win::changesDetected(changes);
+        }
+
+        bool waitForUpdate(uint64_t timeoutMs = 100000) const;
+};
+#else
+class MockLocalFileSystemObserverWorker : public LocalFileSystemObserverWorker_unix {
+    public:
+        MockLocalFileSystemObserverWorker(std::shared_ptr<SyncPal> syncPal, const std::string &name,
+                                          const std::string &shortName) :
+            LocalFileSystemObserverWorker_unix(syncPal, name, shortName) {}
+
+        void changesDetected(const std::list<std::pair<std::filesystem::path, OperationType>> &changes) final {
+            Utility::msleep(200);
+            LocalFileSystemObserverWorker_unix::changesDetected(changes);
+        }
+        bool waitForUpdate(uint64_t timeoutMs = 100000) const;
+};
+#endif
 
 class TestLocalFileSystemObserverWorker : public CppUnit::TestFixture {
         CPPUNIT_TEST_SUITE(TestLocalFileSystemObserverWorker);
@@ -38,7 +69,7 @@ class TestLocalFileSystemObserverWorker : public CppUnit::TestFixture {
         CPPUNIT_TEST(testLFSOWithDuplicateFileNames);
         CPPUNIT_TEST(testLFSODeleteDir);
         CPPUNIT_TEST(testLFSOWithDirs);
-        CPPUNIT_TEST(testLFSOFastMoveDelete);
+        CPPUNIT_TEST(testLFSOFastMoveDeleteMove);
         CPPUNIT_TEST(testLFSOWithSpecialCases1);
         CPPUNIT_TEST(testLFSOWithSpecialCases2);
         CPPUNIT_TEST_SUITE_END();
@@ -61,7 +92,7 @@ class TestLocalFileSystemObserverWorker : public CppUnit::TestFixture {
         void testLFSOWithDuplicateFileNames();
         void testLFSOWithDirs();
         void testLFSODeleteDir();
-        void testLFSOFastMoveDelete();
+        void testLFSOFastMoveDeleteMove();
         void testLFSOWithSpecialCases1();
         void testLFSOWithSpecialCases2();
 };

diff --git a/test/libsyncengine/update_detection/file_system_observer/testsnapshot.cpp b/test/libsyncengine/update_detection/file_system_observer/testsnapshot.cpp
@@ -117,6 +117,25 @@ void TestSnapshot::testSnapshot() {
     CPPUNIT_ASSERT_EQUAL(static_cast<uint64_t>(1), snapshot.nbItems());
 }
 
+void TestSnapshot::testDuplicatedItem() {
+    const NodeId rootNodeId = *SyncDb::driveRootNode().nodeIdLocal();
+
+    const DbNode dummyRootNode(0, std::nullopt, Str("Local Drive"), SyncName(), "1", "1", std::nullopt, std::nullopt,
+                               std::nullopt, NodeType::Directory, 0, std::nullopt);
+    Snapshot snapshot(ReplicaSide::Local, dummyRootNode);
+
+    const SnapshotItem file1("A", rootNodeId, Str("file1"), 1640995201, -1640995201, NodeType::File, 123,
+                               false, true, true);
+    const SnapshotItem file2("B", rootNodeId, Str("file1"), 1640995201, -1640995201, NodeType::File, 123,
+                               false, true, true);
+
+    snapshot.updateItem(file1);
+    snapshot.updateItem(file2);
+
+    CPPUNIT_ASSERT(!snapshot.exists("A"));
+    CPPUNIT_ASSERT(snapshot.exists("B"));
+}
+
 void TestSnapshot::testSnapshotInsertionWithDifferentEncodings() {
     const NodeId rootNodeId = *SyncDb::driveRootNode().nodeIdLocal();
 

diff --git a/test/libsyncengine/update_detection/file_system_observer/testsnapshot.h b/test/libsyncengine/update_detection/file_system_observer/testsnapshot.h
@@ -28,6 +28,7 @@ namespace KDC {
 class TestSnapshot : public CppUnit::TestFixture {
         CPPUNIT_TEST_SUITE(TestSnapshot);
         CPPUNIT_TEST(testSnapshot);
+        CPPUNIT_TEST(testDuplicatedItem);
         CPPUNIT_TEST(testSnapshotInsertionWithDifferentEncodings);
         CPPUNIT_TEST_SUITE_END();
 
@@ -37,6 +38,7 @@ class TestSnapshot : public CppUnit::TestFixture {
 
     private:
         void testSnapshot();
+        void testDuplicatedItem();
         void testSnapshotInsertionWithDifferentEncodings();
 };