ExecuteSQL(): error out if SetSpatialFilter() fails

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");

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")

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)

gcore/gdaldataset.cpp

@@ -6972,23 +6972,27 @@ OGRLayer *GDALDataset::BuildLayerFromSelectInfo(
     swq_select *psSelectInfo, OGRGeometry *poSpatialFilter,
     const char *pszDialect, swq_select_parse_options *poSelectParseOptions)
 {
-    OGRGenSQLResultsLayer *poResults = nullptr;
+    std::unique_ptr<OGRGenSQLResultsLayer> 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<OGRGenSQLResultsLayer>(
+            this, psSelectInfo, poSpatialFilter, psParseInfo->pszWHERE,
+            pszDialect);
+        if (CPLGetErrorCounter() > nErrorCounter &&
+            CPLGetLastErrorType() != CE_None)
+            poResults.reset();
     }
     else
     {
         delete psSelectInfo;
     }
     DestroyParseInfo(psParseInfo);
 
-    return poResults;
+    return poResults.release();
 }
 
 /************************************************************************/

ogr/ogrsf_frmts/generic/ogrlayer.cpp

@@ -1439,13 +1439,57 @@ 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()                          */
 /************************************************************************/
 
 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))

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 */

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

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);
+    }
 };
 
 /************************************************************************/

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;
 };
 
 /************************************************************************/

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"),

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;