RFC 107: make OGRLayer::SetSpatialFilter[Rect]() non-virtual and add …

…virtual ISetSpatialFilter()

MIGRATION_GUIDE.TXT

@@ -12,6 +12,13 @@ MIGRATION GUIDE FROM GDAL 3.10 to GDAL 3.11
   ``bool bForce``). Drivers must implement IGetExtent3D(int iGeomField, OGREnvelope3D*,
   bool bForce). The user-facing method checks that the iGeomField value is in range.
 
+- The OGRLayer::SetSpatialFilter() and SetSpatialFilterRect() methods are
+  no longer virtual methods that are implemented by drivers. They are now
+  user facing methods (with the change that they return OGRErr instead of void,
+  and accept a ``const OGRGeometry*``). Drivers must implement
+  ISetSpatialFilter(int iGeomField, const OGRGeometry*). The user-facing methods
+  check that the iGeomField value is in range.
+
 - GDAL drivers may now return raster bands with the new data types
   GDT_Float16 or GDT_CFloat16. Code that use the GDAL API must be
   ready to react to the new data type, possibly by doing RasterIO()

apps/ogr2ogr_lib.cpp

@@ -795,26 +795,10 @@ class OGRSplitListFieldLayer : public OGRLayer
         return poSrcLayer->GetStyleTable();
     }
 
-    virtual void SetSpatialFilter(OGRGeometry *poGeom) override
+    virtual OGRErr ISetSpatialFilter(int iGeom,
+                                     const OGRGeometry *poGeom) override
     {
-        poSrcLayer->SetSpatialFilter(poGeom);
-    }
-
-    virtual void SetSpatialFilter(int iGeom, OGRGeometry *poGeom) override
-    {
-        poSrcLayer->SetSpatialFilter(iGeom, poGeom);
-    }
-
-    virtual void SetSpatialFilterRect(double dfMinX, double dfMinY,
-                                      double dfMaxX, double dfMaxY) override
-    {
-        poSrcLayer->SetSpatialFilterRect(dfMinX, dfMinY, dfMaxX, dfMaxY);
-    }
-
-    virtual void SetSpatialFilterRect(int iGeom, double dfMinX, double dfMinY,
-                                      double dfMaxX, double dfMaxY) override
-    {
-        poSrcLayer->SetSpatialFilterRect(iGeom, dfMinX, dfMinY, dfMaxX, dfMaxY);
+        return poSrcLayer->SetSpatialFilter(iGeom, poGeom);
     }
 
     virtual OGRErr SetAttributeFilter(const char *pszFilter) override

autotest/ogr/ogr_geojson.py

@@ -648,7 +648,8 @@ def test_ogr_geojson_23(tmp_vsimem):
     lyr.CreateFeature(feat)
     assert lyr.GetExtent() == (1.0, 2.0, 10.0, 20.0)
     assert lyr.GetExtent(geom_field=0) == (1.0, 2.0, 10.0, 20.0)
-    assert lyr.GetExtent(geom_field=1, can_return_null=True) is None
+    with gdaltest.disable_exceptions():
+        assert lyr.GetExtent(geom_field=1, can_return_null=True) is None
     lyr = None
     ds = None
 

autotest/ogr/ogr_oapif.py

@@ -548,7 +548,7 @@ def NO_LONGER_USED_test_ogr_oapif_fc_links_next_headers():
 ###############################################################################
 
 
-def test_ogr_oapif_spatial_filter():
+def test_ogr_oapif_spatial_filter_deprecated_api():
 
     # Deprecated API
     handler = webserver.SequentialHandler()
@@ -568,7 +568,31 @@ def test_ogr_oapif_spatial_filter():
         ds = ogr.Open("OAPIF:http://localhost:%d/oapif" % gdaltest.webserver_port)
     lyr = ds.GetLayer(0)
     assert lyr.TestCapability(ogr.OLCFastGetExtent)
