Extend the existing possibilities of writing ogr datasets. #31

mthh · 2017-01-12T20:04:09Z

This PR (related to #30) aims to extend the existing possibilities of writing ogr datasets.

It allows to set a definition schema (name, type, precision and width for each field) on a layer.
It still allows to create a feature directly from geometry but now also with the various values to set on that feature.
It adds three functions on the Field struct, allowing to retrieve the width, type and precision when reading a layer.
It adds an IntergerValue to the FieldValue enum (allowing to get/set integer values too).
It implements the Clone trait on the Geometry struct

I honestly don't know if I did it the right way or if there is too much redundancy in the way i implemented that (there is two commented examples (read_write_ogr.rs and write_ogr.rs) to facilitate your potential reviews).
Let me know if you have advices/requirements on how it should be designed (I should have asked for some guidance before doing it..!) and/or if it combines badly with the existing functionalities allowing to create/write ogr features.

For example in src/vector/test/mod.rs a dataset is currently written like this :

{
	let driver = Driver::get("GeoJSON").unwrap();
	let mut ds = driver.create(fixture!("output.geojson")).unwrap();
	let mut layer = ds.create_layer().unwrap();
	layer.create_feature(Geometry::from_wkt("POINT (1 2)").unwrap()).unwrap();
	// dataset is closed here
}

Now it could be possible to write :

{
	let driver = Driver::get("GeoJSON").unwrap();
	let mut ds = driver.create(fixture!("output.geojson")).unwrap();
	let mut layer = ds.create_layer().unwrap();
	layer.create_defn_fields(&[("Name", OFT_STRING), ("Value", OFT_REAL)]);
	layer.create_feature_fields(
		Geometry::from_wkt("POINT (1 2)").unwrap(),
		&["Name", "Value"],
		&[FieldValue::StringValue("Feature 1".to_string()), FieldValue::RealValue(45.78)]
		);
	// dataset is closed here
}

…ng dataset

jdroenner · 2017-01-13T06:52:13Z

gdal-sys/src/ogr.rs

    pub fn OGRFree(ptr: *mut c_void);
    pub fn VSIFree(ptr: *mut c_void);
 }

+pub const OFT_INTEGER:            c_int = 0;


I think it would be nice to change the OFT types into an enum and move it into ogr_enums.rs. This way all the FFI methods could take OgrFieldType instead of c_int as parameter. (link to the OGR enum)

I was thinking too about moving OFT types into an enum, so no problem, I will do it.

jdroenner · 2017-01-13T07:06:31Z

src/vector/feature.rs

+    }
+
+    pub fn set_field_string(&self, field_name: &str, value: &str){
+        let c_str_field_name = CString::new(field_name).unwrap();


If it is possible please try not to use unwrap in library methods. Methods like this should return Result<()>.
I think we should add the returned error in errors.rs if we don't handle it already.

Alright for returning Result on these methods.
However is it possible here to not use unwrap() ?

Error handling in rust is a bit difficult. Where is a lot about it in the book.

To make this easier rust-gdal uses the error-chain crate. We already have some wrapping errors in errors.rs:

FfiNulError(::std::ffi::NulError); StrUtf8Error(::std::str::Utf8Error);

To handle the Ok or return the (wrapped) Err you can use the try!{} macro or ?. In this case all you need to do is to replace unwrap() with ?: let c_str_field_name = CString::new(field_name)?;
CString::new returns Result<CString, ::std::ffi::NulError>. In the error case ? will convert NulError into the wrapping error (FfiNulError) and return it.

jdroenner · 2017-01-13T07:16:53Z

@mthh this looks really nice :) I can have a closer look this weekend.

frewsxcv

I added a bunch of nits related to naming, they aren't big issues.

frewsxcv · 2017-01-14T18:38:29Z

src/vector/defn.rs

@@ -26,6 +29,11 @@ impl Defn {
            total: total
        };
    }
+
+    pub fn new_from_layer(lyr: &Layer) -> Defn {


This should be just from_layer if you wanted to match conventions in std.

Sure, I will change it.

frewsxcv · 2017-01-14T18:38:40Z

src/vector/defn.rs

@@ -66,4 +74,16 @@ impl<'a> Field<'a> {
        let rv = unsafe { ogr::OGR_Fld_GetNameRef(self.c_field_defn) };
        return _string(rv);
    }
+
+    pub fn get_type(&'a self) -> i32 {


This should be just type if you wanted to match conventions in std.

I guess I can't use type as a function name as it is keyword in rust (that's why I chose get_type in replacement and thus to be consistent over the methods of this object I also used get_width and get_precision). Any suggestions ?

As this will return a OGRFieldType you could change the method name to field_type.

frewsxcv · 2017-01-14T18:38:46Z

src/vector/defn.rs

+        unsafe { ogr::OGR_Fld_GetType(self.c_field_defn) }
+    }
+
+    pub fn get_width(&'a self) -> i32 {


This should be just width if you wanted to match conventions in std.

frewsxcv · 2017-01-14T18:38:52Z

src/vector/defn.rs

+        unsafe { ogr::OGR_Fld_GetWidth(self.c_field_defn) }
+    }
+
+    pub fn get_precision(&'a self) -> i32 {


This should be just precision if you wanted to match conventions in std.

frewsxcv · 2017-01-14T18:42:12Z

src/vector/feature.rs

@@ -87,4 +150,12 @@ impl FieldValue {
            _ => panic!("not a RealValue")
        }
    }
