Merge pull request #4819 from nextcloud/feature/syncjournaldb-handle-errors

Feature/syncjournaldb handle errors

Author: mgallien

CMakeLists.txt

@@ -1,5 +1,5 @@
 cmake_minimum_required(VERSION 3.16)
-set(CMAKE_CXX_STANDARD 14)
+set(CMAKE_CXX_STANDARD 17)
 cmake_policy(SET CMP0071 NEW) # Enable use of QtQuick compiler/generated code
 
 project(client)

src/common/syncjournaldb.cpp

@@ -1721,7 +1721,9 @@ void SyncJournalDb::deleteStaleFlagsEntries()
         return;
 
     SqlQuery delQuery("DELETE FROM flags WHERE path != '' AND path NOT IN (SELECT path from metadata);", _db);
-    delQuery.exec();
+    if (!delQuery.exec()) {
+        sqlFail(QStringLiteral("deleteStaleFlagsEntries"), delQuery);
+    }
 }
 
 int SyncJournalDb::errorBlackListEntryCount()
@@ -1862,14 +1864,18 @@ void SyncJournalDb::setPollInfo(const SyncJournalDb::PollInfo &info)
         qCDebug(lcDb) << "Deleting Poll job" << info._file;
         SqlQuery query("DELETE FROM async_poll WHERE path=?", _db);
         query.bindValue(1, info._file);
-        query.exec();
+        if (!query.exec()) {
+            sqlFail(QStringLiteral("setPollInfo DELETE FROM async_poll"), query);
+        }
     } else {
         SqlQuery query("INSERT OR REPLACE INTO async_poll (path, modtime, filesize, pollpath) VALUES( ? , ? , ? , ? )", _db);
         query.bindValue(1, info._file);
         query.bindValue(2, info._modtime);
         query.bindValue(3, info._fileSize);
         query.bindValue(4, info._url);
-        query.exec();
+        if (!query.exec()) {
+            sqlFail(QStringLiteral("setPollInfo INSERT OR REPLACE INTO async_poll"), query);
+        }
     }
 }
 
@@ -1955,7 +1961,10 @@ void SyncJournalDb::avoidRenamesOnNextSync(const QByteArray &path)
     SqlQuery query(_db);
     query.prepare("UPDATE metadata SET fileid = '', inode = '0' WHERE " IS_PREFIX_PATH_OR_EQUAL("?1", "path"));
     query.bindValue(1, path);
-    query.exec();
+
+    if (!query.exec()) {
+        sqlFail(QStringLiteral("avoidRenamesOnNextSync path: %1").arg(QString::fromUtf8(path)), query);
+    }
 
     // We also need to remove the ETags so the update phase refreshes the directory paths
     // on the next sync
@@ -1980,7 +1989,10 @@ void SyncJournalDb::schedulePathForRemoteDiscovery(const QByteArray &fileName)
     // Note: CSYNC_FTW_TYPE_DIR == 2
     query.prepare("UPDATE metadata SET md5='_invalid_' WHERE " IS_PREFIX_PATH_OR_EQUAL("path", "?1") " AND type == 2;");
     query.bindValue(1, argument);
-    query.exec();
+
+    if (!query.exec()) {
+        sqlFail(QStringLiteral("schedulePathForRemoteDiscovery path: %1").arg(QString::fromUtf8(fileName)), query);
+    }
 
     // Prevent future overwrite of the etags of this folder and all
     // parent folders for this sync
@@ -2009,7 +2021,10 @@ void SyncJournalDb::forceRemoteDiscoveryNextSyncLocked()
     qCInfo(lcDb) << "Forcing remote re-discovery by deleting folder Etags";
     SqlQuery deleteRemoteFolderEtagsQuery(_db);
     deleteRemoteFolderEtagsQuery.prepare("UPDATE metadata SET md5='_invalid_' WHERE type=2;");
-    deleteRemoteFolderEtagsQuery.exec();
+
+    if (!deleteRemoteFolderEtagsQuery.exec()) {
+        sqlFail(QStringLiteral("forceRemoteDiscoveryNextSyncLocked"), deleteRemoteFolderEtagsQuery);
+    }
 }
 
 
