nextcloud · mgallien · Oct 11, 2022 · Oct 7, 2022 · Oct 7, 2022 · Oct 7, 2022
diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp
@@ -345,7 +345,7 @@ void ProcessDirectoryJob::processFile(PathTuple path,
 {
     const char *hasServer = serverEntry.isValid() ? "true" : _queryServer == ParentNotChanged ? "db" : "false";
     const char *hasLocal = localEntry.isValid() ? "true" : _queryLocal == ParentNotChanged ? "db" : "false";
-    const auto serverFileIsLocked = serverEntry.locked == SyncFileItem::LockStatus::LockedItem ? "locked" : "not locked";
+    const auto serverFileIsLocked = (serverEntry.isValid() ? (serverEntry.locked == SyncFileItem::LockStatus::LockedItem ? "locked" : "not locked")  : "");
     const auto localFileIsLocked = dbEntry._lockstate._locked ? "locked" : "not locked";
     qCInfo(lcDisco).nospace() << "Processing " << path._original
                               << " | (db/local/remote)"
@@ -394,29 +394,6 @@ void ProcessDirectoryJob::processFile(PathTuple path,
     if (item->_type == ItemTypeVirtualFileDehydration)
         item->_type = ItemTypeFile;
 
-    // We want to check the lock state of this file after the lock time has expired
-    if(serverEntry.locked == SyncFileItem::LockStatus::LockedItem) {
-        const auto lockExpirationTime = serverEntry.lockTime + serverEntry.lockTimeout;
-        const auto timeRemaining = QDateTime::currentDateTime().secsTo(QDateTime::fromSecsSinceEpoch(lockExpirationTime));
-        // Add on a second as a precaution, sometimes we catch the server before it has had a chance to update
-        const auto lockExpirationTimeout = qMax(5LL, timeRemaining + 1);
-
-        qCInfo(lcDisco) << "File:" << path._original << "is locked."
-                        << "Lock expires in:" << lockExpirationTimeout << "seconds."
-                        << "A sync run will be scheduled for around that time.";
-
-        _discoveryData->_anotherSyncNeeded = true;
-        _discoveryData->_filesNeedingScheduledSync.insert(path._original, lockExpirationTimeout);
-
-    } else if (serverEntry.locked == SyncFileItem::LockStatus::UnlockedItem && dbEntry._lockstate._locked) {
-        // We have received data that this file has been unlocked remotely, so let's notify the sync engine
-        // that we no longer need a scheduled sync run for this file
-        qCInfo(lcDisco) << "File:" << path._original << "is unlocked and a scheduled sync is no longer needed."
-                        << "Will remove scheduled sync if there is one.";
-
-        _discoveryData->_filesUnscheduleSync.append(path._original);
-    }
-
     // VFS suffixed files on the server are ignored
     if (isVfsWithSuffix()) {
         if (hasVirtualFileSuffix(serverEntry.name)
@@ -523,14 +500,28 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
         }
     }
 
-    // We need to make sure that we update the info in the database if the lockstate has changed
-    const auto checkFileLockState = [&item, &dbEntry, &serverEntry] {
-        const bool isServerEntryLocked = serverEntry.locked == SyncFileItem::LockStatus::LockedItem;
+    // We want to check the lock state of this file after the lock time has expired
+    if(serverEntry.locked == SyncFileItem::LockStatus::LockedItem) {
+        const auto lockExpirationTime = serverEntry.lockTime + serverEntry.lockTimeout;
+        const auto timeRemaining = QDateTime::currentDateTime().secsTo(QDateTime::fromSecsSinceEpoch(lockExpirationTime));
+        // Add on a second as a precaution, sometimes we catch the server before it has had a chance to update
+        const auto lockExpirationTimeout = qMax(5LL, timeRemaining + 1);
 
-        if(isServerEntryLocked != dbEntry._lockstate._locked) {
-            item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA;
-        }
-    };
+        qCInfo(lcDisco) << "File:" << path._original << "is locked."
+                        << "Lock expires in:" << lockExpirationTimeout << "seconds."
+                        << "A sync run will be scheduled for around that time.";
+
+        _discoveryData->_anotherSyncNeeded = true;
+        _discoveryData->_filesNeedingScheduledSync.insert(path._original, lockExpirationTimeout);
+
+    } else if (serverEntry.locked == SyncFileItem::LockStatus::UnlockedItem && dbEntry._lockstate._locked) {
+        // We have received data that this file has been unlocked remotely, so let's notify the sync engine
+        // that we no longer need a scheduled sync run for this file
+        qCInfo(lcDisco) << "File:" << path._original << "is unlocked and a scheduled sync is no longer needed."
+                        << "Will remove scheduled sync if there is one.";
+
+        _discoveryData->_filesUnscheduleSync.append(path._original);
+    }
 
     // The file is known in the db already
     if (dbEntry.isValid()) {
@@ -613,12 +604,10 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
                 return ParentNotChanged;
             }();
 
-            checkFileLockState();
             processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, serverQueryMode);
             return;
         }
 
-        checkFileLockState();
         processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer);
         return;
     }

