OwnSql: Fixup after feedback for #6388

src/common/ownsql.cpp

@@ -236,9 +236,6 @@ SqlQuery::~SqlQuery()
     if (_stmt) {
         finish();
     }
-    if (_sqldb) {
-        _sqldb->_queries.remove(this);
-    }
 }
 
 SqlQuery::SqlQuery(const QByteArray &sql, SqlDatabase &db)
@@ -270,17 +267,22 @@ int SqlQuery::prepare(const QByteArray &sql, bool allow_failure)
             _error = QString::fromUtf8(sqlite3_errmsg(_db));
             qCWarning(lcSql) << "Sqlite prepare statement error:" << _error << "in" << _sql;
             ENFORCE(allow_failure, "SQLITE Prepare error");
+        } else {
+            ASSERT(_stmt);
+            _sqldb->_queries.insert(this);
         }
-        _sqldb->_queries.insert(this);
     }
     return _errId;
 }
 
-// There is no overloads to QByteArray::startWith that takes Qt::CaseInsensitive
-template <int N>
-static bool startsWithInsensitive(const QByteArray &a, const char (&cal)[N])
+/**
+ * There is no overloads to QByteArray::startWith that takes Qt::CaseInsensitive.
+ * Returns true if 'a' starts with 'b' in a case insensitive way
+ */
+static bool startsWithInsensitive(const QByteArray &a, const char *b)
 {
-    return a.size() >= N - 1 && qstrnicmp(a.constData(), cal, N - 1) == 0;
+    int len = strlen(b);
+    return a.size() >= len && qstrnicmp(a.constData(), b, len) == 0;
 }
 
 bool SqlQuery::isSelect()
@@ -457,8 +459,13 @@ int SqlQuery::numRowsAffected()
 
 void SqlQuery::finish()
 {
+    if (!_stmt)
+        return;
     SQLITE_DO(sqlite3_finalize(_stmt));
     _stmt = 0;
+    if (_sqldb) {
+        _sqldb->_queries.remove(this);
+    }
 }
 
 void SqlQuery::reset_and_clear_bindings()
@@ -469,7 +476,7 @@ void SqlQuery::reset_and_clear_bindings()
     }
 }
 
