• [x] I agree to follow the project's code of conduct.
• [x] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This is an attempt to improve the ergonomics of parsing GeoJson using serde (FIXES https://github.com/georust/geojson/issues/184) .

~~This PR is a draft because there are a lot of error paths related to invalid parsing that I'd like to add tests for, but I first wanted to check in on overall direction of the API. What do people think?~~ I think this is ready for review!

Examples

Given some geojson like this:

let feature_collection_string = r#"{
    "type": "FeatureCollection",
    "features": [
        {
           "type": "Feature",
           "geometry": {
             "type": "Point",
             "coordinates": [125.6, 10.1]
           },
           "properties": {
             "name": "Dinagat Islands",
             "age": 123
           }
        },
        {
           "type": "Feature",
           "geometry": {
             "type": "Point",
             "coordinates": [2.3, 4.5]
           },
           "properties": {
             "name": "Neverland",
             "age": 456
           }
         }
   ]
}"#
.as_bytes();

let io_reader = std::io::BufReader::new(feature_collection_string);

Before

Deserialization

You used to parse it like this:

use geojson:: FeatureIterator;
let feature_reader = FeatureIterator::new(io_reader);
for feature in feature_reader.features() {
    let feature = feature.expect("valid geojson feature");

    let name = feature.property("name").unwrap().as_str().unwrap();
    let age = feature.property("age").unwrap().as_u64().unwrap();
    let geometry = feature.geometry.value.try_into().unwrap();

    if name == "Dinagat Islands" {
        assert_eq!(123, age);
        assert_matches!(geometry, geo_types::Point::new(125.6, 10.1).into());
    } else if name == "Neverland" {
        assert_eq!(456, age);
        assert_matches!(geometry, geo_types::Point::new(2.3, 4.5).into());
    } else {
        panic!("unexpected name: {}", name);
    }
}

Serialization

Then, to write it back to geojson, you'd have to either do all your processing strictly with the geojson types, or somehow convert your entities from and back to one of the GeoJson entities:

Something like:

// The implementation of this method is potentially a little messy / boilerplatey
let feature_collection: geojson::FeatureCollection = some_custom_method_to_convert_to_geojson(&my_structs);

// This part is easy enough though
serde_json::to_string(&geojson);

After

But now you also have the option of parsing it into your own declarative struct using serde like this:

Declaration

use geojson::{ser::serialize_geometry, de::deserialize_geometry};
use serde::{Serialize, Deserialize};

#[derive(Serialize, Deserialize)]
struct MyStruct {
    // You can parse directly to geo_types via these helpers, otherwise this field will need to be a `geojson::Geometry`
    #[serde(serialize_with = "serialize_geometry", deserialize_with = "deserialize_geometry")]
    geometry: geo_types::Point<f64>,
    name: String,
    age: u64,
}

Deserialization

for feature in geojson::de::deserialize_feature_collection::<MyStruct>(io_reader).unwrap() {
    let my_struct = feature.expect("valid geojson feature");

    if my_struct.name == "Dinagat Islands" {
        assert_eq!(my_struct.age, 123);
        assert_eq!(my_struct.geometry, geo_types::Point::new(125.6, 10.1));
    } else if my_struct.name == "Neverland" {
        assert_eq!(my_struct.age, 456);
        assert_eq!(my_struct.geometry, geo_types::Point::new(2.3, 4.5));
    } else {
        panic!("unexpected name: {}", my_struct.name);
    }
}

Serialization

let my_structs: Vec<MyStruct> = get_my_structs();
geojson::ser::to_feature_collection_writer(writer, &my_structs).expect("valid serialization");

Caveats

Performance

Performance currently isn't great. There's a couple of things which seem ridiculous in the code that I've marked with PERF: that I don't have an immediate solution for. This is my first time really diving into the internals of serde and it's kind of a lot! My hope is that performance improvements would be possible with no or little changes to the API.