diff --git a/test/syncenginetestutils.cpp b/test/syncenginetestutils.cpp
@@ -103,6 +103,10 @@ void DiskFileModifier::setModTime(const QString &relativePath, const QDateTime &
     OCC::FileSystem::setModTime(_rootDir.filePath(relativePath), OCC::Utility::qDateTimeToTime_t(modTime));
 }
 
+void DiskFileModifier::modifyLockState([[maybe_unused]] const QString &relativePath, [[maybe_unused]] LockState lockState, [[maybe_unused]] int lockType, [[maybe_unused]] const QString &lockOwner, [[maybe_unused]] const QString &lockOwnerId, [[maybe_unused]] const QString &lockEditorId, [[maybe_unused]] quint64 lockTime, [[maybe_unused]] quint64 lockTimeout)
+{
+}
+
 FileInfo FileInfo::A12_B12_C12_S12()
 {
     FileInfo fi { QString {}, {
@@ -195,6 +199,19 @@ void FileInfo::setModTimeKeepEtag(const QString &relativePath, const QDateTime &
     file->lastModified = modTime;
 }
 
+void FileInfo::modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout)
+{
+    FileInfo *file = findInvalidatingEtags(relativePath);
+    Q_ASSERT(file);
+    file->lockState = lockState;
+    file->lockType = lockType;
+    file->lockOwner = lockOwner;
+    file->lockOwnerId = lockOwnerId;
+    file->lockEditorId = lockEditorId;
+    file->lockTime = lockTime;
+    file->lockTimeout = lockTimeout;
+}
+
 FileInfo *FileInfo::find(PathComponents pathComponents, const bool invalidateEtags)
 {
     if (pathComponents.isEmpty()) {
@@ -334,6 +351,13 @@ FakePropfindReply::FakePropfindReply(FileInfo &remoteRootFileInfo, QNetworkAcces
         xml.writeTextElement(ocUri, QStringLiteral("permissions"), !fileInfo.permissions.isNull() ? QString(fileInfo.permissions.toString()) : fileInfo.isShared ? QStringLiteral("SRDNVCKW") : QStringLiteral("RDNVCKW"));
         xml.writeTextElement(ocUri, QStringLiteral("id"), QString::fromUtf8(fileInfo.fileId));
         xml.writeTextElement(ocUri, QStringLiteral("checksums"), QString::fromUtf8(fileInfo.checksums));
+        xml.writeTextElement(ncUri, QStringLiteral("lock-owner"), fileInfo.lockOwnerId);
+        xml.writeTextElement(ncUri, QStringLiteral("lock"), fileInfo.lockState == FileInfo::LockState::FileLocked ? QStringLiteral("1") : QStringLiteral("0"));
+        xml.writeTextElement(ncUri, QStringLiteral("lock-owner-type"), fileInfo.lockOwnerId);
+        xml.writeTextElement(ncUri, QStringLiteral("lock-owner-displayname"), fileInfo.lockOwnerId);
+        xml.writeTextElement(ncUri, QStringLiteral("lock-owner-editor"), fileInfo.lockOwnerId);
+        xml.writeTextElement(ncUri, QStringLiteral("lock-time"), QString::number(fileInfo.lockTime));
+        xml.writeTextElement(ncUri, QStringLiteral("lock-timeout"), QString::number(fileInfo.lockTimeout));
         buffer.write(fileInfo.extraDavProperties);
         xml.writeEndElement(); // prop
         xml.writeTextElement(davUri, QStringLiteral("status"), QStringLiteral("HTTP/1.1 200 OK"));

diff --git a/test/syncenginetestutils.h b/test/syncenginetestutils.h
@@ -76,6 +76,11 @@ class PathComponents : public QStringList {
 class FileModifier
 {
 public:
+    enum class LockState {
+        FileLocked,
+        FileUnlocked,
+    };
+
     virtual ~FileModifier() = default;
     virtual void remove(const QString &relativePath) = 0;
     virtual void insert(const QString &relativePath, qint64 size = 64, char contentChar = 'W') = 0;
@@ -84,6 +89,7 @@ class FileModifier
     virtual void mkdir(const QString &relativePath) = 0;
     virtual void rename(const QString &relativePath, const QString &relativeDestinationDirectory) = 0;
     virtual void setModTime(const QString &relativePath, const QDateTime &modTime) = 0;
+    virtual void modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout) = 0;
 };
 
 class DiskFileModifier : public FileModifier
@@ -99,6 +105,7 @@ class DiskFileModifier : public FileModifier
     void mkdir(const QString &relativePath) override;
     void rename(const QString &from, const QString &to) override;
     void setModTime(const QString &relativePath, const QDateTime &modTime) override;
+    void modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout) override;
 };
 
 class FileInfo : public FileModifier
@@ -130,6 +137,8 @@ class FileInfo : public FileModifier
 
     void setModTimeKeepEtag(const QString &relativePath, const QDateTime &modTime);
 
+    void modifyLockState(const QString &relativePath, LockState lockState, int lockType, const QString &lockOwner, const QString &lockOwnerId, const QString &lockEditorId, quint64 lockTime, quint64 lockTimeout) override;
+
     FileInfo *find(PathComponents pathComponents, const bool invalidateEtags = false);
 
     FileInfo *createDir(const QString &relativePath);
@@ -163,6 +172,13 @@ class FileInfo : public FileModifier
     QByteArray extraDavProperties;
     qint64 size = 0;
     char contentChar = 'W';
+    LockState lockState = LockState::FileUnlocked;
+    int lockType = 0;
+    QString lockOwner;
+    QString lockOwnerId;
+    QString lockEditorId;
+    quint64 lockTime = 0;
+    quint64 lockTimeout = 0;
 
     // Sorted by name to be able to compare trees
     QMap<QString, FileInfo> children;

diff --git a/test/testlocaldiscovery.cpp b/test/testlocaldiscovery.cpp
@@ -621,15 +621,7 @@ private slots:
 
         fakeFolder.remoteModifier().insert(fooFileRootFolder);
         fakeFolder.remoteModifier().insert(barFileRootFolder);
-
-        const auto lockedFileDavProps = QByteArray("<nc:lock>1</nc:lock>"
-                                                   "<nc:lock-owner-type>0</nc:lock-owner-type>"
-                                                   "<nc:lock-owner>user1</nc:lock-owner>"
-                                                   "<nc:lock-owner-displayname>user1</nc:lock-owner-displayname>"
-                                                   "<nc:lock-owner-editor>user1</nc:lock-owner-editor>"
-                                                   "<nc:lock-time>1648046707</nc:lock-time>");
-
-        fakeFolder.remoteModifier().find("bar")->extraDavProperties = lockedFileDavProps;
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("bar"), FileInfo::LockState::FileLocked, 0, QStringLiteral("user1"), {}, QStringLiteral("user1"), 1648046707, 0);
 
         fakeFolder.remoteModifier().mkdir(QStringLiteral("subfolder"));
         fakeFolder.remoteModifier().insert(fooFileSubFolder);
@@ -648,8 +640,7 @@ private slots:
         QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("bar"), &fileRecordBefore));
         QVERIFY(fileRecordBefore._lockstate._locked);
 
