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
owncloud · Jun 28, 2021 · b10acce · b10acce
1 parent f5f0675
commit b10acce
Show file tree

Hide file tree

Showing 14 changed files with 83 additions and 42 deletions.
diff --git a/changelog/unreleased/8787 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
@@ -864,7 +864,7 @@ qint64 SyncJournalDb::getPHash(const QByteArray &file)
     return h;
 }
 
-bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record)
+Result<void, QString> 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
@@ -64,7 +64,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     bool getFileRecordsByFileId(const QByteArray &fileId, const std::function<void(const SyncJournalFileRecord &)> &rowCallback);
     bool getFilesBelowPath(const QByteArray &path, const std::function<void(const SyncJournalFileRecord&)> &rowCallback);
     bool listFilesInPath(const QByteArray &path, const std::function<void(const SyncJournalFileRecord&)> &rowCallback);
-    bool setFileRecord(const SyncJournalFileRecord &record);
+    Result<void, QString> 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
@@ -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
@@ -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<Mode> 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<void, QString> convertToPlaceholder(
+    virtual OC_REQUIRED_RESULT Result<ConvertToPlaceholderResult, QString> convertToPlaceholder(
         const QString &filename,
         const SyncFileItem &item,
         const QString &replacesFile = QString()) = 0;
@@ -293,7 +300,7 @@ class OCSYNC_EXPORT VfsOff : public Vfs
     Result<void, QString> updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; }
     Result<void, QString> createPlaceholder(const SyncFileItem &) override { return {}; }
     Result<void, QString> dehydratePlaceholder(const SyncFileItem &) override { return {}; }
-    Result<void, QString> convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override { return {}; }
+    Result<ConvertToPlaceholderResult, QString> 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
@@ -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<Vfs::ConvertToPlaceholderResult, QString> 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 currently in use");
             }
         }
     }

diff --git 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<Vfs::ConvertToPlaceholderResult, QString> updateMetadata(const SyncFileItem &item);
 
 private slots:
 

diff --git 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 currently in use").arg(_item->_file));
         return;
     }
     propagator()->_journal->setDownloadInfo(_item->_file, SyncJournalDb::DownloadInfo());

diff --git 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 currently in use").arg(_item->_file));
         return;
     }
 

diff --git 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 currently in use").arg(newItem._file));
         return;
     }
     if (pinState && *pinState != PinState::Inherited

diff --git 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 currently in use").arg(_item->_file));
         return;
     }
 

diff --git 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 currently in use").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 currently in use").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
@@ -124,10 +124,10 @@ Result<void, QString> VfsSuffix::dehydratePlaceholder(const SyncFileItem &item)
     return {};
 }
 
-Result<void, QString> VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &)
+Result<Vfs::ConvertToPlaceholderResult, QString> 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
@@ -42,7 +42,7 @@ class VfsSuffix : public Vfs
 
     Result<void, QString> createPlaceholder(const SyncFileItem &item) override;
     Result<void, QString> dehydratePlaceholder(const SyncFileItem &item) override;
-    Result<void, QString> convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override;
+    Result<Vfs::ConvertToPlaceholderResult, QString> convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override;
 
     bool needsMetadataUpdate(const SyncFileItem &) override { return false; }
     bool isDehydratedPlaceholder(const QString &filePath) override;