Sqlite: Generalize invalid id handling

An id is now always saved as a null value and a null value is always
converted to an invalid id if you are requesting an id. It prevents
accidental conversion of null values into a id with the value of zero.
It prevent too writing of the value -1 for an id. This has two
advantages. Sqlite can optimize null values if there are only null
values following in a row. And with strict tables it is forbidden to use
a null value as a key. So there can be no accidential invalid ids
anymore in the database.

Change-Id: I5ec813f2fe8e686324eab6dd632c03d5007e093d
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Tim Jenssen <tim.jenssen@qt.io>

src/libs/sqlite/sqlitebasestatement.cpp

@@ -137,13 +137,18 @@ void BaseStatement::step() const
     next();
 }
 
-void BaseStatement::bind(int index, NullValue)
+void BaseStatement::bindNull(int index)
 {
     int resultCode = sqlite3_bind_null(m_compiledStatement.get(), index);
     if (resultCode != SQLITE_OK)
         checkForBindingError(resultCode);
 }
 
+void BaseStatement::bind(int index, NullValue)
+{
+    bindNull(index);
+}
+
 void BaseStatement::bind(int index, int value)
 {
     int resultCode = sqlite3_bind_int(m_compiledStatement.get(), index, value);

src/libs/sqlite/sqlitebasestatement.h

@@ -89,6 +89,7 @@ public:
     template<typename Type>
     Type fetchValue(int column) const;
 
+    void bindNull(int index);
     void bind(int index, NullValue);
     void bind(int index, int value);
     void bind(int index, long long value);
@@ -106,7 +107,10 @@ public:
     template<typename Type, typename = std::enable_if_t<Type::IsBasicId::value>>
     void bind(int index, Type id)
     {
-        bind(index, &id);
+        if (id)
+            bind(index, &id);
+        else
+            bindNull(index);
     }
 
     template<typename Enumeration, std::enable_if_t<std::is_enum_v<Enumeration>, bool> = true>
@@ -477,10 +481,14 @@ private:
                  typename = std::enable_if_t<ConversionType::IsBasicId::value>>
         constexpr operator ConversionType()
         {
-            if constexpr (std::is_same_v<typename ConversionType::DatabaseType, int>)
+            if (statement.fetchType(column) == Type::Integer) {
-                return ConversionType::create(statement.fetchIntValue(column));
+                if constexpr (std::is_same_v<typename ConversionType::DatabaseType, int>)
-            else
+                    return ConversionType::create(statement.fetchIntValue(column));
-                return ConversionType::create(statement.fetchLongLongValue(column));
+                else
+                    return ConversionType::create(statement.fetchLongLongValue(column));
+            }
+
+            return ConversionType{};
         }
 
         template<typename Enumeration, std::enable_if_t<std::is_enum_v<Enumeration>, bool> = true>

src/plugins/qmldesigner/designercore/projectstorage/projectstorage.h

@@ -2556,7 +2556,7 @@ public:
         "    UNION ALL "
         "      SELECT prototypeId, typeSelection.level+1 FROM types JOIN typeSelection "
         "        USING(typeId) WHERE prototypeId IS NOT NULL)"
-        "SELECT nullif(propertyTypeId, -1), propertyDeclarationId, propertyTraits "
+        "SELECT propertyTypeId, propertyDeclarationId, propertyTraits "
         "  FROM propertyDeclarations JOIN typeSelection USING(typeId) "
         "  WHERE name=?2 ORDER BY level LIMIT 1",
         database};
@@ -2588,16 +2588,16 @@ public:
     mutable ReadStatement<3> selectAllSourcesStatement{
         "SELECT sourceName, sourceContextId, sourceId  FROM sources", database};
     mutable ReadStatement<6, 1> selectTypeByTypeIdStatement{
-        "SELECT sourceId, t.name, t.typeId, ifnull(prototypeId, -1), accessSemantics, pd.name "
+        "SELECT sourceId, t.name, t.typeId, prototypeId, accessSemantics, pd.name "
         "FROM types AS t LEFT JOIN propertyDeclarations AS pd "
         "  ON defaultPropertyId=propertyDeclarationId WHERE t.typeId=?",
         database};
     mutable ReadStatement<4, 1> selectExportedTypesByTypeIdStatement{
-        "SELECT moduleId, name, ifnull(majorVersion, -1), ifnull(minorVersion, -1) FROM "
+        "SELECT moduleId, name, majorVersion, minorVersion FROM "
         "exportedTypeNames WHERE typeId=?",
         database};
     mutable ReadStatement<6> selectTypesStatement{
-        "SELECT sourceId, t.name, t.typeId, ifnull(prototypeId, -1), accessSemantics, pd.name "
+        "SELECT sourceId, t.name, t.typeId, prototypeId, accessSemantics, pd.name "
         "FROM types AS t LEFT JOIN propertyDeclarations AS pd "
         "  ON defaultPropertyId=propertyDeclarationId",
         database};