+
+    /// Interpret the value as `i32`. Panics if the value is something else.
+    pub fn as_int(self) -> i32 {


Methods that consume self are usually named into_<type>, so in this case, it would be into_int, but I notice above that the other methods on this structure use as_<type>.

Should I also change the name of the two previous methods (as_real -> to_real and as_string to_string ?)

i guess this would be the right thing to do. Also i think these methods should return an Option instead of panicking...

I made the other changes but not this last one (returning an Option instead of panicking), because I didn't know how to handle this when using a FieldValue to set a field :

pub fn set_field(&self, field_name: &str, type_value: OGRFieldType, value: FieldValue) -> Result<()> { match type_value { OGRFieldType::OFTReal => self.set_field_double(field_name, value.to_real()),

like here, when using value.to_real(). Is the use of is_some ok ? Or should I look for a way to handle this nicely without using is_some then unwrap ? (i will look into the error_chain crate documentation then make the changes).

jdroenner

I really appreciate this contribution. It adds so much functionality to rust-gdal.
There are some unwraps left and i think we need more error types :)

jdroenner · 2017-01-15T17:28:24Z

src/vector/feature.rs

+
+    pub fn set_field_string(&self, field_name: &str, value: &str) -> Result<()> {
+        let c_str_field_name = CString::new(field_name)?;
+        let c_str_value = CString::new(value).unwrap();


unwrap to ? -> CString::new(value)?;

jdroenner · 2017-01-15T18:18:46Z

src/vector/feature.rs

+    pub fn set_field_string(&self, field_name: &str, value: &str) -> Result<()> {
+        let c_str_field_name = CString::new(field_name)?;
+        let c_str_value = CString::new(value).unwrap();
+        let idx = unsafe { ogr::OGR_F_GetFieldIndex(self.c_feature, c_str_field_name.as_ptr())};


OGR_F_GetFieldIndex will return -1 if a field name does not exist. I guess we need a new error. Maybe InvalidFieldName ?

jdroenner · 2017-01-15T18:24:48Z

src/vector/feature.rs

+
+    pub fn set_field(&self, field_name: &str, type_value: OGRFieldType, value: FieldValue) -> Result<()> {
+        match type_value {
+            OGRFieldType::OFTReal => self.set_field_double(field_name, value.to_real()),


Regarding the conversion of FieldValue to the matching OGRFieldType:
As i see it this method fails if FieldValue and OGRFieldType don't match. So at this point we don't need the OGRFieldType at all... We can match based on the FieldValue:

pub enum FieldValue { IntegerValue(i32), StringValue(String), RealValue(f64), } pub fn set_field(&self, field_name: &str, value: FieldValue) -> Result<()> { match value { FieldValue::RealValue(value) => self.set_field_double(field_name, value)?, ...

Also the set_field_double method returns a result which should be used/handled here (using the ? operator).

Shouldn't the Result from set_field_double (and others) just be passed to the caller of set_field like this?

pub fn set_field(&self, field_name: &str, value: FieldValue) -> Result<()> { match value { FieldValue::RealValue(value) => self.set_field_double(field_name, value), FieldValue::StringValue(value) => self.set_field_string(field_name, value.as_str()), FieldValue::IntegerValue(value) => self.set_field_integer(field_name, value) } }

This way, with an invalid field name we get the ErrKind::InvalidFieldName error from the set_field_double/string/.. function, and we can't enter an invalid FieldValue due to the type checking.

Yes you are right this should be perfectly fine. I was thinking to complicated.

jdroenner · 2017-01-15T18:32:52Z

src/vector/feature.rs

+            OGRFieldType::OFTReal => self.set_field_double(field_name, value.to_real()),
+            OGRFieldType::OFTString => self.set_field_string(field_name, value.to_string().as_str()),
+            OGRFieldType::OFTInteger => self.set_field_integer(field_name, value.to_int()),
+            _ => Err(ErrorKind::InvalidInput("set_field").into())


Maybe a better name for the error is InvalidFieldType or 'UnhandledFieldType'.

jdroenner · 2017-01-15T18:38:35Z

src/vector/feature.rs

+    }
+
+    pub fn set_geometry(&mut self, geom: Geometry) -> Result<()> {
+        self.geometry = geom;


What happens if the following method call fails? Wouldn't it be more safe if this is done after the GDAL call and before Ok(()) ?

jdroenner · 2017-01-15T19:20:21Z

src/vector/layer.rs

+            let val = &values[i];
+            match val {
+                &FieldValue::StringValue(ref v) => {
+                    unsafe { ogr::OGR_F_SetFieldString(c_feature, idx, CString::new(v.as_str()).unwrap().as_ptr()) };


unwrap to ?

jdroenner · 2017-01-15T19:24:32Z

src/vector/layer.rs

+
+impl FieldDefn {
+    pub fn new(name: &str, field_type: ogr_enums::OGRFieldType) -> FieldDefn {
+        let c_str = CString::new(name).unwrap();


Let the method return a Result and change unwrap to ?

jdroenner · 2017-01-15T19:26:41Z

src/vector/layer.rs

+    }
+    pub fn add_to_layer(&self, layer: &Layer) {
+        let rv = unsafe { ogr::OGR_L_CreateField(layer.gdal_object_ptr(), self.c_obj, 1) };
+        assert_eq!(rv, ogr_enums::OGRErr::OGRERR_NONE);


the last unwrap/assert :)

jdroenner · 2017-01-15T19:29:29Z

src/errors.rs

+        InvalidInput(method_name: &'static str) {
+            description("Invalid input")
+            display("Invalid input : {}", method_name)
+        }


Is there a more descriptive name for this error (maybe InvalidFieldType)?
Also i added the method name to the errors related to the GDAL FFI calls to enable users to look up methods in the GDAL API. I'm not sure if it is needed here.

jdroenner · 2017-01-15T20:03:52Z

src/vector/feature.rs

+                let rv = unsafe { ogr::OGR_F_GetFieldAsInteger(self.c_feature, field_id) };
+                return Some(FieldValue::IntegerValue(rv as i32));
+            },
+            _ => panic!("Unknown field type {:?}", field_type)


There should be an error for this :) . Maybe UnhandledFieldType ?

mthh · 2017-01-16T00:29:33Z

Many thanks for the detailed and instructive review!! I made most of the changes but i'm still unsure about 1 or 2 things.
I'll push it on github it in the day.

…ds' method

mthh · 2017-01-16T10:14:45Z

I guess I modified the various things discussed, except the clone method for which I did not know how to handle errors that might occur.

jdroenner · 2017-01-16T20:29:50Z

This is great!
I think we could merge this and create an issue for Geometry::clone. We may need to change some things in the Geometry implementation to solve this.

mthh · 2017-01-17T10:35:38Z

Great that it can be merged !
As I plan to dig a bit into rust (and to use rust-gdal) I will keep an eye on this future issue and see if can figure something out to improve some points of the Geometry implementation.

jdroenner · 2017-01-17T11:00:55Z

Merged.
Actually there already is an issue for Geometry handling: #4

mthh added 7 commits January 8, 2017 16:04

Add basic support for writing ogr data

505996c

Allow to clone geometries + Add example of reading-reprojecting-writi…

e00c7a3

…ng dataset

Allow to get/set more field types + improve read/write example

2614188

minor clean-up + add example layer

0304f14

Add shortest methods to create/set fields/values

aec9987

Try yo improve examples

9353476

Update tests to reflect changes

7cba326

jdroenner reviewed Jan 13, 2017

View reviewed changes

frewsxcv reviewed Jan 14, 2017

View reviewed changes

mthh added 2 commits January 15, 2017 17:41

Add OGRFieldType enum + better error handling + methods renaming

ca9abcd

fix method name in test in comment

8963266

jdroenner reviewed Jan 15, 2017

View reviewed changes

mthh added 2 commits January 16, 2017 09:25

Avoid using unwrap in library methods + simplify 'create_feature_fiel…

63c97a8

…ds' method

Return a Result on Feature 'field' method + fix test in comments

a4361e1

frewsxcv approved these changes Jan 17, 2017

View reviewed changes

jdroenner merged commit 2f4e9e7 into georust:master Jan 17, 2017

Extend the existing possibilities of writing ogr datasets. #31

Extend the existing possibilities of writing ogr datasets. #31

Conversation

mthh commented Jan 12, 2017

Choose a reason for hiding this comment

mthh Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdroenner Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

jdroenner commented Jan 13, 2017

frewsxcv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthh Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

jdroenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mthh commented Jan 16, 2017

mthh commented Jan 16, 2017

jdroenner commented Jan 16, 2017

mthh commented Jan 17, 2017

jdroenner commented Jan 17, 2017

mthh Jan 15, 2017 •

edited

Loading

jdroenner Jan 15, 2017 •

edited

Loading

mthh Jan 15, 2017 •

edited

Loading