[OGR provider] Complementary fix for #18596, related to adding a new field to a GeoJSON file

rouault · rouault · commit e94b1acd77a0 · 2018-06-05T14:26:23.000+02:00
diff --git a/src/providers/ogr/qgsogrprovider.cpp b/src/providers/ogr/qgsogrprovider.cpp
@@ -4264,7 +4264,14 @@ bool QgsOgrProvider::leaveUpdateMode()
     // use GDALDatasetFlush().
     if ( mGDALDriverName == QLatin1String( "GeoJSON" ) )
     {
+      // Backup fields since if we created new fields, but didn't populate it
+      // with any feature yet, it will disappear.
+      QgsFields oldFields = mAttributeFields;
       reloadData();
+      if ( mValid )
+      {
+        mAttributeFields = oldFields;
+      }
     }
     return true;
   }
diff --git a/tests/src/python/test_provider_ogr.py b/tests/src/python/test_provider_ogr.py
@@ -18,7 +18,8 @@
 import tempfile
 
 from osgeo import gdal, ogr  # NOQA
-from qgis.core import (QgsFeature, QgsFeatureRequest, QgsSettings, QgsDataProvider,
+from qgis.PyQt.QtCore import QVariant
+from qgis.core import (QgsFeature, QgsFeatureRequest, QgsField, QgsSettings, QgsDataProvider,
                        QgsVectorDataProvider, QgsVectorLayer, QgsWkbTypes, QgsNetworkAccessManager)
 from qgis.testing import start_app, unittest
 
@@ -318,10 +319,10 @@ def testSetupProxy(self):
         self.assertEqual(gdal.GetConfigOption("GDAL_HTTP_PROXY"), "myproxyhostname.com")
         self.assertEqual(gdal.GetConfigOption("GDAL_HTTP_PROXYUSERPWD"), "username")
 
-    def testEditGeoJson(self):
-        """ Test bugfix of https://issues.qgis.org/issues/18596 """
+    def testEditGeoJsonRemoveField(self):
+        """ Test bugfix of https://issues.qgis.org/issues/18596 (deleting an existing field)"""
 
-        datasource = os.path.join(self.basetestpath, 'testEditGeoJson.json')
+        datasource = os.path.join(self.basetestpath, 'testEditGeoJsonRemoveField.json')
         with open(datasource, 'wt') as f:
             f.write("""{
 "type": "FeatureCollection",
@@ -333,13 +334,41 @@ def testEditGeoJson(self):
         self.assertTrue(vl.startEditing())
         self.assertTrue(vl.deleteAttribute(1))
         self.assertTrue(vl.commitChanges())
+        self.assertEqual(len(vl.dataProvider().fields()), 4 - 1)
 
         f = QgsFeature()
         self.assertTrue(vl.getFeatures(QgsFeatureRequest()).nextFeature(f))
         self.assertEqual(f['x'], 1)
         self.assertEqual(f['z'], 3)
         self.assertEqual(f['w'], 4)
 
+    def testEditGeoJsonAddField(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)
+
+        f = QgsFeature()
+        self.assertTrue(vl.getFeatures(QgsFeatureRequest()).nextFeature(f))
+        self.assertIsNone(f['strfield'])
+
+        # Completely reload file
+        vl = QgsVectorLayer(datasource, 'test', 'ogr')
+        # As we didn't set any value to the new field, it is not written at
+        # all in the GeoJSON file, so it has disappeared
+        self.assertEqual(len(vl.fields()), 1)
+
 
 if __name__ == '__main__':
     unittest.main()

Original file line number	Diff line number	Diff line change
`@@ -4264,7 +4264,14 @@ bool QgsOgrProvider::leaveUpdateMode()`
`4264`	`4264`	`// use GDALDatasetFlush().`
`4265`	`4265`	`if ( mGDALDriverName == QLatin1String( "GeoJSON" ) )`
`4266`	`4266`	`{`
	`4267`	`+ // Backup fields since if we created new fields, but didn't populate it`
	`4268`	`+ // with any feature yet, it will disappear.`
	`4269`	`+ QgsFields oldFields = mAttributeFields;`
`4267`	`4270`	`reloadData();`
	`4271`	`+ if ( mValid )`
	`4272`	`+ {`
	`4273`	`+ mAttributeFields = oldFields;`
	`4274`	`+ }`
`4268`	`4275`	`}`
`4269`	`4276`	`return true;`
`4270`	`4277`	`}`