[OGR provider] Do not corrupt values when updating a GPKG feature

and that the fid value is included in the fields to update, and that it doesn't change. Also rollback all changes if an update of FID is attempted. Fixes qgis#42274
rouault · May 24, 2021 · 574399c · 574399c
1 parent 75bb5d6
commit 574399c
Show file tree

Hide file tree

Showing 3 changed files with 33 additions and 5 deletions.
diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp
@@ -2385,8 +2385,9 @@ bool QgsOgrProvider::changeAttributeValues( const QgsChangedAttributesMap &attr_
           if ( it2->toLongLong() != fid )
           {
             pushError( tr( "Changing feature id of feature %1 is not allowed." ).arg( fid ) );
-            continue;
+            returnValue = false;
           }
+          continue;
         }
         else
         {

diff --git a/tests/src/python/test_provider_ogr_gpkg.py b/tests/src/python/test_provider_ogr_gpkg.py
@@ -1059,23 +1059,50 @@ def testGeopackageLargeFID(self):
         vl = QgsVectorLayer(u'{}'.format(tmpfile) + "|layername=" + "test", 'test', u'ogr')
         f = QgsFeature()
         f.setAttributes([1234567890123, None])
+        f2 = QgsFeature()
+        f2.setAttributes([1234567890124, None])
         self.assertTrue(vl.startEditing())
-        self.assertTrue(vl.dataProvider().addFeatures([f]))
+        self.assertTrue(vl.dataProvider().addFeatures([f, f2]))
         self.assertTrue(vl.commitChanges())
 
-        got = [feat for feat in vl.getFeatures()][0]
+        got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1234567890123))][0]
         self.assertEqual(got['fid'], 1234567890123)
 
         self.assertTrue(vl.startEditing())
         self.assertTrue(vl.changeGeometry(1234567890123, QgsGeometry.fromWkt('Point (3 50)')))
         self.assertTrue(vl.changeAttributeValue(1234567890123, 1, 'foo'))
         self.assertTrue(vl.commitChanges())
 
-        got = [feat for feat in vl.getFeatures()][0]
+        got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1234567890123))][0]
         self.assertEqual(got['str_field'], 'foo')
         got_geom = got.geometry()
         self.assertIsNotNone(got_geom)
 
+        # We don't change the FID, so OK
+        self.assertTrue(vl.startEditing())
+        self.assertTrue(vl.dataProvider().changeAttributeValues({1234567890123: {0: 1234567890123, 1: 'bar'},
+                                                                 1234567890124: {0: 1234567890124, 1: 'bar2'}}))
+        self.assertTrue(vl.commitChanges())
+
+        got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1234567890123))][0]
+        self.assertEqual(got['str_field'], 'bar')
+
+        got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1234567890124))][0]
+        self.assertEqual(got['str_field'], 'bar2')
+
+        # We try to change the FID, not allowed
+        # also check that all changes where reverted
+        self.assertTrue(vl.startEditing())
+        self.assertFalse(vl.dataProvider().changeAttributeValues({1234567890123: {0: 1, 1: 'baz'},
+                                                                  1234567890124: {1: 'baz2'}}))
+        self.assertTrue(vl.commitChanges())
+
+        got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1234567890123))][0]
+        self.assertEqual(got['str_field'], 'bar')
+
+        got = [feat for feat in vl.getFeatures(QgsFeatureRequest(1234567890124))][0]
+        self.assertEqual(got['str_field'], 'bar2')
+
         self.assertTrue(vl.startEditing())
         self.assertTrue(vl.deleteFeature(1234567890123))
         self.assertTrue(vl.commitChanges())

diff --git a/tests/src/python/test_provider_ogr_sqlite.py b/tests/src/python/test_provider_ogr_sqlite.py
@@ -119,7 +119,7 @@ def testFidSupport(self):
         self.assertEqual(got, [(9876543210, 'baw')])
 
         # Not allowed: changing the fid regular field
-        self.assertTrue(vl.dataProvider().changeAttributeValues({9876543210: {0: 12, 1: 'baw'}}))
+        self.assertFalse(vl.dataProvider().changeAttributeValues({9876543210: {0: 12, 1: 'baw'}}))
 
         got = [(f.attribute('fid'), f.attribute('strfield')) for f in vl.getFeatures(QgsFeatureRequest().setFilterExpression("fid = 9876543210"))]
         self.assertEqual(got, [(9876543210, 'baw')])