-    assert lyr.GetExtent() == (-10.0, 15.0, 40.0, 50.0)
+
+    handler = webserver.SequentialHandler()
+    _add_dummy_root_and_api_pages(handler)
+    handler.add(
+        "GET",
+        "/oapif/collections/foo/items?limit=20",
+        200,
+        {"Content-Type": "application/geo+json"},
+        """{ "type": "FeatureCollection", "features": [
+                    {
+                        "type": "Feature",
+                        "properties": {
+                            "foo": "bar"
+                        }
+                    }
+                ] }""",
+    )
+    with webserver.install_http_handler(handler):
+        assert lyr.GetExtent() == (-10.0, 15.0, 40.0, 50.0)
+
+
+###############################################################################
+
+
+def test_ogr_oapif_spatial_filter():
 
     # Nominal API
     handler = webserver.SequentialHandler()
@@ -591,7 +615,6 @@ def test_ogr_oapif_spatial_filter():
     with webserver.install_http_handler(handler):
         ds = ogr.Open("OAPIF:http://localhost:%d/oapif" % gdaltest.webserver_port)
     lyr = ds.GetLayer(0)
-    assert lyr.GetExtent() == (-10.0, 15.0, 40.0, 50.0)
 
     handler = webserver.SequentialHandler()
     _add_dummy_root_and_api_pages(handler)
@@ -610,6 +633,7 @@ def test_ogr_oapif_spatial_filter():
                 ] }""",
     )
     with webserver.install_http_handler(handler):
+        assert lyr.GetExtent() == (-10.0, 15.0, 40.0, 50.0)
         assert lyr.GetLayerDefn().GetFieldCount() == 1
 
     lyr.SetSpatialFilterRect(2, 49, 3, 50)
@@ -1365,11 +1389,6 @@ def test_ogr_oapif_storage_crs_easting_northing():
     with webserver.install_http_handler(handler):
         ds = ogr.Open("OAPIF:http://localhost:%d/oapif" % gdaltest.webserver_port)
     lyr = ds.GetLayer(0)
-    minx, maxx, miny, maxy = lyr.GetExtent()
-    assert (minx, miny, maxx, maxy) == pytest.approx(
-        (-611288.854779237, 4427761.561734099, 1525592.2813932528, 5620112.89047953),
-        abs=1e-3,
-    )
 
     handler = webserver.SequentialHandler()
     _add_dummy_root_and_api_pages(handler)
@@ -1389,10 +1408,16 @@ def test_ogr_oapif_storage_crs_easting_northing():
                 ] }""",
     )
     with webserver.install_http_handler(handler):
