Sync: Deal with file/folder conflicts #6312

Previously conflicts with a different type on both ends lead to sync
errors. Now they are handled in the expected way: the local item gets
renamed and the remote item gets propagated downwards.

This also adds a unittest for the TYPE_CHANGE case. That one looks like
parts of it might be unified with CONFLICT cases.
This commit is contained in:
Christian Kamm 2018-01-17 10:59:47 +01:00 committed by ckamm
parent 127e675838
commit b9c7aa8df3
6 changed files with 346 additions and 74 deletions

View file

@ -22,6 +22,7 @@
#include "propagateremotemove.h"
#include "propagateremotemkdir.h"
#include "propagatorjobs.h"
#include "filesystem.h"
#include "common/utility.h"
#include "account.h"
#include "common/asserts.h"
@ -368,8 +369,10 @@ PropagateItemJob *OwncloudPropagator::createJob(const SyncFileItemPtr &item)
return new PropagateRemoteDelete(this, item);
case CSYNC_INSTRUCTION_NEW:
case CSYNC_INSTRUCTION_TYPE_CHANGE:
case CSYNC_INSTRUCTION_CONFLICT:
if (item->isDirectory()) {
if (item->_direction == SyncFileItem::Down) {
// CONFLICT has _direction == None
if (item->_direction != SyncFileItem::Up) {
auto job = new PropagateLocalMkdir(this, item);
job->setDeleteExistingFile(deleteExisting);
return job;
@ -380,7 +383,6 @@ PropagateItemJob *OwncloudPropagator::createJob(const SyncFileItemPtr &item)
}
} //fall through
case CSYNC_INSTRUCTION_SYNC:
case CSYNC_INSTRUCTION_CONFLICT:
if (item->_direction != SyncFileItem::Up) {
auto job = new PropagateDownloadFile(this, item);
job->setDeleteExistingFolder(deleteExisting);
@ -431,6 +433,7 @@ void OwncloudPropagator::start(const SyncFileItemVector &items)
directories.push(qMakePair(QString(), _rootJob.data()));
QVector<PropagatorJob *> directoriesToRemove;
QString removedDirectory;
QString maybeConflictDirectory;
foreach (const SyncFileItemPtr &item, items) {
if (!removedDirectory.isEmpty() && item->_file.startsWith(removedDirectory)) {
// this is an item in a directory which is going to be removed.
@ -464,6 +467,20 @@ void OwncloudPropagator::start(const SyncFileItemVector &items)
}
}
// If a CONFLICT item contains files these can't be processed because
// the conflict handling is likely to rename the directory. This can happen
// when there's a new local directory at the same time as a remote file.
if (!maybeConflictDirectory.isEmpty()) {
if (item->destination().startsWith(maybeConflictDirectory)) {
qCInfo(lcPropagator) << "Skipping job inside CONFLICT directory"
<< item->_file << item->_instruction;
item->_instruction = CSYNC_INSTRUCTION_NONE;
continue;
} else {
maybeConflictDirectory.clear();
}
}
while (!item->destination().startsWith(directories.top().first)) {
directories.pop();
}
@ -513,6 +530,12 @@ void OwncloudPropagator::start(const SyncFileItemVector &items)
} else {
directories.top().second->appendTask(item);
}
if (item->_instruction == CSYNC_INSTRUCTION_CONFLICT) {
// This might be a file or a directory on the local side. If it's a
// directory we want to skip processing items inside it.
maybeConflictDirectory = item->_file + "/";
}
}
}
@ -702,6 +725,72 @@ OwncloudPropagator::DiskSpaceResult OwncloudPropagator::diskSpaceCheck() const
return DiskSpaceOk;
}
bool OwncloudPropagator::createConflict(const SyncFileItemPtr &item,
PropagatorCompositeJob *composite, QString *error)
{
QString fn = getFilePath(item->_file);
QString renameError;
auto conflictModTime = FileSystem::getModTime(fn);
QString conflictFileName = Utility::makeConflictFileName(
item->_file, Utility::qDateTimeFromTime_t(conflictModTime));
QString conflictFilePath = getFilePath(conflictFileName);
emit touchedFile(fn);
emit touchedFile(conflictFilePath);
if (!FileSystem::rename(fn, conflictFilePath, &renameError)) {
// If the rename fails, don't replace it.
// If the file is locked, we want to retry this sync when it
// becomes available again.
if (FileSystem::isFileLocked(fn)) {
emit seenLockedFile(fn);
}
if (error)
*error = renameError;
return false;
}
qCInfo(lcPropagator) << "Created conflict file" << fn << "->" << conflictFileName;
// Create a new conflict record. To get the base etag, we need to read it from the db.
ConflictRecord conflictRecord;
conflictRecord.path = conflictFileName.toUtf8();
conflictRecord.baseModtime = item->_previousModtime;
SyncJournalFileRecord baseRecord;
if (_journal->getFileRecord(item->_originalFile, &baseRecord) && baseRecord.isValid()) {
conflictRecord.baseEtag = baseRecord._etag;
conflictRecord.baseFileId = baseRecord._fileId;
} else {
// We might very well end up with no fileid/etag for new/new conflicts
}
_journal->setConflictRecord(conflictRecord);
// Create a new upload job if the new conflict file should be uploaded
if (account()->capabilities().uploadConflictFiles()) {
if (composite && !QFileInfo(conflictFilePath).isDir()) {
SyncFileItemPtr conflictItem = SyncFileItemPtr(new SyncFileItem);
conflictItem->_file = conflictFileName;
conflictItem->_type = ItemTypeFile;
conflictItem->_direction = SyncFileItem::Up;
conflictItem->_instruction = CSYNC_INSTRUCTION_NEW;
conflictItem->_modtime = conflictModTime;
conflictItem->_size = item->_previousSize;
emit newItem(conflictItem);
composite->appendTask(conflictItem);
} else {
// Directories we can't process in one go. The next sync run
// will take care of uploading the conflict dir contents.
_anotherSyncNeeded = true;
}
}
return true;
}
// ================================================================================
PropagatorJob::PropagatorJob(OwncloudPropagator *propagator)
@ -741,7 +830,7 @@ void PropagatorCompositeJob::slotSubJobAbortFinished()
void PropagatorCompositeJob::appendJob(PropagatorJob *job)
{
job->setCompositeParent(this);
job->setAssociatedComposite(this);
_jobsToDo.append(job);
}
@ -858,6 +947,7 @@ PropagateDirectory::PropagateDirectory(OwncloudPropagator *propagator, const Syn
{
if (_firstJob) {
connect(_firstJob.data(), &PropagatorJob::finished, this, &PropagateDirectory::slotFirstJobFinished);
_firstJob->setAssociatedComposite(&_subJobs);
}
connect(&_subJobs, &PropagatorJob::finished, this, &PropagateDirectory::slotSubJobsFinished);
}
@ -901,7 +991,9 @@ void PropagateDirectory::slotFirstJobFinished(SyncFileItem::Status status)
{
_firstJob.take()->deleteLater();
if (status != SyncFileItem::Success && status != SyncFileItem::Restoration) {
if (status != SyncFileItem::Success
&& status != SyncFileItem::Restoration
&& status != SyncFileItem::Conflict) {
if (_state != Finished) {
// Synchronously abort
abort(AbortType::Synchronous);

View file

@ -103,11 +103,13 @@ public:
*/
virtual qint64 committedDiskSpace() const { return 0; }
/** Set the composite parent job
/** Set the associated composite job
*
* Used only from PropagatorCompositeJob itself, when a job is added.
* Used only from PropagatorCompositeJob itself, when a job is added
* and from PropagateDirectory to associate the subJobs with the first
* job.
*/
void setCompositeParent(PropagatorCompositeJob *job) { _compositeParent = job; }
void setAssociatedComposite(PropagatorCompositeJob *job) { _associatedComposite = job; }
public slots:
/*
@ -137,11 +139,14 @@ protected:
OwncloudPropagator *propagator() const;
/** If this job gets added to a composite job, this will point to the parent.
*
* For the PropagateDirectory::_firstJob it will point to
* PropagateDirectory::_subJobs.
*
* That can be useful for jobs that want to spawn follow-up jobs without
* becoming composite jobs themselves.
*/
PropagatorCompositeJob *_compositeParent = nullptr;
PropagatorCompositeJob *_associatedComposite = nullptr;
};
/*
@ -492,6 +497,18 @@ public:
*/
DiskSpaceResult diskSpaceCheck() const;
/** Handles a conflict by renaming the file 'item'.
*
* Sets up conflict records.
*
* It also creates a new upload job in composite if the item that's
* moved away is a file and conflict uploads are requested.
*
* Returns true on success, false and error on error.
*/
bool createConflict(const SyncFileItemPtr &item,
PropagatorCompositeJob *composite, QString *error);
private slots:
void abortTimeout()

View file

@ -697,14 +697,9 @@ void PropagateDownloadFile::deleteExistingFolder()
// on error, just try to move it away...
}
QString conflictDir = Utility::makeConflictFileName(
existingDir, Utility::qDateTimeFromTime_t(FileSystem::getModTime(existingDir)));
emit propagator()->touchedFile(existingDir);
emit propagator()->touchedFile(conflictDir);
QString renameError;
if (!FileSystem::rename(existingDir, conflictDir, &renameError)) {
done(SyncFileItem::NormalError, renameError);
QString error;
if (!propagator()->createConflict(_item, _associatedComposite, &error)) {
done(SyncFileItem::NormalError, error);
}
}
@ -819,58 +814,14 @@ void PropagateDownloadFile::downloadFinished()
return;
}
// In case of conflict, make a backup of the old file
// Ignore conflicts where both files are binary equal
bool isConflict = _item->_instruction == CSYNC_INSTRUCTION_CONFLICT
&& !FileSystem::fileEquals(fn, _tmpFile.fileName());
&& (QFileInfo(fn).isDir() || !FileSystem::fileEquals(fn, _tmpFile.fileName()));
if (isConflict) {
QString renameError;
auto conflictModTime = FileSystem::getModTime(fn);
QString conflictFileName = Utility::makeConflictFileName(
_item->_file, Utility::qDateTimeFromTime_t(conflictModTime));
QString conflictFilePath = propagator()->getFilePath(conflictFileName);
if (!FileSystem::rename(fn, conflictFilePath, &renameError)) {
// If the rename fails, don't replace it.
// If the file is locked, we want to retry this sync when it
// becomes available again.
if (FileSystem::isFileLocked(fn)) {
emit propagator()->seenLockedFile(fn);
}
done(SyncFileItem::SoftError, renameError);
QString error;
if (!propagator()->createConflict(_item, _associatedComposite, &error)) {
done(SyncFileItem::SoftError, error);
return;
}
qCInfo(lcPropagateDownload) << "Created conflict file" << fn << "->" << conflictFileName;
// Create a new conflict record. To get the base etag, we need to read it from the db.
ConflictRecord conflictRecord;
conflictRecord.path = conflictFileName.toUtf8();
conflictRecord.baseModtime = _item->_previousModtime;
SyncJournalFileRecord baseRecord;
if (propagator()->_journal->getFileRecord(_item->_originalFile, &baseRecord) && baseRecord.isValid()) {
conflictRecord.baseEtag = baseRecord._etag;
conflictRecord.baseFileId = baseRecord._fileId;
} else {
// We might very well end up with no fileid/etag for new/new conflicts
}
propagator()->_journal->setConflictRecord(conflictRecord);
// Create a new upload job if the new conflict file should be uploaded
if (propagator()->account()->capabilities().uploadConflictFiles()) {
SyncFileItemPtr conflictItem = SyncFileItemPtr(new SyncFileItem);
conflictItem->_file = conflictFileName;
conflictItem->_type = ItemTypeFile;
conflictItem->_direction = SyncFileItem::Up;
conflictItem->_instruction = CSYNC_INSTRUCTION_NEW;
conflictItem->_modtime = conflictModTime;
conflictItem->_size = _item->_previousSize;
ASSERT(_compositeParent);
emit propagator()->newItem(conflictItem);
_compositeParent->appendTask(conflictItem);
}
}
FileSystem::setModTime(_tmpFile.fileName(), _item->_modtime);

View file

@ -151,13 +151,21 @@ void PropagateLocalMkdir::start()
// When turning something that used to be a file into a directory
// we need to delete the file first.
QFileInfo fi(newDirStr);
if (_deleteExistingFile && fi.exists() && fi.isFile()) {
QString removeError;
if (!FileSystem::remove(newDirStr, &removeError)) {
done(SyncFileItem::NormalError,
tr("could not delete file %1, error: %2")
.arg(newDirStr, removeError));
return;
if (fi.exists() && fi.isFile()) {
if (_deleteExistingFile) {
QString removeError;
if (!FileSystem::remove(newDirStr, &removeError)) {
done(SyncFileItem::NormalError,
tr("could not delete file %1, error: %2")
.arg(newDirStr, removeError));
return;
}
} else if (_item->_instruction == CSYNC_INSTRUCTION_CONFLICT) {
QString error;
if (!propagator()->createConflict(_item, _associatedComposite, &error)) {
done(SyncFileItem::SoftError, error);
return;
}
}
}
@ -186,7 +194,10 @@ void PropagateLocalMkdir::start()
}
propagator()->_journal->commit("localMkdir");
done(SyncFileItem::Success);
auto resultStatus = _item->_instruction == CSYNC_INSTRUCTION_CONFLICT
? SyncFileItem::Conflict
: SyncFileItem::Success;
done(resultStatus);
}
void PropagateLocalMkdir::setDeleteExistingFile(bool enabled)

View file

@ -432,6 +432,7 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other,
item->_modtime = file->modtime;
item->_size = file->size;
item->_checksumHeader = file->checksumHeader;
item->_type = file->type;
} else {
if (instruction != CSYNC_INSTRUCTION_NONE) {
qCWarning(lcEngine) << "ERROR: Instruction" << item->_instruction << "vs" << instruction << "for" << fileUtf8;
@ -576,8 +577,6 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other,
item->_inode = file->inode;
}
item->_type = file->type;
SyncFileItem::Direction dir = SyncFileItem::None;
int re = 0;

View file

@ -345,6 +345,208 @@ private slots:
QFETCH(QString, output);
QCOMPARE(Utility::conflictFileBaseName(input.toUtf8()), output.toUtf8());
}
void testLocalDirRemoteFileConflict()
{
FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
fakeFolder.syncEngine().account()->setCapabilities({ { "uploadConflictFiles", true } });
QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
auto cleanup = [&]() {
completeSpy.clear();
};
cleanup();
// 1) a NEW/NEW conflict
fakeFolder.localModifier().mkdir("Z");
fakeFolder.localModifier().mkdir("Z/subdir");
fakeFolder.localModifier().insert("Z/foo");
fakeFolder.remoteModifier().insert("Z", 63);
// 2) local file becomes a dir; remote file changes
fakeFolder.localModifier().remove("A/a1");
fakeFolder.localModifier().mkdir("A/a1");
fakeFolder.localModifier().insert("A/a1/bar");
fakeFolder.remoteModifier().appendByte("A/a1");
// 3) local dir gets a new file; remote dir becomes a file
fakeFolder.localModifier().insert("B/zzz");
fakeFolder.remoteModifier().remove("B");
fakeFolder.remoteModifier().insert("B", 31);
QVERIFY(fakeFolder.syncOnce());
auto conflicts = findConflicts(fakeFolder.currentLocalState());
conflicts += findConflicts(fakeFolder.currentLocalState().children["A"]);
QCOMPARE(conflicts.size(), 3);
std::sort(conflicts.begin(), conflicts.end());
auto conflictRecords = fakeFolder.syncJournal().conflictRecordPaths();
QCOMPARE(conflictRecords.size(), 3);
std::sort(conflictRecords.begin(), conflictRecords.end());
// 1)
QVERIFY(itemConflict(completeSpy, "Z"));
QCOMPARE(fakeFolder.currentLocalState().find("Z")->size, 63);
QVERIFY(conflicts[2].contains("Z"));
QCOMPARE(conflicts[2].toUtf8(), conflictRecords[2]);
QVERIFY(QFileInfo(fakeFolder.localPath() + conflicts[2]).isDir());
QVERIFY(QFile::exists(fakeFolder.localPath() + conflicts[2] + "/foo"));
// 2)
QVERIFY(itemConflict(completeSpy, "A/a1"));
QCOMPARE(fakeFolder.currentLocalState().find("A/a1")->size, 5);
QVERIFY(conflicts[0].contains("A/a1"));
QCOMPARE(conflicts[0].toUtf8(), conflictRecords[0]);
QVERIFY(QFileInfo(fakeFolder.localPath() + conflicts[0]).isDir());
QVERIFY(QFile::exists(fakeFolder.localPath() + conflicts[0] + "/bar"));
// 3)
QVERIFY(itemConflict(completeSpy, "B"));
QCOMPARE(fakeFolder.currentLocalState().find("B")->size, 31);
QVERIFY(conflicts[1].contains("B"));
QCOMPARE(conflicts[1].toUtf8(), conflictRecords[1]);
QVERIFY(QFileInfo(fakeFolder.localPath() + conflicts[1]).isDir());
QVERIFY(QFile::exists(fakeFolder.localPath() + conflicts[1] + "/zzz"));
// The contents of the conflict directories will only be uploaded after
// another sync.
QVERIFY(fakeFolder.syncEngine().isAnotherSyncNeeded() == ImmediateFollowUp);
cleanup();
QVERIFY(fakeFolder.syncOnce());
QVERIFY(itemSuccessful(completeSpy, conflicts[0], CSYNC_INSTRUCTION_NEW));
QVERIFY(itemSuccessful(completeSpy, conflicts[0] + "/bar", CSYNC_INSTRUCTION_NEW));
QVERIFY(itemSuccessful(completeSpy, conflicts[1], CSYNC_INSTRUCTION_NEW));
QVERIFY(itemSuccessful(completeSpy, conflicts[1] + "/zzz", CSYNC_INSTRUCTION_NEW));
QVERIFY(itemSuccessful(completeSpy, conflicts[2], CSYNC_INSTRUCTION_NEW));
QVERIFY(itemSuccessful(completeSpy, conflicts[2] + "/foo", CSYNC_INSTRUCTION_NEW));
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
}
void testLocalFileRemoteDirConflict()
{
FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
fakeFolder.syncEngine().account()->setCapabilities({ { "uploadConflictFiles", true } });
QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
// 1) a NEW/NEW conflict
fakeFolder.remoteModifier().mkdir("Z");
fakeFolder.remoteModifier().mkdir("Z/subdir");
fakeFolder.remoteModifier().insert("Z/foo");
fakeFolder.localModifier().insert("Z");
// 2) local dir becomes file: remote dir adds file
fakeFolder.localModifier().remove("A");
fakeFolder.localModifier().insert("A", 63);
fakeFolder.remoteModifier().insert("A/bar");
// 3) local file changes; remote file becomes dir
fakeFolder.localModifier().appendByte("B/b1");
fakeFolder.remoteModifier().remove("B/b1");
fakeFolder.remoteModifier().mkdir("B/b1");
fakeFolder.remoteModifier().insert("B/b1/zzz");
QVERIFY(fakeFolder.syncOnce());
auto conflicts = findConflicts(fakeFolder.currentLocalState());
conflicts += findConflicts(fakeFolder.currentLocalState().children["B"]);
QCOMPARE(conflicts.size(), 3);
std::sort(conflicts.begin(), conflicts.end());
auto conflictRecords = fakeFolder.syncJournal().conflictRecordPaths();
QCOMPARE(conflictRecords.size(), 3);
std::sort(conflictRecords.begin(), conflictRecords.end());
// 1)
QVERIFY(itemConflict(completeSpy, "Z"));
QVERIFY(conflicts[2].contains("Z"));
QCOMPARE(conflicts[2].toUtf8(), conflictRecords[2]);
// 2)
QVERIFY(itemConflict(completeSpy, "A"));
QVERIFY(conflicts[0].contains("A"));
QCOMPARE(conflicts[0].toUtf8(), conflictRecords[0]);
// 3)
QVERIFY(itemConflict(completeSpy, "B/b1"));
QVERIFY(conflicts[1].contains("B/b1"));
QCOMPARE(conflicts[1].toUtf8(), conflictRecords[1]);
// Also verifies that conflicts were uploaded
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
}
void testTypeConflictWithMove()
{
FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
// the remote becomes a file, but a file inside the dir has moved away!
fakeFolder.remoteModifier().remove("A");
fakeFolder.remoteModifier().insert("A");
fakeFolder.localModifier().rename("A/a1", "a1");
// same, but with a new file inside the dir locally
fakeFolder.remoteModifier().remove("B");
fakeFolder.remoteModifier().insert("B");
fakeFolder.localModifier().rename("B/b1", "b1");
fakeFolder.localModifier().insert("B/new");
QVERIFY(fakeFolder.syncOnce());
QVERIFY(itemSuccessful(completeSpy, "A", CSYNC_INSTRUCTION_TYPE_CHANGE));
QVERIFY(itemConflict(completeSpy, "B"));
auto conflicts = findConflicts(fakeFolder.currentLocalState());
std::sort(conflicts.begin(), conflicts.end());
QVERIFY(conflicts.size() == 2);
QVERIFY(conflicts[0].contains("A_conflict"));
QVERIFY(conflicts[1].contains("B_conflict"));
for (auto conflict : conflicts)
QDir(fakeFolder.localPath() + conflict).removeRecursively();
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
// Currently a1 and b1 don't get moved, but redownloaded
}
void testTypeChange()
{
FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
// dir becomes file
fakeFolder.remoteModifier().remove("A");
fakeFolder.remoteModifier().insert("A");
fakeFolder.localModifier().remove("B");
fakeFolder.localModifier().insert("B");
// file becomes dir
fakeFolder.remoteModifier().remove("C/c1");
fakeFolder.remoteModifier().mkdir("C/c1");
fakeFolder.remoteModifier().insert("C/c1/foo");
fakeFolder.localModifier().remove("C/c2");
fakeFolder.localModifier().mkdir("C/c2");
fakeFolder.localModifier().insert("C/c2/bar");
QVERIFY(fakeFolder.syncOnce());
QVERIFY(itemSuccessful(completeSpy, "A", CSYNC_INSTRUCTION_TYPE_CHANGE));
QVERIFY(itemSuccessful(completeSpy, "B", CSYNC_INSTRUCTION_TYPE_CHANGE));
QVERIFY(itemSuccessful(completeSpy, "C/c1", CSYNC_INSTRUCTION_TYPE_CHANGE));
QVERIFY(itemSuccessful(completeSpy, "C/c2", CSYNC_INSTRUCTION_TYPE_CHANGE));
// A becomes a conflict because we don't delete folders with files
// inside of them!
auto conflicts = findConflicts(fakeFolder.currentLocalState());
QVERIFY(conflicts.size() == 1);
QVERIFY(conflicts[0].contains("A_conflict"));
for (auto conflict : conflicts)
QDir(fakeFolder.localPath() + conflict).removeRecursively();
QVERIFY(fakeFolder.syncEngine().isAnotherSyncNeeded() == ImmediateFollowUp);
QVERIFY(fakeFolder.syncOnce());
QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
}
};
QTEST_GUILESS_MAIN(TestSyncConflict)