Skip to content

Commit 72d75ff

Browse files
committed
[OGR provider] Make changeGeometryValues() accept null geometry
Fixes #15081
1 parent 7b0bec7 commit 72d75ff

File tree

2 files changed

+46
-22
lines changed

2 files changed

+46
-22
lines changed

src/providers/ogr/qgsogrprovider.cpp

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1323,9 +1323,6 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
13231323
if ( !doInitialActionsForEdition() )
13241324
return false;
13251325

1326-
OGRFeatureH theOGRFeature = nullptr;
1327-
OGRGeometryH theNewGeometry = nullptr;
1328-
13291326
setRelevantFields( ogrLayer, true, attributeIndexes() );
13301327

13311328
for ( QgsGeometryMap::const_iterator it = geometry_map.constBegin(); it != geometry_map.constEnd(); ++it )
@@ -1336,46 +1333,55 @@ bool QgsOgrProvider::changeGeometryValues( const QgsGeometryMap &geometry_map )
13361333
continue;
13371334
}
13381335

1339-
theOGRFeature = OGR_L_GetFeature( ogrLayer, static_cast<long>( FID_TO_NUMBER( it.key() ) ) );
1336+
OGRFeatureH theOGRFeature = OGR_L_GetFeature( ogrLayer, static_cast<long>( FID_TO_NUMBER( it.key() ) ) );
13401337
if ( !theOGRFeature )
13411338
{
13421339
pushError( tr( "OGR error changing geometry: feature %1 not found" ).arg( it.key() ) );
13431340
continue;
13441341
}
13451342

1346-
//create an OGRGeometry
1347-
if ( OGR_G_CreateFromWkb( const_cast<unsigned char*>( it->asWkb() ),
1348-
OGR_L_GetSpatialRef( ogrLayer ),
1349-
&theNewGeometry,
1350-
it->wkbSize() ) != OGRERR_NONE )
1343+
OGRGeometryH theNewGeometry = nullptr;
1344+
// We might receive null geometries. It is ok, but don't go through the
1345+
// OGR_G_CreateFromWkb() route then
1346+
if ( it->wkbSize() != 0 )
13511347
{
1352-
pushError( tr( "OGR error creating geometry for feature %1: %2" ).arg( it.key() ).arg( CPLGetLastErrorMsg() ) );
1353-
OGR_G_DestroyGeometry( theNewGeometry );
1354-
theNewGeometry = nullptr;
1355-
continue;
1356-
}
1348+
//create an OGRGeometry
1349+
if ( OGR_G_CreateFromWkb( const_cast<unsigned char*>( it->asWkb() ),
1350+
OGR_L_GetSpatialRef( ogrLayer ),
1351+
&theNewGeometry,
1352+
it->wkbSize() ) != OGRERR_NONE )
1353+
{
1354+
pushError( tr( "OGR error creating geometry for feature %1: %2" ).arg( it.key() ).arg( CPLGetLastErrorMsg() ) );
1355+
OGR_G_DestroyGeometry( theNewGeometry );
1356+
theNewGeometry = nullptr;
1357+
OGR_F_Destroy( theOGRFeature );
1358+
continue;
1359+
}
13571360

1358-
if ( !theNewGeometry )
1359-
{
1360-
pushError( tr( "OGR error in feature %1: geometry is null" ).arg( it.key() ) );
1361-
continue;
1361+
if ( !theNewGeometry )
1362+
{
1363+
pushError( tr( "OGR error in feature %1: geometry is null" ).arg( it.key() ) );
1364+
OGR_F_Destroy( theOGRFeature );
1365+
continue;
1366+
}
13621367
}
13631368

13641369
//set the new geometry
13651370
if ( OGR_F_SetGeometryDirectly( theOGRFeature, theNewGeometry ) != OGRERR_NONE )
13661371
{
13671372
pushError( tr( "OGR error setting geometry of feature %1: %2" ).arg( it.key() ).arg( CPLGetLastErrorMsg() ) );
1368-
OGR_G_DestroyGeometry( theNewGeometry );
1369-
theNewGeometry = nullptr;
1373+
// Shouldn't happen normally. If it happens, ownership of the geometry
1374+
// may be not really well defined, so better not destroy it, but just
1375+
// the feature.
1376+
OGR_F_Destroy( theOGRFeature );
13701377
continue;
13711378
}
13721379

13731380

13741381
if ( OGR_L_SetFeature( ogrLayer, theOGRFeature ) != OGRERR_NONE )
13751382
{
13761383
pushError( tr( "OGR error setting feature %1: %2" ).arg( it.key() ).arg( CPLGetLastErrorMsg() ) );
1377-
OGR_G_DestroyGeometry( theNewGeometry );
1378-
theNewGeometry = nullptr;
1384+
OGR_F_Destroy( theOGRFeature );
13791385
continue;
13801386
}
13811387
mShapefileMayBeCorrupted = true;

tests/src/python/test_provider_shapefile.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,5 +244,23 @@ def testreloadData(self):
244244
# And now check that fields are up-to-date
245245
self.assertEquals(len(vl1.fields()), len(vl2.fields()))
246246

247+
def testDeleteGeometry(self):
248+
''' Test changeGeometryValues() with a null geometry '''
249+
250+
tmpdir = tempfile.mkdtemp()
251+
self.dirs_to_cleanup.append(tmpdir)
252+
srcpath = os.path.join(TEST_DATA_DIR, 'provider')
253+
for file in glob.glob(os.path.join(srcpath, 'shapefile.*')):
254+
shutil.copy(os.path.join(srcpath, file), tmpdir)
255+
datasource = os.path.join(tmpdir, 'shapefile.shp')
256+
257+
vl = QgsVectorLayer(u'{}|layerid=0'.format(datasource), u'test', u'ogr')
258+
self.assertTrue(vl.dataProvider().changeGeometryValues({0: QgsGeometry()}))
259+
vl = None
260+
261+
vl = QgsVectorLayer(u'{}|layerid=0'.format(datasource), u'test', u'ogr')
262+
fet = next(vl.getFeatures())
263+
self.assertIsNone(fet.geometry())
264+
247265
if __name__ == '__main__':
248266
unittest.main()

0 commit comments

Comments
 (0)