Skip to content

Commit

Permalink
[spatialite] Fix pk-less queries cannot be retrieved by id
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@elpaso
elpaso committed Sep 28, 2018
1 parent b85db09 commit a02d448
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 11 deletions.
2 changes: 2 additions & 0 deletions src/providers/spatialite/qgsspatialitefeatureiterator.cpp
View file
Expand Up @@ -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
Expand Down
46 changes: 45 additions & 1 deletion src/providers/spatialite/qgsspatialiteprovider.cpp
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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++;
Expand Down
6 changes: 6 additions & 0 deletions src/providers/spatialite/qgsspatialiteprovider.h
View file
Expand Up @@ -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;

Expand Down
79 changes: 69 additions & 10 deletions tests/src/python/test_provider_spatialite.py
View file
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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'
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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()

0 comments on commit a02d448

Please sign in to comment.