@@ -2197,10 +2212,12 @@ QByteArray SyncJournalDb::conflictFileBaseName(const QByteArray &conflictName)
     auto conflict = conflictRecord(conflictName);
     QByteArray result;
     if (conflict.isValid()) {
-        getFileRecordsByFileId(conflict.baseFileId, [&result](const SyncJournalFileRecord &record) {
+        if (!getFileRecordsByFileId(conflict.baseFileId, [&result](const SyncJournalFileRecord &record) {
             if (!record._path.isEmpty())
                 result = record._path;
-        });
+        })) {
+            qCWarning(lcDb) << "conflictFileBaseName failed to getFileRecordsByFileId: " << conflictName;
+        }
     }
 
     if (result.isEmpty()) {
@@ -2214,7 +2231,10 @@ void SyncJournalDb::clearFileTable()
     QMutexLocker lock(&_mutex);
     SqlQuery query(_db);
     query.prepare("DELETE FROM metadata;");
-    query.exec();
+
+    if (!query.exec()) {
+        sqlFail(QStringLiteral("clearFileTable"), query);
+    }
 }
 
 void SyncJournalDb::markVirtualFileForDownloadRecursively(const QByteArray &path)
@@ -2228,15 +2248,21 @@ void SyncJournalDb::markVirtualFileForDownloadRecursively(const QByteArray &path
                    "(" IS_PREFIX_PATH_OF("?1", "path") " OR ?1 == '') "
                    "AND type=4;", _db);
     query.bindValue(1, path);
-    query.exec();
+
+    if (!query.exec()) {
+        sqlFail(QStringLiteral("markVirtualFileForDownloadRecursively UPDATE metadata SET type=5 path: %1").arg(QString::fromUtf8(path)), query);
+    }
 
     // We also must make sure we do not read the files from the database (same logic as in schedulePathForRemoteDiscovery)
     // This includes all the parents up to the root, but also all the directory within the selected dir.
     static_assert(ItemTypeDirectory == 2, "");
     query.prepare("UPDATE metadata SET md5='_invalid_' WHERE "
                   "(" IS_PREFIX_PATH_OF("?1", "path") " OR ?1 == '' OR " IS_PREFIX_PATH_OR_EQUAL("path", "?1") ") AND type == 2;");
     query.bindValue(1, path);
-    query.exec();
+
+    if (!query.exec()) {
+        sqlFail(QStringLiteral("markVirtualFileForDownloadRecursively UPDATE metadata SET md5='_invalid_' path: %1").arg(QString::fromUtf8(path)), query);
+    }
 }
 
 Optional<PinState> SyncJournalDb::PinStateInterface::rawForPath(const QByteArray &path)
@@ -2366,7 +2392,12 @@ SyncJournalDb::PinStateInterface::rawList()
         return {};
 
     SqlQuery query("SELECT path, pinState FROM flags;", _db->_db);
-    query.exec();
+
+    if (!query.exec()) {
+        qCWarning(lcDb) << "SQL Error" << "PinStateInterface::rawList" << query.error();
+        _db->close();
+        ASSERT(false);
+    }
 
     QVector<QPair<QByteArray, PinState>> result;
     forever {

src/common/syncjournaldb.h

@@ -59,24 +59,25 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     static bool maybeMigrateDb(const QString &localPath, const QString &absoluteJournalPath);
 
     // To verify that the record could be found check with SyncJournalFileRecord::isValid()
-    bool getFileRecord(const QString &filename, SyncJournalFileRecord *rec) { return getFileRecord(filename.toUtf8(), rec); }
-    bool getFileRecord(const QByteArray &filename, SyncJournalFileRecord *rec);
-    bool getFileRecordByE2eMangledName(const QString &mangledName, SyncJournalFileRecord *rec);
-    bool getFileRecordByInode(quint64 inode, SyncJournalFileRecord *rec);
-    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);
-    Result<void, QString> setFileRecord(const SyncJournalFileRecord &record);
+    [[nodiscard]] bool getFileRecord(const QString &filename, SyncJournalFileRecord *rec) { return getFileRecord(filename.toUtf8(), rec); }
+    [[nodiscard]] bool getFileRecord(const QByteArray &filename, SyncJournalFileRecord *rec);
+    [[nodiscard]] bool getFileRecordByE2eMangledName(const QString &mangledName, SyncJournalFileRecord *rec);
+    [[nodiscard]] bool getFileRecordByInode(quint64 inode, SyncJournalFileRecord *rec);
+    [[nodiscard]] bool getFileRecordsByFileId(const QByteArray &fileId, const std::function<void(const SyncJournalFileRecord &)> &rowCallback);
+    [[nodiscard]] bool getFilesBelowPath(const QByteArray &path, const std::function<void(const SyncJournalFileRecord&)> &rowCallback);
+    [[nodiscard]] bool listFilesInPath(const QByteArray &path, const std::function<void(const SyncJournalFileRecord&)> &rowCallback);
+    [[nodiscard]] Result<void, QString> setFileRecord(const SyncJournalFileRecord &record);
 
     void keyValueStoreSet(const QString &key, QVariant value);
