Merge pull request #134 from dead10ck/null-fields

Feature::field returns Result<Option<FieldValue>>

CHANGES.md

@@ -1,5 +1,37 @@
 # Changes
+
 ## Unreleased
+* **Breaking**: Change `Feature::field` return type from
+  `Result<FieldValue>` to `Result<Option<FieldValue>>`. Fields
+  can be null. Before this change, if a field was null, the value
+  returned was the default value for the underlying type.
+  However, this made it impossible to distinguish between null
+  fields and legitimate values which happen to be default value,
+  for example, an Integer field that is absent (null) from a 0,
+  which can be a valid value. After this change, if a field is
+  null, `None` is returned, rather than the default value.
+
+  If you happened to rely on this behavior, you can fix your code
+  by explicitly choosing a default value when the field is null.
+  For example, if you had this before:
+
+  ```rust
+  let str_var = feature.field("string_field")?
+      .into_string()
+      .unwrap();
+  ```
+
+  You could maintain the old behavior with:
+
+  ```rust
+  use gdal::vector::FieldValue;
+
+  let str_var = feature.field("string_field")?
+      .unwrap_or(FieldValue::StringValue("".into()))
+      .into_string()
+      .unwrap();
+  ```
+    * <https://github.com/georust/gdal/pull/134>
 * Add basic support to read overviews
 * BREAKING: update geo-types to 0.7.0. geo-types Coordinate<T> now implement `Debug`
   * <https://github.com/georust/gdal/pull/146>

examples/read_write_ogr.rs

