Skip to content

Commit

Permalink
ogr2ogr: error out if WKT argument of -clipsrc/-clipdst is an invalid…
Browse files Browse the repository at this point in the history 
… geometry (OSGeo#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 OSGeo#10289
  • Loading branch information
@rouault
rouault authored Jun 24, 2024
1 parent a44e402 commit 1015b1c
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 20 deletions.
1 change: 1 addition & 0 deletions apps/gdal_utils_priv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ struct GDALVectorTranslateOptionsForBinary
CPLStringList aosOpenOptions{};
std::string osFormat;
GDALVectorTranslateAccessMode eAccessMode = ACCESS_CREATION;
bool bShowUsageIfError = false;

/* Allowed input drivers. */
CPLStringList aosAllowInputDrivers{};
Expand Down
3 changes: 2 additions & 1 deletion apps/ogr2ogr_bin.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ MAIN_START(nArgc, papszArgv)

if (psOptions == nullptr)
{
Usage();
if (sOptionsForBinary.bShowUsageIfError)
Usage();
goto exit;
}

Expand Down
104 changes: 91 additions & 13 deletions apps/ogr2ogr_lib.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,8 @@ static OGRLayer *GetLayerAndOverwriteIfNecessary(GDALDataset *poDstDS,
static std::unique_ptr<OGRGeometry> 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>(
GDALDataset::Open(osDS.c_str(), GDAL_OF_VECTOR));
Expand Down Expand Up @@ -661,16 +662,39 @@ static std::unique_ptr<OGRGeometry> 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<OGRGeometry>(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
{
Expand Down Expand Up @@ -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,
Expand All @@ -2328,19 +2353,68 @@ 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<OGRGeometry>(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,
"cannot load dest clip geometry");
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<OGRGeometry>(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;
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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;
}
}
Expand Down
74 changes: 68 additions & 6 deletions autotest/utilities/test_ogr2ogr_lib.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 1015b1c

Please sign in to comment.