Merge pull request #7977 from rouault/fix_ogrdataitems_layername

[OGR provider] Make sure to use layername= syntax in URI of multilayer datasources (fixes #19885)
qgis · Sep 21, 2018 · bb40385 · bb40385
2 parents eb24bdb + ea2cc36
commit bb40385
Show file tree

Hide file tree

Showing 8 changed files with 108 additions and 24 deletions.
diff --git 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/python/core/conversions.sip b/python/core/conversions.sip
@@ -1457,15 +1457,10 @@ template <TYPE>
 
         if (tobj == NULL || PyList_SetItem(l, i, tobj) < 0)
         {
+            Py_DECREF(tobj);
             Py_DECREF(l);
-
-            if (!tobj)
-                delete t;
-
             return NULL;
         }
-
-        Py_DECREF(tobj);
     }
 
     return l;

diff --git 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
@@ -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<QgsDataItem *> 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
@@ -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
@@ -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
@@ -19,7 +19,8 @@
 
 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 @@ def testEditGeoJsonAddFieldAndThenAddFeatures(self):
         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
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<kml xmlns="http://www.opengis.net/kml/2.2" xmlns:kml="http://www.opengis.net/kml/2.2">
+    <Document> 
+        <name>Test</name>
+        <Folder id="Layer1">
+            <name>Layer1</name>
+            <Placemark>
+                <name>Placemark1</name>
+                <Point>
+                    <coordinates>2,49,0</coordinates>
+                </Point>
+            </Placemark>
+        </Folder>
+        <Folder id="Layer2">
+            <name>Layer2</name>
+            <Placemark>
+                <name>Placemark2</name>
+                <Point>
+                    <coordinates>3,50,0</coordinates>
+                </Point>
+            </Placemark>
+        </Folder>
+    </Document>
+</kml>