1
0
mirror of https://github.com/chylex/Nextcloud-Desktop.git synced 2025-05-12 23:34:11 +02:00

VFS: Unbreak behavior for rename+hydrate

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.
This commit is contained in:
Christian Kamm 2019-03-07 14:35:39 +01:00 committed by Kevin Ottens
parent 4346567a03
commit 0cf19123a7
No known key found for this signature in database
GPG Key ID: 074BBBCB8DECC9E2
3 changed files with 239 additions and 51 deletions

View File

@ -325,10 +325,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;
@ -374,7 +376,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;
@ -802,38 +807,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);
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);
}
// 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;
// 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;
}
}
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;
// The old file must have been deleted.
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 (!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;
}
return true;
};
// Finally make it a NEW or a RENAME
if (!isMove) {
if (!moveCheck()) {
postProcessLocalNew();
} else {
auto wasDeletedOnClient = _discoveryData->findAndCancelDeletedJob(originalPath);
@ -854,6 +886,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) {

View File

@ -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();
@ -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;

View File

@ -553,7 +553,8 @@ private slots:
// If a file is renamed to <name>.nextcloud, it becomes virtual
fakeFolder.localModifier().rename("A/a1", "A/a1.nextcloud");
// If a file is renamed to <random>.nextcloud, the file sticks around (to preserve user data)
// If a file is renamed to <random>.nextcloud, the rename propagates but the
// file isn't made virtual the first sync run.
fakeFolder.localModifier().rename("A/a2", "A/rand.nextcloud");
// dangling virtual files are removed
fakeFolder.localModifier().insert("A/dangling.nextcloud", 1, ' ');
@ -567,13 +568,13 @@ private slots:
QVERIFY(!fakeFolder.currentLocalState().find("A/a2"));
QVERIFY(!fakeFolder.currentLocalState().find("A/a2.nextcloud"));
QVERIFY(fakeFolder.currentLocalState().find("A/rand.nextcloud"));
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.nextcloud").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.nextcloud"));
cleanup();
}
@ -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.nextcloud"));
QVERIFY(fakeFolder.currentLocalState().find("file2.nextcloud"));
QVERIFY(fakeFolder.currentLocalState().find("file3.nextcloud"));
cleanup();
fakeFolder.localModifier().rename("file1.nextcloud", "renamed1.nextcloud");
fakeFolder.localModifier().rename("file2.nextcloud", "renamed2.nextcloud");
triggerDownload(fakeFolder, "file2");
triggerDownload(fakeFolder, "file3");
QVERIFY(fakeFolder.syncOnce());
QVERIFY(!fakeFolder.currentLocalState().find("file1.nextcloud"));
@ -610,12 +614,109 @@ private slots:
QVERIFY(dbRecord(fakeFolder, "renamed1.nextcloud").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.nextcloud"));
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.nextcloud"));
QVERIFY(fakeFolder.currentRemoteState().find("renamed2"));
QVERIFY(itemInstruction(completeSpy, "renamed2.nextcloud", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "renamed2.nextcloud")._type == ItemTypeVirtualFile);
QVERIFY(itemInstruction(completeSpy, "file3", CSYNC_INSTRUCTION_NEW));
cleanup();
// Test rename while adding/removing vfs suffix
fakeFolder.localModifier().rename("renamed1.nextcloud", "R1");
// Contents of file2 could also change at the same time...
fakeFolder.localModifier().rename("file3", "R3.nextcloud");
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.nextcloud"));
QVERIFY(fakeFolder.currentLocalState().find("case4"));
QVERIFY(fakeFolder.currentLocalState().find("case5.nextcloud"));
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.nextcloud", "case3-rename");
// Case 4: foo -> bar.oc (db unchanged)
fakeFolder.localModifier().rename("case4", "case4-rename.nextcloud");
// Case 5: foo -> bar (db dehydrate)
fakeFolder.localModifier().rename("case5.nextcloud", "case5-rename.nextcloud");
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.nextcloud"));
QVERIFY(!fakeFolder.currentLocalState().find("case3-rename"));
QVERIFY(fakeFolder.currentLocalState().find("case3-rename.nextcloud"));
QVERIFY(!fakeFolder.currentRemoteState().find("case3"));
QVERIFY(fakeFolder.currentRemoteState().find("case3-rename"));
QVERIFY(itemInstruction(completeSpy, "case3-rename.nextcloud", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "case3-rename.nextcloud")._type == ItemTypeVirtualFile);
// Case 4: the rename went though, dehydration is forgotten
QVERIFY(!fakeFolder.currentLocalState().find("case4"));
QVERIFY(!fakeFolder.currentLocalState().find("case4.nextcloud"));
QVERIFY(fakeFolder.currentLocalState().find("case4-rename"));
QVERIFY(!fakeFolder.currentLocalState().find("case4-rename.nextcloud"));
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.nextcloud"));
QVERIFY(!fakeFolder.currentLocalState().find("case5-rename"));
QVERIFY(fakeFolder.currentLocalState().find("case5-rename.nextcloud"));
QVERIFY(!fakeFolder.currentRemoteState().find("case5"));
QVERIFY(fakeFolder.currentRemoteState().find("case5-rename"));
QVERIFY(itemInstruction(completeSpy, "case5-rename.nextcloud", CSYNC_INSTRUCTION_RENAME));
QVERIFY(dbRecord(fakeFolder, "case5-rename.nextcloud")._type == ItemTypeVirtualFile);
// Case 6: the rename went though, dehydration is forgotten
QVERIFY(!fakeFolder.currentLocalState().find("case6"));
QVERIFY(!fakeFolder.currentLocalState().find("case6.nextcloud"));
QVERIFY(fakeFolder.currentLocalState().find("case6-rename"));
QVERIFY(!fakeFolder.currentLocalState().find("case6-rename.nextcloud"));
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