Merge #238

238: Owned layers from a dataset that are `Send` r=rmanoka a=ChristianBeilschmidt

- [X] I agree to follow the project's [code of conduct](https://github.com/georust/gdal/blob/master/CODE_OF_CONDUCT.md).
- [ ] I added an entry to `CHANGES.md` if knowledge of this change could be valuable to users.
---

We ran into the problem of having non-`Send` `Layer`s. Without `Send` it is barely usable in an asynchronous or threading context. You cannot store it somewhere and create tasks that do something with them since you would need to send a `Layer` to another thread.

From previous discussions about thread-safety in GDAL we found out that we must not access `Dataset`s or `Layer`s at the same time from different threads. But for `Dataset`s, we found that it is safe to `Send` them to another thread since GDAL 2.4 (or something). For `Layer`s, it is actually okay to assume that the are `Send` as long as we have a one-to-one relationship between `Dataset`s or `Layer`s, or, stated differently, we must ensure that there are no two `Layer`s that point to one `Dataset`. If we would have to `Layer`s pointing to the same `Dataset` and they are `Send`, then we could have two different threads accessing the same `Dataset`. We must not allow this.

However, from discussions with `@jdroenner` , we thought that if we would introduce a `Layer` that owns its `Dataset`, we could make it `Send` since we can ensure that the `Dataset` isn't accessed another time, for instance, from another thread.

So in this draft PR I've created an `OwnedLayer` that does exactly this, it encapsulates a `Layer` and its `Dataset`. In `Dataset`, we can then choose if we want to have a reference to `Layer` or we want to convert our `Dataset` into an `OwnedLayer`. I've added a test that this behavior works.

Since the operations on `Layer` and `OwnedLayer` are more or less the same, I've transferred them into a `LayerAccess` trait. This unfortunately would mean a breaking change. On the other hand, it means not much more code for preserving both types.

Having said all this, what are your opinions on `OwnedLayer`s?
Do you like or criticize the concept?
What are your thoughts on naming the two types?


Co-authored-by: Christian Beilschmidt <[email protected]>

examples/read_write_ogr_datetime.rs

