Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/delete folders during propagation even when propagation has errors #5104

Merged
merged 5 commits into from
Nov 7, 2022
+71 −75
Merged

Bugfix/delete folders during propagation even when propagation has errors #5104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9a06128
remove dead code related to item renaming
mgallien Oct 26, 2022
7fe6a3d
logs from discovery are now namespaced by nextcloud to show debug logs
mgallien Oct 27, 2022
f8fc720
improve logs of E2EE API to not pollute logs
mgallien Oct 27, 2022
783cefe
new autotest to ensure that we delete folders despite blacklist errors
mgallien Oct 27, 2022
402d959
do not skip folder deletion if BlacklistedError during propagation
mgallien Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/libsync/capabilities.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
67 changes: 1 addition & 66 deletions src/libsync/discovery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@

namespace OCC {

Q_LOGGING_CATEGORY(lcDisco, "sync.discovery", QtInfoMsg)
Q_LOGGING_CATEGORY(lcDisco, "nextcloud.sync.discovery", QtInfoMsg)

void ProcessDirectoryJob::start()
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions src/libsync/discoveryphase.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
34 changes: 33 additions & 1 deletion src/libsync/owncloudpropagator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down
9 changes: 7 additions & 2 deletions src/libsync/owncloudpropagator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,8 +365,6 @@ class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory
{
Q_OBJECT
public:
PropagatorCompositeJob _dirDeletionJobs;

explicit PropagateRootDirectory(OwncloudPropagator *propagator);

bool scheduleSelfOrChild() override;
Expand All @@ -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;
};

/**
Expand Down
23 changes: 23 additions & 0 deletions test/testsyncmove.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down