diff --git a/apps/test_ogrsf.cpp b/apps/test_ogrsf.cpp index 06be2b69257e..9b2edcaf3ec2 100644 --- a/apps/test_ogrsf.cpp +++ b/apps/test_ogrsf.cpp @@ -3791,11 +3791,32 @@ static int TestLayerSQL(GDALDataset *poDS, OGRLayer *poLayer) oPoly.addRing(&oRing); CPLErrorReset(); - poSQLLyr = LOG_ACTION(poDS->ExecuteSQL(osSQL.c_str(), &oPoly, nullptr)); - if (CPLGetLastErrorType() == CE_Failure) + if (poLayer->GetLayerDefn()->GetGeomFieldCount() == 0) { - bRet = FALSE; - printf("ERROR: ExecuteSQL() triggered an unexpected error.\n"); + CPLPushErrorHandler(CPLQuietErrorHandler); + poSQLLyr = LOG_ACTION(poDS->ExecuteSQL(osSQL.c_str(), &oPoly, nullptr)); + CPLPopErrorHandler(); + if (poSQLLyr) + { + printf("WARNING: ExecuteSQL() with a spatial filter on a " + "non-spatial layer should have triggered an error.\n"); + } + } + else + { + poSQLLyr = LOG_ACTION(poDS->ExecuteSQL(osSQL.c_str(), &oPoly, nullptr)); + if (CPLGetLastErrorType() == CE_Failure && + poLayer->GetLayerDefn()->GetGeomFieldCount() > 0) + { + bRet = FALSE; + printf("ERROR: ExecuteSQL() triggered an unexpected error.\n"); + } + if (!poSQLLyr) + { + printf("ERROR: ExecuteSQL() should have returned a non-NULL " + "result.\n"); + bRet = FALSE; + } } if (poSQLLyr) { @@ -3815,11 +3836,6 @@ static int TestLayerSQL(GDALDataset *poDS, OGRLayer *poLayer) DestroyFeatureAndNullify(poSQLFeat); LOG_ACTION(poDS->ReleaseResultSet(poSQLLyr)); } - else - { - printf("ERROR: ExecuteSQL() should have returned a non-NULL result.\n"); - bRet = FALSE; - } if (bRet && bVerbose) printf("INFO: TestLayerSQL passed.\n"); diff --git a/autotest/ogr/ogr_sql_sqlite.py b/autotest/ogr/ogr_sql_sqlite.py index 9793ca1d3dbf..5c66950c9f07 100755 --- a/autotest/ogr/ogr_sql_sqlite.py +++ b/autotest/ogr/ogr_sql_sqlite.py @@ -2477,3 +2477,36 @@ def test_ogr_sql_sqlite_like_utf8(): dialect="SQLite", ) as sql_lyr: assert sql_lyr.GetFeatureCount() == 0 + + +############################################################################### +# Test error on setting a spatial filter during ExecuteSQL + + +@gdaltest.enable_exceptions() +def test_ogr_sql_sqlite_execute_sql_error_on_spatial_filter_mem_layer(): + + ds = ogr.GetDriverByName("Memory").CreateDataSource("") + ds.CreateLayer("test") + geom = ogr.CreateGeometryFromWkt("POLYGON((0 0,0 1,1 1,1 0,0 0))") + with pytest.raises( + Exception, match="Cannot set spatial filter: no geometry field selected" + ): + ds.ExecuteSQL("SELECT 1 FROM test", spatialFilter=geom, dialect="SQLITE") + + +############################################################################### +# Test error on setting a spatial filter during ExecuteSQL + + +@gdaltest.enable_exceptions() +def test_ogr_sql_sqlite_execute_sql_error_on_spatial_filter_shp_layer(tmp_vsimem): + + filename = str(tmp_vsimem / "test.shp") + ds = ogr.GetDriverByName("ESRI Shapefile").CreateDataSource(filename) + ds.CreateLayer("test") + geom = ogr.CreateGeometryFromWkt("POLYGON((0 0,0 1,1 1,1 0,0 0))") + with pytest.raises( + Exception, match="Cannot set spatial filter: no geometry field selected" + ): + ds.ExecuteSQL("SELECT 1 FROM test", spatialFilter=geom, dialect="SQLITE") diff --git a/autotest/ogr/ogr_sql_test.py b/autotest/ogr/ogr_sql_test.py index 4702e824c63a..96838eb953d7 100755 --- a/autotest/ogr/ogr_sql_test.py +++ b/autotest/ogr/ogr_sql_test.py @@ -1863,3 +1863,19 @@ def test_ogr_sql_ilike_utf8(): lyr.SetAttributeFilter("'éven' ILIKE '%xen'") assert lyr.GetFeatureCount() == 0 + + +############################################################################### +# Test error on setting a spatial filter during ExecuteSQL + + +@gdaltest.enable_exceptions() +def test_ogr_sql_test_execute_sql_error_on_spatial_filter_mem_layer(): + + ds = ogr.GetDriverByName("Memory").CreateDataSource("") + ds.CreateLayer("test", geom_type=ogr.wkbNone) + geom = ogr.CreateGeometryFromWkt("POLYGON((0 0,0 1,1 1,1 0,0 0))") + with pytest.raises( + Exception, match="Cannot set spatial filter: no geometry field present in layer" + ): + ds.ExecuteSQL("SELECT 1 FROM test", spatialFilter=geom) diff --git a/gcore/gdaldataset.cpp b/gcore/gdaldataset.cpp index dcfe31e3faa0..49118992ed81 100644 --- a/gcore/gdaldataset.cpp +++ b/gcore/gdaldataset.cpp @@ -6972,15 +6972,19 @@ OGRLayer *GDALDataset::BuildLayerFromSelectInfo( swq_select *psSelectInfo, OGRGeometry *poSpatialFilter, const char *pszDialect, swq_select_parse_options *poSelectParseOptions) { - OGRGenSQLResultsLayer *poResults = nullptr; + std::unique_ptr poResults; GDALSQLParseInfo *psParseInfo = BuildParseInfo(psSelectInfo, poSelectParseOptions); if (psParseInfo) { - poResults = - new OGRGenSQLResultsLayer(this, psSelectInfo, poSpatialFilter, - psParseInfo->pszWHERE, pszDialect); + const auto nErrorCounter = CPLGetErrorCounter(); + poResults = std::make_unique( + this, psSelectInfo, poSpatialFilter, psParseInfo->pszWHERE, + pszDialect); + if (CPLGetErrorCounter() > nErrorCounter && + CPLGetLastErrorType() != CE_None) + poResults.reset(); } else { @@ -6988,7 +6992,7 @@ OGRLayer *GDALDataset::BuildLayerFromSelectInfo( } DestroyParseInfo(psParseInfo); - return poResults; + return poResults.release(); } /************************************************************************/ diff --git a/ogr/ogrsf_frmts/generic/ogrlayer.cpp b/ogr/ogrsf_frmts/generic/ogrlayer.cpp index 5a1a621f14a0..0e6c5460b92c 100644 --- a/ogr/ogrsf_frmts/generic/ogrlayer.cpp +++ b/ogr/ogrsf_frmts/generic/ogrlayer.cpp @@ -1439,6 +1439,47 @@ OGRGeometryH OGR_L_GetSpatialFilter(OGRLayerH hLayer) OGRLayer::FromHandle(hLayer)->GetSpatialFilter()); } +/************************************************************************/ +/* ValidateGeometryFieldIndexForSetSpatialFilter() */ +/************************************************************************/ + +//! @cond Doxygen_Suppress +bool OGRLayer::ValidateGeometryFieldIndexForSetSpatialFilter( + int iGeomField, const OGRGeometry *poGeomIn, bool bIsSelectLayer) +{ + if (iGeomField == 0 && poGeomIn == nullptr && + GetLayerDefn()->GetGeomFieldCount() == 0) + { + // Setting a null spatial filter on geometry field idx 0 + // when there are no geometry field can't harm, and is accepted silently + // for backward compatibility with existing practice. + } + else if (iGeomField < 0 || + iGeomField >= GetLayerDefn()->GetGeomFieldCount()) + { + if (iGeomField == 0) + { + CPLError( + CE_Failure, CPLE_AppDefined, + bIsSelectLayer + ? "Cannot set spatial filter: no geometry field selected." + : "Cannot set spatial filter: no geometry field present in " + "layer."); + } + else + { + CPLError(CE_Failure, CPLE_AppDefined, + "Cannot set spatial filter on non-existing geometry field " + "of index %d.", + iGeomField); + } + return false; + } + return true; +} + +//! @endcond + /************************************************************************/ /* SetSpatialFilter() */ /************************************************************************/ @@ -1446,6 +1487,9 @@ OGRGeometryH OGR_L_GetSpatialFilter(OGRLayerH hLayer) void OGRLayer::SetSpatialFilter(OGRGeometry *poGeomIn) { + if (poGeomIn && !ValidateGeometryFieldIndexForSetSpatialFilter(0, poGeomIn)) + return; + m_iGeomFieldFilter = 0; if (InstallFilter(poGeomIn)) ResetReading(); @@ -1456,17 +1500,18 @@ void OGRLayer::SetSpatialFilter(int iGeomField, OGRGeometry *poGeomIn) { if (iGeomField == 0) { + if (poGeomIn && + !ValidateGeometryFieldIndexForSetSpatialFilter(0, poGeomIn)) + return; + m_iGeomFieldFilter = iGeomField; SetSpatialFilter(poGeomIn); } else { - if (iGeomField < 0 || iGeomField >= GetLayerDefn()->GetGeomFieldCount()) - { - CPLError(CE_Failure, CPLE_AppDefined, - "Invalid geometry field index : %d", iGeomField); + if (!ValidateGeometryFieldIndexForSetSpatialFilter(iGeomField, + poGeomIn)) return; - } m_iGeomFieldFilter = iGeomField; if (InstallFilter(poGeomIn)) diff --git a/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h b/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h index 617bd019b92e..031cabcf565d 100644 --- a/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h +++ b/ogr/ogrsf_frmts/gpkg/ogr_geopackage.h @@ -1164,6 +1164,16 @@ class OGRGeoPackageSelectLayer final : public OGRGeoPackageLayer, { return OGRGeoPackageLayer::GetExtent(iGeomField, psExtent, bForce); } + + bool + ValidateGeometryFieldIndexForSetSpatialFilter(int iGeomField, + const OGRGeometry *poGeomIn, + bool bIsSelectLayer) override + { + return OGRGeoPackageLayer:: + ValidateGeometryFieldIndexForSetSpatialFilter(iGeomField, poGeomIn, + bIsSelectLayer); + } }; #endif /* OGR_GEOPACKAGE_H_INCLUDED */ diff --git a/ogr/ogrsf_frmts/ogrsf_frmts.h b/ogr/ogrsf_frmts/ogrsf_frmts.h index 27c4b059b6ea..309be765f8e3 100644 --- a/ogr/ogrsf_frmts/ogrsf_frmts.h +++ b/ogr/ogrsf_frmts/ogrsf_frmts.h @@ -113,6 +113,10 @@ class CPL_DLL OGRLayer : public GDALMajorObject // int FilterGeometry( OGRGeometry *, OGREnvelope* // psGeometryEnvelope); int InstallFilter(OGRGeometry *); + bool + ValidateGeometryFieldIndexForSetSpatialFilter(int iGeomField, + const OGRGeometry *poGeomIn, + bool bIsSelectLayer = false); OGRErr GetExtentInternal(int iGeomField, OGREnvelope *psExtent, int bForce); //! @endcond diff --git a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h index 0916309c6797..3e869119a66e 100644 --- a/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h +++ b/ogr/ogrsf_frmts/sqlite/ogr_sqlite.h @@ -656,6 +656,15 @@ class OGRSQLiteSelectLayer CPL_NON_FINAL : public OGRSQLiteLayer, { return OGRSQLiteLayer::GetExtent(iGeomField, psExtent, bForce); } + + bool + ValidateGeometryFieldIndexForSetSpatialFilter(int iGeomField, + const OGRGeometry *poGeomIn, + bool bIsSelectLayer) override + { + return OGRSQLiteLayer::ValidateGeometryFieldIndexForSetSpatialFilter( + iGeomField, poGeomIn, bIsSelectLayer); + } }; /************************************************************************/ diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h b/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h index 5bd7dbdd905e..eb5eec07aa24 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h +++ b/ogr/ogrsf_frmts/sqlite/ogrsqlitebase.h @@ -279,6 +279,8 @@ class IOGRSQLiteSelectLayer virtual OGRErr BaseGetExtent(OGREnvelope *psExtent, int bForce) = 0; virtual OGRErr BaseGetExtent(int iGeomField, OGREnvelope *psExtent, int bForce) = 0; + virtual bool ValidateGeometryFieldIndexForSetSpatialFilter( + int iGeomField, const OGRGeometry *poGeomIn, bool bIsSelectLayer) = 0; }; /************************************************************************/ diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqliteexecutesql.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqliteexecutesql.cpp index 1fc5b52abce7..a8d90aef5770 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqliteexecutesql.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqliteexecutesql.cpp @@ -1161,7 +1161,16 @@ OGRLayer *OGRSQLiteExecuteSQL(GDALDataset *poDS, const char *pszStatement, bUseStatementForGetNextFeature, bEmptyLayer, bCanReopenBaseDS); if (poSpatialFilter != nullptr) + { + const auto nErrorCounter = CPLGetErrorCounter(); poLayer->SetSpatialFilter(0, poSpatialFilter); + if (CPLGetErrorCounter() > nErrorCounter && + CPLGetLastErrorType() != CE_None) + { + delete poLayer; + return nullptr; + } + } if (poSingleSrcLayer != nullptr) poLayer->SetMetadata(poSingleSrcLayer->GetMetadata("NATIVE_DATA"), diff --git a/ogr/ogrsf_frmts/sqlite/ogrsqliteselectlayer.cpp b/ogr/ogrsf_frmts/sqlite/ogrsqliteselectlayer.cpp index 09fbbc7afa98..2458e6beac5a 100644 --- a/ogr/ogrsf_frmts/sqlite/ogrsqliteselectlayer.cpp +++ b/ogr/ogrsf_frmts/sqlite/ogrsqliteselectlayer.cpp @@ -382,6 +382,9 @@ void OGRSQLiteSelectLayer::SetSpatialFilter(int iGeomField, { if (!m_bCanReopenBaseDS && iGeomField == 0) { + if (!ValidateGeometryFieldIndexForSetSpatialFilter(iGeomField, poGeomIn, + true)) + return; // For a Memory datasource, short-circuit // OGRSQLiteExecuteSQL::SetSpatialFilter() // that would try to re-open the Memory datasource, which would fail. @@ -397,18 +400,9 @@ void OGRSQLiteSelectLayerCommonBehaviour::SetSpatialFilter( int iGeomField, OGRGeometry *poGeomIn) { - if (iGeomField == 0 && poGeomIn == nullptr && - m_poLayer->GetLayerDefn()->GetGeomFieldCount() == 0) - { - /* do nothing */ - } - else if (iGeomField < 0 || - iGeomField >= m_poLayer->GetLayerDefn()->GetGeomFieldCount()) - { - CPLError(CE_Failure, CPLE_AppDefined, - "Invalid geometry field index : %d", iGeomField); + if (!m_poLayer->ValidateGeometryFieldIndexForSetSpatialFilter( + iGeomField, poGeomIn, true)) return; - } m_bAllowResetReadingEvenIfIndexAtZero = true;