Some specific numbers (from some admittedly crude benchmarks):

Old Deserialization:

FeatureReader::features (countries.geojson)                                                                                                                                  
                        time:   [5.8497 ms 5.8607 ms 5.8728 ms]                                                                                                              

New Deserialization:

FeatureReader::deserialize (countries.geojson)                                                                                                                               
                        time:   [7.1702 ms 7.1865 ms 7.2035 ms]                                                                                                              

Old serialization:

serialize geojson::FeatureCollection struct (countries.geojson)                                                                             
                        time:   [3.1471 ms 3.1552 ms 3.1637 ms]

New serialization:

serialize custom struct (countries.geojson)                                                                             
                        time:   [3.8076 ms 3.8144 ms 3.8219 ms]

So the new "ergonomic" serialization/deserialization takes about 1.2x the time as the old way. Though it's actually probably a bit better than that because with the new form you have all your data ready to use. With the old way, you still need to go through this dance before you can start your analysis:

let name = feature.property("name").unwrap().as_str().unwrap();
let age = feature.property("age").unwrap().as_u64().unwrap();
let geometry = feature.geometry.value.try_into().unwrap();

Anyway, I think this kind of speed difference is well worth the improved usability for my use cases.

Foreign Members

This doesn't support anything besides geometry and properties - e.g. foreign members are dropped. I'm hopeful that this is useful even with that limitation, but if not, maybe we can think of a way to accommodate most people's needs.

Apologies if I'm missing something obvious but I'm working on a project to try to learn Rust and cannot see how to save a list of polygons.

A single polygon works fine with the following code:

fn main() {
    let polygon_list = clockboard(Point::new(0.0, 0.0), Params::default(), None);
    let geojson_list = geojson::Value::from(&polygon_list[0]);
    println!("{}", geojson_list);
}

Which outputs

{"coordinates":[[[1.0,0.0],[0.866025,0.499999],[0.5,0.866025],[0.0,1.0],[-0.499999,0.866025],[-0.866025,0.5],[-1.0,0.0],[-0.866025,-0.499999],[-0.5,-0.866025],[-0.0,-1.0],[0.499999,-0.866025],[0.866025,-0.5],[1.0,0.0]]],"type":"Polygon"}

And so does the next feature when the relevant line is set to:

    let geojson_list = geojson::Value::from(&polygon_list[1]);

But the following fails:

    let geojson_list = geojson::Value::from(&polygon_list);

With the following error message:

cargo run 
...
error[E0277]: the trait bound `geojson::Value: std::convert::From<&std::vec::Vec<geo::Polygon<f64>>>` is not satisfied
 --> src/main.rs:7:24
  |