-        srs = lyr.GetSpatialRef()
-        assert srs
-        assert srs.GetAuthorityCode(None) == "32631"
-        assert lyr.GetLayerDefn().GetFieldCount() == 1
+        minx, maxx, miny, maxy = lyr.GetExtent()
+    assert (minx, miny, maxx, maxy) == pytest.approx(
+        (-611288.854779237, 4427761.561734099, 1525592.2813932528, 5620112.89047953),
+        abs=1e-3,
+    )
+
+    srs = lyr.GetSpatialRef()
+    assert srs
+    assert srs.GetAuthorityCode(None) == "32631"
+    assert lyr.GetLayerDefn().GetFieldCount() == 1
 
     handler = webserver.SequentialHandler()
     handler.add(
@@ -1467,8 +1492,6 @@ def test_ogr_oapif_storage_crs_latitude_longitude():
     with webserver.install_http_handler(handler):
         ds = ogr.Open("OAPIF:http://localhost:%d/oapif" % gdaltest.webserver_port)
     lyr = ds.GetLayer(0)
-    minx, maxx, miny, maxy = lyr.GetExtent()
-    assert (minx, miny, maxx, maxy) == pytest.approx((-10, 40, 15, 50), abs=1e-3)
 
     handler = webserver.SequentialHandler()
     _add_dummy_root_and_api_pages(handler)
@@ -1488,12 +1511,15 @@ def test_ogr_oapif_storage_crs_latitude_longitude():
                 ] }""",
     )
     with webserver.install_http_handler(handler):
-        srs = lyr.GetSpatialRef()
-        assert srs
-        assert srs.GetAuthorityCode(None) == "4326"
-        assert srs.GetDataAxisToSRSAxisMapping() == [2, 1]
-        assert srs.GetCoordinateEpoch() == 2022.5
-        assert lyr.GetLayerDefn().GetFieldCount() == 1
+        minx, maxx, miny, maxy = lyr.GetExtent()
+    assert (minx, miny, maxx, maxy) == pytest.approx((-10, 40, 15, 50), abs=1e-3)
+
+    srs = lyr.GetSpatialRef()
+    assert srs
+    assert srs.GetAuthorityCode(None) == "4326"
+    assert srs.GetDataAxisToSRSAxisMapping() == [2, 1]
+    assert srs.GetCoordinateEpoch() == 2022.5
+    assert lyr.GetLayerDefn().GetFieldCount() == 1
 
     handler = webserver.SequentialHandler()
     # Coordinates must be in lat, lon order in the GeoJSON answer
@@ -1573,11 +1599,6 @@ def test_ogr_oapif_storage_crs_latitude_longitude_non_compliant_server():
             open_options=["SERVER_FEATURE_AXIS_ORDER=GIS_FRIENDLY"],
         )
     lyr = ds.GetLayer(0)
-    minx, maxx, miny, maxy = lyr.GetExtent()
-    assert (minx, miny, maxx, maxy) == pytest.approx((-10, 40, 15, 50), abs=1e-3)
-
-    supported_srs_list = lyr.GetSupportedSRSList()
-    assert supported_srs_list is None
 
     handler = webserver.SequentialHandler()
     _add_dummy_root_and_api_pages(handler)
@@ -1597,12 +1618,18 @@ def test_ogr_oapif_storage_crs_latitude_longitude_non_compliant_server():
                 ] }""",
     )
     with webserver.install_http_handler(handler):
-        srs = lyr.GetSpatialRef()
-        assert srs
-        assert srs.GetAuthorityCode(None) == "4326"
-        assert srs.GetDataAxisToSRSAxisMapping() == [2, 1]
-        assert srs.GetCoordinateEpoch() == 2022.5
-        assert lyr.GetLayerDefn().GetFieldCount() == 1
+        minx, maxx, miny, maxy = lyr.GetExtent()
+    assert (minx, miny, maxx, maxy) == pytest.approx((-10, 40, 15, 50), abs=1e-3)
+
+    supported_srs_list = lyr.GetSupportedSRSList()
+    assert supported_srs_list is None
+
+    srs = lyr.GetSpatialRef()
+    assert srs
+    assert srs.GetAuthorityCode(None) == "4326"
+    assert srs.GetDataAxisToSRSAxisMapping() == [2, 1]
+    assert srs.GetCoordinateEpoch() == 2022.5
+    assert lyr.GetLayerDefn().GetFieldCount() == 1
 
     handler = webserver.SequentialHandler()
     # Coordinates must be in lat, lon order in the GeoJSON answer
@@ -1665,19 +1692,6 @@ def get_collections_handler():
         assert ds
         lyr = ds.GetLayer(0)
 
-    minx, maxx, miny, maxy = lyr.GetExtent()
-    assert (minx, miny, maxx, maxy) == pytest.approx(
-        (-611288.854779237, 4427761.561734099, 1525592.2813932528, 5620112.89047953),
-        abs=1e-3,
-    )
-
-    supported_srs_list = lyr.GetSupportedSRSList()
-    assert supported_srs_list
-    assert len(supported_srs_list) == 2
-    assert supported_srs_list[0].GetAuthorityCode(None) == "32631"
-    # Below doesn't work with early PROJ 6 versions
-    # assert supported_srs_list[1].GetAuthorityCode(None) == "CRS84"
-
     def get_items_handler():
         handler = webserver.SequentialHandler()
         _add_dummy_root_and_api_pages(handler)
@@ -1699,9 +1713,22 @@ def get_items_handler():
         return handler
 
     with webserver.install_http_handler(get_items_handler()):
