[spatialite] Fix pk-less queries cannot be retrieved by id

Fixes #1993 - Zoom to feature" does not work

This actually fixes many more bugs related with the
QGIS generated feature id (incremental) when using
queries that was not consistent when using
QgsFeature request with ids or bbox.

This is not a regression: the same error is in 2.18

src/providers/spatialite/qgsspatialitefeatureiterator.cpp

@@ -355,6 +355,8 @@ bool QgsSpatiaLiteFeatureIterator::prepareStatement( const QString &whereClause,
     if ( limit >= 0 )
       sql += QStringLiteral( " LIMIT %1" ).arg( limit );
 
+    qDebug() << sql;
+
     if ( sqlite3_prepare_v2( mHandle->handle(), sql.toUtf8().constData(), -1, &sqliteStatement, nullptr ) != SQLITE_OK )
     {
       // some error occurred

src/providers/spatialite/qgsspatialiteprovider.cpp

@@ -1066,6 +1066,9 @@ void QgsSpatiaLiteProvider::loadFields()
 
         if ( name == mPrimaryKey )
         {
+          // Skip if ROWID has been added to the query by the provider
+          if ( mRowidInjectedInQuery )
+            continue;
           pkCount++;
           if ( mPrimaryKeyAttrs.isEmpty() )
             pkName = name;
@@ -4583,10 +4586,51 @@ bool QgsSpatiaLiteProvider::checkLayerType()
              .arg( mQuery,
                    quotedIdentifier( alias ) );
 
-    sql = QStringLiteral( "SELECT 0 FROM %1 LIMIT 1" ).arg( mQuery );
+    sql = QStringLiteral( "SELECT 0, %1 FROM %2 LIMIT 1" ).arg( quotedIdentifier( mGeometryColumn ), mQuery );
     ret = sqlite3_get_table( mSqliteHandle, sql.toUtf8().constData(), &results, &rows, &columns, &errMsg );
     if ( ret == SQLITE_OK && rows == 1 )
     {
+      // Check if we can get use the ROWID from the table that provides the geometry
+      sqlite3_stmt *stmt = nullptr;
+      // 1. find the table that provides geometry
+      if ( sqlite3_prepare_v2( mSqliteHandle, sql.toUtf8().constData(), -1, &stmt, nullptr ) == SQLITE_OK )
+      {
+        mQueryGeomTableName = sqlite3_column_table_name( stmt, 1 );
+      }
+      // 2. check if the table has a useable ROWID
+      if ( ! mQueryGeomTableName.isEmpty() )
+      {
+        sql = QStringLiteral( "SELECT ROWID FROM %1 WHERE ROWID IS NOT NULL LIMIT 1" ).arg( quotedIdentifier( mQueryGeomTableName ) );
+        ret = sqlite3_get_table( mSqliteHandle, sql.toUtf8().constData(), &results, &rows, &columns, &errMsg );
+        if ( ret != SQLITE_OK || rows != 1 )
+        {
+          mQueryGeomTableName = QString();
+        }
+      }
+      // 3. check if ROWID injection works
+      if ( ! mQueryGeomTableName.isEmpty() )
+      {
+        QString newSql( mQuery.replace( QStringLiteral( "SELECT " ),
+                                        QStringLiteral( "SELECT %1.%2, " )
+                                        .arg( quotedIdentifier( mQueryGeomTableName ), QStringLiteral( "ROWID" ) ),
+                                        Qt::CaseInsensitive ) );
+        sql = QStringLiteral( "SELECT ROWID FROM %1 WHERE ROWID IS NOT NULL LIMIT 1" ).arg( newSql );
+        ret = sqlite3_get_table( mSqliteHandle, sql.toUtf8().constData(), &results, &rows, &columns, &errMsg );
+        if ( ret == SQLITE_OK && rows == 1 )
+        {
+          mQuery = newSql;
+          mPrimaryKey = QStringLiteral( "ROWID" );
+          mRowidInjectedInQuery = true;
+        }
+      }
+      // 4. if it does not work, simply clear the message and fallback to the original behavior
+      if ( errMsg )
+      {
+        QgsMessageLog::logMessage( tr( "SQLite error while trying to inject ROWID: %2\nSQL: %1" ).arg( sql, errMsg ), tr( "SpatiaLite" ) );
+        sqlite3_free( errMsg );
+        errMsg = nullptr;
+      }
+      sqlite3_finalize( stmt );
       mIsQuery = true;
       mReadOnly = true;
       count++;

src/providers/spatialite/qgsspatialiteprovider.h

@@ -230,6 +230,12 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider
     //! Flag indicating if the layer data source is based on a query
     bool mIsQuery = false;
 
+    //! String containing the name of the table that provides the geometry if the layer data source is based on a query
+    QString mQueryGeomTableName;
+
+    //! Flag indicating if ROWID has been injected in the query
+    bool mRowidInjectedInQuery = false;
+
     //! Flag indicating if the layer data source is based on a plain Table
     bool mTableBased = false;
 

tests/src/python/test_provider_spatialite.py

@@ -91,7 +91,7 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_pg', 'geometry', 4326, 'POLYGON', 'XY')"
         cur.execute(sql)
         sql = "INSERT INTO test_pg (id, name, geometry) "
-        sql += "VALUES (1, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        sql += "VALUES (1, 'toto 1', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
         cur.execute(sql)
 
         # table with Z dimension geometry
@@ -100,7 +100,7 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_z', 'geometry', 4326, 'POINT', 'XYZ')"
         cur.execute(sql)
         sql = "INSERT INTO test_z (id, name, geometry) "
-        sql += "VALUES (1, 'toto', GeomFromText('POINT Z (0 0 1)', 4326))"
+        sql += "VALUES (1, 'toto 2', GeomFromText('POINT Z (0 0 1)', 4326))"
         cur.execute(sql)
 
         # table with M value geometry
@@ -109,7 +109,7 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_m', 'geometry', 4326, 'POINT', 'XYM')"
         cur.execute(sql)
         sql = "INSERT INTO test_m (id, name, geometry) "
-        sql += "VALUES (1, 'toto', GeomFromText('POINT M (0 0 1)', 4326))"
+        sql += "VALUES (1, 'toto 3', GeomFromText('POINT M (0 0 1)', 4326))"
         cur.execute(sql)
 
         # table with Z dimension and M value geometry
@@ -118,7 +118,7 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_zm', 'geometry', 4326, 'POINT', 'XYZM')"
         cur.execute(sql)
         sql = "INSERT INTO test_zm (id, name, geometry) "
-        sql += "VALUES (1, 'toto', GeomFromText('POINT ZM (0 0 1 1)', 4326))"
+        sql += "VALUES (1, 'toto 1', GeomFromText('POINT ZM (0 0 1 1)', 4326))"
         cur.execute(sql)
 
         # table with multiple column primary key
@@ -127,7 +127,7 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_pg_mk', 'geometry', 4326, 'POLYGON', 'XY')"
         cur.execute(sql)
         sql = "INSERT INTO test_pg_mk (id, name, geometry) "
-        sql += "VALUES (1, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        sql += "VALUES (1, 'toto 1', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
         cur.execute(sql)
 
         # simple table with primary key
@@ -136,10 +136,10 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_q', 'geometry', 4326, 'POLYGON', 'XY')"
         cur.execute(sql)
         sql = "INSERT INTO test_q (id, name, geometry) "
-        sql += "VALUES (11, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        sql += "VALUES (11, 'toto 11', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
         cur.execute(sql)
         sql = "INSERT INTO test_q (id, name, geometry) "
-        sql += "VALUES (21, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        sql += "VALUES (21, 'toto 12', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
         cur.execute(sql)
 
         # simple table with a geometry column named 'Geometry'
@@ -148,10 +148,10 @@ def setUpClass(cls):
         sql = "SELECT AddGeometryColumn('test_n', 'Geometry', 4326, 'POLYGON', 'XY')"
         cur.execute(sql)
         sql = "INSERT INTO test_n (id, name, geometry) "
-        sql += "VALUES (1, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        sql += "VALUES (1, 'toto 1', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
         cur.execute(sql)
         sql = "INSERT INTO test_n (id, name, geometry) "
-        sql += "VALUES (2, 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
+        sql += "VALUES (2, 'toto 1', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))"
         cur.execute(sql)
 
         # table with different array types, stored as JSON
@@ -357,7 +357,7 @@ def test_queries(self):
         sum_id1 = sum(f.id() for f in l.getFeatures())
         # the attribute 'id' works
         sum_id2 = sum(f.attributes()[0] for f in l.getFeatures())
-        self.assertEqual(sum_id1, 3)   # 1+2
+        self.assertEqual(sum_id1, 32)  # 11 + 21
         self.assertEqual(sum_id2, 32)  # 11 + 21
 
         # and now with an id declared
@@ -789,6 +789,65 @@ def testLoadStyle(self):
         err, ok = vl.loadDefaultStyle()
         self.assertTrue(ok)
 
+    def testPkLessQuery(self):
+        """Test if features in queries with/without pk can be retrieved by id"""
+        # create test db
+        dbname = os.path.join(tempfile.gettempdir(), "test_pkless.sqlite")
+        if os.path.exists(dbname):
+            os.remove(dbname)
+        con = spatialite_connect(dbname, isolation_level=None)
+        cur = con.cursor()
+        cur.execute("BEGIN")
+        sql = "SELECT InitSpatialMetadata()"
+        cur.execute(sql)
+
+        # simple table with primary key
+        sql = "CREATE TABLE test_pk (id INTEGER NOT NULL PRIMARY KEY, name TEXT NOT NULL)"
+        cur.execute(sql)
+
+        sql = "SELECT AddGeometryColumn('test_pk', 'geometry', 4326, 'POINT', 'XY')"
+        cur.execute(sql)
+
+        for i in range(11, 21):
+            sql = "INSERT INTO test_pk (id, name, geometry) "
+            sql += "VALUES ({id}, 'name {id}', GeomFromText('POINT({id} {id})', 4326))".format(id=i)
+            cur.execute(sql)
+
+        # simple table without primary key
+        sql = "CREATE TABLE test_no_pk (name TEXT NOT NULL)"
+        cur.execute(sql)
+
+        sql = "SELECT AddGeometryColumn('test_no_pk', 'geometry', 4326, 'POINT', 'XY')"
+        cur.execute(sql)
+
+        for i in range(11, 21):
+            sql = "INSERT INTO test_no_pk (name, geometry) "
+            sql += "VALUES ('name {id}', GeomFromText('POINT({id} {id})', 4326))".format(id=i)
+            cur.execute(sql)
+
+        cur.execute("COMMIT")
+        con.close()
+
+        def _check_features(vl, offset):
+            self.assertEqual(vl.featureCount(), 10)
+            i = 11
+            for f in vl.getFeatures():
+                self.assertTrue(f.isValid())
+                self.assertTrue(vl.getFeature(i - offset).isValid())
+                self.assertEqual(vl.getFeature(i - offset)['name'], 'name {id}'.format(id=i))
+                self.assertEqual(f.id(), i - offset)
+                self.assertEqual(f['name'], 'name {id}'.format(id=i))
+                self.assertEqual(f.geometry().asWkt(), 'Point ({id} {id})'.format(id=i))
+                i += 1
+
+        vl_pk = QgsVectorLayer('dbname=\'%s\' table="(select * from test_pk)" (geometry) sql=' % dbname, 'pk', 'spatialite')
+        self.assertTrue(vl_pk.isValid())
+        _check_features(vl_pk, 0)
+
+        vl_no_pk = QgsVectorLayer('dbname=\'%s\' table="(select * from test_no_pk)" (geometry) sql=' % dbname, 'pk', 'spatialite')
+        self.assertTrue(vl_no_pk.isValid())
+        _check_features(vl_no_pk, 10)
+
 
 if __name__ == '__main__':
     unittest.main()