From 1015b1c32040a3d9db614489d29c2f68f23378d0 Mon Sep 17 00:00:00 2001 From: Even Rouault Date: Mon, 24 Jun 2024 21:20:19 +0200 Subject: [PATCH] ogr2ogr: error out if WKT argument of -clipsrc/-clipdst is an invalid geometry (#10292) If specifying -makevalid, MakeValid() will be run on it to make it valid. For consistency, change behavior of -clipsrc/-clipdst {datasetname} to not automatically make valid invalid clip geometries, but do it only if -makevalid is specified. Fixes #10289 --- apps/gdal_utils_priv.h | 1 + apps/ogr2ogr_bin.cpp | 3 +- apps/ogr2ogr_lib.cpp | 104 +++++++++++++++++++++---- autotest/utilities/test_ogr2ogr_lib.py | 74 ++++++++++++++++-- 4 files changed, 162 insertions(+), 20 deletions(-) diff --git a/apps/gdal_utils_priv.h b/apps/gdal_utils_priv.h index 5f148e118e57..2aac09d0dd1f 100644 --- a/apps/gdal_utils_priv.h +++ b/apps/gdal_utils_priv.h @@ -85,6 +85,7 @@ struct GDALVectorTranslateOptionsForBinary CPLStringList aosOpenOptions{}; std::string osFormat; GDALVectorTranslateAccessMode eAccessMode = ACCESS_CREATION; + bool bShowUsageIfError = false; /* Allowed input drivers. */ CPLStringList aosAllowInputDrivers{}; diff --git a/apps/ogr2ogr_bin.cpp b/apps/ogr2ogr_bin.cpp index 9e12a64c647c..1fa30d46b5ff 100644 --- a/apps/ogr2ogr_bin.cpp +++ b/apps/ogr2ogr_bin.cpp @@ -104,7 +104,8 @@ MAIN_START(nArgc, papszArgv) if (psOptions == nullptr) { - Usage(); + if (sOptionsForBinary.bShowUsageIfError) + Usage(); goto exit; } diff --git a/apps/ogr2ogr_lib.cpp b/apps/ogr2ogr_lib.cpp index 097e5ba33f2c..0d24991f801d 100644 --- a/apps/ogr2ogr_lib.cpp +++ b/apps/ogr2ogr_lib.cpp @@ -616,7 +616,8 @@ static OGRLayer *GetLayerAndOverwriteIfNecessary(GDALDataset *poDstDS, static std::unique_ptr LoadGeometry(const std::string &osDS, const std::string &osSQL, const std::string &osLyr, - const std::string &osWhere) + const std::string &osWhere, + bool bMakeValid) { auto poDS = std::unique_ptr( GDALDataset::Open(osDS.c_str(), GDAL_OF_VECTOR)); @@ -661,16 +662,39 @@ static std::unique_ptr LoadGeometry(const std::string &osDS, { if (!poSrcGeom->IsValid()) { - CPLError(CE_Warning, CPLE_AppDefined, - "Geometry of feature " CPL_FRMT_GIB " of %s " - "is invalid. Trying to make it valid", - poFeat->GetFID(), osDS.c_str()); + if (!bMakeValid) + { + CPLError(CE_Failure, CPLE_AppDefined, + "Geometry of feature " CPL_FRMT_GIB " of %s " + "is invalid. You can try to make it valid by " + "specifying -makevalid, but the results of " + "the operation should be manually inspected.", + poFeat->GetFID(), osDS.c_str()); + oGC.empty(); + break; + } auto poValid = std::unique_ptr(poSrcGeom->MakeValid()); if (poValid) { + CPLError(CE_Warning, CPLE_AppDefined, + "Geometry of feature " CPL_FRMT_GIB " of %s " + "was invalid and has been made valid, " + "but the results of the operation " + "should be manually inspected.", + poFeat->GetFID(), osDS.c_str()); + oGC.addGeometryDirectly(poValid.release()); } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Geometry of feature " CPL_FRMT_GIB " of %s " + "is invalid, and could not been made valid.", + poFeat->GetFID(), osDS.c_str()); + oGC.empty(); + break; + } } else { @@ -2302,7 +2326,8 @@ GDALDatasetH GDALVectorTranslate(const char *pszDest, GDALDatasetH hDstDS, { psOptions->poClipSrc = LoadGeometry(psOptions->osClipSrcDS, psOptions->osClipSrcSQL, - psOptions->osClipSrcLayer, psOptions->osClipSrcWhere); + psOptions->osClipSrcLayer, psOptions->osClipSrcWhere, + psOptions->bMakeValid); if (psOptions->poClipSrc == nullptr) { CPLError(CE_Failure, CPLE_IllegalArg, @@ -2328,12 +2353,37 @@ GDALDatasetH GDALVectorTranslate(const char *pszDest, GDALDatasetH hDstDS, *pbUsageError = TRUE; return nullptr; } + if (psOptions->poClipSrc && !psOptions->poClipSrc->IsValid()) + { + if (!psOptions->bMakeValid) + { + CPLError(CE_Failure, CPLE_IllegalArg, + "-clipsrc geometry is invalid. You can try to make it " + "valid with -makevalid, but the results of the operation " + "should be manually inspected."); + return nullptr; + } + auto poValid = + std::unique_ptr(psOptions->poClipSrc->MakeValid()); + if (!poValid) + { + CPLError(CE_Failure, CPLE_IllegalArg, + "-clipsrc geometry is invalid and cannot be made valid."); + return nullptr; + } + CPLError(CE_Warning, CPLE_AppDefined, + "-clipsrc geometry was invalid and has been made valid, " + "but the results of the operation " + "should be manually inspected."); + psOptions->poClipSrc = std::move(poValid); + } if (!psOptions->osClipDstDS.empty()) { psOptions->poClipDst = LoadGeometry(psOptions->osClipDstDS, psOptions->osClipDstSQL, - psOptions->osClipDstLayer, psOptions->osClipDstWhere); + psOptions->osClipDstLayer, psOptions->osClipDstWhere, + psOptions->bMakeValid); if (psOptions->poClipDst == nullptr) { CPLError(CE_Failure, CPLE_IllegalArg, @@ -2341,6 +2391,30 @@ GDALDatasetH GDALVectorTranslate(const char *pszDest, GDALDatasetH hDstDS, return nullptr; } } + if (psOptions->poClipDst && !psOptions->poClipDst->IsValid()) + { + if (!psOptions->bMakeValid) + { + CPLError(CE_Failure, CPLE_IllegalArg, + "-clipdst geometry is invalid. You can try to make it " + "valid with -makevalid, but the results of the operation " + "should be manually inspected."); + return nullptr; + } + auto poValid = + std::unique_ptr(psOptions->poClipDst->MakeValid()); + if (!poValid) + { + CPLError(CE_Failure, CPLE_IllegalArg, + "-clipdst geometry is invalid and cannot be made valid."); + return nullptr; + } + CPLError(CE_Warning, CPLE_AppDefined, + "-clipdst geometry was invalid and has been made valid, " + "but the results of the operation " + "should be manually inspected."); + psOptions->poClipDst = std::move(poValid); + } GDALDataset *poDS = GDALDataset::FromHandle(hSrcDS); GDALDataset *poODS = nullptr; @@ -7708,9 +7782,10 @@ GDALVectorTranslateOptions *GDALVectorTranslateOptionsNew( psOptions->poClipSrc.reset(poGeom); if (psOptions->poClipSrc == nullptr) { - CPLError(CE_Failure, CPLE_IllegalArg, - "Invalid geometry. Must be a valid POLYGON or " - "MULTIPOLYGON WKT"); + CPLError( + CE_Failure, CPLE_IllegalArg, + "Invalid -clipsrc geometry. Must be a valid POLYGON or " + "MULTIPOLYGON WKT"); return nullptr; } } @@ -7762,9 +7837,10 @@ GDALVectorTranslateOptions *GDALVectorTranslateOptionsNew( psOptions->poClipDst.reset(poGeom); if (psOptions->poClipDst == nullptr) { - CPLError(CE_Failure, CPLE_IllegalArg, - "Invalid geometry. Must be a valid POLYGON or " - "MULTIPOLYGON WKT"); + CPLError( + CE_Failure, CPLE_IllegalArg, + "Invalid -clipdst geometry. Must be a valid POLYGON or " + "MULTIPOLYGON WKT"); return nullptr; } } @@ -7812,6 +7888,8 @@ GDALVectorTranslateOptions *GDALVectorTranslateOptionsNew( catch (const std::exception &err) { CPLError(CE_Failure, CPLE_AppDefined, "%s", err.what()); + if (psOptionsForBinary) + psOptionsForBinary->bShowUsageIfError = true; return nullptr; } } diff --git a/autotest/utilities/test_ogr2ogr_lib.py b/autotest/utilities/test_ogr2ogr_lib.py index 352564bd20f4..b94847000ba0 100755 --- a/autotest/utilities/test_ogr2ogr_lib.py +++ b/autotest/utilities/test_ogr2ogr_lib.py @@ -1206,12 +1206,58 @@ def test_ogr2ogr_lib_clipsrc_discard_lower_dimensionality(): ############################################################################### -# Test -clipsrc with a clip layer with an invalid polygon +# Test -clipsrc/-clipdst with a clip layer with an invalid polygon (specified "inline" as WKT) + + +@pytest.mark.require_geos +@gdaltest.enable_exceptions() +@pytest.mark.parametrize("clipSrc", [True, False]) +def test_ogr2ogr_lib_clip_invalid_polygon_inline(tmp_vsimem, clipSrc): + + srcDS = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown) + srs = osr.SpatialReference() + srs.ImportFromEPSG(4326) + srcLayer = srcDS.CreateLayer("test", srs=srs, geom_type=ogr.wkbLineString) + f = ogr.Feature(srcLayer.GetLayerDefn()) + f.SetGeometry(ogr.CreateGeometryFromWkt("POINT(0.25 0.25)")) + srcLayer.CreateFeature(f) + f = ogr.Feature(srcLayer.GetLayerDefn()) + f.SetGeometry(ogr.CreateGeometryFromWkt("POINT(-0.5 0.5)")) + srcLayer.CreateFeature(f) + + # Intersection of above geometry with clipSrc bounding box is a point + with pytest.raises(Exception, match="geometry is invalid"): + gdal.VectorTranslate( + "", + srcDS, + format="Memory", + clipSrc="POLYGON((0 0,1 1,0 1,1 0,0 0))" if clipSrc else None, + clipDst="POLYGON((0 0,1 1,0 1,1 0,0 0))" if not clipSrc else None, + ) + + with gdal.quiet_errors(): + ds = gdal.VectorTranslate( + "", + srcDS, + format="Memory", + makeValid=True, + clipSrc="POLYGON((0 0,1 1,0 1,1 0,0 0))" if clipSrc else None, + clipDst="POLYGON((0 0,1 1,0 1,1 0,0 0))" if not clipSrc else None, + ) + lyr = ds.GetLayer(0) + assert lyr.GetFeatureCount() == 1 + ds = None + + +############################################################################### +# Test -clipsrc with a clip layer with an invalid polygon (in a dataset) @pytest.mark.require_driver("GPKG") -@pytest.mark.require_geos(3, 8) -def test_ogr2ogr_lib_clipsrc_invalid_polygon(tmp_vsimem): +@pytest.mark.require_geos +@gdaltest.enable_exceptions() +@pytest.mark.parametrize("clipSrc", [True, False]) +def test_ogr2ogr_lib_clip_invalid_polygon(tmp_vsimem, clipSrc): srcDS = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown) srs = osr.SpatialReference() @@ -1235,8 +1281,24 @@ def test_ogr2ogr_lib_clipsrc_invalid_polygon(tmp_vsimem): clip_ds = None # Intersection of above geometry with clipSrc bounding box is a point + with pytest.raises(Exception, match=r"cannot load.*clip geometry"): + gdal.VectorTranslate( + "", + srcDS, + format="Memory", + clipSrc=clip_path if clipSrc else None, + clipDst=clip_path if not clipSrc else None, + ) + with gdal.quiet_errors(): - ds = gdal.VectorTranslate("", srcDS, format="Memory", clipSrc=clip_path) + ds = gdal.VectorTranslate( + "", + srcDS, + format="Memory", + makeValid=True, + clipSrc=clip_path if clipSrc else None, + clipDst=clip_path if not clipSrc else None, + ) lyr = ds.GetLayer(0) assert lyr.GetFeatureCount() == 1 ds = None @@ -1247,7 +1309,7 @@ def test_ogr2ogr_lib_clipsrc_invalid_polygon(tmp_vsimem): @pytest.mark.require_driver("GPKG") -@pytest.mark.require_geos(3, 8) +@pytest.mark.require_geos def test_ogr2ogr_lib_clipsrc_3d_polygon(tmp_vsimem): srcDS = gdal.GetDriverByName("Memory").Create("", 0, 0, 0, gdal.GDT_Unknown) @@ -1417,7 +1479,7 @@ def test_ogr2ogr_lib_clipdst_discard_lower_dimensionality(): ############################################################################### -# Test /-clipsrc-clipdst with reprojection +# Test -clipsrc / -clipdst with reprojection @pytest.mark.require_geos