7 |     let geojson_list = geojson::Value::from(&polygon_list);
  |                        ^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<&std::vec::Vec<geo::Polygon<f64>>>` is not implemented for `geojson::Value`

Context: https://github.com/zonebuilders/zonebuilder-rust/pull/18

While addressing #161, I once again had the feeling that this crate can be made of papercuts sometimes. Based on the assumption that one of the primary uses of GeoJSON as a format is the production of FeatureCollection objects, this PR provides some additional conversions from Value enum members and Geometry objects to Feature objects. I'm also considering an additional From impl to construct a FeatureCollection from a geo_types::GeometryCollection Done!

This PR:

• Switches to thiserror, removing the need for manual trait impls and definitions
• Adds several more granular errors where appropriate
• Returns the Value causing the error where possible in order to add context

thiserror is an additional dependency, but it's not part of the public API and we already depend on syn etc due to serde, so compile times aren't greatly affected.

this is a breaking API change due to the addition of new errors

@georust/core Would be great to get opinions on this:

I'm not very satisfied with how this project as it's data laid out. Right now:

• Every single type has its own struct. This has one large disadvantage. For Geometry types like Point, MultiPoint, LineString, etc., they all share the same extra data: an optional Crs and an optional Bbox. Those two fields are not yet implemented, but if they were, it'd require an extra two fields on every single struct. This is not bad, but it's not great.
• We have two extra types that aren't really part of the spec: Ring and Poly. These are largely unnecessary and make the library less ergonomic since it requires an additional layer of abstraction for some Geometry types.

I thought about this tonight and ended up restructuring everything:

pub type Bbox = Vec<f64>;

pub enum Crs {
    Named {
        name: String,
    },
    Linked {
        href: String,
        type_: Option<String>,
    },
}

pub type Position = Vec<f64>;

pub type Point = Position;
pub type LineString = Vec<Position>;
pub type Polygon = Vec<Vec<Position>>;

pub enum Value {
    Point(Point),
    MultiPoint(Vec<Point>),
    LineString(LineString),
    MultiLineString(Vec<LineString>),
    Polygon(Polygon),
    MultiPolygon(Vec<Polygon>),
    GeometryCollection(Vec<Geometry>),
}

pub struct Geometry {
    pub bbox: Option<Bbox>,
    pub value: Value,
    pub crs: Option<Crs>,
}

pub struct Feature {
    pub bbox: Option<Bbox>,
    pub crs: Option<Crs>,
    pub geometry: Geometry,
    pub id: Option<String>,
    pub properties: Option<json::Object>,
}

pub struct FeatureCollection {
    pub bbox: Option<Bbox>,
    pub crs: Option<Crs>,
    pub features: Vec<Feature>,
}

pub enum GeoJson {
    Geometry(Geometry),
    Feature(Feature),
    FeatureCollection(FeatureCollection),
}

Big differences:

• As far as I know, the new structure covers the entire GeoJSON spec!
• Bbox and Crs are added
  • These are getting added regardless of the restructure
• Geometry struct and Value enum
  • To prevent redundancy, I put the common fields in Geometry and the individual data in their respective enum variant
• Point, LineString, and Polygon types
  • These sort of replace Ring and Poly. They mainly exist so we don't have to write Vec<Vec<Vec<Positions>>> everywhere.

Please let me know any opinions, concerns, thoughts on this. If I don't hear anything within a week, I'll move forward with this and bump to version 0.1.x.

EDITS:

• Wrapped Feature::properties in Option
• Renamed Geometry::_type to Geometry::coordinates
• Renamed Positions to Position
• GeometryCollection(Vec<GeometryType>) -> GeometryCollection(Vec<Geometry>)
• Rename GeometryType to Value, Geometry::type_ to Geometry::value
• For the sake of better ergonomics, change Bbox and Position to be types instead of structs
  • Before: Value::Point(Position(vec![1.0, 2.0, -3.0]))
  • After: Value::Point(vec![1.0, 2.0, -3.0])
• Add GeoJson enum
  • This is where from_json and to_json will go as the main entry point API for this library. Just like the spec, every valid GeoJson object is either a Geometry, a Feature, or a FeatureCollection.
• Add pub prefixes

• [x] I agree to follow the project's code of conduct.
• [x] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Implementation Checklist

To summarize, this is the impl. checklist I'm suggesting:

1. [x] FromIterator for FeatureCollection
2. [ ] ~~From<&'a T> for Feature where From<&'a T> for Value, T: BoundingRect~~
3. [x] Ensure correct bbox calculations in 1 ~~and 2 above~~

@michaelkirk @urschrei I realize we can't use BoundingRect as we only depend on geo-types. Now, I'm a bit hesitant to add impl from geo-types -> Feature given it's just a conversion to Value, then to Feature. The users could do this themselves by converting to Value then using .into() method.

The PR currently just adds FromIterator impl. Should add some tests for this; have an examples collections with bbox to try out with? We could also improve the docs for Value; currently it is hidden deep, and doesn't explain usage.

Closes #40

I thought I'd make a PR for my progress sooner rather than later to get some feedback on the approach. Default framework is rustc_serialize, so user-code shouldn't have to change. Serde is feature-gated by with_serde.

I've tried to abstract the concept of JsonValue and JsonObject between rustc_serialize and serde so that stuff like Feature don't need duplicate fields for each framework.

Serialization also looks like it's a two-hop process right now? json string -> json tree (serde_json::Value or rustc_serialize::json::Object) -> T, rather than just json string -> T. I'm trying to make as few changes as necessary though so haven't given it much thought.

(Stable will work when I finish redoing the other modules, only Feature is done so far)

Hello, this PR has a couple of parsing optimizations in two commits:

• When creating Vecs for GeoJSON objects, initialize with Vec::with_capacity(...) since we know what size to expect.
• Avoid a couple of critical clone() calls.

Currently, when parsing from a string, the entire JSON value is cloned after it's parsed, then each Feature is cloned again when parsing a FeatureCollection. I believe this is due to serde_json's as_array() and as_object() functions returning borrowed references. I avoid those calls by doing the match directly (by adding the _owned macros), which allows moving those objects without cloning.

I tested this on a release target on a ~330MB GeoJSON file (all Polygons), current master parses in about 18 seconds. After these changes, it takes under 10s. Parsing using just serde_json is about 8.2 s.

I'm pretty new to Rust so feel free to let me know if these changes are a bad idea -- or any other suggestions.

I'm running into an issue that I'm sure others have had too. I think my confusion should be covered in the README/lib.rs documentation. I'd happily write a PR once I know what the solution is.

How do I convert a geojson object (say, a Value::Polygon) into a geo one? (e.g. geo::Polygon)? Here's my attempt, copied from the tests in conversion.rs, which doesn't compile:

extern crate geo;
extern crate geojson;
extern crate num_traits;

use geojson::{GeoJson, Value};
use geojson::conversion::TryInto;
use num_traits::Float;

fn main() {
    let coord1 = vec![100.0, 0.0];
    let coord2 = vec![101.0, 1.0];
    let coord3 = vec![101.0, 1.0];
    let coord4 = vec![104.0, 0.2];
    let coord5 = vec![100.9, 0.2];
    let coord6 = vec![100.9, 0.7];

    let geojson_multi_line_string_type1 = vec![
        vec![
            coord1.clone(),
            coord2.clone(),
            coord3.clone(),
            coord1.clone(),
        ],
        vec![
            coord4.clone(),
            coord5.clone(),
            coord6.clone(),
            coord4.clone(),
        ],
    ];
    let geojson_polygon = Value::Polygon(geojson_multi_line_string_type1);
    let geo_polygon: geo::Polygon<f64> = geojson_polygon.try_into().unwrap();
}

This fails to compile with the error:

error[E0277]: the trait bound `geojson::Value: geojson::conversion::TryInto<geo::Polygon<f64>>` is not satisfied
  --> examples/geojson.rs:32:58
   |
32 |     let geo_polygon: geo::Polygon<f64> = geojson_polygon.try_into().unwrap();
   |                                                          ^^^^^^^^ the trait `geojson::conversion::TryInto<geo::Polygon<f64>>` is not implemented for `geojson::Value`
   |
   = help: the following implementations were found:
             <geojson::Value as geojson::conversion::TryInto<geo::types::Point<T>>>
             <geojson::Value as geojson::conversion::TryInto<geo::types::MultiPoint<T>>>
             <geojson::Value as geojson::conversion::TryInto<geo::types::LineString<T>>>
             <geojson::Value as geojson::conversion::TryInto<geo::types::MultiLineString<T>>>
           and 4 others

Which doesn't make sense to me. That implementation patently is there. What am I missing?

Closes #149. Uses the existing Display implementation on Geometry to display the GeoJSON string for geojson::Value.

The issue mentions needing to wrap the Value in both a Geometry and GeoJson, but I'm only wrapping in a Geometry here so let me know if I missed anything. I'm also not sure if there's a clean way of avoiding the clone() when creating the Geometry. Happy to make any changes, thanks!

Docs ( https://georust.github.io/rust-geojson/geojson/index.html ) imply that serde is an optional alternative, but it looks like it only uses serde?

We should expose a feature float_roundtrip which enables the same feature on serde_json for those users willing to pay a performance penalty for more precise parsing of floating point numbers.

https://github.com/georust/geo/issues/901#issuecomment-1235359140

FeatureIterator is intended to be an efficient way to iterate through individual Features in a FeatureCollection without ever having to parse the entire FeatureCollection into memory at once.

To do so, it seeks through the input stream until it finds the features array, and then starts parsing the individual features one by one.

Currently however, the way we seek to the "start of the features" array is looking for any occurrence of [. This would fail with a document like:

{ 
  "type": "FeatureCollection",
  "bbox": [100.0, 0.0, 105.0, 1.0],
  "features": [
    ...
  ] 
}

or more complicated ones like this:

{ 
  "type": "FeatureCollection"
  "some_foreign_member": { "a": [1], "b" : { "foo": [2, 3] } },
  "features": [
    ...
  ],
}

We should update FeatureIterator to be more robust in how it parses a FeatureCollection.

At the time of writing, the Error enum is 216 bytes. We can reduce the size by splitting the enum into different enums (e.g. DeserializeError, SerializeError, ToGeoError, etc). And also consider not embedding types like Feature (which is 208 bytes) into the enum fields (maybe).

See https://github.com/georust/geojson/compare/frewsxcv-test-test?expand=1

This test fails with:

called `Result::unwrap()` on an `Err` value: Custom { kind: InvalidData, error: Error("invalid type: floating point `1`, expected a map", line: 1, column: 4) }

(copied from discord)

what do people think about adding macros in the geojson crate to ease creation?

let feature = geojson::feature! {
   geometry: geojson::point![1., 1.],
   properties: {
      foo: "bar",
   },
};

versus

let properties = HashMap::<String, serde_json::JsonValue>::new():
properties.insert("foo".to_string(), "bar".to_string());
let feature = geojson::Feature {
   geometry: Some(geojson::Geometry::new(
      geojson::Value::Point(vec![1., 1.,]),
   )),
   properties: Some(properties),
   ...Default::default(),
};

not the exact syntax necessarily. just think it would be great to have an easier way to construct the structures

it'd be really slick if the body of the macro was identical syntax to geojson

By default, serde_json uses a BTreeMap to store JSValue's, which means that properties can get unexpectedly re-ordered. serde_json has an optional (disabled by default) preserve_order feature for users who want fields to preserve the order in which they were inserted.

Most of the time reordering is fine, and it's faster, so probably we should maintain the default behavior.

The issue I'm facing right now is while working on geozero, when converting formats back and fourth, we have a notion of field order which is respected for some formats, but not geojson. It'd be convenient to be able to enforce field ordering in geojson for those who want it.

Another small thing, even though JSON equality is order agnostic, I think there is an expected ordering of fields in a geojson for readability, like this:

{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [125.6, 10.1]
  },
  "properties": {
    "name": "Dinagat Islands"
  }
}

Not:

{
  "properties": {
    "name": "Dinagat Islands"
  }
  "geometry": {
    "coordinates": [125.6, 10.1]
    "type": "Point",
  },
  "type": "Feature",
}

Adding the feature to Cargo.toml is pretty easy, but it breaks a lot of tests with hard-coded expectations around property order:

Left:  {"type":"Feature","geometry":{"type":"Point","coordinates":[1.1,2.1]},"properties":{}}
Right: {"geometry":{"coordinates":[1.1,2.1],"type":"Point"},"properties":{},"type":"Feature"}

We could change all these to be checks for structural equality, rather than exact string matching, but I wanted to check in before I spend the hour or so doing that, since there are a dozen or so broken tests.