-    qint64 keyValueStoreGetInt(const QString &key, qint64 defaultValue);
+    [[nodiscard]] qint64 keyValueStoreGetInt(const QString &key, qint64 defaultValue);
     void keyValueStoreDelete(const QString &key);
 
-    bool deleteFileRecord(const QString &filename, bool recursively = false);
-    bool updateFileRecordChecksum(const QString &filename,
+    [[nodiscard]] bool deleteFileRecord(const QString &filename, bool recursively = false);
+    [[nodiscard]] bool updateFileRecordChecksum(
+        const QString &filename,
         const QByteArray &contentChecksum,
         const QByteArray &contentChecksumType);
-    bool updateLocalMetadata(const QString &filename,
+    [[nodiscard]] bool updateLocalMetadata(const QString &filename,
         qint64 modtime, qint64 size, quint64 inode);
 
     /// Return value for hasHydratedOrDehydratedFiles()
@@ -99,7 +100,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     void setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord &item);
     void wipeErrorBlacklistEntry(const QString &file);
     void wipeErrorBlacklistCategory(SyncJournalErrorBlacklistRecord::Category category);
-    int wipeErrorBlacklist();
+    [[nodiscard]] int wipeErrorBlacklist();
     int errorBlackListEntryCount();
 
     struct DownloadInfo
@@ -145,7 +146,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     QVector<uint> deleteStaleUploadInfos(const QSet<QString> &keep);
 
     SyncJournalErrorBlacklistRecord errorBlacklistEntry(const QString &);
-    bool deleteStaleErrorBlacklistEntries(const QSet<QString> &keep);
+    [[nodiscard]] bool deleteStaleErrorBlacklistEntries(const QSet<QString> &keep);
 
     /// Delete flags table entries that have no metadata correspondent
     void deleteStaleFlagsEntries();
@@ -372,9 +373,9 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
 
 private:
     int getFileRecordCount();
-    bool updateDatabaseStructure();
-    bool updateMetadataTableStructure();
-    bool updateErrorBlacklistTableStructure();
+    [[nodiscard]] bool updateDatabaseStructure();
+    [[nodiscard]] bool updateMetadataTableStructure();
+    [[nodiscard]] bool updateErrorBlacklistTableStructure();
     bool sqlFail(const QString &log, const SqlQuery &query);
     void commitInternal(const QString &context, bool startTrans = true);
     void startTransaction();
@@ -388,7 +389,7 @@ class OCSYNC_EXPORT SyncJournalDb : public QObject
     // Returns the integer id of the checksum type
     //
     // Returns 0 on failure and for empty checksum types.
-    int mapChecksumType(const QByteArray &checksumType);
+    [[nodiscard]] int mapChecksumType(const QByteArray &checksumType);
 
     SqlDatabase _db;
     QString _dbFile;

src/gui/accountsettings.cpp

@@ -507,7 +507,9 @@ void AccountSettings::slotSubfolderContextMenuRequested(const QModelIndex& index
         // It might be an E2EE mangled path, so let's try to demangle it
         const auto journal = folder->journalDb();
         SyncJournalFileRecord rec;
-        journal->getFileRecordByE2eMangledName(remotePath, &rec);
+        if (!journal->getFileRecordByE2eMangledName(remotePath, &rec)) {
+            qCWarning(lcFolderStatus) << "Could not get file record by E2E Mangled Name from local DB" << remotePath;
+        }
 
         const auto path = rec.isValid() ? rec._path : remotePath;
 

src/gui/connectionvalidator.cpp

@@ -278,7 +278,7 @@ bool ConnectionValidator::setAndCheckServerVersion(const QString &version)
     if (auto job = qobject_cast<AbstractNetworkJob *>(sender())) {
         if (auto reply = job->reply()) {
             _account->setHttp2Supported(
-                reply->attribute(QNetworkRequest::HTTP2WasUsedAttribute).toBool());
+                reply->attribute(QNetworkRequest::Http2WasUsedAttribute).toBool());
         }
     }
 #endif

src/gui/folder.cpp

@@ -577,7 +577,9 @@ void Folder::slotWatchedPathChanged(const QString &path, ChangeReason reason)
 
 
     SyncJournalFileRecord record;
-    _journal.getFileRecord(relativePathBytes, &record);
+    if (!_journal.getFileRecord(relativePathBytes, &record)) {
+        qCWarning(lcFolder) << "could not get file from local DB" << relativePathBytes;
+    }
     if (reason != ChangeReason::UnLock) {
         // Check that the mtime/size actually changed or there was
         // an attribute change (pin state) that caused the notification
@@ -613,7 +615,11 @@ void Folder::implicitlyHydrateFile(const QString &relativepath)
 
     // Set in the database that we should download the file
     SyncJournalFileRecord record;
-    _journal.getFileRecord(relativepath.toUtf8(), &record);
+    ;
+    if (!_journal.getFileRecord(relativepath.toUtf8(), &record)) {
+        qCWarning(lcFolder) << "could not get file from local DB" << relativepath;
+        return;
+    }
     if (!record.isValid()) {
         qCInfo(lcFolder) << "Did not find file in db";
         return;
@@ -622,8 +628,14 @@ void Folder::implicitlyHydrateFile(const QString &relativepath)
         qCInfo(lcFolder) << "The file is not virtual";
         return;
     }
+
     record._type = ItemTypeVirtualFileDownload;
-    _journal.setFileRecord(record);
+
+    const auto result = _journal.setFileRecord(record);
+    if (!result) {
+        qCWarning(lcFolder) << "Error when setting the file record to the database" << record._path << result.error();
+        return;
+    }
 
     // Change the file's pin state if it's contradictory to being hydrated
     // (suffix-virtual file's pin state is stored at the hydrated path)

src/gui/folderstatusdelegate.cpp

@@ -400,7 +400,7 @@ QRect FolderStatusDelegate::optionsButtonRect(QRect within, Qt::LayoutDirection
     opt.rect.setSize(QSize(e,e));
     QSize size = QApplication::style()->sizeFromContents(QStyle::CT_ToolButton, &opt, opt.rect.size()).expandedTo(QApplication::globalStrut());
 
-    int margin = QApplication::style()->pixelMetric(QStyle::PM_DefaultLayoutSpacing);
+    int margin = QApplication::style()->pixelMetric(QStyle::PM_LayoutHorizontalSpacing);
     QRect r(QPoint(within.right() - size.width() - margin,
                 within.top() + within.height() / 2 - size.height() / 2),
         size);

src/gui/folderstatusmodel.cpp

@@ -743,7 +743,9 @@ void FolderStatusModel::slotUpdateDirectories(const QStringList &list)
         newInfo._path = relativePath;
 
         SyncJournalFileRecord rec;
-        parentInfo->_folder->journalDb()->getFileRecordByE2eMangledName(removeTrailingSlash(relativePath), &rec);
+        if (!parentInfo->_folder->journalDb()->getFileRecordByE2eMangledName(removeTrailingSlash(relativePath), &rec)) {
+            qCWarning(lcFolderStatus) << "Could not get file record by E2E Mangled Name from local DB" << removeTrailingSlash(relativePath);
+        }
         if (rec.isValid()) {
             newInfo._name = removeTrailingSlash(rec._path).split('/').last();
             if (rec._isE2eEncrypted && !rec._e2eMangledName.isEmpty()) {

src/gui/socketapi/socketapi.cpp

@@ -1149,7 +1149,9 @@ SyncJournalFileRecord SocketApi::FileData::journalRecord() const
     SyncJournalFileRecord record;
     if (!folder)
         return record;
-    folder->journalDb()->getFileRecord(folderRelativePath, &record);
+    if (!folder->journalDb()->getFileRecord(folderRelativePath, &record)) {
+        qCWarning(lcSocketApi) << "Failed to get journal record for path" << folderRelativePath;
+    }
     return record;
 }
 

src/gui/tray/activitylistmodel.cpp

@@ -156,7 +156,9 @@ QVariant ActivityListModel::data(const QModelIndex &index, int role) const
             // If this is an E2EE file or folder, pretend we got no path, hiding the share button which is what we want
             if (folder) {
                 SyncJournalFileRecord rec;
-                folder->journalDb()->getFileRecord(fileName.mid(1), &rec);
+                if (!folder->journalDb()->getFileRecord(fileName.mid(1), &rec)) {
+                    qCWarning(lcActivity) << "could not get file from local DB" << fileName.mid(1);
+                }
                 if (rec.isValid() && (rec._isE2eEncrypted || !rec._e2eMangledName.isEmpty())) {
                     return QString();
                 }