@@ -1,3 +1,5 @@
+use gdal::vector::LayerAccess;
+
 fn run() -> gdal::errors::Result<()> {
     use chrono::Duration;
     use gdal::vector::{Defn, Feature, FieldDefn, FieldValue};

examples/write_ogr.rs

@@ -1,5 +1,5 @@
 use gdal::errors::Result;
-use gdal::vector::{Defn, Feature, FieldDefn, FieldValue, Geometry, OGRFieldType};
+use gdal::vector::{Defn, Feature, FieldDefn, FieldValue, Geometry, LayerAccess, OGRFieldType};
 use gdal::Driver;
 use std::fs;
 

src/dataset.rs

@@ -12,8 +12,7 @@ use crate::cpl::CslStringList;
 use crate::errors::*;
 use crate::raster::RasterCreationOption;
 use crate::utils::{_last_cpl_err, _last_null_pointer_err, _path_to_c_string, _string};
-use crate::vector::sql;
-use crate::vector::Geometry;
+use crate::vector::{sql, Geometry, OwnedLayer};
 use crate::{
     gdal_major_object::MajorObject, raster::RasterBand, spatial_ref::SpatialRef, vector::Layer,
     Driver, Metadata,
@@ -453,6 +452,10 @@ impl Dataset {
         unsafe { Layer::from_c_layer(self, c_layer) }
     }
 
+    fn into_child_layer(self, c_layer: OGRLayerH) -> OwnedLayer {
+        unsafe { OwnedLayer::from_c_layer(self, c_layer) }
+    }
+
     /// Get the number of layers in this dataset.
     pub fn layer_count(&self) -> isize {
         (unsafe { gdal_sys::OGR_DS_GetLayerCount(self.c_dataset) }) as isize
@@ -470,6 +473,18 @@ impl Dataset {
         Ok(self.child_layer(c_layer))
     }
 
+    /// Fetch a layer by index.
+    ///
+    /// Applies to vector datasets, and fetches by the given
+    /// _0-based_ index.
+    pub fn into_layer(self, idx: isize) -> Result<OwnedLayer> {
+        let c_layer = unsafe { gdal_sys::OGR_DS_GetLayer(self.c_dataset, idx as c_int) };
+        if c_layer.is_null() {
+            return Err(_last_null_pointer_err("OGR_DS_GetLayer"));
+        }
+        Ok(self.into_child_layer(c_layer))
+    }
+
     /// Fetch a layer by name.
     pub fn layer_by_name(&self, name: &str) -> Result<Layer> {
         let c_name = CString::new(name)?;
@@ -480,6 +495,16 @@ impl Dataset {
         Ok(self.child_layer(c_layer))
     }
 
+    /// Fetch a layer by name.
+    pub fn into_layer_by_name(self, name: &str) -> Result<OwnedLayer> {
+        let c_name = CString::new(name)?;
+        let c_layer = unsafe { gdal_sys::OGR_DS_GetLayerByName(self.c_dataset(), c_name.as_ptr()) };
+        if c_layer.is_null() {
+            return Err(_last_null_pointer_err("OGR_DS_GetLayerByName"));
+        }
+        Ok(self.into_child_layer(c_layer))
+    }
+
     /// Returns an iterator over the layers of the dataset.
     pub fn layers(&self) -> LayerIterator {
         LayerIterator::with_dataset(self)
@@ -648,6 +673,7 @@ impl Dataset {
     ///
     /// ```
     /// # use gdal::{Dataset, LayerOptions};
+    /// # use gdal::vector::LayerAccess;
     /// #
     /// fn create_point_grid(dataset: &mut Dataset) -> gdal::errors::Result<()> {
     ///     use gdal::vector::Geometry;
@@ -713,6 +739,7 @@ impl Dataset {
     /// # use gdal::Dataset;
     /// # use std::path::Path;
     /// use gdal::vector::sql;
+    /// use gdal::vector::LayerAccess;
     ///
     /// let ds = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap();
     /// let query = "SELECT kind, is_bridge, highway FROM roads WHERE highway = 'pedestrian'";
@@ -933,7 +960,7 @@ impl<'a> Drop for Transaction<'a> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::vector::Geometry;
+    use crate::vector::{Geometry, LayerAccess};
     use tempfile::TempPath;
 
     macro_rules! fixture {

src/lib.rs

@@ -7,6 +7,7 @@
 //! ```
 //! use std::path::Path;
 //! use gdal::Dataset;
+//! use gdal::vector::LayerAccess;
 //!
 //! let dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap();
 //! let mut layer = dataset.layer(0).unwrap();

src/vector/defn.rs

@@ -1,6 +1,6 @@
 use crate::spatial_ref::SpatialRef;
 use crate::utils::{_last_null_pointer_err, _string};
-use crate::vector::layer::Layer;
+use crate::vector::LayerAccess;
 use gdal_sys::{
     self, OGRFeatureDefnH, OGRFieldDefnH, OGRFieldType, OGRGeomFieldDefnH, OGRwkbGeometryType,
 };
@@ -55,7 +55,7 @@ impl Defn {
         }
     }
 
-    pub fn from_layer(lyr: &Layer) -> Defn {
+    pub fn from_layer<L: LayerAccess>(lyr: &L) -> Defn {
         let c_defn = unsafe { gdal_sys::OGR_L_GetLayerDefn(lyr.c_layer()) };
         Defn { c_defn }
     }

src/vector/feature.rs

@@ -1,7 +1,6 @@
 use crate::utils::{_last_null_pointer_err, _string, _string_array};
 use crate::vector::geometry::Geometry;
-use crate::vector::layer::Layer;
-use crate::vector::Defn;
+use crate::vector::{Defn, LayerAccess};
 use gdal_sys::{self, OGRErr, OGRFeatureH, OGRFieldType};
 use libc::c_longlong;
 use libc::{c_double, c_int};
@@ -487,7 +486,7 @@ impl<'a> Feature<'a> {
         Ok(&self.geometry[idx])
     }
 
-    pub fn create(&self, lyr: &Layer) -> Result<()> {
+    pub fn create<L: LayerAccess>(&self, lyr: &L) -> Result<()> {
         let rv = unsafe { gdal_sys::OGR_L_CreateFeature(lyr.c_layer(), self.c_feature) };
         if rv != OGRErr::OGRERR_NONE {
             return Err(GdalError::OgrError {