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.

diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp
index aa173c86148404021ff86286a63fe86521bfb7a6..2146e69450f6d96a8abc1d807adab8104857c9f3 100644 (file)
--- a/src/common/syncjournaldb.cpp
+++ b/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);
     }
 

diff --git a/src/common/syncjournaldb.h b/src/common/syncjournaldb.h
index 0afefebcf178e33f211bf1e2d2461bd2c9039173..8ad2b08520627b0522edea1357ac9f84120b6923 100644 (file)
--- a/src/common/syncjournaldb.h
+++ b/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

diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp
index a6e6e3171174a1f00d1b3cd74eb603fa65c79e5a..9f20f53fdee4c4197d04a40d78f6fcce24d755f6 100644 (file)
--- a/src/gui/folder.cpp
+++ b/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
         }

diff --git a/src/gui/owncloudgui.cpp b/src/gui/owncloudgui.cpp
index 6c57442ea547ba10fb0bf127d3dd0ce223a69d7a..76128a85e8db1caef7d77322ee0238d0c23b4237 100644 (file)
--- a/src/gui/owncloudgui.cpp
+++ b/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;

diff --git a/src/gui/socketapi.cpp b/src/gui/socketapi.cpp
index 91e7b20b6dbce2e114a79ea8614b6c6d91331ea4..647d24e79e38660e9c5977810961c5e0b60f0cbc 100644 (file)
--- a/src/gui/socketapi.cpp
+++ b/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);

diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp
index 46c3899ae1702faf11bf6c5abedc5f7a955b6459..8af558fa73701098d1e55ae2b39dc30d9c3ab927 100644 (file)
--- a/src/libsync/owncloudpropagator.cpp
+++ b/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);

diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp
index 4bb4495c905a6ff394093f3020180a704f2d5c74..6466d13b61e1e834409ed54bad3d2c7b7933f504 100644 (file)
--- a/src/libsync/propagatedownload.cpp
+++ b/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;
             }

diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp
index db8ab18368ee472bb107f3d6cd047c450e820856..74152f02815579d9af850ea34478e1c6ab739c7a 100644 (file)
--- a/src/libsync/propagateremotemove.cpp
+++ b/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

diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
index ecf46f2853a12d7d664f38ada2675e71283372bc..949f36b4b684c07c9ef2ae3fe2ee674f96cc8791 100644 (file)
--- a/src/libsync/propagatorjobs.cpp
+++ b/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.

diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp
index 48d439b769339c352b7d02e20377c51666bd6dd3..d34c516cd776a79bad9eed08f93cf36280c9a4a1 100644 (file)
--- a/src/libsync/syncengine.cpp
+++ b/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);
                 }

diff --git a/src/libsync/syncfilestatustracker.cpp b/src/libsync/syncfilestatustracker.cpp
index 855abff7a38b6c9b490fdc8c5810c73387abfe88..1cae76986cf2904320ed68ee6db978e222b4c0d6 100644 (file)
--- a/src/libsync/syncfilestatustracker.cpp
+++ b/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);
     }
 

diff --git a/test/testsyncengine.cpp b/test/testsyncengine.cpp
index 9de20a015496ce7bafe9f963102b3a390f9a748f..032937b625c9ef95b03cd359d377a2be2fbf8416 100644 (file)
--- a/test/testsyncengine.cpp
+++ b/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;
         };
 

diff --git a/test/testsyncjournaldb.cpp b/test/testsyncjournaldb.cpp
index c7d8b85c3c2ca031940687bc1cf4eb1572a2348b..5219dd8cf0e373afd22d0325b0abe3fe75ce6d55 100644 (file)
--- a/test/testsyncjournaldb.cpp
+++ b/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);
         }
     }