OwnSql: Distinguish no-data from error #6677

This could fix a problem where the client incorrectly decides to delete
local data.

Previously any sqlite3_step() return value that wasn't SQLITE_ROW would
be interpreted as "there's no more data here". Thus an sqlite error at a
bad time could cause the remote discovery to fail to read an unchanged
subtree from the database. These files would then be deleted locally.

With this change sqlite errors from sqlite3_step are detected and
logged. For the particular case of SyncJournalDb::getFilesBelowPath()
the error will now be propagated and the sync run will fail instead of
performing spurious deletes.

Note that many other database functions still don't distinguish
not-found from error cases. Most of them won't have as severe effects on
affected sync runs though.

diff --git a/src/common/ownsql.cpp b/src/common/ownsql.cpp
index 6b3584642c6dbea78d4db609b19b30528331b1d6..3411f38b083891064d4c622b1ac6188267247319 100644 (file)
--- a/src/common/ownsql.cpp
+++ b/src/common/ownsql.cpp
@@ -334,10 +334,31 @@ bool SqlQuery::exec()
     return true;
 }
 
-bool SqlQuery::next()
+auto SqlQuery::next() -> NextResult
 {
-    SQLITE_DO(sqlite3_step(_stmt));
-    return _errId == SQLITE_ROW;
+    const bool firstStep = !sqlite3_stmt_busy(_stmt);
+
+    int n = 0;
+    forever {
+        _errId = sqlite3_step(_stmt);
+        if (n < SQLITE_REPEAT_COUNT && firstStep && (_errId == SQLITE_LOCKED || _errId == SQLITE_BUSY)) {
+            sqlite3_reset(_stmt); // not necessary after sqlite version 3.6.23.1
+            n++;
+            OCC::Utility::usleep(SQLITE_SLEEP_TIME_USEC);
+        } else {
+            break;
+        }
+    }
+
+    NextResult result;
+    result.ok = _errId == SQLITE_ROW || _errId == SQLITE_DONE;
+    result.hasData = _errId == SQLITE_ROW;
+    if (!result.ok) {
+        _error = QString::fromUtf8(sqlite3_errmsg(_db));
+        qCWarning(lcSql) << "Sqlite step statement error:" << _errId << _error << "in" << _sql;
+    }
+
+    return result;
 }
 
 void SqlQuery::bindValue(int pos, const QVariant &value)

diff --git a/src/common/ownsql.h b/src/common/ownsql.h
index a45734a4162da1b7e05dc4f7aa0cc28fc0907c98..7e0b5f058ee2f43cd5128291eeadc58beb594b95 100644 (file)
--- a/src/common/ownsql.h
+++ b/src/common/ownsql.h
@@ -128,7 +128,14 @@ public:
     bool isSelect();
     bool isPragma();
     bool exec();
-    bool next();
+
+    struct NextResult
+    {
+        bool ok = false;
+        bool hasData = false;
+    };
+    NextResult next();
+
     void bindValue(int pos, const QVariant &value);
     QString lastQuery() const;
     int numRowsAffected();

diff --git a/src/common/syncjournaldb.cpp b/src/common/syncjournaldb.cpp
index d4b0f2f83ce186d4191d111f82b135b6fda1bf2b..33f3f49ffb21397cb69eedd35210c0ebfc7ca868 100644 (file)
--- a/src/common/syncjournaldb.cpp
+++ b/src/common/syncjournaldb.cpp
@@ -510,7 +510,7 @@ bool SyncJournalDb::checkConnect()
     bool forceRemoteDiscovery = false;
 
     SqlQuery versionQuery("SELECT major, minor, patch FROM version;", _db);
