Skip to content

Commit

Permalink
VFS: Unbreak behavior for rename+hydrate #7001
Browse files Browse the repository at this point in the history 
Users can rename a file *and* add/remove the vfs suffix at the same time
leading to very complex sync actions. This patch doesn't add support for
them, but adds tests and makes sure these cases do not cause unintened
behavior.

The rename will be propagated, but the users's hydrate/dehydrate request
will be ignored.
  • Loading branch information
@ckamm
ckamm committed Mar 21, 2019
1 parent fc90adf commit 0ad9b19
Show file tree
Hide file tree
Showing 3 changed files with 237 additions and 49 deletions.
97 changes: 69 additions & 28 deletions src/libsync/discovery.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,12 @@ void ProcessDirectoryJob::processFile(PathTuple path,
// Downloading a virtual file is like a server action and can happen even if
// server-side nothing has changed
// NOTE: Normally setting the VirtualFileDownload flag means that local and
// remote will be rediscovered. This is just a fallback.
// remote will be rediscovered. This is just a fallback for a similar check
// in processFileAnalyzeRemoteInfo().
if (_queryServer == ParentNotChanged
&& (dbEntry._type == ItemTypeVirtualFileDownload
|| localEntry.type == ItemTypeVirtualFileDownload)) {
|| localEntry.type == ItemTypeVirtualFileDownload)
&& (localEntry.isValid() || _queryLocal == ParentNotChanged)) {
item->_direction = SyncFileItem::Down;
item->_instruction = CSYNC_INSTRUCTION_NEW;
item->_type = ItemTypeVirtualFileDownload;
Expand Down Expand Up @@ -375,7 +377,10 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo(
item->_direction = SyncFileItem::Down;
item->_modtime = serverEntry.modtime;
item->_size = serverEntry.size;
} else if (dbEntry._type == ItemTypeVirtualFileDownload || localEntry.type == ItemTypeVirtualFileDownload) {
} else if ((dbEntry._type == ItemTypeVirtualFileDownload || localEntry.type == ItemTypeVirtualFileDownload)
&& (localEntry.isValid() || _queryLocal == ParentNotChanged)) {
// The above check for the localEntry existing is important. Otherwise it breaks
// the case where a file is moved and simultaneously tagged for download in the db.
item->_direction = SyncFileItem::Down;
item->_instruction = CSYNC_INSTRUCTION_NEW;
item->_type = ItemTypeVirtualFileDownload;
Expand Down Expand Up @@ -794,38 +799,65 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
dbError();
return;
}
bool isMove = base.isValid() && base._type == item->_type
&& ((base._modtime == localEntry.modtime && base._fileSize == localEntry.size)
// Directories and virtual files don't need size/mtime equality
|| localEntry.isDirectory || localEntry.isVirtualFile);
const auto originalPath = QString::fromUtf8(base._path);

// Function to gradually check conditions for accepting a move-candidate
auto moveCheck = [&]() {
if (!base.isValid()) {
qCInfo(lcDisco) << "Not a move, no item in db with inode" << localEntry.inode;
return false;
}
if (base.isDirectory() != item->isDirectory()) {
qCInfo(lcDisco) << "Not a move, types don't match" << base._type << item->_type << localEntry.type;
return false;
}
// Directories and virtual files don't need size/mtime equality
if (!localEntry.isDirectory && !base.isVirtualFile()
&& (base._modtime != localEntry.modtime || base._fileSize != localEntry.size)) {
qCInfo(lcDisco) << "Not a move, mtime or size differs, "
<< "modtime:" << base._modtime << localEntry.modtime << ", "
<< "size:" << base._fileSize << localEntry.size;
return false;
}

auto originalPath = QString::fromUtf8(base._path);
if (isMove) {
// The old file must have been deleted.
isMove = !QFile::exists(_discoveryData->_localDir + base._path)
// Exception: If the rename changes case only (like "foo" -> "Foo") the
// old filename might still point to the same file.
|| (Utility::fsCasePreserving()
&& originalPath.compare(path._local, Qt::CaseInsensitive) == 0);
}
if (QFile::exists(_discoveryData->_localDir + base._path)
// Exception: If the rename changes case only (like "foo" -> "Foo") the
// old filename might still point to the same file.
&& !(Utility::fsCasePreserving()
&& originalPath.compare(path._local, Qt::CaseInsensitive) == 0)) {
qCInfo(lcDisco) << "Not a move, base file still exists at" << originalPath;
return false;
}

// Verify the checksum where possible
if (isMove && !base._checksumHeader.isEmpty() && item->_type == ItemTypeFile) {
if (computeLocalChecksum(base._checksumHeader, _discoveryData->_localDir + path._original, item)) {
qCInfo(lcDisco) << "checking checksum of potential rename " << path._original << item->_checksumHeader << base._checksumHeader;
isMove = item->_checksumHeader == base._checksumHeader;
// Verify the checksum where possible
if (!base._checksumHeader.isEmpty() && item->_type == ItemTypeFile) {
if (computeLocalChecksum(base._checksumHeader, _discoveryData->_localDir + path._original, item)) {
qCInfo(lcDisco) << "checking checksum of potential rename " << path._original << item->_checksumHeader << base._checksumHeader;
if (item->_checksumHeader != base._checksumHeader) {
qCInfo(lcDisco) << "Not a move, checksums differ";
return false;
}
}
}

if (_discoveryData->isRenamed(originalPath)) {
qCInfo(lcDisco) << "Not a move, base path already renamed";
return false;
}

// Check local permission if we are allowed to put move the file here
// Technically we should use the one from the server, but we'll assume it is the same
if (!checkMovePermissions(base._remotePerm, originalPath, item->isDirectory())) {
qCInfo(lcDisco) << "Not a move, no permission to rename base file";
return false;
}
}
if (isMove && _discoveryData->isRenamed(originalPath))
isMove = false;

//Check local permission if we are allowed to put move the file here
// Technically we should use the one from the server, but we'll assume it is the same
if (isMove && !checkMovePermissions(base._remotePerm, originalPath, item->isDirectory()))
isMove = false;
return true;
};

// Finally make it a NEW or a RENAME
if (!isMove) {
if (!moveCheck()) {
postProcessLocalNew();
} else {
auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath);
Expand All @@ -846,6 +878,15 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo(
item->_remotePerm = base._remotePerm;
item->_etag = base._etag;
item->_type = base._type;

// Discard any download/dehydrate tags on the base file.
// They could be preserved and honored in a follow-up sync,
// but it complicates handling a lot and will happen rarely.
if (item->_type == ItemTypeVirtualFileDownload)
item->_type = ItemTypeVirtualFile;
if (item->_type == ItemTypeVirtualFileDehydration)
item->_type = ItemTypeFile;

qCInfo(lcDisco) << "Rename detected (up) " << item->_file << " -> " << item->_renameTarget;
};
if (wasDeletedOnClient.first) {
Expand Down
68 changes: 57 additions & 11 deletions src/libsync/propagateremotemove.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,21 +89,67 @@ void PropagateRemoteMove::start()
return;
}

QString source = propagator()->_remoteFolder + origin;
QString destination = QDir::cleanPath(propagator()->account()->davUrl().path() + propagator()->_remoteFolder + _item->_renameTarget);
QString remoteSource = propagator()->_remoteFolder + origin;
QString remoteDestination = QDir::cleanPath(propagator()->account()->davUrl().path() + propagator()->_remoteFolder + _item->_renameTarget);

auto &vfs = propagator()->syncOptions()._vfs;
if (vfs->mode() == Vfs::WithSuffix
&& (_item->_type == ItemTypeVirtualFile || _item->_type == ItemTypeVirtualFileDownload)) {
auto itype = _item->_type;
ASSERT(itype != ItemTypeVirtualFileDownload && itype != ItemTypeVirtualFileDehydration);
if (vfs->mode() == Vfs::WithSuffix && itype != ItemTypeDirectory) {
const auto suffix = vfs->fileSuffix();
ASSERT(source.endsWith(suffix) && destination.endsWith(suffix));
if (source.endsWith(suffix) && destination.endsWith(suffix)) {
source.chop(suffix.size());
destination.chop(suffix.size());
bool sourceHadSuffix = remoteSource.endsWith(suffix);
bool destinationHadSuffix = remoteDestination.endsWith(suffix);

// Remote source and destination definitely shouldn't have the suffix
if (sourceHadSuffix)
remoteSource.chop(suffix.size());
if (destinationHadSuffix)
remoteDestination.chop(suffix.size());

QString folderTarget = _item->_renameTarget;

// Users can rename the file *and at the same time* add or remove the vfs
// suffix. That's a complicated case where a remote rename plus a local hydration
// change is requested. We don't currently deal with that. Instead, the rename
// is propagated and the local vfs suffix change is reverted.
// The discovery would still set up _renameTarget without the changed
// suffix, since that's what must be propagated to the remote but the local
// file may have a different name. folderTargetAlt will contain this potential
// name.
QString folderTargetAlt = folderTarget;
if (itype == ItemTypeFile) {
ASSERT(!sourceHadSuffix && !destinationHadSuffix);

// If foo -> bar.owncloud, the rename target will be "bar"
folderTargetAlt = folderTarget + suffix;

} else if (itype == ItemTypeVirtualFile) {
ASSERT(sourceHadSuffix && destinationHadSuffix);

// If foo.owncloud -> bar, the rename target will be "bar.owncloud"
folderTargetAlt.chop(suffix.size());
}

QString localTarget = propagator()->getFilePath(folderTarget);
QString localTargetAlt = propagator()->getFilePath(folderTargetAlt);

// If the expected target doesn't exist but a file with different hydration
// state does, rename the local file to bring it in line with what the discovery
// has set up.
if (!FileSystem::fileExists(localTarget) && FileSystem::fileExists(localTargetAlt)) {
QString error;
if (!FileSystem::uncheckedRenameReplace(localTargetAlt, localTarget, &error)) {
done(SyncFileItem::NormalError, tr("Could not rename %1 to %2, error: %3")
.arg(folderTargetAlt, folderTarget, error));
return;
}
qCInfo(lcPropagateRemoteMove) << "Suffix vfs required local rename of"
<< folderTargetAlt << "to" << folderTarget;
}
}
qCDebug(lcPropagateRemoteMove) << source << destination;
qCDebug(lcPropagateRemoteMove) << remoteSource << remoteDestination;

_job = new MoveJob(propagator()->account(), source, destination, this);
_job = new MoveJob(propagator()->account(), remoteSource, remoteDestination, this);
connect(_job.data(), &MoveJob::finishedSignal, this, &PropagateRemoteMove::slotMoveJobFinished);
propagator()->_activeJobList.append(this);
_job->start();
Expand Down Expand Up @@ -162,9 +208,9 @@ void PropagateRemoteMove::finalize()
propagator()->_journal->deleteFileRecord(_item->_originalFile);

SyncFileItem newItem(*_item);
newItem._type = _item->_type;
if (oldRecord.isValid()) {
newItem._checksumHeader = oldRecord._checksumHeader;
newItem._type = oldRecord._type;
if (newItem._size != oldRecord._fileSize) {
qCWarning(lcPropagateRemoteMove) << "File sizes differ on server vs sync journal: " << newItem._size << oldRecord._fileSize;

Expand Down
121 changes: 111 additions & 10 deletions test/testsyncvirtualfiles.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,8 @@ private slots:

// If a file is renamed to <name>.owncloud, it becomes virtual
fakeFolder.localModifier().rename("A/a1", "A/a1.owncloud");
// If a file is renamed to <random>.owncloud, the file sticks around (to preserve user data)
// If a file is renamed to <random>.owncloud, the rename propagates but the
// file isn't made virtual the first sync run.
fakeFolder.localModifier().rename("A/a2", "A/rand.owncloud");
// dangling virtual files are removed
fakeFolder.localModifier().insert("A/dangling.owncloud", 1, ' ');
Expand All @@ -567,13 +568,13 @@ private slots:

QVERIFY(!fakeFolder.currentLocalState().find("A/a2"));
QVERIFY(!fakeFolder.currentLocalState().find("A/a2.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("A/rand.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("A/rand"));
QVERIFY(!fakeFolder.currentRemoteState().find("A/a2"));
QVERIFY(itemInstruction(completeSpy, "A/a2", CSYNC_INSTRUCTION_REMOVE));
QVERIFY(!dbRecord(fakeFolder, "A/rand.owncloud").isValid());
QVERIFY(fakeFolder.currentRemoteState().find("A/rand"));
QVERIFY(itemInstruction(completeSpy, "A/rand", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "A/rand")._type == ItemTypeFile);

QVERIFY(!fakeFolder.currentLocalState().find("A/dangling.owncloud"));

cleanup();
}

Expand All @@ -591,15 +592,18 @@ private slots:

fakeFolder.remoteModifier().insert("file1", 128, 'C');
fakeFolder.remoteModifier().insert("file2", 256, 'C');
fakeFolder.remoteModifier().insert("file3", 256, 'C');
QVERIFY(fakeFolder.syncOnce());

QVERIFY(fakeFolder.currentLocalState().find("file1.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("file2.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("file3.owncloud"));
cleanup();

fakeFolder.localModifier().rename("file1.owncloud", "renamed1.owncloud");
fakeFolder.localModifier().rename("file2.owncloud", "renamed2.owncloud");
triggerDownload(fakeFolder, "file2");
triggerDownload(fakeFolder, "file3");
QVERIFY(fakeFolder.syncOnce());

QVERIFY(!fakeFolder.currentLocalState().find("file1.owncloud"));
Expand All @@ -610,12 +614,109 @@ private slots:
QVERIFY(dbRecord(fakeFolder, "renamed1.owncloud").isValid());

// file2 has a conflict between the download request and the rename:
// currently the download wins
// the rename wins, the download is ignored
QVERIFY(!fakeFolder.currentLocalState().find("file2"));
QVERIFY(!fakeFolder.currentLocalState().find("file2.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("file2"));
QVERIFY(fakeFolder.currentRemoteState().find("file2"));
QVERIFY(itemInstruction(completeSpy, "file2", CSYNC_INSTRUCTION_NEW));
QVERIFY(dbRecord(fakeFolder, "file2").isValid());
QVERIFY(fakeFolder.currentLocalState().find("renamed2.owncloud"));
QVERIFY(fakeFolder.currentRemoteState().find("renamed2"));
QVERIFY(itemInstruction(completeSpy, "renamed2.owncloud", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "renamed2.owncloud")._type == ItemTypeVirtualFile);

QVERIFY(itemInstruction(completeSpy, "file3", CSYNC_INSTRUCTION_NEW));
cleanup();

// Test rename while adding/removing vfs suffix
fakeFolder.localModifier().rename("renamed1.owncloud", "R1");
// Contents of file2 could also change at the same time...
fakeFolder.localModifier().rename("file3", "R3.owncloud");
QVERIFY(fakeFolder.syncOnce());
cleanup();
}

void testRenameVirtual2()
{
FakeFolder fakeFolder{ FileInfo() };
setupVfs(fakeFolder);
QSignalSpy completeSpy(&fakeFolder.syncEngine(), SIGNAL(itemCompleted(const SyncFileItemPtr &)));
auto cleanup = [&]() {
completeSpy.clear();
};
cleanup();

fakeFolder.remoteModifier().insert("case3", 128, 'C');
fakeFolder.remoteModifier().insert("case4", 256, 'C');
fakeFolder.remoteModifier().insert("case5", 256, 'C');
fakeFolder.remoteModifier().insert("case6", 256, 'C');
QVERIFY(fakeFolder.syncOnce());

triggerDownload(fakeFolder, "case4");
triggerDownload(fakeFolder, "case6");
QVERIFY(fakeFolder.syncOnce());

QVERIFY(fakeFolder.currentLocalState().find("case3.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("case4"));
QVERIFY(fakeFolder.currentLocalState().find("case5.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("case6"));
cleanup();

// Case 1: foo -> bar (tested elsewhere)
// Case 2: foo.oc -> bar.oc (tested elsewhere)

// Case 3: foo.oc -> bar (db unchanged)
fakeFolder.localModifier().rename("case3.owncloud", "case3-rename");

// Case 4: foo -> bar.oc (db unchanged)
fakeFolder.localModifier().rename("case4", "case4-rename.owncloud");

// Case 5: foo -> bar (db dehydrate)
fakeFolder.localModifier().rename("case5.owncloud", "case5-rename.owncloud");
triggerDownload(fakeFolder, "case5");

// Case 6: foo.oc -> bar.oc (db hydrate)
fakeFolder.localModifier().rename("case6", "case6-rename");
markForDehydration(fakeFolder, "case6");

QVERIFY(fakeFolder.syncOnce());

// Case 3: the rename went though, hydration is forgotten
QVERIFY(!fakeFolder.currentLocalState().find("case3"));
QVERIFY(!fakeFolder.currentLocalState().find("case3.owncloud"));
QVERIFY(!fakeFolder.currentLocalState().find("case3-rename"));
QVERIFY(fakeFolder.currentLocalState().find("case3-rename.owncloud"));
QVERIFY(!fakeFolder.currentRemoteState().find("case3"));
QVERIFY(fakeFolder.currentRemoteState().find("case3-rename"));
QVERIFY(itemInstruction(completeSpy, "case3-rename.owncloud", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "case3-rename.owncloud")._type == ItemTypeVirtualFile);

// Case 4: the rename went though, dehydration is forgotten
QVERIFY(!fakeFolder.currentLocalState().find("case4"));
QVERIFY(!fakeFolder.currentLocalState().find("case4.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("case4-rename"));
QVERIFY(!fakeFolder.currentLocalState().find("case4-rename.owncloud"));
QVERIFY(!fakeFolder.currentRemoteState().find("case4"));
QVERIFY(fakeFolder.currentRemoteState().find("case4-rename"));
QVERIFY(itemInstruction(completeSpy, "case4-rename", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "case4-rename")._type == ItemTypeFile);

// Case 5: the rename went though, hydration is forgotten
QVERIFY(!fakeFolder.currentLocalState().find("case5"));
QVERIFY(!fakeFolder.currentLocalState().find("case5.owncloud"));
QVERIFY(!fakeFolder.currentLocalState().find("case5-rename"));
QVERIFY(fakeFolder.currentLocalState().find("case5-rename.owncloud"));
QVERIFY(!fakeFolder.currentRemoteState().find("case5"));
QVERIFY(fakeFolder.currentRemoteState().find("case5-rename"));
QVERIFY(itemInstruction(completeSpy, "case5-rename.owncloud", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "case5-rename.owncloud")._type == ItemTypeVirtualFile);

// Case 6: the rename went though, dehydration is forgotten
QVERIFY(!fakeFolder.currentLocalState().find("case6"));
QVERIFY(!fakeFolder.currentLocalState().find("case6.owncloud"));
QVERIFY(fakeFolder.currentLocalState().find("case6-rename"));
QVERIFY(!fakeFolder.currentLocalState().find("case6-rename.owncloud"));
QVERIFY(!fakeFolder.currentRemoteState().find("case6"));
QVERIFY(fakeFolder.currentRemoteState().find("case6-rename"));
QVERIFY(itemInstruction(completeSpy, "case6-rename", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "case6-rename")._type == ItemTypeFile);
}

// Dehydration via sync works
Expand Down

0 comments on commit 0ad9b19

Please sign in to comment.