diff --git a/python/core/auto_generated/providers/qgsabstractdatabaseproviderconnection.sip.in b/python/core/auto_generated/providers/qgsabstractdatabaseproviderconnection.sip.in index 4a0250e952a2..f68884ff366c 100644 --- a/python/core/auto_generated/providers/qgsabstractdatabaseproviderconnection.sip.in +++ b/python/core/auto_generated/providers/qgsabstractdatabaseproviderconnection.sip.in @@ -635,10 +635,12 @@ Returns the fields of a ``table`` and ``schema``. .. note:: - The default implementation creates a temporary vector layer, providers may - choose to override this method for a greater efficiency. + the default implementation creates a temporary vector layer, providers may + choose to override this method for a greater efficiency of to overcome provider's + behavior when the layer does not expose all fields (GPKG for example hides geometry + and primary key column). -:raises QgsProviderConnectionException: if any errors are encountered. +:raises QgsProviderConnectionException: .. versionadded:: 3.16 %End diff --git a/src/core/providers/ogr/qgsgeopackageproviderconnection.cpp b/src/core/providers/ogr/qgsgeopackageproviderconnection.cpp index 6086fe5f041d..846fc9307d4b 100644 --- a/src/core/providers/ogr/qgsgeopackageproviderconnection.cpp +++ b/src/core/providers/ogr/qgsgeopackageproviderconnection.cpp @@ -405,28 +405,64 @@ QgsAbstractDatabaseProviderConnection::QueryResult QgsGeoPackageProviderConnecti if ( fet.reset( OGR_L_GetNextFeature( ogrLayer ) ), fet ) { - QgsFields fields { QgsOgrUtils::readOgrFields( fet.get(), QTextCodec::codecForName( "UTF-8" ) ) }; - iterator->setFields( fields ); + // May need to prepend PK and append geometries to the columns + thread_local const QRegularExpression pkRegExp { QStringLiteral( R"(^select\s+(\*|fid)[,\s+](.*)from)" ), QRegularExpression::PatternOption::CaseInsensitiveOption }; + if ( pkRegExp.match( sql.trimmed() ).hasMatch() ) + { + iterator->setPrimaryKeyColumnName( QStringLiteral( "fid" ) ); + results.appendColumn( QStringLiteral( "fid" ) ); + } + QgsFields fields { QgsOgrUtils::readOgrFields( fet.get(), QTextCodec::codecForName( "UTF-8" ) ) }; for ( const auto &f : std::as_const( fields ) ) { results.appendColumn( f.name() ); } + + // Geometry columns + OGRFeatureDefnH featureDef = OGR_F_GetDefnRef( fet.get() ); + + if ( featureDef ) + { + QStringList geomColumnsList; + for ( int idx = 0; idx < OGR_F_GetGeomFieldCount( fet.get() ); ++idx ) + { + OGRGeomFieldDefnH geomFldDef { OGR_F_GetGeomFieldDefnRef( fet.get(), idx ) }; + if ( geomFldDef ) + { + const QString geomColumnName { OGR_GFld_GetNameRef( geomFldDef ) }; + geomColumnsList.append( geomColumnName ); + results.appendColumn( geomColumnName ); + } + } + iterator->setGeometryColumnNames( geomColumnsList ); + } + + iterator->setFields( fields ); } // Check for errors - errCause = CPLGetLastErrorMsg( ); + if ( CE_Failure == CPLGetLastErrorType() || CE_Fatal == CPLGetLastErrorType() ) + { + errCause = CPLGetLastErrorMsg( ); + } if ( ! errCause.isEmpty() ) { - throw QgsProviderConnectionException( QObject::tr( "Error executing SQL %1: %2" ).arg( sql, errCause ) ); + throw QgsProviderConnectionException( QObject::tr( "Error executing SQL statement %1: %2" ).arg( sql, errCause ) ); } OGR_L_ResetReading( ogrLayer ); iterator->nextRow(); return results; } - errCause = CPLGetLastErrorMsg( ); + + // Check for errors + if ( CE_Failure == CPLGetLastErrorType() || CE_Fatal == CPLGetLastErrorType() ) + { + errCause = CPLGetLastErrorMsg( ); + } + } else { @@ -456,6 +492,12 @@ QVariantList QgsGeoPackageProviderResultIterator::nextRowInternal() gdal::ogr_feature_unique_ptr fet; if ( fet.reset( OGR_L_GetNextFeature( mOgrLayer ) ), fet ) { + // PK + if ( ! mPrimaryKeyColumnName.isEmpty() ) + { + row.push_back( OGR_F_GetFID( fet.get() ) ); + } + if ( ! mFields.isEmpty() ) { QgsFeature f { QgsOgrUtils::readOgrFeature( fet.get(), mFields, QTextCodec::codecForName( "UTF-8" ) ) }; @@ -464,6 +506,13 @@ QVariantList QgsGeoPackageProviderResultIterator::nextRowInternal() { row.push_back( attribute ); } + + // Geoms go last + for ( const auto &geomColName : std::as_const( mGeometryColumnNames ) ) + { + row.push_back( f.geometry().asWkb() ); + } + } else // Fallback to strings { @@ -493,6 +542,16 @@ void QgsGeoPackageProviderResultIterator::setFields( const QgsFields &fields ) mFields = fields; } +void QgsGeoPackageProviderResultIterator::setGeometryColumnNames( const QStringList &geometryColumnNames ) +{ + mGeometryColumnNames = geometryColumnNames; +} + +void QgsGeoPackageProviderResultIterator::setPrimaryKeyColumnName( const QString &primaryKeyColumnName ) +{ + mPrimaryKeyColumnName = primaryKeyColumnName; +} + QList QgsGeoPackageProviderConnection::nativeTypes() const { QgsVectorLayer::LayerOptions options { false, true }; @@ -508,6 +567,53 @@ QList QgsGeoPackageProviderConnection::native return vl.dataProvider()->nativeTypes(); } +QgsFields QgsGeoPackageProviderConnection::fields( const QString &schema, const QString &table ) const +{ + + Q_UNUSED( schema ) + + // Get fields from layer + QgsFields fieldList; + + // Prepend PK fid + fieldList.append( QgsField{ QStringLiteral( "fid" ), QVariant::LongLong } ); + + QgsVectorLayer::LayerOptions options { false, true }; + options.skipCrsValidation = true; + QgsVectorLayer vl { tableUri( schema, table ), QStringLiteral( "temp_layer" ), mProviderKey, options }; + if ( vl.isValid() ) + { + const auto parentFields { vl.fields() }; + for ( const auto &pField : std::as_const( parentFields ) ) + { + fieldList.append( pField ); + } + // Append name of the geometry column, the data provider does not expose this information so we need an extra query:/ + const QString sql = QStringLiteral( "SELECT g.column_name " + "FROM gpkg_contents c LEFT JOIN gpkg_geometry_columns g ON (c.table_name = g.table_name) " + "WHERE c.table_name = %1" ).arg( QgsSqliteUtils::quotedString( table ) ); + try + { + const auto results = executeSql( sql ); + if ( ! results.isEmpty() ) + { + fieldList.append( QgsField{ results.first().first().toString(), QVariant::String, QStringLiteral( "geometry" ) } ); + } + } + catch ( QgsProviderConnectionException &ex ) + { + throw QgsProviderConnectionException( QObject::tr( "Error retrieving fields information for uri %1: %2" ).arg( vl.publicSource(), ex.what() ) ); + } + + } + else + { + throw QgsProviderConnectionException( QObject::tr( "Error retrieving fields information for uri: %1" ).arg( vl.publicSource() ) ); + } + + return fieldList; +} + QgsGeoPackageProviderResultIterator::~QgsGeoPackageProviderResultIterator() { if ( mHDS ) diff --git a/src/core/providers/ogr/qgsgeopackageproviderconnection.h b/src/core/providers/ogr/qgsgeopackageproviderconnection.h index 2d4201e979ad..ccaf5a755c60 100644 --- a/src/core/providers/ogr/qgsgeopackageproviderconnection.h +++ b/src/core/providers/ogr/qgsgeopackageproviderconnection.h @@ -27,6 +27,7 @@ struct QgsGeoPackageProviderResultIterator: public QgsAbstractDatabaseProviderConnection::QueryResult::QueryResultIterator { + QgsGeoPackageProviderResultIterator( gdal::ogr_datasource_unique_ptr hDS, OGRLayerH ogrLayer ) : mHDS( std::move( hDS ) ) , mOgrLayer( ogrLayer ) @@ -35,6 +36,8 @@ struct QgsGeoPackageProviderResultIterator: public QgsAbstractDatabaseProviderCo ~QgsGeoPackageProviderResultIterator(); void setFields( const QgsFields &fields ); + void setGeometryColumnNames( const QStringList &geometryColumnNames ); + void setPrimaryKeyColumnName( const QString &primaryKeyColumnName ); private: @@ -42,6 +45,8 @@ struct QgsGeoPackageProviderResultIterator: public QgsAbstractDatabaseProviderCo OGRLayerH mOgrLayer; QgsFields mFields; QVariantList mNextRow; + QStringList mGeometryColumnNames; + QString mPrimaryKeyColumnName; QVariantList nextRowPrivate() override; QVariantList nextRowInternal(); @@ -76,12 +81,15 @@ class QgsGeoPackageProviderConnection : public QgsAbstractDatabaseProviderConnec const TableFlags &flags = TableFlags() ) const override; QIcon icon() const override; QList nativeTypes() const override; + QgsFields fields( const QString &schema, const QString &table ) const override; private: void setDefaultCapabilities(); //! Use GDAL to execute SQL QueryResult executeGdalSqlPrivate( const QString &sql, QgsFeedback *feedback = nullptr ) const; + + }; diff --git a/src/core/providers/qgsabstractdatabaseproviderconnection.cpp b/src/core/providers/qgsabstractdatabaseproviderconnection.cpp index 8abb57f15668..8e71bdcc7345 100644 --- a/src/core/providers/qgsabstractdatabaseproviderconnection.cpp +++ b/src/core/providers/qgsabstractdatabaseproviderconnection.cpp @@ -301,6 +301,7 @@ QgsFields QgsAbstractDatabaseProviderConnection::fields( const QString &schema, QgsVectorLayer vl { tableUri( schema, tableName ), QStringLiteral( "temp_layer" ), mProviderKey, options }; if ( vl.isValid() ) { + // Note: this implementation works for providers that do not hide any "special" field (geometry or PKs) return vl.fields(); } else diff --git a/src/core/providers/qgsabstractdatabaseproviderconnection.h b/src/core/providers/qgsabstractdatabaseproviderconnection.h index 7a0bd0d717ce..3f014ef6613c 100644 --- a/src/core/providers/qgsabstractdatabaseproviderconnection.h +++ b/src/core/providers/qgsabstractdatabaseproviderconnection.h @@ -736,9 +736,11 @@ class CORE_EXPORT QgsAbstractDatabaseProviderConnection : public QgsAbstractProv /** * Returns the fields of a \a table and \a schema. * - * \note The default implementation creates a temporary vector layer, providers may - * choose to override this method for a greater efficiency. - * \throws QgsProviderConnectionException if any errors are encountered. + * \note the default implementation creates a temporary vector layer, providers may + * choose to override this method for a greater efficiency of to overcome provider's + * behavior when the layer does not expose all fields (GPKG for example hides geometry + * and primary key column). + * \throws QgsProviderConnectionException * \since QGIS 3.16 */ virtual QgsFields fields( const QString &schema, const QString &table ) const SIP_THROW( QgsProviderConnectionException ); diff --git a/src/providers/spatialite/qgsspatialiteproviderconnection.cpp b/src/providers/spatialite/qgsspatialiteproviderconnection.cpp index 0441a63fbfe1..e8c38f359b22 100644 --- a/src/providers/spatialite/qgsspatialiteproviderconnection.cpp +++ b/src/providers/spatialite/qgsspatialiteproviderconnection.cpp @@ -319,7 +319,8 @@ QList QgsSpatiaLiteProviderConne // Need to store it here because provider (and underlying gaia library) returns views as spatial table if they have geometries QStringList viewNames; - for ( const auto &tn : executeSqlPrivate( QStringLiteral( "SELECT name FROM sqlite_master WHERE type = 'view'" ) ).rows() ) + const auto viewRows { executeSqlPrivate( QStringLiteral( "SELECT name FROM sqlite_master WHERE type = 'view'" ) ).rows() }; + for ( const auto &tn : std::as_const( viewRows ) ) { viewNames.push_back( tn.first().toString() ); } @@ -327,7 +328,8 @@ QList QgsSpatiaLiteProviderConne // Another weirdness: table names are converted to lowercase when out of spatialite gaia functions, let's get them back to their real case here, // may need LAUNDER on open, but let's try to make it consistent with how GPKG works. QgsStringMap tableNotLowercaseNames; - for ( const auto &tn : executeSqlPrivate( QStringLiteral( "SELECT name FROM sqlite_master WHERE LOWER(name) != name" ) ).rows() ) + const auto lowerTables { executeSqlPrivate( QStringLiteral( "SELECT name FROM sqlite_master WHERE LOWER(name) != name" ) ).rows() }; + for ( const auto &tn : std::as_const( lowerTables ) ) { const QString tName { tn.first().toString() }; tableNotLowercaseNames.insert( tName.toLower(), tName ); @@ -357,10 +359,26 @@ QList QgsSpatiaLiteProviderConne property.setGeometryColumnTypes( {{ QgsWkbTypes::NoGeometry, QgsCoordinateReferenceSystem() }} ); property.setFlag( QgsSpatiaLiteProviderConnection::TableFlag::Aspatial ); } + if ( viewNames.contains( tableName ) ) { property.setFlag( QgsSpatiaLiteProviderConnection::TableFlag::View ); } + + const auto constPkIdxs { vl->dataProvider()->pkAttributeIndexes() }; + QStringList pkNames; + for ( const auto &pkIdx : std::as_const( constPkIdxs ) ) + { + // Better safe than sorry + if ( pkIdx < vl->fields().count() ) + pkNames.append( vl->fields().at( pkIdx ).name() ); + } + + if ( ! pkNames.isEmpty() ) + { + property.setPrimaryKeyColumns( pkNames ); + } + tableInfo.push_back( property ); } else diff --git a/tests/src/python/test_qgsproviderconnection_ogr_gpkg.py b/tests/src/python/test_qgsproviderconnection_ogr_gpkg.py index fcd1db4318fc..00e7edfbd973 100644 --- a/tests/src/python/test_qgsproviderconnection_ogr_gpkg.py +++ b/tests/src/python/test_qgsproviderconnection_ogr_gpkg.py @@ -16,6 +16,7 @@ import os import shutil from test_qgsproviderconnection_base import TestPyQgsProviderConnectionBase +from qgis.PyQt.QtCore import QTemporaryDir from qgis.core import ( QgsWkbTypes, QgsAbstractDatabaseProviderConnection, @@ -56,17 +57,13 @@ def setUpClass(cls): """Run before all tests""" TestPyQgsProviderConnectionBase.setUpClass() gpkg_original_path = '{}/qgis_server/test_project_wms_grouped_layers.gpkg'.format(TEST_DATA_DIR) - cls.gpkg_path = '{}/qgis_server/test_project_wms_grouped_layers_test.gpkg'.format(TEST_DATA_DIR) + cls.temp_dir = QTemporaryDir() + cls.gpkg_path = '{}/test_project_wms_grouped_layers.gpkg'.format(cls.temp_dir.path()) shutil.copy(gpkg_original_path, cls.gpkg_path) vl = QgsVectorLayer('{}|layername=cdb_lines'.format(cls.gpkg_path), 'test', 'ogr') assert vl.isValid() cls.uri = cls.gpkg_path - @classmethod - def tearDownClass(cls): - """Run after all tests""" - os.unlink(cls.gpkg_path) - def test_gpkg_connections_from_uri(self): """Create a connection from a layer uri and retrieve it""" @@ -85,8 +82,8 @@ def test_gpkg_table_uri(self): self.assertTrue(vl.isValid()) # Test table(), throws if not found - table_info = conn.table('', 'osm') - table_info = conn.table('', 'cdb_lines') + conn.table('', 'osm') + conn.table('', 'cdb_lines') self.assertEqual(conn.tableUri('', 'osm'), "GPKG:%s:osm" % self.uri) rl = QgsRasterLayer(conn.tableUri('', 'osm'), 'r', 'gdal') @@ -136,7 +133,10 @@ def test_gpkg_fields(self): md = QgsProviderRegistry.instance().providerMetadata('ogr') conn = md.createConnection(self.uri, {}) fields = conn.fields('', 'cdb_lines') - self.assertEqual(fields.names(), ['fid', 'id', 'typ', 'name', 'ortsrat', 'id_long']) + table_info = conn.table('', 'cdb_lines') + self.assertIn(table_info.geometryColumn(), fields.names()) + self.assertIn(table_info.primaryKeyColumns()[0], fields.names()) + self.assertEqual(fields.names(), ['fid', 'id', 'typ', 'name', 'ortsrat', 'id_long', 'geom']) def test_create_vector_layer(self): """Test query layers""" @@ -153,6 +153,29 @@ def test_create_vector_layer(self): self.assertEqual(len(features), 2) self.assertEqual(features[0].attributes(), [8, 'Sülfeld']) + def test_execute_sql_pk_geoms(self): + """OGR hides fid and geom from attributes, check if we can still get them""" + + md = QgsProviderRegistry.instance().providerMetadata('ogr') + conn = md.createConnection(self.uri, {}) + + # Check errors + with self.assertRaises(QgsProviderConnectionException): + sql = 'SELECT not_exists, name, geom FROM cdb_lines WHERE name LIKE \'S%\' LIMIT 2' + results = conn.executeSql(sql) + + sql = 'SELECT fid, name, geom FROM cdb_lines WHERE name LIKE \'S%\' LIMIT 2' + results = conn.executeSql(sql) + self.assertEqual(results[0][:2], [8, 'Sülfeld']) + self.assertEqual(results[1][:2], [16, 'Steimker Berg']) + self.assertEqual(results[0][2][:20], b'\x01\x03\x00\x00\x00\x01\x00\x00\x00/\x00\x00\x00\xf6\x88\x16Y\xad\xb2"') + self.assertEqual(results[1][2][:20], b'\x01\x03\x00\x00\x00\x01\x00\x00\x00F\x00\x00\x00 \xc1\x9f\xda\xb4\xfb"') + + sql = 'SELECT name, st_astext(geom) FROM cdb_lines WHERE name LIKE \'S%\' LIMIT 2' + results = conn.executeSql(sql) + self.assertEqual(results[0], ['Sülfeld', + 'POLYGON((612694.674 5807839.658, 612668.715 5808176.815, 612547.354 5808414.452, 612509.527 5808425.73, 612522.932 5808473.02, 612407.901 5808519.082, 612505.836 5808632.763, 612463.449 5808781.115, 612433.57 5808819.061, 612422.685 5808980.281999, 612473.423 5808995.424999, 612333.856 5809647.731, 612307.316 5809781.446, 612267.099 5809852.803, 612308.221 5810040.995, 613920.397 5811079.478, 613947.16 5811129.3, 614022.726 5811154.456, 614058.436 5811260.36, 614194.037 5811331.972, 614307.176 5811360.06, 614343.842 5811323.238, 614443.449 5811363.03, 614526.199 5811059.031, 614417.83 5811057.603, 614787.296 5809648.422, 614772.062 5809583.246, 614981.93 5809245.35, 614811.885 5809138.271, 615063.452 5809100.954, 615215.476 5809029.413, 615469.441 5808883.282, 615569.846 5808829.522, 615577.239 5808806.242, 615392.964 5808736.873, 615306.34 5808662.171, 615335.445 5808290.588, 615312.192 5808290.397, 614890.582 5808077.956, 615018.854 5807799.895, 614837.326 5807688.363, 614435.698 5807646.847, 614126.351 5807661.841, 613555.813 5807814.801, 612826.66 5807964.828, 612830.113 5807856.315, 612694.674 5807839.658))']) + if __name__ == '__main__': unittest.main() diff --git a/tests/src/python/test_qgsproviderconnection_postgres.py b/tests/src/python/test_qgsproviderconnection_postgres.py index 1761e36fd27e..e4b8d94a87e3 100644 --- a/tests/src/python/test_qgsproviderconnection_postgres.py +++ b/tests/src/python/test_qgsproviderconnection_postgres.py @@ -339,7 +339,7 @@ def test_fields(self): conn.executeSql(sql) fields = conn.fields('qgis_test', 'gh_37666') - self.assertEqual([f.name() for f in fields], ['id', 'geom', 'geog']) + self.assertEqual(fields.names(), ['id', 'geom', 'geog']) self.assertEqual([f.typeName() for f in fields], ['int4', 'geometry', 'geography']) table = conn.table('qgis_test', 'gh_37666') self.assertEqual(table.primaryKeyColumns(), ['id']) diff --git a/tests/src/python/test_qgsproviderconnection_spatialite.py b/tests/src/python/test_qgsproviderconnection_spatialite.py index 48104470b674..5f7681699bf1 100644 --- a/tests/src/python/test_qgsproviderconnection_spatialite.py +++ b/tests/src/python/test_qgsproviderconnection_spatialite.py @@ -122,29 +122,17 @@ def test_spatialite_connections(self): self.assertFalse('myNewTable' in table_names) self.assertTrue('myNewAspatialTable' in table_names) - def test_gpkg_fields(self): + def test_spatialite_fields(self): """Test fields""" md = QgsProviderRegistry.instance().providerMetadata('spatialite') conn = md.createConnection(self.uri, {}) fields = conn.fields('', 'cdb_lines') + table_info = conn.table('', 'cdb_lines') + self.assertIn(table_info.geometryColumn(), fields.names()) + self.assertIn(table_info.primaryKeyColumns()[0], fields.names()) self.assertEqual(fields.names(), ['pk', 'geom', 'fid', 'id', 'typ', 'name', 'ortsrat', 'id_long']) - def test_create_vector_layer(self): - """Test query layers""" - - md = QgsProviderRegistry.instance().providerMetadata('spatialite') - conn = md.createConnection(self.uri, {}) - - options = QgsAbstractDatabaseProviderConnection.SqlVectorLayerOptions() - options.sql = 'SELECT fid, name, geom FROM cdb_lines WHERE name LIKE \'S%\' LIMIT 2' - vl = conn.createSqlVectorLayer(options) - self.assertTrue(vl.isValid()) - self.assertEqual(vl.geometryType(), QgsWkbTypes.PolygonGeometry) - features = [f for f in vl.getFeatures()] - self.assertEqual(len(features), 2) - self.assertEqual(features[0].attributes(), [8, 'Sülfeld']) - def test_create_vector_layer(self): """Test query layers""" diff --git a/tests/testdata/projects/relative_paths_gh30387.gpkg b/tests/testdata/projects/relative_paths_gh30387.gpkg index d71043964555..2e88886f04b4 100644 Binary files a/tests/testdata/projects/relative_paths_gh30387.gpkg and b/tests/testdata/projects/relative_paths_gh30387.gpkg differ diff --git a/tests/testdata/qgis_server/bug_gh30264_empty_layer_wrong_bbox.gpkg b/tests/testdata/qgis_server/bug_gh30264_empty_layer_wrong_bbox.gpkg index ad4ab8c613a6..4b2455338023 100644 Binary files a/tests/testdata/qgis_server/bug_gh30264_empty_layer_wrong_bbox.gpkg and b/tests/testdata/qgis_server/bug_gh30264_empty_layer_wrong_bbox.gpkg differ