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 20, 2019
1 parent fc90adf commit 34b8190
Show file tree
Hide file tree
Showing 3 changed files with 256 additions and 49 deletions.
116 changes: 88 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,84 @@ 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;
// Moves that simultaneously change the virtualness of the file are complicated
// if (isMove && ((base.isFile() && localEntry.isVirtualFile) || (base.isVirtualFile() && !localEntry.isVirtualFile))) {
// // Particularly on suffix-vfs, since users can trigger them by renaming the file
// // Example: "a.txt" -> "b.txt.owncloud" or "foo.owncloud" -> "bar"
// if (_discoveryData->_syncOptions._vfs->mode() == Vfs::WithSuffix) {
// // Propagating a move+hydrate would be complex. It'd be a remote move and a download.
// // For now, we accept the move request but disregard the hydration change request.
// // That way users won't lose data and part of the request will still be fulfilled.
// if (localEntry.isVirtualFile) {
// qWarning() << "shit rename to virtual" << path._local << base._path << localEntry.name;
// //item->_type = ItemTypeVirtualFileDehydration;
// } else {
// qWarning() << "shit rename from virtual" << path._local << base._path << localEntry.name;
// }
// qWarning() << item->_type << base._type;
// } else {
// 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 +897,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
Loading

0 comments on commit 34b8190

Please sign in to comment.