@@ -2617,13 +2617,13 @@ public:
         "DELETE FROM signalDeclarations WHERE typeId=?", database};
     WriteStatement<1> deleteTypeStatement{"DELETE FROM types  WHERE typeId=?", database};
     mutable ReadStatement<4, 1> selectPropertyDeclarationsByTypeIdStatement{
-        "SELECT name, nullif(propertyTypeId, -1), propertyTraits, (SELECT name FROM "
+        "SELECT name, propertyTypeId, propertyTraits, (SELECT name FROM "
         "propertyDeclarations WHERE propertyDeclarationId=pd.aliasPropertyDeclarationId) FROM "
         "propertyDeclarations AS pd WHERE typeId=?",
         database};
     ReadStatement<6, 1> selectPropertyDeclarationsForTypeIdStatement{
         "SELECT name, propertyTraits, propertyTypeId, propertyImportedTypeNameId, "
-        "propertyDeclarationId, ifnull(aliasPropertyDeclarationId, -1) FROM propertyDeclarations "
+        "propertyDeclarationId, aliasPropertyDeclarationId FROM propertyDeclarations "
         "WHERE typeId=? ORDER BY name",
         database};
     ReadWriteStatement<1, 5> insertPropertyDeclarationStatement{
@@ -2756,7 +2756,7 @@ public:
         "name=?3",
         database};
     mutable ReadStatement<5, 2> selectDocumentImportForSourceIdStatement{
-        "SELECT importId, sourceId, moduleId, ifnull(majorVersion, -1), ifnull(minorVersion, -1) "
+        "SELECT importId, sourceId, moduleId, majorVersion, minorVersion "
         "FROM documentImports WHERE sourceId IN carray(?1) AND kind=?2 ORDER BY sourceId, "
         "moduleId, majorVersion, minorVersion",
         database};
