From ea2cc365db8c532a3aaf0528c50351adfcf1738d Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Fri, 21 Sep 2018 15:32:19 +0200 Subject: [PATCH] [OGR provider] Make sure to use layername= syntax in URI of multilayer datasources (fixes #19885) --- .../auto_generated/qgsdataitemprovider.sip.in | 2 +- src/core/qgsdataitemprovider.h | 2 +- src/providers/ogr/qgsogrdataitems.cpp | 50 +++++++++++++------ src/providers/ogr/qgsogrdataitems.h | 14 ++++++ src/providers/ogr/qgsogrprovider.cpp | 2 +- tests/src/python/test_provider_ogr.py | 31 +++++++++++- tests/testdata/multilayer.kml | 24 +++++++++ 7 files changed, 107 insertions(+), 18 deletions(-) create mode 100644 tests/testdata/multilayer.kml diff --git a/python/core/auto_generated/qgsdataitemprovider.sip.in b/python/core/auto_generated/qgsdataitemprovider.sip.in index c8877b3b75a..83dd4993244 100644 --- a/python/core/auto_generated/qgsdataitemprovider.sip.in +++ b/python/core/auto_generated/qgsdataitemprovider.sip.in @@ -21,7 +21,7 @@ The method createDataItem() is ever called only if capabilities() return non-zer There are two occasions when createDataItem() is called: 1. to create root items (passed path is empty, parent item is null). 2. to create items in directory structure. For this capabilities have to return at least -of the following: QgsDataProider.Dir or QgsDataProvider.File. Passed path is the file +of the following: QgsDataProvider.Dir or QgsDataProvider.File. Passed path is the file or directory being inspected, parent item is a valid QgsDirectoryItem .. versionadded:: 2.10 diff --git a/src/core/qgsdataitemprovider.h b/src/core/qgsdataitemprovider.h index d923e358541..67b5c238441 100644 --- a/src/core/qgsdataitemprovider.h +++ b/src/core/qgsdataitemprovider.h @@ -36,7 +36,7 @@ typedef bool handlesDirectoryPath_t( const QString &path ) SIP_SKIP; * There are two occasions when createDataItem() is called: * 1. to create root items (passed path is empty, parent item is null). * 2. to create items in directory structure. For this capabilities have to return at least - * of the following: QgsDataProider::Dir or QgsDataProvider::File. Passed path is the file + * of the following: QgsDataProvider::Dir or QgsDataProvider::File. Passed path is the file * or directory being inspected, parent item is a valid QgsDirectoryItem * * \since QGIS 2.10 diff --git a/src/providers/ogr/qgsogrdataitems.cpp b/src/providers/ogr/qgsogrdataitems.cpp index f97aceb569c..3cadcde98cf 100644 --- a/src/providers/ogr/qgsogrdataitems.cpp +++ b/src/providers/ogr/qgsogrdataitems.cpp @@ -371,7 +371,10 @@ void QgsOgrLayerItem::deleteLayer() // ------- -static QgsOgrLayerItem *dataItemForLayer( QgsDataItem *parentItem, QString name, QString path, GDALDatasetH hDataSource, int layerId, bool isSubLayer = false ) +static QgsOgrLayerItem *dataItemForLayer( QgsDataItem *parentItem, QString name, + QString path, GDALDatasetH hDataSource, + int layerId, + bool isSubLayer, bool uniqueNames ) { OGRLayerH hLayer = GDALDatasetGetLayer( hDataSource, layerId ); OGRFeatureDefnH hDef = OGR_L_GetLayerDefn( hLayer ); @@ -401,16 +404,22 @@ static QgsOgrLayerItem *dataItemForLayer( QgsDataItem *parentItem, QString name, QString layerUri = path; - if ( name.isEmpty() ) + if ( isSubLayer ) { // we are in a collection name = QString::fromUtf8( OGR_FD_GetName( hDef ) ); QgsDebugMsg( "OGR layer name : " + name ); - - layerUri += "|layerid=" + QString::number( layerId ); - + if ( !uniqueNames ) + { + layerUri += "|layerid=" + QString::number( layerId ); + } + else + { + layerUri += "|layername=" + name; + } path += '/' + name; } + Q_ASSERT( !name.isEmpty() ); QgsDebugMsgLevel( "OGR layer uri : " + layerUri, 2 ); @@ -433,10 +442,26 @@ QVector QgsOgrDataCollectionItem::createChildren() return children; int numLayers = GDALDatasetGetLayerCount( hDataSource.get() ); + // Check if layer names are unique, so we can use |layername= in URI + QMap< QString, int > mapLayerNameToCount; + bool uniqueNames = true; + for ( int i = 0; i < numLayers; ++i ) + { + OGRLayerH hLayer = GDALDatasetGetLayer( hDataSource.get(), i ); + OGRFeatureDefnH hDef = OGR_L_GetLayerDefn( hLayer ); + QString layerName = QString::fromUtf8( OGR_FD_GetName( hDef ) ); + ++mapLayerNameToCount[layerName]; + if ( mapLayerNameToCount[layerName] > 1 ) + { + uniqueNames = false; + break; + } + } + children.reserve( numLayers ); for ( int i = 0; i < numLayers; ++i ) { - QgsOgrLayerItem *item = dataItemForLayer( this, QString(), mPath, hDataSource.get(), i, true ); + QgsOgrLayerItem *item = dataItemForLayer( this, QString(), mPath, hDataSource.get(), i, true, uniqueNames ); children.append( item ); } @@ -477,13 +502,10 @@ bool QgsOgrDataCollectionItem::createConnection( const QString &name, const QStr // --------------------------------------------------------------------------- -QGISEXTERN int dataCapabilities() -{ - return QgsDataProvider::File | QgsDataProvider::Dir; -} -QGISEXTERN QgsDataItem *dataItem( QString path, QgsDataItem *parentItem ) +QgsDataItem *QgsOgrDataItemProvider::createDataItem( const QString &pathIn, QgsDataItem *parentItem ) { + QString path( pathIn ); if ( path.isEmpty() ) return nullptr; @@ -607,7 +629,7 @@ QGISEXTERN QgsDataItem *dataItem( QString path, QgsDataItem *parentItem ) // class // TODO: add more OGR supported multiple layers formats here! QStringList ogrSupportedDbLayersExtensions; - ogrSupportedDbLayersExtensions << QStringLiteral( "gpkg" ) << QStringLiteral( "sqlite" ) << QStringLiteral( "db" ) << QStringLiteral( "gdb" ); + ogrSupportedDbLayersExtensions << QStringLiteral( "gpkg" ) << QStringLiteral( "sqlite" ) << QStringLiteral( "db" ) << QStringLiteral( "gdb" ) << QStringLiteral( "kml" ); QStringList ogrSupportedDbDriverNames; ogrSupportedDbDriverNames << QStringLiteral( "GPKG" ) << QStringLiteral( "db" ) << QStringLiteral( "gdb" ); @@ -688,12 +710,12 @@ QGISEXTERN QgsDataItem *dataItem( QString path, QgsDataItem *parentItem ) } else { - item = dataItemForLayer( parentItem, name, path, hDS.get(), 0 ); + item = dataItemForLayer( parentItem, name, path, hDS.get(), 0, false, true ); } return item; } -QGISEXTERN bool handlesDirectoryPath( const QString &path ) +bool QgsOgrDataItemProvider::handlesDirectoryPath( const QString &path ) { QFileInfo info( path ); QString suffix = info.suffix().toLower(); diff --git a/src/providers/ogr/qgsogrdataitems.h b/src/providers/ogr/qgsogrdataitems.h index 0427d70a695..f2ad4786272 100644 --- a/src/providers/ogr/qgsogrdataitems.h +++ b/src/providers/ogr/qgsogrdataitems.h @@ -102,5 +102,19 @@ class QgsOgrDataCollectionItem : public QgsDataCollectionItem }; +//! Provider for OGR root data item +class QgsOgrDataItemProvider : public QgsDataItemProvider +{ + public: + QString name() override { return QStringLiteral( "OGR" ); } + + int capabilities() override { return QgsDataProvider::File | QgsDataProvider::Dir; } + + QgsDataItem *createDataItem( const QString &path, QgsDataItem *parentItem ) override; + + bool handlesDirectoryPath( const QString &path ) override; +}; + + #endif // QGSOGRDATAITEMS_H diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp index 28f2a7d59a0..c0f14e1963a 100644 --- a/src/providers/ogr/qgsogrprovider.cpp +++ b/src/providers/ogr/qgsogrprovider.cpp @@ -3375,10 +3375,10 @@ QGISEXTERN bool createEmptyDataSource( const QString &uri, return true; } - QGISEXTERN QList< QgsDataItemProvider * > *dataItemProviders() { QList< QgsDataItemProvider * > *providers = new QList< QgsDataItemProvider * >(); + *providers << new QgsOgrDataItemProvider; *providers << new QgsGeoPackageDataItemProvider; return providers; } diff --git a/tests/src/python/test_provider_ogr.py b/tests/src/python/test_provider_ogr.py index fea76aee1c8..12d6a451a47 100644 --- a/tests/src/python/test_provider_ogr.py +++ b/tests/src/python/test_provider_ogr.py @@ -19,7 +19,8 @@ import tempfile from osgeo import gdal, ogr # NOQA from qgis.PyQt.QtCore import QVariant -from qgis.core import (QgsFeature, QgsFeatureRequest, QgsField, QgsSettings, QgsDataProvider, +from qgis.core import (QgsApplication, + QgsFeature, QgsFeatureRequest, QgsField, QgsSettings, QgsDataProvider, QgsVectorDataProvider, QgsVectorLayer, QgsWkbTypes, QgsNetworkAccessManager) from qgis.testing import start_app, unittest @@ -404,6 +405,34 @@ class PyQgsOGRProvider(unittest.TestCase): vl = QgsVectorLayer(datasource, 'test', 'ogr') self.assertEqual(len(vl.fields()), 2) + def testDataItems(self): + + registry = QgsApplication.dataItemProviderRegistry() + ogrprovider = next(provider for provider in registry.providers() if provider.name() == 'OGR') + + # Single layer + item = ogrprovider.createDataItem(os.path.join(TEST_DATA_DIR, 'lines.shp'), None) + self.assertTrue(item.uri().endswith('lines.shp')) + + # Multiple layer + item = ogrprovider.createDataItem(os.path.join(TEST_DATA_DIR, 'multilayer.kml'), None) + children = item.createChildren() + self.assertEqual(len(children), 2) + self.assertIn('multilayer.kml|layername=Layer1', children[0].uri()) + self.assertIn('multilayer.kml|layername=Layer2', children[1].uri()) + + # Multiple layer (geopackage) + tmpfile = os.path.join(self.basetestpath, 'testDataItems.gpkg') + ds = ogr.GetDriverByName('GPKG').CreateDataSource(tmpfile) + lyr = ds.CreateLayer('Layer1', geom_type=ogr.wkbPoint) + lyr = ds.CreateLayer('Layer2', geom_type=ogr.wkbPoint) + ds = None + item = ogrprovider.createDataItem(tmpfile, None) + children = item.createChildren() + self.assertEqual(len(children), 2) + self.assertIn('testDataItems.gpkg|layername=Layer1', children[0].uri()) + self.assertIn('testDataItems.gpkg|layername=Layer2', children[1].uri()) + if __name__ == '__main__': unittest.main() diff --git a/tests/testdata/multilayer.kml b/tests/testdata/multilayer.kml new file mode 100644 index 00000000000..a3723ec7fb8 --- /dev/null +++ b/tests/testdata/multilayer.kml @@ -0,0 +1,24 @@ + + + + Test + + Layer1 + + Placemark1 + + 2,49,0 + + + + + Layer2 + + Placemark2 + + 3,50,0 + + + + +