From 7f61dc495491752fd26c7f3bad56adc1f8950e3f Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 8 Feb 2019 16:47:11 +0100 Subject: [PATCH 1/4] Fix spatialite text pks and other issues ... also fixes the geometry information query when spatialite version is >= 4 Fixes #21176 With tests --- .../qgsspatialitefeatureiterator.cpp | 3 +- .../spatialite/qgsspatialiteprovider.cpp | 384 ++++++++++-------- .../spatialite/qgsspatialiteprovider.h | 6 +- tests/src/python/test_provider_spatialite.py | 59 +++ 4 files changed, 284 insertions(+), 168 deletions(-) diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp index ce9f47512761..104772897f67 100644 --- a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp +++ b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp @@ -355,7 +355,7 @@ bool QgsSpatiaLiteFeatureIterator::prepareStatement( const QString &whereClause, if ( limit >= 0 ) sql += QStringLiteral( " LIMIT %1" ).arg( limit ); - // qDebug() << sql; + QgsDebugMsgLevel( sql, 4 ); if ( sqlite3_prepare_v2( mHandle->handle(), sql.toUtf8().constData(), -1, &sqliteStatement, nullptr ) != SQLITE_OK ) { @@ -515,6 +515,7 @@ bool QgsSpatiaLiteFeatureIterator::getFeature( sqlite3_stmt *stmt, QgsFeature &f } else { + QgsDebugMsgLevel( QStringLiteral( "Primary key is not an integer field: setting autoincrement fid" ), 3 ); // autoincrement a row number mRowNumber++; feature.setId( mRowNumber ); diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp index d656b58f2d3c..14c186bf692a 100644 --- a/src/providers/spatialite/qgsspatialiteprovider.cpp +++ b/src/providers/spatialite/qgsspatialiteprovider.cpp @@ -458,6 +458,7 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider return; } + // Setup handle mSqliteHandle = mHandle->handle(); if ( mSqliteHandle ) { @@ -474,19 +475,23 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider } } - bool alreadyDone = false; bool ret = false; gaiaVectorLayersListPtr list = nullptr; gaiaVectorLayerPtr lyr = nullptr; - bool specialCase = false; - if ( mGeometryColumn.isEmpty() ) - specialCase = true; // non-spatial table - if ( mQuery.startsWith( '(' ) && mQuery.endsWith( ')' ) ) - specialCase = true; - if ( !specialCase ) + // Set special cases (views, queries, no geometry specified) + bool specialCase { mGeometryColumn.isEmpty() || + ( mQuery.startsWith( '(' ) &&mQuery.endsWith( ')' ) ) }; + + // Normal case + if ( ! specialCase ) { + // Set pk to ROWID in case the pk passed in the URL is not usable + if ( mPrimaryKey.isEmpty() || ! tablePrimaryKeys( mTableName ).contains( mPrimaryKey ) ) + { + mPrimaryKey = QStringLiteral( "ROWID" ); + } // using v.4.0 Abstract Interface ret = true; list = gaiaGetVectorLayersList( mSqliteHandle, @@ -498,13 +503,9 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider ret = lyr && checkLayerTypeAbstractInterface( lyr ); QgsDebugMsg( QStringLiteral( "Using checkLayerTypeAbstractInterface" ) ); - alreadyDone = true; } - - if ( !alreadyDone ) + else // views, no geometry etc { - // check if this one Layer is based on a Table, View or VirtualShapefile - // by using the traditional methods ret = checkLayerType(); } @@ -517,6 +518,8 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider closeDb(); return; } + + // TODO: move after pk discovery is definitely done? mEnabledCapabilities = mPrimaryKey.isEmpty() ? QgsVectorDataProvider::Capabilities() : ( QgsVectorDataProvider::SelectAtId ); if ( ( mTableBased || mViewBased ) && !mReadOnly ) { @@ -530,8 +533,6 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider mEnabledCapabilities |= QgsVectorDataProvider::CreateAttributeIndex; } - alreadyDone = false; - if ( lyr ) { // using the v.4.0 AbstractInterface @@ -555,10 +556,8 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider // load the columns list loadFieldsAbstractInterface( lyr ); gaiaFreeVectorLayersList( list ); - alreadyDone = true; } - - if ( !alreadyDone ) + else { // using the traditional methods if ( !getGeometryDetails() ) // gets srid and geometry type @@ -579,13 +578,15 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider // load the columns list loadFields(); } + if ( !mSqliteHandle ) { QgsDebugMsg( QStringLiteral( "Invalid SpatiaLite layer" ) ); return; } - if ( mTableBased && hasRowid() && mPrimaryKey.isEmpty() ) + // Fallback to ROWID is pk is empty or not usable after fields configuration + if ( mTableBased && hasRowid() && ( mPrimaryKey.isEmpty() || ! tablePrimaryKeys( mTableName ).contains( mPrimaryKey ) ) ) { mPrimaryKey = QStringLiteral( "ROWID" ); } @@ -748,7 +749,8 @@ void QgsSpatiaLiteProvider::loadFieldsAbstractInterface( gaiaVectorLayerPtr lyr } } - if ( pk.toInt() == 0 ) + if ( pk.toInt() == 0 || ( type.compare( QLatin1String( "integer" ), Qt::CaseSensitivity::CaseInsensitive ) != 0 && + type.compare( QLatin1String( "bigint" ), Qt::CaseSensitivity::CaseInsensitive ) != 0 ) ) continue; if ( mPrimaryKeyAttrs.isEmpty() ) @@ -816,6 +818,35 @@ QString QgsSpatiaLiteProvider::spatialiteVersion() return mSpatialiteVersionInfo; } +bool QgsSpatiaLiteProvider::versionIsAbove( sqlite3 *sqlite_handle, int major, int minor ) +{ + char **results = nullptr; + char *errMsg = nullptr; + int rows, columns; + bool above = false; + int ret = sqlite3_get_table( sqlite_handle, "select spatialite_version()", &results, &rows, &columns, nullptr ); + if ( ret == SQLITE_OK ) + { + if ( rows == 1 && columns == 1 ) + { + QString version = QString::fromUtf8( results[1] ); + QStringList parts = version.split( ' ', QString::SkipEmptyParts ); + if ( !parts.empty() ) + { + QStringList verparts = parts.at( 0 ).split( '.', QString::SkipEmptyParts ); + above = verparts.size() >= 2 && ( verparts.at( 0 ).toInt() > major || ( verparts.at( 0 ).toInt() == major && verparts.at( 1 ).toInt() >= minor ) ); + } + } + sqlite3_free_table( results ); + } + else + { + QgsLogger::warning( QStringLiteral( "SQLite error querying version: %1" ).arg( errMsg ) ); + sqlite3_free( errMsg ); + } + return above; +} + QString QgsSpatiaLiteProvider::tableSchemaCondition( const QgsDataSourceUri &dsUri ) { return dsUri.schema().isEmpty() ? @@ -998,7 +1029,8 @@ void QgsSpatiaLiteProvider::loadFields() QString type = QString::fromUtf8( results[( i * columns ) + 2] ).toLower(); QString pk = results[( i * columns ) + 5]; - if ( pk.toInt() != 0 ) + if ( pk.toInt() != 0 && ( type.compare( QLatin1String( "integer" ), Qt::CaseSensitivity::CaseInsensitive ) == 0 || + type.compare( QLatin1String( "bigint" ), Qt::CaseSensitivity::CaseInsensitive ) == 0 ) ) { // found a Primary Key column pkCount++; @@ -1128,26 +1160,40 @@ QStringList QgsSpatiaLiteProvider::tablePrimaryKeys( const QString &tableName ) QStringList result; const QString sql = QStringLiteral( "PRAGMA table_info(%1)" ).arg( QgsSqliteUtils::quotedIdentifier( tableName ) ); char **results = nullptr; + sqlite3_stmt *stmt = nullptr; int rows; int columns; char *errMsg = nullptr; - int ret = sqlite3_get_table( mSqliteHandle, sql.toUtf8().constData(), &results, &rows, &columns, &errMsg ); - if ( ret == SQLITE_OK ) + if ( sqlite3_prepare_v2( mSqliteHandle, sql.toUtf8().constData(), -1, &stmt, nullptr ) != SQLITE_OK ) { - for ( int row = 1; row <= rows; ++row ) + // some error occurred + QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), + tr( "SpatiaLite" ) ); + } + else + { + int ret = sqlite3_get_table( mSqliteHandle, sql.toUtf8().constData(), &results, &rows, &columns, &errMsg ); + if ( ret == SQLITE_OK ) { - if ( QString::fromUtf8( results[row * columns + 5] ) == QChar( '1' ) ) + for ( int row = 1; row <= rows; ++row ) { - result << QString::fromUtf8( results[row * columns + 1] ); + QString type = QString::fromUtf8( results[( row * columns ) + 2] ).toLower(); + if ( QString::fromUtf8( results[row * columns + 5] ) == QChar( '1' ) && + ( type.compare( QLatin1String( "integer" ), Qt::CaseSensitivity::CaseInsensitive ) == 0 || + type.compare( QLatin1String( "bigint" ), Qt::CaseSensitivity::CaseInsensitive ) == 0 ) ) + { + result << QString::fromUtf8( results[row * columns + 1] ); + } } + sqlite3_free_table( results ); + } + else + { + QgsLogger::warning( QStringLiteral( "SQLite error discovering integer primary keys: %1" ).arg( errMsg ) ); + sqlite3_free( errMsg ); } - sqlite3_free_table( results ); - } - else - { - QgsLogger::warning( QStringLiteral( "SQLite error discovering primary keys: %1" ).arg( errMsg ) ); - sqlite3_free( errMsg ); } + sqlite3_finalize( stmt ); return result; } @@ -3759,49 +3805,49 @@ QSet QgsSpatiaLiteProvider::uniqueValues( int index, int limit ) const { // some error occurred QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); - return uniqueValues; } - - while ( true ) + else { - // this one is an infinitive loop, intended to fetch any row - int ret = sqlite3_step( stmt ); - - if ( ret == SQLITE_DONE ) + while ( true ) { - // there are no more rows to fetch - we can stop looping - break; - } + // this one is an infinitive loop, intended to fetch any row + int ret = sqlite3_step( stmt ); - if ( ret == SQLITE_ROW ) - { - // fetching one column value - switch ( sqlite3_column_type( stmt, 0 ) ) + if ( ret == SQLITE_DONE ) { - case SQLITE_INTEGER: - uniqueValues.insert( QVariant( sqlite3_column_int( stmt, 0 ) ) ); - break; - case SQLITE_FLOAT: - uniqueValues.insert( QVariant( sqlite3_column_double( stmt, 0 ) ) ); - break; - case SQLITE_TEXT: - uniqueValues.insert( QVariant( QString::fromUtf8( ( const char * ) sqlite3_column_text( stmt, 0 ) ) ) ); - break; - default: - uniqueValues.insert( QVariant( mAttributeFields.at( index ).type() ) ); - break; + // there are no more rows to fetch - we can stop looping + break; + } + + if ( ret == SQLITE_ROW ) + { + // fetching one column value + switch ( sqlite3_column_type( stmt, 0 ) ) + { + case SQLITE_INTEGER: + uniqueValues.insert( QVariant( sqlite3_column_int( stmt, 0 ) ) ); + break; + case SQLITE_FLOAT: + uniqueValues.insert( QVariant( sqlite3_column_double( stmt, 0 ) ) ); + break; + case SQLITE_TEXT: + uniqueValues.insert( QVariant( QString::fromUtf8( ( const char * ) sqlite3_column_text( stmt, 0 ) ) ) ); + break; + default: + uniqueValues.insert( QVariant( mAttributeFields.at( index ).type() ) ); + break; + } + } + else + { + QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); + sqlite3_finalize( stmt ); + return uniqueValues; } - } - else - { - QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); - sqlite3_finalize( stmt ); - return uniqueValues; } } sqlite3_finalize( stmt ); - return uniqueValues; } @@ -3839,42 +3885,42 @@ QStringList QgsSpatiaLiteProvider::uniqueStringsMatching( int index, const QStri { // some error occurred QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); - return results; - } - while ( ( limit < 0 || results.size() < limit ) && ( !feedback || !feedback->isCanceled() ) ) + } + else { - // this one is an infinitive loop, intended to fetch any row - int ret = sqlite3_step( stmt ); - - if ( ret == SQLITE_DONE ) + while ( ( limit < 0 || results.size() < limit ) && ( !feedback || !feedback->isCanceled() ) ) { - // there are no more rows to fetch - we can stop looping - break; - } + // this one is an infinitive loop, intended to fetch any row + int ret = sqlite3_step( stmt ); - if ( ret == SQLITE_ROW ) - { - // fetching one column value - switch ( sqlite3_column_type( stmt, 0 ) ) + if ( ret == SQLITE_DONE ) { - case SQLITE_TEXT: - results.append( QString::fromUtf8( ( const char * ) sqlite3_column_text( stmt, 0 ) ) ); - break; - default: - break; + // there are no more rows to fetch - we can stop looping + break; + } + + if ( ret == SQLITE_ROW ) + { + // fetching one column value + switch ( sqlite3_column_type( stmt, 0 ) ) + { + case SQLITE_TEXT: + results.append( QString::fromUtf8( ( const char * ) sqlite3_column_text( stmt, 0 ) ) ); + break; + default: + break; + } + } + else + { + QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); + sqlite3_finalize( stmt ); + return results; } - } - else - { - QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); - sqlite3_finalize( stmt ); - return results; } } - sqlite3_finalize( stmt ); - return results; } @@ -4175,33 +4221,36 @@ bool QgsSpatiaLiteProvider::deleteFeatures( const QgsFeatureIds &id ) pushError( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ) ); return false; } - - for ( QgsFeatureIds::const_iterator it = id.begin(); it != id.end(); ++it ) + else { - // looping on each feature to be deleted - // resetting Prepared Statement and bindings - sqlite3_reset( stmt ); - sqlite3_clear_bindings( stmt ); + for ( QgsFeatureIds::const_iterator it = id.begin(); it != id.end(); ++it ) + { + // looping on each feature to be deleted + // resetting Prepared Statement and bindings + sqlite3_reset( stmt ); + sqlite3_clear_bindings( stmt ); - qint64 fid = FID_TO_NUMBER( *it ); - sqlite3_bind_int64( stmt, 1, fid ); + qint64 fid = FID_TO_NUMBER( *it ); + sqlite3_bind_int64( stmt, 1, fid ); - // performing actual row deletion - ret = sqlite3_step( stmt ); - if ( ret == SQLITE_DONE || ret == SQLITE_ROW ) - { - mNumberFeatures--; - } - else - { - // some unexpected error occurred - const char *err = sqlite3_errmsg( mSqliteHandle ); - errMsg = ( char * ) sqlite3_malloc( ( int ) strlen( err ) + 1 ); - strcpy( errMsg, err ); - handleError( sql, errMsg, true ); - return false; + // performing actual row deletion + ret = sqlite3_step( stmt ); + if ( ret == SQLITE_DONE || ret == SQLITE_ROW ) + { + mNumberFeatures--; + } + else + { + // some unexpected error occurred + const char *err = sqlite3_errmsg( mSqliteHandle ); + errMsg = ( char * ) sqlite3_malloc( ( int ) strlen( err ) + 1 ); + strcpy( errMsg, err ); + handleError( sql, errMsg, true ); + return false; + } } } + sqlite3_finalize( stmt ); ret = sqlite3_exec( mSqliteHandle, "COMMIT", nullptr, nullptr, &errMsg ); @@ -4408,40 +4457,42 @@ bool QgsSpatiaLiteProvider::changeGeometryValues( const QgsGeometryMap &geometry { // some error occurred QgsMessageLog::logMessage( tr( "SQLite error: %2\nSQL: %1" ).arg( sql, sqlite3_errmsg( mSqliteHandle ) ), tr( "SpatiaLite" ) ); - return false; } - - for ( QgsGeometryMap::const_iterator iter = geometry_map.constBegin(); iter != geometry_map.constEnd(); ++iter ) + else { - // resetting Prepared Statement and bindings - sqlite3_reset( stmt ); - sqlite3_clear_bindings( stmt ); - - // binding GEOMETRY to Prepared Statement - unsigned char *wkb = nullptr; - int wkb_size; - QByteArray iterWkb = iter->asWkb(); - convertFromGeosWKB( reinterpret_cast( iterWkb.constData() ), iterWkb.length(), &wkb, &wkb_size, nDims ); - if ( !wkb ) - sqlite3_bind_null( stmt, 1 ); - else - sqlite3_bind_blob( stmt, 1, wkb, wkb_size, deleteWkbBlob ); - sqlite3_bind_int64( stmt, 2, FID_TO_NUMBER( iter.key() ) ); - - // performing actual row update - ret = sqlite3_step( stmt ); - if ( ret == SQLITE_DONE || ret == SQLITE_ROW ) - ; - else + for ( QgsGeometryMap::const_iterator iter = geometry_map.constBegin(); iter != geometry_map.constEnd(); ++iter ) { - // some unexpected error occurred - const char *err = sqlite3_errmsg( mSqliteHandle ); - errMsg = ( char * ) sqlite3_malloc( ( int ) strlen( err ) + 1 ); - strcpy( errMsg, err ); - handleError( sql, errMsg, true ); - return false; + // resetting Prepared Statement and bindings + sqlite3_reset( stmt ); + sqlite3_clear_bindings( stmt ); + + // binding GEOMETRY to Prepared Statement + unsigned char *wkb = nullptr; + int wkb_size; + QByteArray iterWkb = iter->asWkb(); + convertFromGeosWKB( reinterpret_cast( iterWkb.constData() ), iterWkb.length(), &wkb, &wkb_size, nDims ); + if ( !wkb ) + sqlite3_bind_null( stmt, 1 ); + else + sqlite3_bind_blob( stmt, 1, wkb, wkb_size, deleteWkbBlob ); + sqlite3_bind_int64( stmt, 2, FID_TO_NUMBER( iter.key() ) ); + + // performing actual row update + ret = sqlite3_step( stmt ); + if ( ret == SQLITE_DONE || ret == SQLITE_ROW ) + ; + else + { + // some unexpected error occurred + const char *err = sqlite3_errmsg( mSqliteHandle ); + errMsg = ( char * ) sqlite3_malloc( ( int ) strlen( err ) + 1 ); + strcpy( errMsg, err ); + handleError( sql, errMsg, true ); + return false; + } } } + sqlite3_finalize( stmt ); ret = sqlite3_exec( mSqliteHandle, "COMMIT", nullptr, nullptr, &errMsg ); @@ -4467,8 +4518,8 @@ bool QgsSpatiaLiteProvider::skipConstraintCheck( int fieldIndex, QgsFieldConstra { Q_UNUSED( constraint ); - // If the field is the primary key, skip in case it's autog-enerated / auto-incrementing - if ( mAttributeFields.at( fieldIndex ).name() == mPrimaryKey && mPrimaryKeyAutoIncrement ) + // If the field is the primary key, skip in case it's autogenerated / auto-incrementing + if ( mAttributeFields.at( fieldIndex ).name() == mPrimaryKey && mPrimaryKeyAutoIncrement ) { const QVariant defVal = mDefaultValues.value( fieldIndex ); return defVal.toInt() == value.toInt(); @@ -4964,9 +5015,19 @@ bool QgsSpatiaLiteProvider::getTableGeometryDetails() mIndexTable = mTableName; mIndexGeometry = mGeometryColumn; - QString sql = QString( "SELECT type, srid, spatial_index_enabled, coord_dimension FROM geometry_columns" - " WHERE upper(f_table_name) = upper(%1) and upper(f_geometry_column) = upper(%2)" ).arg( QgsSqliteUtils::quotedString( mTableName ), - QgsSqliteUtils::quotedString( mGeometryColumn ) ); + QString sql; + if ( ! versionIsAbove( mSqliteHandle, 3, 1 ) ) + { + sql = QString( "SELECT type, srid, spatial_index_enabled, coord_dimension FROM geometry_columns" + " WHERE upper(f_table_name) = upper(%1) and upper(f_geometry_column) = upper(%2)" ).arg( QgsSqliteUtils::quotedString( mTableName ), + QgsSqliteUtils::quotedString( mGeometryColumn ) ); + } + else + { + sql = QString( "SELECT geometry_type, srid, spatial_index_enabled, coord_dimension FROM geometry_columns" + " WHERE upper(f_table_name) = upper(%1) and upper(f_geometry_column) = upper(%2)" ).arg( QgsSqliteUtils::quotedString( mTableName ), + QgsSqliteUtils::quotedString( mGeometryColumn ) ); + } ret = sqlite3_get_table( mSqliteHandle, sql.toUtf8().constData(), &results, &rows, &columns, &errMsg ); if ( ret != SQLITE_OK ) @@ -4985,30 +5046,31 @@ bool QgsSpatiaLiteProvider::getTableGeometryDetails() QString spatialIndex = results[( i * columns ) + 2]; QString dims = results[( i * columns ) + 3]; - if ( fType == QLatin1String( "POINT" ) ) + if ( fType == QLatin1String( "POINT" ) || fType == QLatin1String( "1" ) ) { mGeomType = QgsWkbTypes::Point; } - else if ( fType == QLatin1String( "MULTIPOINT" ) ) + else if ( fType == QLatin1String( "MULTIPOINT" ) || fType == QLatin1String( "4" ) ) { mGeomType = QgsWkbTypes::MultiPoint; } - else if ( fType == QLatin1String( "LINESTRING" ) ) + else if ( fType == QLatin1String( "LINESTRING" ) || fType == QLatin1String( "2" ) ) { mGeomType = QgsWkbTypes::LineString; } - else if ( fType == QLatin1String( "MULTILINESTRING" ) ) + else if ( fType == QLatin1String( "MULTILINESTRING" ) || fType == QLatin1String( "5" ) ) { mGeomType = QgsWkbTypes::MultiLineString; } - else if ( fType == QLatin1String( "POLYGON" ) ) + else if ( fType == QLatin1String( "POLYGON" ) || fType == QLatin1String( "3" ) ) { mGeomType = QgsWkbTypes::Polygon; } - else if ( fType == QLatin1String( "MULTIPOLYGON" ) ) + else if ( fType == QLatin1String( "MULTIPOLYGON" ) || fType == QLatin1String( "6" ) ) { mGeomType = QgsWkbTypes::MultiPolygon; } + mSrid = xSrid.toInt(); if ( spatialIndex.toInt() == 1 ) { @@ -5032,7 +5094,7 @@ bool QgsSpatiaLiteProvider::getTableGeometryDetails() nDims = GAIA_XY_M; mGeomType = QgsWkbTypes::addM( mGeomType ); } - else if ( dims == QLatin1String( "XYZM" ) ) + else if ( dims == QLatin1String( "XYZM" ) || dims == QLatin1String( "4" ) ) { nDims = GAIA_XY_Z_M; mGeomType = QgsWkbTypes::zmType( mGeomType, true, true ); @@ -5498,6 +5560,7 @@ QGISEXTERN QgsVectorLayerExporter::ExportError createEmptyLayer( // ------------- + static bool initializeSpatialMetadata( sqlite3 *sqlite_handle, QString &errCause ) { // attempting to perform self-initialization for a newly created DB @@ -5523,18 +5586,7 @@ static bool initializeSpatialMetadata( sqlite3 *sqlite_handle, QString &errCause if ( count > 0 ) return false; - bool above41 = false; - ret = sqlite3_get_table( sqlite_handle, "select spatialite_version()", &results, &rows, &columns, nullptr ); - if ( ret == SQLITE_OK && rows == 1 && columns == 1 ) - { - QString version = QString::fromUtf8( results[1] ); - QStringList parts = version.split( ' ', QString::SkipEmptyParts ); - if ( !parts.empty() ) - { - QStringList verparts = parts.at( 0 ).split( '.', QString::SkipEmptyParts ); - above41 = verparts.size() >= 2 && ( verparts.at( 0 ).toInt() > 4 || ( verparts.at( 0 ).toInt() == 4 && verparts.at( 1 ).toInt() >= 1 ) ); - } - } + const bool above41 = QgsSpatiaLiteProvider::versionIsAbove( sqlite_handle, 4, 1 ); sqlite3_free_table( results ); diff --git a/src/providers/spatialite/qgsspatialiteprovider.h b/src/providers/spatialite/qgsspatialiteprovider.h index 8fe8fb2e3ff6..5832e148cf39 100644 --- a/src/providers/spatialite/qgsspatialiteprovider.h +++ b/src/providers/spatialite/qgsspatialiteprovider.h @@ -174,6 +174,10 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider }; + //! Check if version is above major and minor + static bool versionIsAbove( sqlite3 *sqlite_handle, int major, int minor = 0 ); + + /** * sqlite3 handles pointer */ @@ -201,7 +205,7 @@ class QgsSpatiaLiteProvider: public QgsVectorDataProvider //! For views, try to get primary key from a dedicated meta table void determineViewPrimaryKey(); - //! Returns primary key(s) from a table name + //! Returns integer primary key(s) from a table name QStringList tablePrimaryKeys( const QString &tableName ) const; //! Check if a table/view has any triggers. Triggers can be used on views to make them editable. diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py index 2ce5c2a97585..ae5d946294a5 100644 --- a/tests/src/python/test_provider_spatialite.py +++ b/tests/src/python/test_provider_spatialite.py @@ -952,6 +952,65 @@ def testAliasedQueries(self): dbname = TEST_DATA_DIR + '/provider/spatialite.db' self._aliased_sql_helper(dbname) + def testTextPks(self): + """Test regression when retrieving fetures from tables with text PKs, see #21176""" + + # create test db + dbname = os.path.join(tempfile.gettempdir(), "test_text_pks.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_pg (id TEXT NOT NULL PRIMARY KEY, name TEXT NOT NULL)" + cur.execute(sql) + + sql = "SELECT AddGeometryColumn('test_pg', 'geometry', 4326, 'POLYGON', 'XY')" + cur.execute(sql) + + sql = "INSERT INTO test_pg (id, name, geometry) " + sql += "VALUES ('one', 'toto', GeomFromText('POLYGON((0 0,1 0,1 1,0 1,0 0))', 4326))" + cur.execute(sql) + + sql = "INSERT INTO test_pg (id, name, geometry) " + sql += "VALUES ('two', 'bogo', GeomFromText('POLYGON((0 0,2 0,2 2,0 2,0 0))', 4326))" + cur.execute(sql) + + cur.execute("COMMIT") + con.close() + + def _test_db(testPath): + vl = QgsVectorLayer(testPath, 'test', 'spatialite') + self.assertTrue(vl.isValid()) + + f = next(vl.getFeatures()) + self.assertTrue(f.isValid()) + fid = f.id() + self.assertTrue(fid > 0) + self.assertTrue(vl.getFeature(fid).isValid()) + f2 = next(vl.getFeatures(QgsFeatureRequest().setFilterFid(fid))) + self.assertTrue(f2.isValid()) + self.assertEqual(f2.id(), f.id()) + self.assertEqual(f2.geometry().asWkt(), f.geometry().asWkt()) + + for f in vl.getFeatures(): + self.assertTrue(f.isValid()) + self.assertTrue(vl.getFeature(f.id()).isValid()) + self.assertEqual(vl.getFeature(f.id()).id(), f.id()) + + testPath = "dbname=%s table='test_pg' (geometry) key='id'" % dbname + _test_db(testPath) + testPath = "dbname=%s table='test_pg' (geometry)" % dbname + _test_db(testPath) + testPath = "dbname=%s table='test_pg' key='id'" % dbname + _test_db(testPath) + testPath = "dbname=%s table='test_pg'" % dbname + _test_db(testPath) + if __name__ == '__main__': unittest.main() From 2dd55073b0f23f58b7dfc400b001f88935bde1bf Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 8 Feb 2019 17:38:26 +0100 Subject: [PATCH 2/4] Modernize code FOREACH -> range for --- src/providers/spatialite/qgsspatialiteprovider.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp index 14c186bf692a..66acd4c25dd9 100644 --- a/src/providers/spatialite/qgsspatialiteprovider.cpp +++ b/src/providers/spatialite/qgsspatialiteprovider.cpp @@ -462,8 +462,8 @@ QgsSpatiaLiteProvider::QgsSpatiaLiteProvider( QString const &uri, const Provider mSqliteHandle = mHandle->handle(); if ( mSqliteHandle ) { - QStringList pragmaList = anUri.params( QStringLiteral( "pragma" ) ); - Q_FOREACH ( const QString &pragma, pragmaList ) + const QStringList pragmaList = anUri.params( QStringLiteral( "pragma" ) ); + for ( const auto &pragma : pragmaList ) { char *errMsg = nullptr; int ret = sqlite3_exec( mSqliteHandle, ( "PRAGMA " + pragma ).toUtf8(), nullptr, nullptr, &errMsg ); @@ -882,8 +882,8 @@ void QgsSpatiaLiteProvider::fetchConstraints() QRegularExpressionMatch match = re.match( sqlDef ); if ( match.hasMatch() ) { - QString matched = match.captured( 1 ); - Q_FOREACH ( QString field, matched.split( ',' ) ) + const QString matched = match.captured( 1 ); + for ( auto &field : matched.split( ',' ) ) { field = field.trimmed(); QString fieldName = field.left( field.indexOf( ' ' ) ); @@ -904,7 +904,7 @@ void QgsSpatiaLiteProvider::fetchConstraints() } sqlite3_free_table( results ); - Q_FOREACH ( int fieldIdx, mPrimaryKeyAttrs ) + for ( const auto fieldIdx : qgis::as_const( mPrimaryKeyAttrs ) ) { QgsFieldConstraints constraints = mAttributeFields.at( fieldIdx ).constraints(); constraints.setConstraint( QgsFieldConstraints::ConstraintUnique, QgsFieldConstraints::ConstraintOriginProvider ); @@ -5682,7 +5682,7 @@ QgsAttributeList QgsSpatiaLiteProvider::pkAttributeIndexes() const QList QgsSpatiaLiteProvider::searchLayers( const QList &layers, const QString &connectionInfo, const QString &tableName ) { QList result; - Q_FOREACH ( QgsVectorLayer *layer, layers ) + for ( auto *layer : layers ) { const QgsSpatiaLiteProvider *slProvider = qobject_cast( layer->dataProvider() ); if ( slProvider && slProvider->mSqlitePath == connectionInfo && slProvider->mTableName == tableName ) @@ -5717,7 +5717,7 @@ QList QgsSpatiaLiteProvider::discoverRelations( const QgsVectorLaye { // first reference field => try to find if we have layers for the referenced table const QList foundLayers = searchLayers( layers, mSqlitePath, refTable ); - Q_FOREACH ( const QgsVectorLayer *foundLayer, foundLayers ) + for ( const auto *foundLayer : foundLayers ) { QgsRelation relation; relation.setName( name ); From ce7b8e269b5b08562276f018002fd2447737eed9 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 8 Feb 2019 19:25:46 +0100 Subject: [PATCH 3/4] Update tests/src/python/test_provider_spatialite.py --- tests/src/python/test_provider_spatialite.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/src/python/test_provider_spatialite.py b/tests/src/python/test_provider_spatialite.py index ae5d946294a5..9bda1d4b97f3 100644 --- a/tests/src/python/test_provider_spatialite.py +++ b/tests/src/python/test_provider_spatialite.py @@ -953,7 +953,7 @@ def testAliasedQueries(self): self._aliased_sql_helper(dbname) def testTextPks(self): - """Test regression when retrieving fetures from tables with text PKs, see #21176""" + """Test regression when retrieving features from tables with text PKs, see #21176""" # create test db dbname = os.path.join(tempfile.gettempdir(), "test_text_pks.sqlite") From 3736fc078d290841d2b9577964ef8cc30520acb0 Mon Sep 17 00:00:00 2001 From: Alessandro Pasotti Date: Fri, 8 Feb 2019 22:26:22 +0100 Subject: [PATCH 4/4] Fix sqlite double free --- src/providers/spatialite/qgsspatialiteprovider.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/providers/spatialite/qgsspatialiteprovider.cpp b/src/providers/spatialite/qgsspatialiteprovider.cpp index 66acd4c25dd9..037d9215968a 100644 --- a/src/providers/spatialite/qgsspatialiteprovider.cpp +++ b/src/providers/spatialite/qgsspatialiteprovider.cpp @@ -5588,8 +5588,6 @@ static bool initializeSpatialMetadata( sqlite3 *sqlite_handle, QString &errCause const bool above41 = QgsSpatiaLiteProvider::versionIsAbove( sqlite_handle, 4, 1 ); - sqlite3_free_table( results ); - // all right, it's empty: proceeding to initialize char *errMsg = nullptr; ret = sqlite3_exec( sqlite_handle, above41 ? "SELECT InitSpatialMetadata(1)" : "SELECT InitSpatialMetadata()", nullptr, nullptr, &errMsg );