@@ -15,9 +15,10 @@ fn run() -> Result<()> {
         .collect::<Vec<_>>();
 
     // Create a new dataset:
-    let _ = fs::remove_file("/tmp/abcde.shp");
+    let path = std::env::temp_dir().join("abcde.shp");
+    let _ = fs::remove_file(&path);
     let drv = Driver::get("ESRI Shapefile")?;
-    let mut ds = drv.create_vector_only("/tmp/abcde.shp")?;
+    let mut ds = drv.create_vector_only(path.to_str().unwrap())?;
     let lyr = ds.create_layer_blank()?;
 
     // Copy the origin layer shema to the destination layer:
@@ -47,7 +48,9 @@ fn run() -> Result<()> {
         ft.set_geometry(new_geom)?;
         // copy each field value of the feature:
         for fd in &fields_defn {
-            ft.set_field(&fd.0, &feature_a.field(&fd.0)?)?;
+            if let Some(value) = feature_a.field(&fd.0)? {
+                ft.set_field(&fd.0, &value)?;
+            }
         }
         // Add the feature to the layer:
         ft.create(&lyr)?;

examples/read_write_ogr_datetime.rs

@@ -14,9 +14,10 @@ fn run() -> Result<()> {
     let layer_a = dataset_a.layer(0)?;
 
     // Create a new dataset:
-    let _ = std::fs::remove_file("/tmp/later.geojson");
+    let path = std::env::temp_dir().join("later.geojson");
+    let _ = std::fs::remove_file(&path);
     let drv = Driver::get("GeoJSON")?;
-    let mut ds = drv.create_vector_only("/tmp/later.geojson")?;
+    let mut ds = drv.create_vector_only(path.to_str().unwrap())?;
     let lyr = ds.create_layer_blank()?;
 
     // Copy the origin layer shema to the destination layer:
@@ -36,7 +37,7 @@ fn run() -> Result<()> {
         for field in defn.fields() {
             ft.set_field(
                 &field.name(),
-                &match feature_a.field(&field.name())? {
+                &match feature_a.field(&field.name())?.unwrap() {
                     // add one day to dates
                     FieldValue::DateValue(value) => {
                         println!("{} = {}", field.name(), value);

examples/write_ogr.rs

@@ -5,9 +5,10 @@ use std::fs;
 
 /// Example 1, the detailed way:
 fn example_1() -> Result<()> {
-    let _ = fs::remove_file("/tmp/output1.geojson");
+    let path = std::env::temp_dir().join("output1.geojson");
+    let _ = fs::remove_file(&path);
     let drv = Driver::get("GeoJSON")?;
-    let mut ds = drv.create_vector_only("/tmp/output1.geojson")?;
+    let mut ds = drv.create_vector_only(path.to_str().unwrap())?;
 
     let lyr = ds.create_layer_blank()?;
 
@@ -49,9 +50,10 @@ fn example_1() -> Result<()> {
 
 /// Example 2, same output, shortened way:
 fn example_2() -> Result<()> {
-    let _ = fs::remove_file("/tmp/output2.geojson");
+    let path = std::env::temp_dir().join("output2.geojson");
+    let _ = fs::remove_file(&path);
     let driver = Driver::get("GeoJSON")?;
-    let mut ds = driver.create_vector_only("/tmp/output2.geojson")?;
+    let mut ds = driver.create_vector_only(path.to_str().unwrap())?;
     let mut layer = ds.create_layer_blank()?;
 
     layer.create_defn_fields(&[

fixtures/null_feature_fields.geojson

@@ -0,0 +1,16 @@
+{
+  "features": [
+    {
+      "geometry": {
+        "coordinates": [ -122.4335161,  47.3136322 ],
+        "type": "Point"
+      },
+      "properties": {
+        "some_int": 0,
+        "some_string": null
+      },
+      "type": "Feature"
+    }
+  ],
+  "type": "FeatureCollection"
+}

src/lib.rs

@@ -11,7 +11,7 @@
 //! let mut dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap();
 //! let layer = dataset.layer(0).unwrap();
 //! for feature in layer.features() {
-//!     let highway_field = feature.field("highway").unwrap();
+//!     let highway_field = feature.field("highway").unwrap().unwrap();
 //!     let geometry = feature.geometry();
 //!     println!("{} {}", highway_field.into_string().unwrap(), geometry.wkt().unwrap());
 //! }

src/vector/feature.rs

@@ -64,10 +64,15 @@ impl<'a> Feature<'a> {
         }
     }
 
-    /// Get the value of a named field. If the field exists, it returns a
-    /// `FieldValue` wrapper, that you need to unpack to a base type
-    /// (string, float, etc). If the field is missing, returns `None`.
-    pub fn field(&self, name: &str) -> Result<FieldValue> {
+    /// Get the value of a named field. If the field exists, it returns a [`FieldValue`] wrapper,
+    /// that you need to unpack to a base type (string, float, etc).
+    ///
+    /// If the field is missing, returns [`GdalError::InvalidFieldName`].
+    ///
+    /// If the field has an unsupported type, returns a [`GdalError::UnhandledFieldType`].
+    ///
+    /// If the field is null, returns `None`.
+    pub fn field(&self, name: &str) -> Result<Option<FieldValue>> {
         let c_name = CString::new(name)?;
         let field_id = unsafe { gdal_sys::OGR_F_GetFieldIndex(self.c_feature, c_name.as_ptr()) };
         if field_id == -1 {
@@ -80,24 +85,34 @@ impl<'a> Feature<'a> {
         }
     }
 
-    fn field_from_id(&self, field_id: i32) -> Result<FieldValue> {
+    /// Get the value of a named field. If the field exists, it returns a [`FieldValue`] wrapper,
+    /// that you need to unpack to a base type (string, float, etc).
+    ///
+    /// If the field has an unhandled type, returns a [`GdalError::UnhandledFieldType`].
+    ///
+    /// If the field is null, returns `None`.
+    fn field_from_id(&self, field_id: i32) -> Result<Option<FieldValue>> {
+        if unsafe { gdal_sys::OGR_F_IsFieldNull(self.c_feature, field_id) } != 0 {
+            return Ok(None);
+        }
+
         let field_defn = unsafe { gdal_sys::OGR_F_GetFieldDefnRef(self.c_feature, field_id) };
         let field_type = unsafe { gdal_sys::OGR_Fld_GetType(field_defn) };
         match field_type {
             OGRFieldType::OFTString => {
                 let rv = unsafe { gdal_sys::OGR_F_GetFieldAsString(self.c_feature, field_id) };
-                Ok(FieldValue::StringValue(_string(rv)))
+                Ok(Some(FieldValue::StringValue(_string(rv))))
             }
             OGRFieldType::OFTStringList => {
                 let rv = unsafe {
                     let ptr = gdal_sys::OGR_F_GetFieldAsStringList(self.c_feature, field_id);
                     _string_array(ptr)
                 };
-                Ok(FieldValue::StringListValue(rv))
+                Ok(Some(FieldValue::StringListValue(rv)))
             }
             OGRFieldType::OFTReal => {
                 let rv = unsafe { gdal_sys::OGR_F_GetFieldAsDouble(self.c_feature, field_id) };
-                Ok(FieldValue::RealValue(rv as f64))
+                Ok(Some(FieldValue::RealValue(rv as f64)))
             }
             OGRFieldType::OFTRealList => {
                 let rv = unsafe {
@@ -106,11 +121,11 @@ impl<'a> Feature<'a> {
                         gdal_sys::OGR_F_GetFieldAsDoubleList(self.c_feature, field_id, &mut len);
                     slice::from_raw_parts(ptr, len as usize).to_vec()
                 };
-                Ok(FieldValue::RealListValue(rv))
+                Ok(Some(FieldValue::RealListValue(rv)))
             }
             OGRFieldType::OFTInteger => {
                 let rv = unsafe { gdal_sys::OGR_F_GetFieldAsInteger(self.c_feature, field_id) };
-                Ok(FieldValue::IntegerValue(rv as i32))
+                Ok(Some(FieldValue::IntegerValue(rv as i32)))
             }
             OGRFieldType::OFTIntegerList => {
                 let rv = unsafe {
@@ -119,11 +134,11 @@ impl<'a> Feature<'a> {
                         gdal_sys::OGR_F_GetFieldAsIntegerList(self.c_feature, field_id, &mut len);
                     slice::from_raw_parts(ptr, len as usize).to_vec()
                 };
-                Ok(FieldValue::IntegerListValue(rv))
+                Ok(Some(FieldValue::IntegerListValue(rv)))
             }
             OGRFieldType::OFTInteger64 => {
                 let rv = unsafe { gdal_sys::OGR_F_GetFieldAsInteger64(self.c_feature, field_id) };
-                Ok(FieldValue::Integer64Value(rv))
+                Ok(Some(FieldValue::Integer64Value(rv)))
             }
             OGRFieldType::OFTInteger64List => {
                 let rv = unsafe {
@@ -132,16 +147,16 @@ impl<'a> Feature<'a> {
                         gdal_sys::OGR_F_GetFieldAsInteger64List(self.c_feature, field_id, &mut len);
                     slice::from_raw_parts(ptr, len as usize).to_vec()
                 };
-                Ok(FieldValue::Integer64ListValue(rv))
+                Ok(Some(FieldValue::Integer64ListValue(rv)))
             }
             #[cfg(feature = "datetime")]
-            OGRFieldType::OFTDateTime => Ok(FieldValue::DateTimeValue(
+            OGRFieldType::OFTDateTime => Ok(Some(FieldValue::DateTimeValue(
                 self.get_field_datetime(field_id)?,
-            )),
+            ))),
             #[cfg(feature = "datetime")]
-            OGRFieldType::OFTDate => Ok(FieldValue::DateValue(
+            OGRFieldType::OFTDate => Ok(Some(FieldValue::DateValue(
                 self.get_field_datetime(field_id)?.date(),
-            )),
+            ))),
             _ => Err(GdalError::UnhandledFieldType {
                 field_type,
                 method_name: "OGR_Fld_GetType",
@@ -397,27 +412,25 @@ impl<'a> FieldValueIterator<'a> {
 }
 
 impl<'a> Iterator for FieldValueIterator<'a> {
-    type Item = (String, FieldValue);
+    type Item = (String, Option<FieldValue>);
 
     #[inline]
-    fn next(&mut self) -> Option<(String, FieldValue)> {
+    fn next(&mut self) -> Option<(String, Option<FieldValue>)> {
         let idx = self.idx;
         if idx < self.count {
             self.idx += 1;
             let field_defn =
                 unsafe { gdal_sys::OGR_F_GetFieldDefnRef(self.feature.c_feature, idx) };
             let field_name = unsafe { gdal_sys::OGR_Fld_GetNameRef(field_defn) };
             let name = _string(field_name);
-            let fv: Option<(String, FieldValue)> = self
+            let fv: Option<(String, Option<FieldValue>)> = self
                 .feature
                 .field_from_id(idx)
                 .ok()
                 .map(|field_value| (name, field_value));
+            //skip unknown types
             if fv.is_none() {
-                //skip unknown types
-                if self.idx < self.count {
-                    return self.next();
-                }
+                return self.next();
             }
             fv
         } else {

src/vector/mod.rs

@@ -9,7 +9,7 @@
 //! let mut dataset = Dataset::open(Path::new("fixtures/roads.geojson")).unwrap();
 //! let layer = dataset.layer(0).unwrap();
 //! for feature in layer.features() {
-//!     let highway_field = feature.field("highway").unwrap();
+//!     let highway_field = feature.field("highway").unwrap().unwrap();
 //!     let geometry = feature.geometry();
 //!     println!("{} {}", highway_field.into_string().unwrap(), geometry.wkt().unwrap());
 //! }

src/vector/vector_tests/mod.rs

@@ -158,15 +158,15 @@ mod tests {
     fn test_string_field() {
         with_feature("roads.geojson", 236194095, |feature| {
             assert_eq!(
-                feature.field("highway").unwrap().into_string(),
+                feature.field("highway").unwrap().unwrap().into_string(),
                 Some("footway".to_string())
             );
         });
         with_features("roads.geojson", |features| {
             assert_eq!(
                 features
                     .filter(|field| {
-                        let highway = field.field("highway").unwrap().into_string();
+                        let highway = field.field("highway").unwrap().unwrap().into_string();
                         highway == Some("residential".to_string())
                     })
                     .count(),
@@ -175,12 +175,24 @@ mod tests {
         });
     }
 
+    #[test]
+    fn test_null_field() {
+        with_features("null_feature_fields.geojson", |mut features| {
+            let feature = features.next().unwrap();
+            assert_eq!(
+                feature.field("some_int").unwrap(),
+                Some(FieldValue::IntegerValue(0))
+            );
+            assert_eq!(feature.field("some_string").unwrap(), None);
+        });
+    }
+
     #[test]
     fn test_string_list_field() {
         with_features("soundg.json", |mut features| {
             let feature = features.next().unwrap();
             assert_eq!(
-                feature.field("a_string_list").unwrap(),
+                feature.field("a_string_list").unwrap().unwrap(),
                 FieldValue::StringListValue(vec![
                     String::from("a"),
                     String::from("list"),
@@ -195,7 +207,7 @@ mod tests {
     fn test_field_in_layer() {
         ds_with_layer("three_layer_ds.s3db", "layer_0", |layer| {
             let feature = layer.features().next().unwrap();
-            assert_eq!(feature.field("id").unwrap(), FieldValue::IntegerValue(0));
+            assert_eq!(feature.field("id").unwrap(), None);
         });
     }
 
@@ -204,7 +216,7 @@ mod tests {
         with_features("soundg.json", |mut features| {
             let feature = features.next().unwrap();
             assert_eq!(
-                feature.field("an_int_list").unwrap(),
+                feature.field("an_int_list").unwrap().unwrap(),
                 FieldValue::IntegerListValue(vec![1, 2])
             );
         });
@@ -215,7 +227,7 @@ mod tests {
         with_features("soundg.json", |mut features| {
             let feature = features.next().unwrap();
             assert_eq!(
-                feature.field("a_real_list").unwrap(),
+                feature.field("a_real_list").unwrap().unwrap(),
                 FieldValue::RealListValue(vec![0.1, 0.2])
             );
         });
@@ -226,7 +238,7 @@ mod tests {
         with_features("soundg.json", |mut features| {
             let feature = features.next().unwrap();
             assert_eq!(
-                feature.field("a_long_list").unwrap(),
+                feature.field("a_long_list").unwrap().unwrap(),
                 FieldValue::Integer64ListValue(vec![5000000000, 6000000000])
             );
         });
@@ -236,7 +248,12 @@ mod tests {
     fn test_float_field() {
         with_feature("roads.geojson", 236194095, |feature| {
             assert_almost_eq(
-                feature.field("sort_key").unwrap().into_real().unwrap(),
+                feature
+                    .field("sort_key")
+                    .unwrap()
+                    .unwrap()
+                    .into_real()
+                    .unwrap(),
                 -9.0,
             );
         });
@@ -468,10 +485,10 @@ mod tests {
         let ft = layer.features().next().unwrap();
         assert_eq!(ft.geometry().wkt().unwrap(), "POINT (1 2)");
         assert_eq!(
-            ft.field("Name").unwrap().into_string(),
+            ft.field("Name").unwrap().unwrap().into_string(),
             Some("Feature 1".to_string())
         );
-        assert_eq!(ft.field("Value").unwrap().into_real(), Some(45.78));
-        assert_eq!(ft.field("Int_value").unwrap().into_int(), Some(1));
+        assert_eq!(ft.field("Value").unwrap().unwrap().into_real(), Some(45.78));
+        assert_eq!(ft.field("Int_value").unwrap().unwrap().into_int(), Some(1));
     }
 }