-    if (!versionQuery.next()) {
+    if (!versionQuery.next().hasData) {
         // If there was no entry in the table, it means we are likely upgrading from 1.5
         qCInfo(lcDb) << "possibleUpgradeFromMirall_1_5 detected!";
         forceRemoteDiscovery = true;
@@ -870,7 +870,7 @@ QVector<QByteArray> SyncJournalDb::tableColumns(const QByteArray &table)
     if (!query.exec()) {
         return columns;
     }
-    while (query.next()) {
+    while (query.next().hasData) {
         columns.append(query.baValue(1));
     }
     qCDebug(lcDb) << "Columns in the current journal: " << columns;
@@ -1023,15 +1023,15 @@ bool SyncJournalDb::getFileRecord(const QByteArray &filename, SyncJournalFileRec
             return false;
         }
 
-        if (_getFileRecordQuery.next()) {
+        auto next = _getFileRecordQuery.next();
+        if (!next.ok) {
+            QString err = _getFileRecordQuery.error();
+            qCWarning(lcDb) << "No journal entry found for " << filename << "Error: " << err;
+            close();
+            return false;
+        }
+        if (next.hasData) {
             fillFileRecordFromGetQuery(*rec, _getFileRecordQuery);
-        } else {
-            int errId = _getFileRecordQuery.errorId();
-            if (errId != SQLITE_DONE) { // only do this if the problem is different from SQLITE_DONE
-                QString err = _getFileRecordQuery.error();
-                qCWarning(lcDb) << "No journal entry found for " << filename << "Error: " << err;
-                close();
-            }
         }
     }
     return true;
@@ -1066,15 +1066,15 @@ bool SyncJournalDb::getFileRecordByE2eMangledName(const QString &mangledName, Sy
             return false;
         }
 
-        if (_getFileRecordQueryByMangledName.next()) {
+        auto next = _getFileRecordQueryByMangledName.next();
+        if (!next.ok) {
+            QString err = _getFileRecordQueryByMangledName.error();
+            qCWarning(lcDb) << "No journal entry found for mangled name" << mangledName << "Error: " << err;
+            close();
+            return false;
+        }
+        if (next.hasData) {
             fillFileRecordFromGetQuery(*rec, _getFileRecordQueryByMangledName);
-        } else {
-            int errId = _getFileRecordQueryByMangledName.errorId();
-            if (errId != SQLITE_DONE) { // only do this if the problem is different from SQLITE_DONE
-                QString err = _getFileRecordQueryByMangledName.error();
-                qCWarning(lcDb) << "No journal entry found for mangled name" << mangledName << "Error: " << err;
-                close();
-            }
         }
     }
     return true;
@@ -1103,7 +1103,10 @@ bool SyncJournalDb::getFileRecordByInode(quint64 inode, SyncJournalFileRecord *r
     if (!_getFileRecordQueryByInode.exec())
         return false;
 
-    if (_getFileRecordQueryByInode.next())
+    auto next = _getFileRecordQueryByInode.next();
+    if (!next.ok)
+        return false;
+    if (next.hasData)
         fillFileRecordFromGetQuery(*rec, _getFileRecordQueryByInode);
 
     return true;
@@ -1127,7 +1130,13 @@ bool SyncJournalDb::getFileRecordsByFileId(const QByteArray &fileId, const std::
     if (!_getFileRecordQueryByFileId.exec())
         return false;
 
-    while (_getFileRecordQueryByFileId.next()) {
+    forever {
+        auto next = _getFileRecordQueryByFileId.next();
+        if (!next.ok)
+            return false;
+        if (!next.hasData)
+            break;
+
         SyncJournalFileRecord rec;
         fillFileRecordFromGetQuery(rec, _getFileRecordQueryByFileId);
         rowCallback(rec);
@@ -1180,7 +1189,13 @@ bool SyncJournalDb::getFilesBelowPath(const QByteArray &path, const std::functio
         return false;
     }
 
-    while (query->next()) {
+    forever {
+        auto next = query->next();
+        if (!next.ok)
+            return false;
+        if (!next.hasData)
+            break;
+
         SyncJournalFileRecord rec;
         fillFileRecordFromGetQuery(rec, *query);
         rowCallback(rec);
@@ -1209,7 +1224,13 @@ bool SyncJournalDb::listFilesInPath(const QByteArray& path,
     if (!_listFilesInPathQuery.exec())
         return false;
 
-    while (_listFilesInPathQuery.next()) {
+    forever {
+        auto next = _listFilesInPathQuery.next();
+        if (!next.ok)
+            return false;
+        if (!next.hasData)
+            break;
+
         SyncJournalFileRecord rec;
         fillFileRecordFromGetQuery(rec, _listFilesInPathQuery);
         if (!rec._path.startsWith(path) || rec._path.indexOf("/", path.size() + 1) > 0) {
@@ -1233,7 +1254,7 @@ int SyncJournalDb::getFileRecordCount()
         return -1;
     }
 
-    if (query.next()) {
+    if (query.next().hasData) {
         int count = query.intValue(0);
         return count;
     }
@@ -1344,10 +1365,8 @@ SyncJournalDb::DownloadInfo SyncJournalDb::getDownloadInfo(const QString &file)
             return res;
         }
 
-        if (_getDownloadInfoQuery.next()) {
+        if (_getDownloadInfoQuery.next().hasData) {
             toDownloadInfo(_getDownloadInfoQuery, &res);
-        } else {
-            res._valid = false;
         }
     }
     return res;
@@ -1401,7 +1420,7 @@ QVector<SyncJournalDb::DownloadInfo> SyncJournalDb::getAndDeleteStaleDownloadInf
     QStringList superfluousPaths;
     QVector<SyncJournalDb::DownloadInfo> deleted_entries;
 
-    while (query.next()) {
+    while (query.next().hasData) {
         const QString file = query.stringValue(3); // path
         if (!keep.contains(file)) {
             superfluousPaths.append(file);
@@ -1428,7 +1447,7 @@ int SyncJournalDb::downloadInfoCount()
         if (!query.exec()) {
             sqlFail("Count number of downloadinfo entries failed", query);
         }
-        if (query.next()) {
+        if (query.next().hasData) {
             re = query.intValue(0);
         }
     }
@@ -1453,7 +1472,7 @@ SyncJournalDb::UploadInfo SyncJournalDb::getUploadInfo(const QString &file)
             return res;
         }
 
-        if (_getUploadInfoQuery.next()) {
+        if (_getUploadInfoQuery.next().hasData) {
             bool ok = true;
             res._chunk = _getUploadInfoQuery.intValue(0);
             res._transferid = _getUploadInfoQuery.int64Value(1);
@@ -1522,7 +1541,7 @@ QVector<uint> SyncJournalDb::deleteStaleUploadInfos(const QSet<QString> &keep)
 
     QStringList superfluousPaths;
 
-    while (query.next()) {
+    while (query.next().hasData) {
         const QString file = query.stringValue(0);
         if (!keep.contains(file)) {
             superfluousPaths.append(file);
@@ -1546,7 +1565,7 @@ SyncJournalErrorBlacklistRecord SyncJournalDb::errorBlacklistEntry(const QString
         _getErrorBlacklistQuery.reset_and_clear_bindings();
         _getErrorBlacklistQuery.bindValue(1, file);
         if (_getErrorBlacklistQuery.exec()) {
-            if (_getErrorBlacklistQuery.next()) {
+            if (_getErrorBlacklistQuery.next().hasData) {
                 entry._lastTryEtag = _getErrorBlacklistQuery.baValue(0);
                 entry._lastTryModtime = _getErrorBlacklistQuery.int64Value(1);
                 entry._retryCount = _getErrorBlacklistQuery.intValue(2);
@@ -1582,7 +1601,7 @@ bool SyncJournalDb::deleteStaleErrorBlacklistEntries(const QSet<QString> &keep)
 
     QStringList superfluousPaths;
 
-    while (query.next()) {
+    while (query.next().hasData) {
         const QString file = query.stringValue(0);
         if (!keep.contains(file)) {
             superfluousPaths.append(file);
@@ -1605,7 +1624,7 @@ int SyncJournalDb::errorBlackListEntryCount()
         if (!query.exec()) {
             sqlFail("Count number of blacklist entries failed", query);
         }
-        if (query.next()) {
+        if (query.next().hasData) {
             re = query.intValue(0);
         }
     }
@@ -1709,7 +1728,7 @@ QVector<SyncJournalDb::PollInfo> SyncJournalDb::getPollInfos()
         return res;
     }
 
-    while (query.next()) {
+    while (query.next().hasData) {
         PollInfo info;
         info._file = query.stringValue(0);
         info._modtime = query.int64Value(1);
@@ -1763,7 +1782,15 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList
         *ok = false;
         return result;
     }
-    while (_getSelectiveSyncListQuery.next()) {
+    forever {
+        auto next = _getSelectiveSyncListQuery.next();
+        if (!next.ok) {
+            *ok = false;
+            return result;
+        }
+        if (!next.hasData)
+            break;
+
         auto entry = _getSelectiveSyncListQuery.stringValue(0);
         if (!entry.endsWith(QLatin1Char('/'))) {
             entry.append(QLatin1Char('/'));
@@ -1886,12 +1913,12 @@ QByteArray SyncJournalDb::getChecksumType(int checksumTypeId)
         return {};
     query.bindValue(1, checksumTypeId);
     if (!query.exec()) {
-        return nullptr;
+        return QByteArray();
     }
 
-    if (!query.next()) {
+    if (!query.next().hasData) {
         qCWarning(lcDb) << "No checksum type mapping found for" << checksumTypeId;
-        return nullptr;
+        return QByteArray();
     }
     return query.baValue(0);
 }
@@ -1922,7 +1949,7 @@ int SyncJournalDb::mapChecksumType(const QByteArray &checksumType)
         return 0;
     }
 
-    if (!_getChecksumTypeIdQuery.next()) {
+    if (!_getChecksumTypeIdQuery.next().hasData) {
         qCWarning(lcDb) << "No checksum type mapping found for" << checksumType;
         return 0;
     }
@@ -1945,7 +1972,7 @@ QByteArray SyncJournalDb::dataFingerprint()
         return QByteArray();
     }
 
-    if (!_getDataFingerprintQuery.next()) {
+    if (!_getDataFingerprintQuery.next().hasData) {
         return QByteArray();
     }
     return _getDataFingerprintQuery.baValue(0);
@@ -2000,7 +2027,7 @@ ConflictRecord SyncJournalDb::conflictRecord(const QByteArray &path)
     ASSERT(query.initOrReset(QByteArrayLiteral("SELECT baseFileId, baseModtime, baseEtag, basePath FROM conflicts WHERE path=?1;"), _db));
     query.bindValue(1, path);
     ASSERT(query.exec());
-    if (!query.next())
+    if (!query.next().hasData)
         return entry;
 
     entry.path = path;
@@ -2033,7 +2060,7 @@ QByteArrayList SyncJournalDb::conflictRecordPaths()
     ASSERT(query.exec());
 
     QByteArrayList paths;
-    while (query.next())
+    while (query.next().hasData)
         paths.append(query.baValue(0));
 
     return paths;
@@ -2099,8 +2126,11 @@ Optional<PinState> SyncJournalDb::PinStateInterface::rawForPath(const QByteArray
     query.bindValue(1, path);
     query.exec();
 
+    auto next = query.next();
+    if (!next.ok)
+        return {};
     // no-entry means Inherited
-    if (!query.next())
+    if (!next.hasData)
         return PinState::Inherited;
 
     return static_cast<PinState>(query.intValue(0));
@@ -2124,8 +2154,11 @@ Optional<PinState> SyncJournalDb::PinStateInterface::effectiveForPath(const QByt
     query.bindValue(1, path);
     query.exec();
 
+    auto next = query.next();
+    if (!next.ok)
+        return {};
     // If the root path has no setting, assume AlwaysLocal
-    if (!query.next())
+    if (!next.hasData)
         return PinState::AlwaysLocal;
 
     return static_cast<PinState>(query.intValue(0));
@@ -2178,7 +2211,12 @@ SyncJournalDb::PinStateInterface::rawList()
     query.exec();
 
     QVector<QPair<QByteArray, PinState>> result;
-    while (query.next()) {
+    forever {
+        auto next = query.next();
+        if (!next.ok)
+            return {};
+        if (!next.hasData)
+            break;
         result.append({ query.baValue(0), static_cast<PinState>(query.intValue(1)) });
     }
     return result;

diff --git a/test/testownsql.cpp b/test/testownsql.cpp
index 86ec77d28dfa7da14d06e8b5b559cfd48f030fda..58e9bc5b6a54e066c8598b1caad51fbf0a217a16 100644 (file)
--- a/test/testownsql.cpp
+++ b/test/testownsql.cpp
@@ -71,7 +71,7 @@ private slots:
         q.prepare(sql);
 
         q.exec();
-        while( q.next() ) {
+        while( q.next().hasData ) {
             qDebug() << "Name: " << q.stringValue(1);
             qDebug() << "Address: " << q.stringValue(2);
         }
@@ -83,7 +83,7 @@ private slots:
         q.prepare(sql);
         q.bindValue(1, 2);
         q.exec();
-        if( q.next() ) {
+        if( q.next().hasData ) {
             qDebug() << "Name:" << q.stringValue(1);
             qDebug() << "Address:" << q.stringValue(2);
         }
@@ -96,7 +96,7 @@ private slots:
         int rc = q.prepare(sql);
         qDebug() << "Pragma:" << rc;
         q.exec();
-        if( q.next() ) {
+        if( q.next().hasData ) {
             qDebug() << "P:" << q.stringValue(1);
         }
     }
@@ -118,7 +118,7 @@ private slots:
         SqlQuery q(_db);
         q.prepare(sql);
 
-        if(q.next()) {
+        if(q.next().hasData) {
             QString name = q.stringValue(1);
             QString address = q.stringValue(2);
             QVERIFY( name == QString::fromUtf8("пятницы") );

diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp
index 696fd3db66231181b460daf30d8663d8559c0d79..c7d5da8f4cfa77d4a9ec91958121256a766b04e7 100644 (file)
--- a/test/testpermissions.cpp
+++ b/test/testpermissions.cpp
@@ -31,7 +31,7 @@ static void assertCsyncJournalOk(SyncJournalDb &journal)
     QVERIFY(db.openReadOnly(journal.databaseFilePath()));
     SqlQuery q("SELECT count(*) from metadata where length(fileId) == 0", db);
     QVERIFY(q.exec());
-    QVERIFY(q.next());
+    QVERIFY(q.next().hasData);
     QCOMPARE(q.intValue(0), 0);
 #if defined(Q_OS_WIN) // Make sure the file does not appear in the FileInfo
     FileSystem::setFileHidden(journal.databaseFilePath() + "-shm", true);