diff --git a/src/core/providers/ogr/qgsogrprovider.cpp b/src/core/providers/ogr/qgsogrprovider.cpp index b7f59686bc67..3fede4df3a98 100644 --- 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 index aeda92ca3aa8..0ce1f5a908af 100644 --- a/tests/src/python/test_provider_ogr_gpkg.py +++ b/tests/src/python/test_provider_ogr_gpkg.py @@ -1059,11 +1059,13 @@ 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()) @@ -1071,11 +1073,36 @@ def testGeopackageLargeFID(self): 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 index 528a50e6defb..9e79918c34b7 100644 --- 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')])