@@ -2803,7 +2803,7 @@ public:
         database};
     ReadStatement<5, 1> selectAliasPropertiesDeclarationForPropertiesWithTypeIdStatement{
         "SELECT alias.typeId, alias.propertyDeclarationId, alias.propertyImportedTypeNameId, "
-        "alias.aliasPropertyDeclarationId, ifnull(alias.aliasPropertyDeclarationTailId, -1) FROM "
+        "alias.aliasPropertyDeclarationId, alias.aliasPropertyDeclarationTailId FROM "
         "propertyDeclarations AS alias JOIN propertyDeclarations AS target ON "
         "alias.aliasPropertyDeclarationId=target.propertyDeclarationId OR "
         "alias.aliasPropertyDeclarationTailId=target.propertyDeclarationId WHERE "
@@ -3074,9 +3074,9 @@ public:
         "  USING(typeId) ORDER BY name",
         database};
     mutable ReadStatement<2> selectTypesWithDefaultPropertyStatement{
-        "SELECT typeId, ifnull(defaultPropertyId, -1) FROM types ORDER BY typeId", database};
+        "SELECT typeId, defaultPropertyId FROM types ORDER BY typeId", database};
     WriteStatement<2> updateDefaultPropertyIdStatement{
-        "UPDATE types SET defaultPropertyId=nullif(?2, -1) WHERE typeId=?1", database};
+        "UPDATE types SET defaultPropertyId=?2 WHERE typeId=?1", database};
     WriteStatement<1> updateDefaultPropertyIdToNullStatement{
         "UPDATE types SET defaultPropertyId=NULL WHERE defaultPropertyId=?1", database};
     mutable ReadStatement<1, 1> selectInfoTypeByTypeIdStatement{

tests/unit/unittest/sqlitestatement-test.cpp

@@ -53,6 +53,11 @@ using Sqlite::ReadWriteStatement;
 using Sqlite::Value;
 using Sqlite::WriteStatement;
 
+enum class BasicIdEnumeration { TestId };
+
+using TestLongLongId = Sqlite::BasicId<BasicIdEnumeration::TestId, long long>;
+using TestIntId = Sqlite::BasicId<BasicIdEnumeration::TestId, int>;
+
 template<typename Type>
 bool compareValue(SqliteTestStatement<2, 1> &statement, Type value, int column)
 {
@@ -237,6 +242,18 @@ TEST_F(SqliteStatement, BindNull)
     database.execute("INSERT INTO  test VALUES (NULL, 323, 344)");
     SqliteTestStatement<2, 1> statement("SELECT name, number FROM test WHERE name IS ?", database);
 
+    statement.bindNull(1);
+    statement.next();
+
+    ASSERT_TRUE(statement.fetchValueView(0).isNull());
+    ASSERT_THAT(statement.fetchValue<int>(1), 323);
+}
+
+TEST_F(SqliteStatement, BindNullValue)
+{
+    database.execute("INSERT INTO  test VALUES (NULL, 323, 344)");
+    SqliteTestStatement<2, 1> statement("SELECT name, number FROM test WHERE name IS ?", database);
+
     statement.bind(1, Sqlite::NullValue{});
     statement.next();
 
@@ -244,6 +261,60 @@ TEST_F(SqliteStatement, BindNull)
     ASSERT_THAT(statement.fetchValue<int>(1), 323);
 }
 
+TEST_F(SqliteStatement, BindInvalidIntIdToNull)
+{
+    TestIntId id;
+    SqliteTestStatement<0, 1> statement("INSERT INTO  test VALUES ('id', 323, ?)", database);
+
+    statement.bind(1, id);
+    statement.next();
+
+    SqliteTestStatement<1, 1> readStatement("SELECT value FROM test WHERE name='id'", database);
+    readStatement.next();
+    ASSERT_THAT(readStatement.fetchType(0), Sqlite::Type::Null);
+}
+
+TEST_F(SqliteStatement, BindIntId)
+{
+    TestIntId id{TestIntId::create(42)};
+    SqliteTestStatement<0, 1> statement("INSERT INTO test VALUES ('id', 323, ?)", database);
+
+    statement.bind(1, id);
+    statement.next();
+
+    SqliteTestStatement<1, 1> readStatement("SELECT value FROM test WHERE name='id'", database);
+    readStatement.next();
+    ASSERT_THAT(readStatement.fetchType(0), Sqlite::Type::Integer);
+    ASSERT_THAT(readStatement.fetchIntValue(0), 42);
+}
+
+TEST_F(SqliteStatement, BindInvalidLongLongIdToNull)
+{
+    TestLongLongId id;
+    SqliteTestStatement<0, 1> statement("INSERT INTO  test VALUES ('id', 323, ?)", database);
+
+    statement.bind(1, id);
+    statement.next();
+
+    SqliteTestStatement<1, 1> readStatement("SELECT value FROM test WHERE name='id'", database);
+    readStatement.next();
+    ASSERT_THAT(readStatement.fetchType(0), Sqlite::Type::Null);
+}
+
+TEST_F(SqliteStatement, BindLongLongId)
+{
+    TestLongLongId id{TestLongLongId::create(42)};
+    SqliteTestStatement<0, 1> statement("INSERT INTO test VALUES ('id', 323, ?)", database);
+
+    statement.bind(1, id);
+    statement.next();
+
+    SqliteTestStatement<1, 1> readStatement("SELECT value FROM test WHERE name='id'", database);
+    readStatement.next();
+    ASSERT_THAT(readStatement.fetchType(0), Sqlite::Type::Integer);
+    ASSERT_THAT(readStatement.fetchIntValue(0), 42);
+}
+
 TEST_F(SqliteStatement, BindString)
 {
     SqliteTestStatement<2, 1> statement("SELECT name, number FROM test WHERE name=?", database);
@@ -1158,6 +1229,50 @@ TEST_F(SqliteStatement, GetTupleValueAndMultipleQueryValue)
     ASSERT_THAT(value, Eq(Tuple{"bar", "blah", 1}));
 }
 
+TEST_F(SqliteStatement, GetSingleInvalidLongLongId)
+{
+    TestLongLongId id;
+    WriteStatement<1>("INSERT INTO  test VALUES ('id', 323, ?)", database).write(id);
+    ReadStatement<1, 0> statement("SELECT value FROM test WHERE name='id'", database);
+
+    auto value = statement.value<TestLongLongId>();
+
+    ASSERT_FALSE(value.isValid());
+}
+
+TEST_F(SqliteStatement, GetSingleLongLongId)
+{
+    TestLongLongId id{TestLongLongId::create(42)};
+    WriteStatement<1>("INSERT INTO  test VALUES ('id', 323, ?)", database).write(id);
+    ReadStatement<1, 0> statement("SELECT value FROM test WHERE name='id'", database);
+
+    auto value = statement.value<TestLongLongId>();
+
+    ASSERT_THAT(&value, Eq(42));
+}
+
+TEST_F(SqliteStatement, GetSingleInvalidIntId)
+{
+    TestIntId id;
+    WriteStatement<1>("INSERT INTO  test VALUES ('id', 323, ?)", database).write(id);
+    ReadStatement<1, 0> statement("SELECT value FROM test WHERE name='id'", database);
+
+    auto value = statement.value<TestIntId>();
+
+    ASSERT_FALSE(value.isValid());
+}
+
+TEST_F(SqliteStatement, GetSingleIntId)
+{
+    TestIntId id{TestIntId::create(42)};
+    WriteStatement<1>("INSERT INTO  test VALUES ('id', 323, ?)", database).write(id);
+    ReadStatement<1, 0> statement("SELECT value FROM test WHERE name='id'", database);
+
+    auto value = statement.value<TestIntId>();
+
+    ASSERT_THAT(&value, Eq(42));
+}
+
 TEST_F(SqliteStatement, GetValueCallsReset)
 {
     struct Value