SyncJournalDB: Allow callers of getFileRecord if the query failed

The current implementation would return the same value whether the query failed
or if no row would be found. This is something that is currently checked by csync
and needs to be provided if we want to use SyncJournalDB there.

Adjusted all call sites to also check the return value even though they
could still just rely on rec.isValid(), but makes it more explicit as to what
happens for database errors in those cases, if we ever want to gracefully handle
them.

src/common/syncjournaldb.cpp

@@ -972,37 +972,42 @@ bool SyncJournalDb::deleteFileRecord(const QString &filename, bool recursively)
 }
 
 
-SyncJournalFileRecord SyncJournalDb::getFileRecord(const QString &filename)
+bool SyncJournalDb::getFileRecord(const QString &filename, SyncJournalFileRecord *rec)
 {
     QMutexLocker locker(&_mutex);
 
-    qlonglong phash = getPHash(filename);
-    SyncJournalFileRecord rec;
+    // Reset the output var in case the caller is reusing it.
+    Q_ASSERT(rec);
+    rec->_path.clear();
+    Q_ASSERT(!rec->isValid());
+
+    if (!checkConnect())
+        return false;
 
-    if (!filename.isEmpty() && checkConnect()) {
+    if (!filename.isEmpty()) {
         _getFileRecordQuery->reset_and_clear_bindings();
-        _getFileRecordQuery->bindValue(1, phash);
+        _getFileRecordQuery->bindValue(1, getPHash(filename));
 
         if (!_getFileRecordQuery->exec()) {
             locker.unlock();
             close();
-            return rec;
+            return false;
         }
 
         if (_getFileRecordQuery->next()) {
-            rec._path = _getFileRecordQuery->stringValue(0);
-            rec._inode = _getFileRecordQuery->intValue(1);
-            //rec._uid     = _getFileRecordQuery->value(2).toInt(&ok); Not Used
-            //rec._gid     = _getFileRecordQuery->value(3).toInt(&ok); Not Used
-            //rec._mode    = _getFileRecordQuery->intValue(4);
-            rec._modtime = Utility::qDateTimeFromTime_t(_getFileRecordQuery->int64Value(5));
-            rec._type = _getFileRecordQuery->intValue(6);
-            rec._etag = _getFileRecordQuery->baValue(7);
-            rec._fileId = _getFileRecordQuery->baValue(8);
-            rec._remotePerm = RemotePermissions(_getFileRecordQuery->baValue(9).constData());
-            rec._fileSize = _getFileRecordQuery->int64Value(10);
-            rec._serverHasIgnoredFiles = (_getFileRecordQuery->intValue(11) > 0);
-            rec._checksumHeader = _getFileRecordQuery->baValue(12);
+            rec->_path = _getFileRecordQuery->stringValue(0);
+            rec->_inode = _getFileRecordQuery->intValue(1);
+            //rec->_uid     = _getFileRecordQuery->value(2).toInt(&ok); Not Used
+            //rec->_gid     = _getFileRecordQuery->value(3).toInt(&ok); Not Used
+            //rec->_mode    = _getFileRecordQuery->intValue(4);
+            rec->_modtime = Utility::qDateTimeFromTime_t(_getFileRecordQuery->int64Value(5));
+            rec->_type = _getFileRecordQuery->intValue(6);
+            rec->_etag = _getFileRecordQuery->baValue(7);
+            rec->_fileId = _getFileRecordQuery->baValue(8);
+            rec->_remotePerm = RemotePermissions(_getFileRecordQuery->baValue(9).constData());
+            rec->_fileSize = _getFileRecordQuery->int64Value(10);
+            rec->_serverHasIgnoredFiles = (_getFileRecordQuery->intValue(11) > 0);
+            rec->_checksumHeader = _getFileRecordQuery->baValue(12);
         } else {
             int errId = _getFileRecordQuery->errorId();
             if (errId != SQLITE_DONE) { // only do this if the problem is different from SQLITE_DONE
@@ -1014,7 +1019,7 @@ SyncJournalFileRecord SyncJournalDb::getFileRecord(const QString &filename)
             }
         }
     }
-    return rec;
+    return true;
 }
 
 bool SyncJournalDb::postSyncCleanup(const QSet<QString> &filepathsToKeep,
@@ -1150,10 +1155,12 @@ bool SyncJournalDb::updateLocalMetadata(const QString &filename,
 
 bool SyncJournalDb::setFileRecordMetadata(const SyncJournalFileRecord &record)
 {
-    SyncJournalFileRecord existing = getFileRecord(record._path);
+    SyncJournalFileRecord existing;
+    if (!getFileRecord(record._path, &existing))
+        return false;
 
     // If there's no existing record, just insert the new one.
-    if (existing._path.isEmpty()) {
+    if (!existing.isValid()) {
         return setFileRecord(record);
     }
 

src/common/syncjournaldb.h

@@ -53,9 +53,8 @@ public:
     /// Migrate a csync_journal to the new path, if necessary. Returns false on error
     static bool maybeMigrateDb(const QString &localPath, const QString &absoluteJournalPath);
 
-    // to verify that the record could be queried successfully check
-    // with SyncJournalFileRecord::isValid()
-    SyncJournalFileRecord getFileRecord(const QString &filename);
+    // To verify that the record could be found check with SyncJournalFileRecord::isValid()
+    bool getFileRecord(const QString &filename, SyncJournalFileRecord *rec);
     bool setFileRecord(const SyncJournalFileRecord &record);
 
     /// Like setFileRecord, but preserves checksums

src/gui/folder.cpp

@@ -467,8 +467,10 @@ void Folder::slotWatchedPathChanged(const QString &path)
     // Check that the mtime actually changed.
     if (path.startsWith(this->path())) {
         auto relativePath = path.mid(this->path().size());
-        auto record = _journal.getFileRecord(relativePath);
-        if (record.isValid() && !FileSystem::fileChanged(path, record._fileSize, Utility::qDateTimeToTime_t(record._modtime))) {
+        SyncJournalFileRecord record;
+        if (_journal.getFileRecord(relativePath, &record)
+            && record.isValid()
+            && !FileSystem::fileChanged(path, record._fileSize, Utility::qDateTimeToTime_t(record._modtime))) {
             qCInfo(lcFolder) << "Ignoring spurious notification for file" << relativePath;
             return; // probably a spurious notification
         }

src/gui/owncloudgui.cpp

@@ -1002,10 +1002,10 @@ void ownCloudGui::slotShowShareDialog(const QString &sharePath, const QString &l
     const auto accountState = folder->accountState();
 
     const QString file = localPath.mid(folder->cleanPath().length() + 1);
-    SyncJournalFileRecord fileRecord = folder->journalDb()->getFileRecord(file);
+    SyncJournalFileRecord fileRecord;
 
     bool resharingAllowed = true; // lets assume the good
-    if (fileRecord.isValid()) {
+    if (folder->journalDb()->getFileRecord(file, &fileRecord) && fileRecord.isValid()) {
         // check the permission: Is resharing allowed?
         if (!fileRecord._remotePerm.isNull() && !fileRecord._remotePerm.hasPermission(RemotePermissions::CanReshare)) {
             resharingAllowed = false;

src/gui/socketapi.cpp

@@ -504,8 +504,8 @@ void fetchPrivateLinkUrl(const QString &localFile, SocketApi *target, void (Sock
     const QString file = localFileClean.mid(shareFolder->cleanPath().length() + 1);
 
     // Generate private link ourselves: used as a fallback
-    const SyncJournalFileRecord rec = shareFolder->journalDb()->getFileRecord(file);
-    if (!rec.isValid())
+    SyncJournalFileRecord rec;
+    if (!shareFolder->journalDb()->getFileRecord(file, &rec) || !rec.isValid())
         return;
     const QString oldUrl =
         shareFolder->accountState()->account()->deprecatedPrivateLinkUrl(rec.numericFileId()).toString(QUrl::FullyEncoded);

src/libsync/owncloudpropagator.cpp

@@ -964,8 +964,8 @@ void CleanupPollsJob::start()
 
     auto info = _pollInfos.first();
     _pollInfos.pop_front();
-    SyncJournalFileRecord record = _journal->getFileRecord(info._file);
-    if (record.isValid()) {
+    SyncJournalFileRecord record;
+    if (_journal->getFileRecord(info._file, &record) && record.isValid()) {
         SyncFileItemPtr item = SyncFileItem::fromSyncJournalFileRecord(record);
         PollJob *job = new PollJob(_account, info._url, item, _journal, _localPath, this);
         connect(job, &PollJob::finishedSignal, this, &CleanupPollsJob::slotPollFinished);

src/libsync/propagatedownload.cpp

@@ -706,8 +706,8 @@ namespace { // Anonymous namespace for the recall feature
             // Path of the recalled file in the local folder
             QString localRecalledFile = recalledFile.mid(folderPath.size());
 
-            SyncJournalFileRecord record = journal.getFileRecord(localRecalledFile);
-            if (!record.isValid()) {
+            SyncJournalFileRecord record;
+            if (!journal.getFileRecord(localRecalledFile, &record) || !record.isValid()) {
                 qCWarning(lcPropagateDownload) << "No db entry for recall of" << localRecalledFile;
                 continue;
             }

src/libsync/propagateremotemove.cpp

@@ -162,8 +162,8 @@ void PropagateRemoteMove::slotMoveJobFinished()
 
 void PropagateRemoteMove::finalize()
 {
-    SyncJournalFileRecord oldRecord =
-        propagator()->_journal->getFileRecord(_item->_originalFile);
+    SyncJournalFileRecord oldRecord;
+    propagator()->_journal->getFileRecord(_item->_originalFile, &oldRecord);
     // if reading from db failed still continue hoping that deleteFileRecord
     // reopens the db successfully.
     // The db is only queried to transfer the content checksum from the old

src/libsync/propagatorjobs.cpp

@@ -231,8 +231,8 @@ void PropagateLocalRename::start()
         }
     }
 
-    SyncJournalFileRecord oldRecord =
-        propagator()->_journal->getFileRecord(_item->_originalFile);
+    SyncJournalFileRecord oldRecord;
+    propagator()->_journal->getFileRecord(_item->_originalFile, &oldRecord);
     propagator()->_journal->deleteFileRecord(_item->_originalFile);
 
     // store the rename file name in the item.

src/libsync/syncengine.cpp

@@ -600,8 +600,10 @@ int SyncEngine::treewalkFile(csync_file_stat_t *file, csync_file_stat_t *other,
                 }
 
                 // If the 'W' remote permission changed, update the local filesystem
-                SyncJournalFileRecord prev = _journal->getFileRecord(item->_file);
-                if (prev.isValid() && prev._remotePerm.hasPermission(RemotePermissions::CanWrite) != item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) {
+                SyncJournalFileRecord prev;
+                if (_journal->getFileRecord(item->_file, &prev)
+                    && prev.isValid()
+                    && prev._remotePerm.hasPermission(RemotePermissions::CanWrite) != item->_remotePerm.hasPermission(RemotePermissions::CanWrite)) {
                     const bool isReadOnly = !item->_remotePerm.isNull() && !item->_remotePerm.hasPermission(RemotePermissions::CanWrite);
                     FileSystem::setFileReadOnlyWeak(filePath, isReadOnly);
                 }

src/libsync/syncfilestatustracker.cpp

@@ -145,8 +145,8 @@ SyncFileStatus SyncFileStatusTracker::fileStatus(const QString &relativePath)
         return SyncFileStatus::StatusSync;
 
     // First look it up in the database to know if it's shared
-    SyncJournalFileRecord rec = _syncEngine->journal()->getFileRecord(relativePath);
-    if (rec.isValid()) {
+    SyncJournalFileRecord rec;
+    if (_syncEngine->journal()->getFileRecord(relativePath, &rec) && rec.isValid()) {
         return resolveSyncAndErrorStatus(relativePath, rec._remotePerm.hasPermission(RemotePermissions::IsShared) ? Shared : NotShared);
     }
 

test/testsyncengine.cpp

@@ -109,7 +109,8 @@ private slots:
         fakeFolder.syncOnce();
 
         auto getDbChecksum = [&](QString path) {
-            auto record = fakeFolder.syncJournal().getFileRecord(path);
+            SyncJournalFileRecord record;
+            fakeFolder.syncJournal().getFileRecord(path, &record);
             return record._checksumHeader;
         };
 

test/testsyncjournaldb.cpp

@@ -45,7 +45,8 @@ private slots:
 
     void testFileRecord()
     {
-        SyncJournalFileRecord record = _db.getFileRecord("nonexistant");
+        SyncJournalFileRecord record;
+        QVERIFY(_db.getFileRecord("nonexistant", &record));
         QVERIFY(!record.isValid());
 
         record._path = "foo";
@@ -59,13 +60,14 @@ private slots:
         record._checksumHeader = "MD5:mychecksum";
         QVERIFY(_db.setFileRecord(record));
 
-        SyncJournalFileRecord storedRecord = _db.getFileRecord("foo");
+        SyncJournalFileRecord storedRecord;
+        QVERIFY(_db.getFileRecord("foo", &storedRecord));
         QVERIFY(storedRecord == record);
 
         // Update checksum
         record._checksumHeader = "Adler32:newchecksum";
         _db.updateFileRecordChecksum("foo", "newchecksum", "Adler32");
-        storedRecord = _db.getFileRecord("foo");
+        QVERIFY(_db.getFileRecord("foo", &storedRecord));
         QVERIFY(storedRecord == record);
 
         // Update metadata
@@ -77,11 +79,11 @@ private slots:
         record._remotePerm = RemotePermissions("NV");
         record._fileSize = 289055;
         _db.setFileRecordMetadata(record);
-        storedRecord = _db.getFileRecord("foo");
+        QVERIFY(_db.getFileRecord("foo", &storedRecord));
         QVERIFY(storedRecord == record);
 
         QVERIFY(_db.deleteFileRecord("foo"));
-        record = _db.getFileRecord("foo");
+        QVERIFY(_db.getFileRecord("foo", &record));
         QVERIFY(!record.isValid());
     }
 
@@ -96,7 +98,8 @@ private slots:
             record._modtime = QDateTime::currentDateTimeUtc();
             QVERIFY(_db.setFileRecord(record));
 
-            SyncJournalFileRecord storedRecord = _db.getFileRecord("foo-checksum");
+            SyncJournalFileRecord storedRecord;
+            QVERIFY(_db.getFileRecord("foo-checksum", &storedRecord));
             QVERIFY(storedRecord._path == record._path);
             QVERIFY(storedRecord._remotePerm == record._remotePerm);
             QVERIFY(storedRecord._checksumHeader == record._checksumHeader);
@@ -112,11 +115,12 @@ private slots:
             SyncJournalFileRecord record;
             record._path = "foo-nochecksum";
             record._remotePerm = RemotePermissions("RWN");
-	    record._modtime = QDateTime::currentDateTimeUtc();
+            record._modtime = QDateTime::currentDateTimeUtc();
 
             QVERIFY(_db.setFileRecord(record));
 
-            SyncJournalFileRecord storedRecord = _db.getFileRecord("foo-nochecksum");
+            SyncJournalFileRecord storedRecord;
+            QVERIFY(_db.getFileRecord("foo-nochecksum", &storedRecord));
             QVERIFY(storedRecord == record);
         }
     }