From: Jocelyn Turcotte Date: Wed, 13 Sep 2017 17:02:38 +0000 (+0200) Subject: SyncJournalDB: Allow callers of getFileRecord if the query failed X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~701^2~41 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=c6610f6fbf9b41ba101726783730f536b040a5c7;p=nextcloud-desktop.git 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 aa173c861..2146e6945 100644 --- 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 &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 0afefebcf..8ad2b0852 100644 --- 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 a6e6e3171..9f20f53fd 100644 --- 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 6c57442ea..76128a85e 100644 --- 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 91e7b20b6..647d24e79 100644 --- 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 46c3899ae..8af558fa7 100644 --- 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 4bb4495c9..6466d13b6 100644 --- 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 db8ab1836..74152f028 100644 --- 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 ecf46f285..949f36b4b 100644 --- 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 48d439b76..d34c516cd 100644 --- 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 855abff7a..1cae76986 100644 --- 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 9de20a015..032937b62 100644 --- 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 c7d8b85c3..5219dd8cf 100644 --- 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); } }