Merge pull request #7207 from rouault/geojson_add

[OGR provider] Fix adding a new GeoJSON field, starting a new edit session and filling it (fixes #7205)

src/providers/ogr/qgsogrprovider.cpp

@@ -1548,29 +1548,13 @@ bool QgsOgrProvider::addFeatures( QgsFeatureList &flist, Flags flags )
   return returnvalue;
 }
 
-bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
+bool QgsOgrProvider::addAttributeOGRLevel( const QgsField &field, bool &ignoreErrorOut )
 {
-  if ( !doInitialActionsForEdition() )
-    return false;
-
-  if ( mGDALDriverName == QLatin1String( "MapInfo File" ) )
-  {
-    // adding attributes in mapinfo requires to be able to delete the .dat file
-    // so drop any cached connections.
-    QgsOgrConnPool::instance()->invalidateConnections( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ) ) );
-  }
-
-  bool returnvalue = true;
-
-  QMap< QString, QgsField > mapFieldNameToOriginalField;
-
-  for ( QList<QgsField>::const_iterator iter = attributes.constBegin(); iter != attributes.constEnd(); ++iter )
-  {
-    mapFieldNameToOriginalField[ iter->name()] = *iter;
+  ignoreErrorOut = false;
 
   OGRFieldType type;
 
-    switch ( iter->type() )
+  switch ( field.type() )
   {
     case QVariant::Int:
     case QVariant::Bool:
@@ -1603,19 +1587,19 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
       type = OFTString;
       break;
     default:
-        pushError( tr( "type %1 for field %2 not found" ).arg( iter->typeName(), iter->name() ) );
-        returnvalue = false;
-        continue;
+      pushError( tr( "type %1 for field %2 not found" ).arg( field.typeName(), field.name() ) );
+      ignoreErrorOut = true;
+      return false;
   }
 
-    gdal::ogr_field_def_unique_ptr fielddefn( OGR_Fld_Create( textEncoding()->fromUnicode( iter->name() ).constData(), type ) );
-    int width = iter->length();
-    if ( iter->precision() )
+  gdal::ogr_field_def_unique_ptr fielddefn( OGR_Fld_Create( textEncoding()->fromUnicode( field.name() ).constData(), type ) );
+  int width = field.length();
+  if ( field.precision() )
     width += 1;
   OGR_Fld_SetWidth( fielddefn.get(), width );
-    OGR_Fld_SetPrecision( fielddefn.get(), iter->precision() );
+  OGR_Fld_SetPrecision( fielddefn.get(), field.precision() );
 
-    switch ( iter->type() )
+  switch ( field.type() )
   {
     case QVariant::Bool:
       OGR_Fld_SetSubType( fielddefn.get(), OFSTBoolean );
@@ -1625,8 +1609,40 @@ bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
 
   if ( mOgrLayer->CreateField( fielddefn.get(), true ) != OGRERR_NONE )
   {
-      pushError( tr( "OGR error creating field %1: %2" ).arg( iter->name(), CPLGetLastErrorMsg() ) );
+    pushError( tr( "OGR error creating field %1: %2" ).arg( field.name(), CPLGetLastErrorMsg() ) );
+    return false;
+  }
+  return true;
+}
+
+bool QgsOgrProvider::addAttributes( const QList<QgsField> &attributes )
+{
+  if ( !doInitialActionsForEdition() )
+    return false;
+
+  if ( mGDALDriverName == QLatin1String( "MapInfo File" ) )
+  {
+    // adding attributes in mapinfo requires to be able to delete the .dat file
+    // so drop any cached connections.
+    QgsOgrConnPool::instance()->invalidateConnections( QgsOgrProviderUtils::connectionPoolId( dataSourceUri( true ) ) );
+  }
+
+  bool returnvalue = true;
+
+  QMap< QString, QgsField > mapFieldNameToOriginalField;
+
+  for ( const auto &field : attributes )
+  {
+    mapFieldNameToOriginalField[ field.name()] = field;
+
+    bool ignoreErrorOut = false;
+    if ( !addAttributeOGRLevel( field, ignoreErrorOut ) )
+    {
       returnvalue = false;
+      if ( !ignoreErrorOut )
+      {
+        break;
+      }
     }
   }
 
@@ -4270,6 +4286,18 @@ bool QgsOgrProvider::leaveUpdateMode()
       reloadData();
       if ( mValid )
       {
+        // Make sure that new fields we added, but didn't populate yet, are
+        // recreated at the OGR level, otherwise we won't be able to populate
+        // them.
+        for ( const auto &field : oldFields )
+        {
+          int idx = mAttributeFields.lookupField( field.name() );
+          if ( idx < 0 )
+          {
+            bool ignoreErrorOut = false;
+            addAttributeOGRLevel( field, ignoreErrorOut );
+          }
+        }
         mAttributeFields = oldFields;
       }
     }

src/providers/ogr/qgsogrprovider.h

@@ -313,6 +313,8 @@ class QgsOgrProvider : public QgsVectorDataProvider
 
     bool doInitialActionsForEdition();
 
+    bool addAttributeOGRLevel( const QgsField &field, bool &ignoreErrorOut );
+
 #ifndef QT_NO_NETWORKPROXY
     void setupProxy();
 #endif

tests/src/python/test_provider_ogr.py

@@ -369,6 +369,41 @@ class PyQgsOGRProvider(unittest.TestCase):
         # all in the GeoJSON file, so it has disappeared
         self.assertEqual(len(vl.fields()), 1)
 
+    def testEditGeoJsonAddFieldAndThenAddFeatures(self):
+        """ Test bugfix of https://issues.qgis.org/issues/18596 (adding a new field)"""
+
+        datasource = os.path.join(self.basetestpath, 'testEditGeoJsonAddField.json')
+        with open(datasource, 'wt') as f:
+            f.write("""{
+"type": "FeatureCollection",
+"features": [
+{ "type": "Feature", "properties": { "x": 1 }, "geometry": { "type": "Point", "coordinates": [ 0, 0 ] } } ] }""")
+
+        vl = QgsVectorLayer(datasource, 'test', 'ogr')
+        self.assertTrue(vl.isValid())
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.addAttribute(QgsField('strfield', QVariant.String)))
+        self.assertTrue(vl.commitChanges())
+        self.assertEqual(len(vl.dataProvider().fields()), 1 + 1)
+        self.assertEqual([f.name() for f in vl.dataProvider().fields()], ['x', 'strfield'])
+
+        f = QgsFeature()
+        self.assertTrue(vl.getFeatures(QgsFeatureRequest()).nextFeature(f))
+        self.assertIsNone(f['strfield'])
+        self.assertEqual([field.name() for field in f.fields()], ['x', 'strfield'])
+
+        self.assertTrue(vl.startEditing())
+        vl.changeAttributeValue(f.id(), 1, 'x')
+        self.assertTrue(vl.commitChanges())
+        f = QgsFeature()
+        self.assertTrue(vl.getFeatures(QgsFeatureRequest()).nextFeature(f))
+        self.assertEqual(f['strfield'], 'x')
+        self.assertEqual([field.name() for field in f.fields()], ['x', 'strfield'])
+
+        # Completely reload file
+        vl = QgsVectorLayer(datasource, 'test', 'ogr')
+        self.assertEqual(len(vl.fields()), 2)
+
 
 if __name__ == '__main__':
     unittest.main()