-bool SqlQuery::init(const QByteArray &sql, OCC::SqlDatabase &db)
+bool SqlQuery::initOrReset(const QByteArray &sql, OCC::SqlDatabase &db)
 {
     ENFORCE(!_sqldb || &db == _sqldb);
     _sqldb = &db;

src/common/ownsql.h

@@ -73,6 +73,26 @@ private:
 /**
  * @brief The SqlQuery class
  * @ingroup libsync
+ *
+ * There is basically 3 ways to initialize and use a query:
+ *
+    SqlQuery q1;
+    [...]
+    q1.initOrReset(...);
+    q1.bindValue(...);
+    q1.exec(...)
+ *
+    SqlQuery q2(db);
+    q2.prepare(...);
+    [...]
+    q2.reset_and_clear_bindings();
+    q2.bindValue(...);
+    q2.exec(...)
+ *
+    SqlQuery q3("...", db);
+    q3.bindValue(...);
+    q3.exec(...)
+ *
  */
 class OCSYNC_EXPORT SqlQuery
 {
@@ -82,11 +102,17 @@ public:
     explicit SqlQuery(SqlDatabase &db);
     explicit SqlQuery(const QByteArray &sql, SqlDatabase &db);
     /**
-     * Prepare the SQLQuery if it was not prepared yet.
+     * Prepare the SqlQuery if it was not prepared yet.
      * Otherwise, clear the results and the bindings.
      * return false if there is an error
      */
-    bool init(const QByteArray &sql, SqlDatabase &db);
+    bool initOrReset(const QByteArray &sql, SqlDatabase &db);
+    /**
+     * Prepare the SqlQuery.
+     * If the query was already prepared, this will first call finish(), and re-prepare it.
+     * This function must only be used if the constructor was setting a SqlDatabase
+     */
+    int prepare(const QByteArray &sql, bool allow_failure = false);
 
     ~SqlQuery();
     QString error() const;
@@ -99,11 +125,9 @@ public:
     int intValue(int index);
     quint64 int64Value(int index);
     QByteArray baValue(int index);
-
     bool isSelect();
     bool isPragma();
     bool exec();
-    int prepare(const QByteArray &sql, bool allow_failure = false);
     bool next();
     void bindValue(int pos, const QVariant &value);
     QString lastQuery() const;

src/common/syncjournaldb.cpp

@@ -559,11 +559,11 @@ bool SyncJournalDb::checkConnect()
         forceRemoteDiscoveryNextSyncLocked();
     }
 
-    if (!_deleteDownloadInfoQuery.init("DELETE FROM downloadinfo WHERE path=?1", _db)) {
+    if (!_deleteDownloadInfoQuery.initOrReset("DELETE FROM downloadinfo WHERE path=?1", _db)) {
         return sqlFail("prepare _deleteDownloadInfoQuery", _deleteDownloadInfoQuery);
     }
 
-    if (!_deleteUploadInfoQuery.init("DELETE FROM uploadinfo WHERE path=?1", _db)) {
+    if (!_deleteUploadInfoQuery.initOrReset("DELETE FROM uploadinfo WHERE path=?1", _db)) {
         return sqlFail("prepare _deleteUploadInfoQuery", _deleteUploadInfoQuery);
     }
 
@@ -574,7 +574,7 @@ bool SyncJournalDb::checkConnect()
         // case insensitively
         sql += " COLLATE NOCASE";
     }
-    if (!_getErrorBlacklistQuery.init(sql, _db)) {
+    if (!_getErrorBlacklistQuery.initOrReset(sql, _db)) {
         return sqlFail("prepare _getErrorBlacklistQuery", _getErrorBlacklistQuery);
     }
 
@@ -856,7 +856,7 @@ bool SyncJournalDb::setFileRecord(const SyncJournalFileRecord &_record)
         parseChecksumHeader(record._checksumHeader, &checksumType, &checksum);
         int contentChecksumTypeId = mapChecksumType(checksumType);
 
-        if (!_setFileRecordQuery.init(QByteArrayLiteral(
+        if (!_setFileRecordQuery.initOrReset(QByteArrayLiteral(
             "INSERT OR REPLACE INTO metadata "
             "(phash, pathlen, path, inode, uid, gid, mode, modtime, type, md5, fileid, remotePerm, filesize, ignoredChildrenRemote, contentChecksum, contentChecksumTypeId) "
             "VALUES (?1 , ?2, ?3 , ?4 , ?5 , ?6 , ?7,  ?8 , ?9 , ?10, ?11, ?12, ?13, ?14, ?15, ?16);"), _db)) {
@@ -903,7 +903,7 @@ bool SyncJournalDb::deleteFileRecord(const QString &filename, bool recursively)
         // if (!recursively) {
         // always delete the actual file.
 
-        if (!_deleteFileRecordPhash.init(QByteArrayLiteral("DELETE FROM metadata WHERE phash=?1"), _db))
+        if (!_deleteFileRecordPhash.initOrReset(QByteArrayLiteral("DELETE FROM metadata WHERE phash=?1"), _db))
             return false;
 
         qlonglong phash = getPHash(filename.toUtf8());
@@ -913,7 +913,7 @@ bool SyncJournalDb::deleteFileRecord(const QString &filename, bool recursively)
             return false;
 
         if (recursively) {
-            if (!_deleteFileRecordRecursively.init(QByteArrayLiteral("DELETE FROM metadata WHERE " IS_PREFIX_PATH_OF("?1", "path")), _db))
+            if (!_deleteFileRecordRecursively.initOrReset(QByteArrayLiteral("DELETE FROM metadata WHERE " IS_PREFIX_PATH_OF("?1", "path")), _db))
                 return false;
             _deleteFileRecordRecursively.bindValue(1, filename);
             if (!_deleteFileRecordRecursively.exec()) {
@@ -944,7 +944,7 @@ bool SyncJournalDb::getFileRecord(const QByteArray &filename, SyncJournalFileRec
         return false;
 
     if (!filename.isEmpty()) {
-        if (!_getFileRecordQuery.init(QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE phash=?1"), _db))
+        if (!_getFileRecordQuery.initOrReset(QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE phash=?1"), _db))
             return false;
 
         _getFileRecordQuery.bindValue(1, getPHash(filename));
@@ -1021,7 +1021,7 @@ bool SyncJournalDb::getFileRecordByInode(quint64 inode, SyncJournalFileRecord *r
     if (!checkConnect())
         return false;
 
-    if (!_getFileRecordQueryByInode.init(QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE inode=?1"), _db))
+    if (!_getFileRecordQueryByInode.initOrReset(QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE inode=?1"), _db))
         return false;
 
     _getFileRecordQueryByInode.bindValue(1, inode);
@@ -1045,7 +1045,7 @@ bool SyncJournalDb::getFileRecordsByFileId(const QByteArray &fileId, const std::
     if (!checkConnect())
         return false;
 
-    if (!_getFileRecordQueryByFileId.init(QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE fileid=?1"), _db))
+    if (!_getFileRecordQueryByFileId.initOrReset(QByteArrayLiteral(GET_FILE_RECORD_QUERY " WHERE fileid=?1"), _db))
         return false;
 
     _getFileRecordQueryByFileId.bindValue(1, fileId);
@@ -1080,13 +1080,13 @@ bool SyncJournalDb::getFilesBelowPath(const QByteArray &path, const std::functio
         // and find nothing. So, unfortunately, we have to use a different query for
         // retrieving the whole tree.
 
-        if (!_getAllFilesQuery.init(QByteArrayLiteral( GET_FILE_RECORD_QUERY " ORDER BY path||'/' ASC"), _db))
+        if (!_getAllFilesQuery.initOrReset(QByteArrayLiteral( GET_FILE_RECORD_QUERY " ORDER BY path||'/' ASC"), _db))
             return false;
         query = &_getAllFilesQuery;
     } else {
         // This query is used to skip discovery and fill the tree from the
         // database instead
-        if (!_getFilesBelowPathQuery.init(QByteArrayLiteral(
+        if (!_getFilesBelowPathQuery.initOrReset(QByteArrayLiteral(
                 GET_FILE_RECORD_QUERY
                 " WHERE " IS_PREFIX_PATH_OF("?1", "path")
                 // We want to ensure that the contents of a directory are sorted
@@ -1199,7 +1199,7 @@ bool SyncJournalDb::updateFileRecordChecksum(const QString &filename,
 
     int checksumTypeId = mapChecksumType(contentChecksumType);
 
-    if (!_setFileRecordChecksumQuery.init(QByteArrayLiteral(
+    if (!_setFileRecordChecksumQuery.initOrReset(QByteArrayLiteral(
             "UPDATE metadata"
             " SET contentChecksum = ?2, contentChecksumTypeId = ?3"
             " WHERE phash == ?1;"), _db)) {
@@ -1226,7 +1226,7 @@ bool SyncJournalDb::updateLocalMetadata(const QString &filename,
     }
 
 
-    if (!_setFileRecordLocalMetadataQuery.init(QByteArrayLiteral(
+    if (!_setFileRecordLocalMetadataQuery.initOrReset(QByteArrayLiteral(
             "UPDATE metadata"
             " SET inode=?2, modtime=?3, filesize=?4"
             " WHERE phash == ?1;"), _db)) {
@@ -1299,7 +1299,7 @@ SyncJournalDb::DownloadInfo SyncJournalDb::getDownloadInfo(const QString &file)
 
     if (checkConnect()) {
 
-        if (!_getDownloadInfoQuery.init(QByteArrayLiteral(
+        if (!_getDownloadInfoQuery.initOrReset(QByteArrayLiteral(
                 "SELECT tmpfile, etag, errorcount FROM downloadinfo WHERE path=?1"), _db)) {
             return res;
         }
@@ -1329,7 +1329,7 @@ void SyncJournalDb::setDownloadInfo(const QString &file, const SyncJournalDb::Do
 
 
     if (i._valid) {
-        if (!_setDownloadInfoQuery.init(QByteArrayLiteral(
+        if (!_setDownloadInfoQuery.initOrReset(QByteArrayLiteral(
                 "INSERT OR REPLACE INTO downloadinfo "
                 "(path, tmpfile, etag, errorcount) "
                 "VALUES ( ?1 , ?2, ?3, ?4 )"), _db)) {
@@ -1408,7 +1408,7 @@ SyncJournalDb::UploadInfo SyncJournalDb::getUploadInfo(const QString &file)
     UploadInfo res;
 
     if (checkConnect()) {
-        if (!_getUploadInfoQuery.init(QByteArrayLiteral(
+        if (!_getUploadInfoQuery.initOrReset(QByteArrayLiteral(
                 "SELECT chunk, transferid, errorcount, size, modtime, contentChecksum FROM "
                 "uploadinfo WHERE path=?1"), _db)) {
             return res;
@@ -1442,7 +1442,7 @@ void SyncJournalDb::setUploadInfo(const QString &file, const SyncJournalDb::Uplo
     }
 
     if (i._valid) {
-        if (!_setUploadInfoQuery.init(QByteArrayLiteral(
+        if (!_setUploadInfoQuery.initOrReset(QByteArrayLiteral(
             "INSERT OR REPLACE INTO uploadinfo "
             "(path, chunk, transferid, errorcount, size, modtime, contentChecksum) "
             "VALUES ( ?1 , ?2, ?3 , ?4 ,  ?5, ?6 , ?7 )"), _db)) {
@@ -1641,7 +1641,7 @@ void SyncJournalDb::setErrorBlacklistEntry(const SyncJournalErrorBlacklistRecord
         return;
     }
 
-    if (!_setErrorBlacklistQuery.init(QByteArrayLiteral(
+    if (!_setErrorBlacklistQuery.initOrReset(QByteArrayLiteral(
         "INSERT OR REPLACE INTO blacklist "
         "(path, lastTryEtag, lastTryModtime, retrycount, errorstring, lastTryTime, ignoreDuration, renameTarget, errorCategory) "
         "VALUES ( ?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9)"), _db)) {
@@ -1719,7 +1719,7 @@ QStringList SyncJournalDb::getSelectiveSyncList(SyncJournalDb::SelectiveSyncList
         return result;
     }
 
-    if (!_getSelectiveSyncListQuery.init(QByteArrayLiteral("SELECT path FROM selectivesync WHERE type=?1"), _db)) {
+    if (!_getSelectiveSyncListQuery.initOrReset(QByteArrayLiteral("SELECT path FROM selectivesync WHERE type=?1"), _db)) {
         *ok = false;
         return result;
     }
@@ -1843,7 +1843,7 @@ QByteArray SyncJournalDb::getChecksumType(int checksumTypeId)
 
     // Retrieve the id
     auto &query = _getChecksumTypeQuery;
-    if (!query.init(QByteArrayLiteral("SELECT name FROM checksumtype WHERE id=?1"), _db))
+    if (!query.initOrReset(QByteArrayLiteral("SELECT name FROM checksumtype WHERE id=?1"), _db))
         return {};
     query.bindValue(1, checksumTypeId);
     if (!query.exec()) {
@@ -1864,7 +1864,7 @@ int SyncJournalDb::mapChecksumType(const QByteArray &checksumType)
     }
 
     // Ensure the checksum type is in the db
-    if (!_insertChecksumTypeQuery.init(QByteArrayLiteral("INSERT OR IGNORE INTO checksumtype (name) VALUES (?1)"), _db))
+    if (!_insertChecksumTypeQuery.initOrReset(QByteArrayLiteral("INSERT OR IGNORE INTO checksumtype (name) VALUES (?1)"), _db))
         return 0;
     _insertChecksumTypeQuery.bindValue(1, checksumType);
     if (!_insertChecksumTypeQuery.exec()) {
@@ -1872,7 +1872,7 @@ int SyncJournalDb::mapChecksumType(const QByteArray &checksumType)
     }
 
     // Retrieve the id
-    if (!_getChecksumTypeIdQuery.init(QByteArrayLiteral("SELECT id FROM checksumtype WHERE name=?1"), _db))
+    if (!_getChecksumTypeIdQuery.initOrReset(QByteArrayLiteral("SELECT id FROM checksumtype WHERE name=?1"), _db))
         return 0;
     _getChecksumTypeIdQuery.bindValue(1, checksumType);
     if (!_getChecksumTypeIdQuery.exec()) {
@@ -1893,7 +1893,7 @@ QByteArray SyncJournalDb::dataFingerprint()
         return QByteArray();
     }
 
-    if (!_getDataFingerprintQuery.init(QByteArrayLiteral("SELECT fingerprint FROM datafingerprint"), _db))
+    if (!_getDataFingerprintQuery.initOrReset(QByteArrayLiteral("SELECT fingerprint FROM datafingerprint"), _db))
         return QByteArray();
 
     if (!_getDataFingerprintQuery.exec()) {
@@ -1913,8 +1913,8 @@ void SyncJournalDb::setDataFingerprint(const QByteArray &dataFingerprint)
         return;
     }
 
-    if (!_setDataFingerprintQuery1.init(QByteArrayLiteral("DELETE FROM datafingerprint;"), _db)
-        || !_setDataFingerprintQuery2.init(QByteArrayLiteral("INSERT INTO datafingerprint (fingerprint) VALUES (?1);"), _db)) {
+    if (!_setDataFingerprintQuery1.initOrReset(QByteArrayLiteral("DELETE FROM datafingerprint;"), _db)
+        || !_setDataFingerprintQuery2.initOrReset(QByteArrayLiteral("INSERT INTO datafingerprint (fingerprint) VALUES (?1);"), _db)) {
         return;
     }
 
@@ -1931,7 +1931,7 @@ void SyncJournalDb::setConflictRecord(const ConflictRecord &record)
         return;
 
     auto &query = _setConflictRecordQuery;
-    ASSERT(query.init(QByteArrayLiteral(
+    ASSERT(query.initOrReset(QByteArrayLiteral(
                           "INSERT OR REPLACE INTO conflicts "
                           "(path, baseFileId, baseModtime, baseEtag) "
                           "VALUES (?1, ?2, ?3, ?4);"),
@@ -1951,7 +1951,7 @@ ConflictRecord SyncJournalDb::conflictRecord(const QByteArray &path)
     if (!checkConnect())
         return entry;
     auto &query = _getConflictRecordQuery;
-    ASSERT(query.init(QByteArrayLiteral("SELECT baseFileId, baseModtime, baseEtag FROM conflicts WHERE path=?1;"), _db));
+    ASSERT(query.initOrReset(QByteArrayLiteral("SELECT baseFileId, baseModtime, baseEtag FROM conflicts WHERE path=?1;"), _db));
     query.bindValue(1, path);
     ASSERT(query.exec());
     if (!query.next())
@@ -1970,7 +1970,7 @@ void SyncJournalDb::deleteConflictRecord(const QByteArray &path)
     if (!checkConnect())
         return;
 
-    ASSERT(_deleteConflictRecordQuery.init("DELETE FROM conflicts WHERE path=?1;", _db));
+    ASSERT(_deleteConflictRecordQuery.initOrReset("DELETE FROM conflicts WHERE path=?1;", _db));
     _deleteConflictRecordQuery.bindValue(1, path);
     ASSERT(_deleteConflictRecordQuery.exec());
 }

test/testownsql.cpp

@@ -126,6 +126,21 @@ private slots:
         }
     }
 
+    void testDestructor()
+    {
+        // This test make sure that the destructor of SqlQuery works even if the SqlDatabase
+        // is destroyed before
+        QScopedPointer<SqlDatabase> db(new SqlDatabase());
+        SqlQuery q1(_db);
+        SqlQuery q2(_db);
+        q2.prepare("SELECT * FROM addresses");
+        SqlQuery q3("SELECT * FROM addresses", _db);
+        SqlQuery q4;
+        SqlQuery q5;
+        q5.initOrReset("SELECT * FROM addresses", _db);
+        db.reset();
+    }
+
 private:
     SqlDatabase _db;
 };