-        srs = lyr.GetSpatialRef()
-        assert srs
-        assert srs.GetAuthorityCode(None) == "32631"
+        minx, maxx, miny, maxy = lyr.GetExtent()
+    assert (minx, miny, maxx, maxy) == pytest.approx(
+        (-611288.854779237, 4427761.561734099, 1525592.2813932528, 5620112.89047953),
+        abs=1e-3,
+    )
+
+    supported_srs_list = lyr.GetSupportedSRSList()
+    assert supported_srs_list
+    assert len(supported_srs_list) == 2
+    assert supported_srs_list[0].GetAuthorityCode(None) == "32631"
+    # Below doesn't work with early PROJ 6 versions
+    # assert supported_srs_list[1].GetAuthorityCode(None) == "CRS84"
+
+    srs = lyr.GetSpatialRef()
+    assert srs
+    assert srs.GetAuthorityCode(None) == "32631"
 
     json_info = gdal.VectorInfo(ds, format="json", featureCount=False)
     assert "supportedSRSList" in json_info["layers"][0]["geometryFields"][0]

autotest/ogr/ogr_sql_sqlite.py

@@ -2473,7 +2473,7 @@ def test_ogr_sql_sqlite_execute_sql_error_on_spatial_filter_mem_layer():
     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"
+        Exception, match="Cannot set spatial filter: no geometry field present in layer"
     ):
         ds.ExecuteSQL("SELECT 1 FROM test", spatialFilter=geom, dialect="SQLITE")
 
@@ -2490,6 +2490,6 @@ def test_ogr_sql_sqlite_execute_sql_error_on_spatial_filter_shp_layer(tmp_vsimem
     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"
+        Exception, match="Cannot set spatial filter: no geometry field present in layer"
     ):
         ds.ExecuteSQL("SELECT 1 FROM test", spatialFilter=geom, dialect="SQLITE")

autotest/ogr/ogr_vrt.py

@@ -2381,9 +2381,9 @@ def test_ogr_vrt_33b(ogr_vrt_33, tmp_path):
         ret = gdaltest.runexternal(
             test_cli_utilities.get_test_ogrsf_path() + f" -ro {tmp_path}/ogr_vrt_33.vrt"
         )
-        os.unlink(tmp_path / "ogr_vrt_33.vrt")
 
-        assert ret.find("INFO") != -1 and ret.find("ERROR") == -1
+        assert "INFO" in ret
+        assert "ERROR" not in ret
 
 
 @pytest.mark.require_driver("CSV")

frmts/eeda/eedadataset.cpp

@@ -140,13 +140,8 @@ class GDALEEDALayer final : public OGRLayer
         return -1;
     }
 
-    virtual void SetSpatialFilter(OGRGeometry *poGeom) CPL_OVERRIDE;
-
-    virtual void SetSpatialFilter(int iGeomField,
-                                  OGRGeometry *poGeom) CPL_OVERRIDE
-    {
-        OGRLayer::SetSpatialFilter(iGeomField, poGeom);
-    }
+    virtual OGRErr ISetSpatialFilter(int iGeomField,
+                                     const OGRGeometry *poGeom) override;
 
     virtual OGRErr SetAttributeFilter(const char *) CPL_OVERRIDE;
 
@@ -938,10 +933,11 @@ OGRErr GDALEEDALayer::SetAttributeFilter(const char *pszQuery)
 }
 
 /************************************************************************/
-/*                          SetSpatialFilter()                          */
+/*                          ISetSpatialFilter()                         */
 /************************************************************************/
 
-void GDALEEDALayer::SetSpatialFilter(OGRGeometry *poGeomIn)
+OGRErr GDALEEDALayer::ISetSpatialFilter(int /* iGeomField */,
+                                        const OGRGeometry *poGeomIn)
 {
     if (poGeomIn)
     {
@@ -960,6 +956,7 @@ void GDALEEDALayer::SetSpatialFilter(OGRGeometry *poGeomIn)
         InstallFilter(poGeomIn);
 
     ResetReading();
+    return OGRERR_NONE;
 }
 
 /************************************************************************/