Skip to content

Commit

Permalink
Merge pull request #5024 from nextcloud/bugfix/filesLockFailMetadata
Browse files Browse the repository at this point in the history 
Bugfix/files lock fail metadata
  • Loading branch information
@mgallien
mgallien authored Oct 11, 2022
2 parents 42f6a63 + a096bbd commit 4ddfaa7
Show file tree
Hide file tree
Showing 5 changed files with 213 additions and 44 deletions.
55 changes: 22 additions & 33 deletions src/libsync/discovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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;
}
Expand Down
24 changes: 24 additions & 0 deletions test/syncenginetestutils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}, {
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -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"));
Expand Down
16 changes: 16 additions & 0 deletions test/syncenginetestutils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
13 changes: 2 additions & 11 deletions test/testlocaldiscovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

Expand Down
149 changes: 149 additions & 0 deletions test/testlockfile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "common/syncjournaldb.h"
#include "common/syncjournalfilerecord.h"
#include "syncenginetestutils.h"
#include "localdiscoverytracker.h"

#include <QTest>
#include <QSignalSpy>
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 4ddfaa7

Please sign in to comment.