From be786310c5a59920f2a451fe19fa2f650fb87aaa Mon Sep 17 00:00:00 2001 From: Hannah von Reth Date: Mon, 28 Jun 2021 12:32:57 +0200 Subject: [PATCH] Improve the error message returned by updateMetadata Also properly handle the case the the file can't be converted to a placeholder as it is locked --- changelog/unreleased/8787 | 6 +++++ src/common/syncjournaldb.cpp | 11 +++++---- src/common/syncjournaldb.h | 2 +- src/common/utility.cpp | 1 + src/common/vfs.h | 11 +++++++-- src/libsync/owncloudpropagator.cpp | 32 ++++++++++++++++----------- src/libsync/owncloudpropagator.h | 3 +-- src/libsync/propagatedownload.cpp | 11 +++++---- src/libsync/propagateremotemkdir.cpp | 8 +++++-- src/libsync/propagateremotemove.cpp | 8 +++++-- src/libsync/propagateupload.cpp | 8 +++++-- src/libsync/propagatorjobs.cpp | 18 ++++++++++----- src/libsync/vfs/suffix/vfs_suffix.cpp | 4 ++-- src/libsync/vfs/suffix/vfs_suffix.h | 2 +- 14 files changed, 83 insertions(+), 42 deletions(-) create mode 100644 changelog/unreleased/8787 diff --git a/changelog/unreleased/8787 b/changelog/unreleased/8787 new file mode 100644 index 00000000000..90d9811d6f0 --- /dev/null +++ b/changelog/unreleased/8787 @@ -0,0 +1,6 @@ +Enhancement: Improved handling of errors during local file updates + +If a local metadata update fails we now provide the proper error in the ui. +In case that the error was caused by a locked file we now retry the operation. + +https://github.com/owncloud/client/pull/8787 diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp index dffdf44ae64..0cfc0be3a96 100644 --- a/src/common/syncjournaldb.cpp +++ b/src/common/syncjournaldb.cpp @@ -864,7 +864,7 @@ qint64 SyncJournalDb::getPHash(const QByteArray &file) return h; } -bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) +Result SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) { SyncJournalFileRecord record = _record; QMutexLocker locker(&_mutex); @@ -900,13 +900,12 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) QByteArray checksumType, checksum; parseChecksumHeader(record._checksumHeader, &checksumType, &checksum); int contentChecksumTypeId = mapChecksumType(checksumType); - const auto query = _queryManager.get(PreparedSqlQueryManager::SetFileRecordQuery, QByteArrayLiteral("INSERT OR REPLACE INTO metadata " "(phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize, ignoredChildrenRemote, contentChecksum, contentChecksumTypeId) " "VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7, ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16);"), _db); if (!query) { - return false; + return query->error(); } query->bindValue(1, phash); @@ -927,16 +926,16 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record) query->bindValue(16, contentChecksumTypeId); if (!query->exec()) { - return false; + return query->error(); } // Can't be true anymore. _metadataTableIsEmpty = false; - return true; + return {}; } else { qCWarning(lcDb) << "Failed to connect database."; - return false; // checkConnect failed. + return tr("Failed to connect database."); // checkConnect failed. } } diff --git a/src/common/syncjournaldb.h b/src/common/syncjournaldb.h index 26e12c83c82..0f8ff548ae3 100644 --- a/src/common/syncjournaldb.h +++ b/src/common/syncjournaldb.h @@ -64,7 +64,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject bool getFileRecordsByFileId(const QByteArray &fileId, const std::function &rowCallback); bool getFilesBelowPath(const QByteArray &path, const std::function &rowCallback); bool listFilesInPath(const QByteArray &path, const std::function &rowCallback); - bool setFileRecord(const SyncJournalFileRecord &record); + Result setFileRecord(const SyncJournalFileRecord &record); bool deleteFileRecord(const QString &filename, bool recursively = false); bool updateFileRecordChecksum(const QString &filename, diff --git a/src/common/utility.cpp b/src/common/utility.cpp index 08576c81ead..e4336db60fd 100644 --- a/src/common/utility.cpp +++ b/src/common/utility.cpp @@ -637,4 +637,5 @@ QString Utility::sanitizeForFileName(const QString &name) } return result; } + } // namespace OCC diff --git a/src/common/vfs.h b/src/common/vfs.h index 678d2d39ea1..04a9f200366 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -97,6 +97,13 @@ class OCSYNC_EXPORT Vfs : public QObject WindowsCfApi, }; Q_ENUM(Mode) + enum class ConvertToPlaceholderResult { + Error, + Ok, + Locked + }; + Q_ENUM(ConvertToPlaceholderResult) + static QString modeToString(Mode mode); static Optional modeFromString(const QString &str); @@ -187,7 +194,7 @@ class OCSYNC_EXPORT Vfs : public QObject * new placeholder shall supersede, for rename-replace actions with new downloads, * for example. */ - virtual OC_REQUIRED_RESULT Result convertToPlaceholder( + virtual OC_REQUIRED_RESULT Result convertToPlaceholder( const QString &filename, const SyncFileItem &item, const QString &replacesFile = QString()) = 0; @@ -293,7 +300,7 @@ class OCSYNC_EXPORT VfsOff : public Vfs Result updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; } Result createPlaceholder(const SyncFileItem &) override { return {}; } Result dehydratePlaceholder(const SyncFileItem &) override { return {}; } - Result convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override { return {}; } + Result convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override { return ConvertToPlaceholderResult::Ok; } bool needsMetadataUpdate(const SyncFileItem &) override { return false; } bool isDehydratedPlaceholder(const QString &) override { return false; } diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 65b3946000e..74ce015206c 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -747,19 +747,21 @@ QString OwncloudPropagator::adjustRenamedPath(const QString &original) const return OCC::adjustRenamedPath(_renamedDirectories, original); } -bool OwncloudPropagator::updateMetadata(const SyncFileItem &item, const QString &localFolderPath, SyncJournalDb &journal, Vfs &vfs) +Result OwncloudPropagator::updateMetadata(const SyncFileItem &item) { - QString fsPath = localFolderPath + item.destination(); - if (!vfs.convertToPlaceholder(fsPath, item)) { - return false; + const QString fsPath = _localDir + item.destination(); + const auto result = syncOptions()._vfs->convertToPlaceholder(fsPath, item); + if (!result) { + return result.error(); + } else if (*result == Vfs::ConvertToPlaceholderResult::Locked) { + return Vfs::ConvertToPlaceholderResult::Locked; } auto record = item.toSyncJournalFileRecordWithInode(fsPath); - return journal.setFileRecord(record); -} - -bool OwncloudPropagator::updateMetadata(const SyncFileItem &item) -{ - return updateMetadata(item, _localDir, *_journal, *syncOptions()._vfs); + const auto dBresult = _journal->setFileRecord(record); + if (!dBresult) { + return dBresult.error(); + } + return Vfs::ConvertToPlaceholderResult::Ok; } // ================================================================================ @@ -1002,10 +1004,14 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) if (_item->_instruction == CSYNC_INSTRUCTION_RENAME || _item->_instruction == CSYNC_INSTRUCTION_NEW || _item->_instruction == CSYNC_INSTRUCTION_UPDATE_METADATA) { - if (!propagator()->updateMetadata(*_item)) { + const auto result = propagator()->updateMetadata(*_item); + if (!result) { status = _item->_status = SyncFileItem::FatalError; - _item->_errorString = tr("Error writing metadata to the database"); - qCWarning(lcDirectory) << "Error writing to the database for file" << _item->_file; + _item->_errorString = tr("Error updating metadata: %1").arg(result.error()); + qCWarning(lcDirectory) << "Error writing to the database for file" << _item->_file << "with" << result.error(); + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + _item->_status = SyncFileItem::SoftError; + _item->_errorString = tr("File is locked"); } } } diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index 94dbdb0b3db..bd12c9bf235 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -554,8 +554,7 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject * * Will also trigger a Vfs::convertToPlaceholder. */ - static bool updateMetadata(const SyncFileItem &item, const QString &localFolderPath, SyncJournalDb &journal, Vfs &vfs); - bool updateMetadata(const SyncFileItem &item); // convenience for the above + Result updateMetadata(const SyncFileItem &item); private slots: diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 1a284203c5a..69fe767b02a 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -1016,10 +1016,13 @@ void PropagateDownloadFile::downloadFinished() void PropagateDownloadFile::updateMetadata(bool isConflict) { - QString fn = propagator()->fullLocalPath(_item->_file); - - if (!propagator()->updateMetadata(*_item)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + const QString fn = propagator()->fullLocalPath(_item->_file); + const auto result = propagator()->updateMetadata(*_item); + if (!result) { + done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error())); + return; + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + done(SyncFileItem::SoftError, tr("The file %1 is locked").arg(_item->_file)); return; } propagator()->_journal->setDownloadInfo(_item->_file, SyncJournalDb::DownloadInfo()); diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index 02179a0f3ed..9ecd8104b88 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -131,8 +131,12 @@ void PropagateRemoteMkdir::success() itemCopy._etag.clear(); // save the file id already so we can detect rename or remove - if (!propagator()->updateMetadata(itemCopy)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + const auto result = propagator()->updateMetadata(itemCopy); + if (!result) { + done(SyncFileItem::FatalError, tr("Error writing metadata to the database: %1").arg(result.error())); + return; + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + done(SyncFileItem::FatalError, tr("The file %1 is locked").arg(_item->_file)); return; } diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index e15aad25ecc..e72067791c0 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -215,8 +215,12 @@ void PropagateRemoteMove::finalize() newItem._size = oldRecord._fileSize; } } - if (!propagator()->updateMetadata(newItem)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + const auto result = propagator()->updateMetadata(newItem); + if (!result) { + done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error())); + return; + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + done(SyncFileItem::SoftError, tr("The file %1 is locked").arg(newItem._file)); return; } if (pinState && *pinState != PinState::Inherited diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 274807037a6..201a39919af 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -562,8 +562,12 @@ void PropagateUploadFileCommon::finalize() quotaIt.value() -= _item->_size; // Update the database entry - if (!propagator()->updateMetadata(*_item)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + const auto result = propagator()->updateMetadata(*_item); + if (!result) { + done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error())); + return; + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + done(SyncFileItem::SoftError, tr("The file %1 is locked").arg(_item->_file)); return; } diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp index 86b62a72e13..95b06ec68c5 100644 --- a/src/libsync/propagatorjobs.cpp +++ b/src/libsync/propagatorjobs.cpp @@ -175,8 +175,12 @@ void PropagateLocalMkdir::start() // before the correct etag is stored. SyncFileItem newItem(*_item); newItem._etag = "_invalid_"; - if (!propagator()->updateMetadata(newItem)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + const auto result = propagator()->updateMetadata(newItem); + if (!result) { + done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error())); + return; + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + done(SyncFileItem::SoftError, tr("The file %1 is locked").arg(newItem._file)); return; } propagator()->_journal->commit(QStringLiteral("localMkdir")); @@ -248,14 +252,18 @@ void PropagateLocalRename::start() if (oldRecord.isValid()) { newItem._checksumHeader = oldRecord._checksumHeader; } - if (!propagator()->updateMetadata(newItem)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + const auto result = propagator()->updateMetadata(newItem); + if (!result) { + done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error())); + return; + } else if (result.get() == Vfs::ConvertToPlaceholderResult::Locked) { + done(SyncFileItem::SoftError, tr("The file %1 is locked").arg(newItem._file)); return; } } else { propagator()->_renamedDirectories.insert(oldFile, _item->_renameTarget); if (!PropagateRemoteMove::adjustSelectiveSync(propagator()->_journal, oldFile, _item->_renameTarget)) { - done(SyncFileItem::FatalError, tr("Error writing metadata to the database")); + done(SyncFileItem::FatalError, tr("Failed to rename file")); return; } } diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp index 56be2bd19a6..ca77c484927 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.cpp +++ b/src/libsync/vfs/suffix/vfs_suffix.cpp @@ -124,10 +124,10 @@ Result VfsSuffix::dehydratePlaceholder(const SyncFileItem &item) return {}; } -Result VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) +Result VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) { // Nothing necessary - return {}; + return Vfs::ConvertToPlaceholderResult::Ok; } bool VfsSuffix::isDehydratedPlaceholder(const QString &filePath) diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h index 0ece74232a9..e3ddaed6935 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.h +++ b/src/libsync/vfs/suffix/vfs_suffix.h @@ -42,7 +42,7 @@ class VfsSuffix : public Vfs Result createPlaceholder(const SyncFileItem &item) override; Result dehydratePlaceholder(const SyncFileItem &item) override; - Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override; + Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override; bool needsMetadataUpdate(const SyncFileItem &) override { return false; } bool isDehydratedPlaceholder(const QString &filePath) override;