Merge pull request #42198 from qgis/backport-42189-to-release-3_18

[Backport release-3_18] [ogr] Fix absence of proper feature ID when adding features to CSV, ODS, and XLSX datasets
qgis · Mar 16, 2021 · 0b36c7b · 0b36c7b
2 parents b2b7956 + 1802269
commit 0b36c7b
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 10 deletions.
diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp
@@ -1696,7 +1696,7 @@ QString QgsOgrProvider::jsonStringValue( const QVariant &value ) const
   return stringValue;
 }
 
-bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags )
+bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags, QgsFeatureId incrementalFeatureId )
 {
   bool returnValue = true;
   QgsOgrFeatureDefn &featureDefinition = mOgrLayer->GetLayerDefn();
@@ -1893,16 +1893,23 @@ bool QgsOgrProvider::addFeaturePrivate( QgsFeature &f, Flags flags )
     pushError( tr( "OGR error creating feature %1: %2" ).arg( f.id() ).arg( CPLGetLastErrorMsg() ) );
     returnValue = false;
   }
-  else if ( !( flags & QgsFeatureSink::FastInsert ) )
+  else
   {
-    QgsFeatureId id = static_cast<QgsFeatureId>( OGR_F_GetFID( feature.get() ) );
-    if ( id >= 0 )
+    if ( !( flags & QgsFeatureSink::FastInsert ) )
     {
-      f.setId( id );
+      QgsFeatureId id = static_cast<QgsFeatureId>( OGR_F_GetFID( feature.get() ) );
+      if ( id >= 0 )
+      {
+        f.setId( id );
 
-      if ( mFirstFieldIsFid && attributes.count() > 0 )
+        if ( mFirstFieldIsFid && attributes.count() > 0 )
+        {
+          f.setAttribute( 0, id );
+        }
+      }
+      else if ( incrementalFeatureId >= 0 )
       {
-        f.setAttribute( 0, id );
+        f.setId( incrementalFeatureId );
       }
     }
   }
@@ -1923,13 +1930,34 @@ bool QgsOgrProvider::addFeatures( QgsFeatureList &flist, Flags flags )
 
   const bool inTransaction = startTransaction();
 
+  QgsFeatureId incrementalFeatureId = -1;
+  if ( !( flags & QgsFeatureSink::FastInsert ) &&
+       ( mGDALDriverName == QLatin1String( "CSV" ) || mGDALDriverName == QLatin1String( "XLSX" ) || mGDALDriverName == QLatin1String( "ODS" ) ) )
+  {
+    QMutex *mutex = nullptr;
+    OGRLayerH layer = mOgrOrigLayer->getHandleAndMutex( mutex );
+    {
+      QMutexLocker locker( mutex );
+
+      if ( !mSubsetString.isEmpty() )
+        OGR_L_SetAttributeFilter( layer, nullptr );
+
+      incrementalFeatureId = static_cast< QgsFeatureId >( OGR_L_GetFeatureCount( layer, false ) ) + 1;
+
+      if ( !mSubsetString.isEmpty() )
+        OGR_L_SetAttributeFilter( layer, textEncoding()->fromUnicode( mSubsetString ).constData() );
+    }
+  }
+
   bool returnvalue = true;
   for ( QgsFeatureList::iterator it = flist.begin(); it != flist.end(); ++it )
   {
-    if ( !addFeaturePrivate( *it, flags ) )
+    if ( !addFeaturePrivate( *it, flags, incrementalFeatureId ) )
     {
       returnvalue = false;
     }
+    if ( incrementalFeatureId >= 0 )
+      incrementalFeatureId++;
   }
 
   if ( inTransaction )

diff --git a/src/core/providers/ogr/qgsogrprovider.h b/src/core/providers/ogr/qgsogrprovider.h
@@ -309,10 +309,12 @@ class QgsOgrProvider final: public QgsVectorDataProvider
 
     mutable QStringList mSubLayerList;
 
-    //! converts \a value from json QVariant to QString
+    //! Converts \a value from json QVariant to QString
     QString jsonStringValue( const QVariant &value ) const;
 
-    bool addFeaturePrivate( QgsFeature &f, QgsFeatureSink::Flags flags );
+    //! The \a incrementalFeatureId will generally be -1, except for a few OGR drivers where QGIS will pass on a value when OGR doesn't set it
+    bool addFeaturePrivate( QgsFeature &f, QgsFeatureSink::Flags flags, QgsFeatureId incrementalFeatureId = -1 );
+
     //! Deletes one feature
     bool deleteFeature( QgsFeatureId id );
 

diff --git a/tests/src/core/testqgsogrprovider.cpp b/tests/src/core/testqgsogrprovider.cpp
@@ -49,6 +49,7 @@ class TestQgsOgrProvider : public QObject
     void decodeUri();
     void encodeUri();
     void testThread();
+    void testCsvFeatureAddition();
 
   private:
     QString mTestDataDir;
@@ -352,6 +353,48 @@ void TestQgsOgrProvider::testThread()
 
 }
 
+void TestQgsOgrProvider::testCsvFeatureAddition()
+{
+  QString csvFilename = QDir::tempPath() + "/csvfeatureadditiontest.csv";
+  QFile csvFile( csvFilename );
+  if ( csvFile.open( QIODevice::WriteOnly | QIODevice::Truncate ) )
+  {
+    QTextStream textStream( &csvFile );
+    textStream << QLatin1String( "col1,col2,col3\n0,0,\"csv0\"\n" );
+    csvFile.close();
+  }
+
+  QgsVectorLayer *csvLayer = new QgsVectorLayer( csvFilename, QStringLiteral( "csv" ) );
+  QVERIFY( csvLayer->isValid() );
+  QCOMPARE( csvLayer->featureCount(), 1 );
+
+  QgsFeature f1( csvLayer->fields() );
+  f1.setAttribute( 0, 1 );
+  f1.setAttribute( 1, 1 );
+  f1.setAttribute( 2, QLatin1String( "csv1" ) );
+  QgsFeature f2( csvLayer->fields() );
+  f2.setAttribute( 0, 2 );
+  f2.setAttribute( 1, 2 );
+  f2.setAttribute( 2, QLatin1String( "csv2" ) );
+
+  QgsFeatureList features;
+  features << f1 << f2;
+  csvLayer->dataProvider()->addFeatures( features );
+  QCOMPARE( features.at( 0 ).id(), 2 );
+  QCOMPARE( features.at( 1 ).id(), 3 );
+
+  csvLayer->setSubsetString( QStringLiteral( "col1 = '2'" ) );
+  QCOMPARE( csvLayer->featureCount(), 1 );
+
+  features.clear();
+  features << f1;
+  csvLayer->dataProvider()->addFeatures( features );
+  QCOMPARE( features.at( 0 ).id(), 4 );
+
+  delete csvLayer;
+  QFile::remove( csvFilename );
+}
+
 
 QGSTEST_MAIN( TestQgsOgrProvider )
 #include "testqgsogrprovider.moc"