(Rust) Unclear documentation on the default behavior of add_feature_geom when the columns do not exist

[IvoWingelaar]

The cfgfn variable of FgbWriter::add_feature_geom does not allow us to set the properties of the feature being added.
Using rust-analyzer I discovered that the implementation of feat.property(..) as used in the usage example refers to the default no-op implementation of geozero::PropertyProcessor.

Luckily a simple workaround exists; instead of following the example code like this:

let geometry: Geometry<f64> = line_string.into();

fgb.add_feature_geom(geometry, |feat| {
    // This next line appears to be a no-op.
    feat.property(0, "field_name", &ColumnValue::Long(i))
        .unwrap();
})?;

... use the property of the FgbWriter to actually set the property values.

let geometry: Geometry<f64> = line_string.into();

fgb.add_feature_geom(geometry, |_| {})?;
// Setting the property manually on `fgb` works...
fgb.property(0, "field_name", &ColumnValue::Long(i))?;

I verified the properties' existence in QGIS.

Maybe the reason this bug happens is related to the fact that the module of FeatureWriter is not public? This fact also made it difficult to reason what was happening by reading the docs.

[pka]

I tried to reproduce this by extending the following write test:

+++ b/src/rust/tests/write.rs
@@ -126,10 +126,29 @@ fn json_to_fgb() -> Result<()> {
     })
     .ok();
 
+    let fgbfile = NamedTempFile::new()?;
+    let mut file = BufWriter::new(&fgbfile);
     // let mut file = BufWriter::new(File::create("test_multipoly.fgb")?);
-    let mut file = BufWriter::new(tempfile()?);
     fgb.write(&mut file)?;
 
+    file.flush()?;
+
+    // Load
+    let file = fgbfile.reopen()?;
+    let mut filein = BufReader::new(&file);
+    let mut fgb = FgbReader::open(&mut filein)?.select_all()?;
+    let feature = fgb.next().unwrap().unwrap();
+    assert_eq!(feature.geometry().unwrap().type_(), GeometryType::Unknown);
+    let props = feature.properties().unwrap();
+    assert_eq!(props["fid"], "42");
+    assert_eq!(props["name"], "New Zealand");
+
+    let feature = fgb.next().unwrap().unwrap();
+    assert_eq!(feature.geometry().unwrap().type_(), GeometryType::Unknown);
+    let props = feature.properties().unwrap();
+    assert_eq!(props["fid"], "43");
+    assert_eq!(props["name"], "South Africa");
+
     Ok(())
 }

Can you reproduce your problem with a test like this?

[IvoWingelaar]

Changing the test does work: apparently you need to call the add_columns function (like your test does) before you can set the properties.

My workaround was actually wrong as the properties were set, but not on the correct features.

In my original code I didn't add the calls to the add_column methods because I thought they were lazily called, as I assumed because of the unwraps not actually panicking, me not finding it stated in the docs, it, and the slightly corrupt fgb file being constructed. So the behavior is correct if you do not just copy-paste the example of the add_feature_geom.

Maybe you would accept a PR to update the documentation with the fact that the columns must actually exist before the method add_feature_geom is called?

[pka]

A PR for improving the documentation would very welcome! Maybe there is also a good way to return an error in such cases, because the current behaviour is bad for sure.