-        const auto unlockedFileDavProps = QByteArray("<nc:lock>0</nc:lock>");
-        fakeFolder.remoteModifier().find("bar")->extraDavProperties = unlockedFileDavProps;
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("bar"), FileInfo::LockState::FileUnlocked, {}, {}, {}, {}, {}, {});
 
         fakeFolder.syncEngine().setLocalDiscoveryOptions(LocalDiscoveryStyle::DatabaseAndFilesystem);
 

diff --git a/test/testlockfile.cpp b/test/testlockfile.cpp
@@ -5,6 +5,7 @@
 #include "common/syncjournaldb.h"
 #include "common/syncjournalfilerecord.h"
 #include "syncenginetestutils.h"
+#include "localdiscoverytracker.h"
 
 #include <QTest>
 #include <QSignalSpy>
@@ -482,6 +483,154 @@ private slots:
         QVERIFY(jobFailure.wait());
         QCOMPARE(jobSuccess.count(), 0);
     }
+
+    void testSyncLockedFilesAlmostExpired()
+    {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        QSignalSpy spySyncCompleted(&fakeFolder.syncEngine(), &OCC::SyncEngine::finished);
+
+        ItemCompletedSpy completeSpy(fakeFolder);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(completeSpy.findItem(QStringLiteral("A/a1"))->_locked, OCC::SyncFileItem::LockStatus::UnlockedItem);
+        OCC::SyncJournalFileRecord fileRecordBefore;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordBefore));
+        QVERIFY(!fileRecordBefore._lockstate._locked);
+
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("A/a1"), FileModifier::LockState::FileLocked, 1, QStringLiteral("Nextcloud Office"), {}, QStringLiteral("richdocuments"), QDateTime::currentDateTime().toSecsSinceEpoch() - 1220, 1226);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(completeSpy.findItem(QStringLiteral("A/a1"))->_locked, OCC::SyncFileItem::LockStatus::LockedItem);
+        OCC::SyncJournalFileRecord fileRecordLocked;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordLocked));
+        QVERIFY(fileRecordLocked._lockstate._locked);
+
+        spySyncCompleted.clear();
+
+        QTest::qWait(5000);
+
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("A/a1"), FileModifier::LockState::FileUnlocked, {}, {}, {}, {}, {}, {});
+
+        QCOMPARE(spySyncCompleted.count(), 0);
+        QVERIFY(spySyncCompleted.wait(3000));
+        QCOMPARE(spySyncCompleted.count(), 1);
+
+        OCC::SyncJournalFileRecord fileRecordUnlocked;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordUnlocked));
+        QVERIFY(!fileRecordUnlocked._lockstate._locked);
+    }
+
+    void testSyncLockedFilesNoExpiredLockedFiles()
+    {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        QSignalSpy spySyncCompleted(&fakeFolder.syncEngine(), &OCC::SyncEngine::finished);
+
+        ItemCompletedSpy completeSpy(fakeFolder);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(completeSpy.findItem(QStringLiteral("A/a1"))->_locked, OCC::SyncFileItem::LockStatus::UnlockedItem);
+        OCC::SyncJournalFileRecord fileRecordBefore;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordBefore));
+        QVERIFY(!fileRecordBefore._lockstate._locked);
+
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("A/a1"), FileModifier::LockState::FileLocked, 1, QStringLiteral("Nextcloud Office"), {}, QStringLiteral("richdocuments"), QDateTime::currentDateTime().toSecsSinceEpoch() - 1220, 1226);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(completeSpy.findItem(QStringLiteral("A/a1"))->_locked, OCC::SyncFileItem::LockStatus::LockedItem);
+        OCC::SyncJournalFileRecord fileRecordLocked;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordLocked));
+        QVERIFY(fileRecordLocked._lockstate._locked);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        spySyncCompleted.clear();
+
+        QTest::qWait(1000);
+
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("A/a1"), FileModifier::LockState::FileUnlocked, {}, {}, {}, {}, {}, {});
+
+        QCOMPARE(spySyncCompleted.count(), 0);
+        QVERIFY(!spySyncCompleted.wait(3000));
+        QCOMPARE(spySyncCompleted.count(), 0);
+
+        OCC::SyncJournalFileRecord fileRecordUnlocked;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordUnlocked));
+        QVERIFY(fileRecordUnlocked._lockstate._locked);
+    }
+
+    void testSyncLockedFiles()
+    {
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        int nGET = 0, nPUT = 0;
+        QObject parent;
+        fakeFolder.setServerOverride([&](QNetworkAccessManager::Operation op, const QNetworkRequest &request, QIODevice *outgoingData) -> QNetworkReply * {
+            Q_UNUSED(outgoingData)
+            Q_UNUSED(request)
+
+            if (op == QNetworkAccessManager::PutOperation) {
+                ++nPUT;
+            } else if (op == QNetworkAccessManager::GetOperation) {
+                ++nGET;
+            }
+
+            return nullptr;
+        });
+
+        ItemCompletedSpy completeSpy(fakeFolder);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+        QCOMPARE(nGET, 0);
+        QCOMPARE(nPUT, 0);
+
+        QCOMPARE(completeSpy.findItem(QStringLiteral("A/a1"))->_locked, OCC::SyncFileItem::LockStatus::UnlockedItem);
+        OCC::SyncJournalFileRecord fileRecordBefore;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordBefore));
+        QVERIFY(!fileRecordBefore._lockstate._locked);
+
+        fakeFolder.remoteModifier().modifyLockState(QStringLiteral("A/a1"), FileModifier::LockState::FileLocked, 1, QStringLiteral("Nextcloud Office"), {}, QStringLiteral("richdocuments"), QDateTime::currentDateTime().toSecsSinceEpoch(), 1226);
+        fakeFolder.remoteModifier().setModTimeKeepEtag(QStringLiteral("A/a1"), QDateTime::currentDateTime());
+        fakeFolder.remoteModifier().appendByte(QStringLiteral("A/a1"));
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(nGET, 1);
+        QCOMPARE(nPUT, 0);
+
+        QCOMPARE(completeSpy.findItem(QStringLiteral("A/a1"))->_locked, OCC::SyncFileItem::LockStatus::LockedItem);
+        OCC::SyncJournalFileRecord fileRecordLocked;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordLocked));
+        QVERIFY(fileRecordLocked._lockstate._locked);
+
+        completeSpy.clear();
+        QVERIFY(fakeFolder.syncOnce());
+
+        QCOMPARE(nGET, 1);
+        QCOMPARE(nPUT, 0);
+
+        OCC::SyncJournalFileRecord fileRecordAfter;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &fileRecordAfter));
+        QVERIFY(fileRecordAfter._lockstate._locked);
+
+        auto expectedState = fakeFolder.currentLocalState();
+        QCOMPARE(fakeFolder.currentRemoteState(), expectedState);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestLockFile)