Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Serialize without going through JsonObject #233

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Implement Serialize without going through JsonObject #233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d4b8c5
Implement `Serialize` without going through JsonObject
zyla Sep 19, 2023
0ee7281
Serialize None properties as null
zyla Sep 20, 2023
222b859
Add comments around to_value unwraps and panics
zyla Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
## Unreleased

* Added conversion from `Vec<Feature>` to `GeoJson`.
* Changed `Serialize` impls to avoid creating intermediate `JsonObject`s.

## 0.24.1

Expand Down
71 changes: 33 additions & 38 deletions src/feature.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ use std::str::FromStr;
use crate::errors::{Error, Result};
use crate::{util, Feature, Geometry, Value};
use crate::{JsonObject, JsonValue};
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use serde_json::json;
use serde::{ser::SerializeMap, Deserialize, Deserializer, Serialize, Serializer};

impl From<Geometry> for Feature {
fn from(geom: Geometry) -> Feature {
Expand Down Expand Up @@ -55,35 +54,13 @@ impl FromStr for Feature {

impl<'a> From<&'a Feature> for JsonObject {
fn from(feature: &'a Feature) -> JsonObject {
let mut map = JsonObject::new();
map.insert(String::from("type"), json!("Feature"));
map.insert(
String::from("geometry"),
serde_json::to_value(&feature.geometry).unwrap(),
);
if let Some(ref properties) = feature.properties {
map.insert(
String::from("properties"),
serde_json::to_value(properties).unwrap(),
);
} else {
map.insert(
String::from("properties"),
serde_json::to_value(Some(serde_json::Map::new())).unwrap(),
);
}
if let Some(ref bbox) = feature.bbox {
map.insert(String::from("bbox"), serde_json::to_value(bbox).unwrap());
}
if let Some(ref id) = feature.id {
map.insert(String::from("id"), serde_json::to_value(id).unwrap());
match serde_json::to_value(feature).unwrap() {
serde_json::Value::Object(obj) => obj,
value => panic!(
"serializing Feature should result in an Object, but got something {:?}",
value
),
}
if let Some(ref foreign_members) = feature.foreign_members {
for (key, value) in foreign_members {
map.insert(key.to_owned(), value.to_owned());
}
}
map
}
}

Expand Down Expand Up @@ -182,7 +159,26 @@ impl Serialize for Feature {
where
S: Serializer,
{
JsonObject::from(self).serialize(serializer)
let mut map = serializer.serialize_map(None)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to suggest passing in the length here, but serde_json doesn't seem to use it.

map.serialize_entry("type", "Feature")?;
map.serialize_entry("geometry", &self.geometry)?;
if let Some(ref properties) = self.properties {
map.serialize_entry("properties", properties)?;
} else {
map.serialize_entry("properties", &JsonObject::new())?;
Copy link
Member

@michaelkirk michaelkirk Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be null instead of an empty object.

I think this reflects a gap in our test coverage. Can you add it if you agree? Something like:

    #[test]    
    fn feature_json_null_properties() {    
        let geojson_str = r#"{
            "geometry": null,    
            "properties": null,    
            "type":"Feature"    
        }"#;    
        let geojson = geojson_str.parse::<GeoJson>().unwrap();
        let GeoJson::Feature(feature) = geojson else {
            panic!("unexpected geojson");
        };    
        assert!(feature.properties.is_none());    
        assert_eq!(feature.to_string(), r#"{"type":"Feature","geometry":null,"properties":null}"#);    
    }

A Feature object has a member with the name "properties". The
value of the properties member is an object (any JSON object or a
JSON null value).

edit: updated the quote. I initially copied the wrong doc snippet. 🤦

(from https://datatracker.ietf.org/doc/html/rfc7946#section-3.2)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Changed the behavior and added a test.

}
if let Some(ref bbox) = self.bbox {
map.serialize_entry("bbox", bbox)?;
}
if let Some(ref id) = self.id {
map.serialize_entry("id", id)?;
}
if let Some(ref foreign_members) = self.foreign_members {
for (key, value) in foreign_members {
map.serialize_entry(key, value)?;
}
}
map.end()
}
}

Expand Down Expand Up @@ -229,8 +225,7 @@ mod tests {
use std::str::FromStr;

fn feature_json_str() -> &'static str {
"{\"geometry\":{\"coordinates\":[1.1,2.1],\"type\":\"Point\"},\"properties\":{},\"type\":\
\"Feature\"}"
"{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[1.1,2.1]},\"properties\":{}}"
}

fn properties() -> Option<JsonObject> {
Expand Down Expand Up @@ -314,8 +309,7 @@ mod tests {
#[test]
fn test_display_feature() {
let f = feature().to_string();
assert_eq!(f, "{\"geometry\":{\"coordinates\":[1.1,2.1],\"type\":\"Point\"},\"properties\":{},\"type\":\
\"Feature\"}");
assert_eq!(f, "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[1.1,2.1]},\"properties\":{}}");
}

#[test]
Expand Down Expand Up @@ -344,7 +338,7 @@ mod tests {

#[test]
fn encode_decode_feature_with_id_number() {
let feature_json_str = "{\"geometry\":{\"coordinates\":[1.1,2.1],\"type\":\"Point\"},\"id\":0,\"properties\":{},\"type\":\"Feature\"}";
let feature_json_str = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[1.1,2.1]},\"properties\":{},\"id\":0}";
let feature = crate::Feature {
geometry: Some(Geometry {
value: Value::Point(vec![1.1, 2.1]),
Expand All @@ -370,7 +364,7 @@ mod tests {

#[test]
fn encode_decode_feature_with_id_string() {
let feature_json_str = "{\"geometry\":{\"coordinates\":[1.1,2.1],\"type\":\"Point\"},\"id\":\"foo\",\"properties\":{},\"type\":\"Feature\"}";
let feature_json_str = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[1.1,2.1]},\"properties\":{},\"id\":\"foo\"}";
let feature = crate::Feature {
geometry: Some(Geometry {
value: Value::Point(vec![1.1, 2.1]),
Expand Down Expand Up @@ -416,7 +410,8 @@ mod tests {
fn encode_decode_feature_with_foreign_member() {
use crate::JsonObject;
use serde_json;
let feature_json_str = "{\"geometry\":{\"coordinates\":[1.1,2.1],\"type\":\"Point\"},\"other_member\":\"some_value\",\"properties\":{},\"type\":\"Feature\"}";
let feature_json_str = "{\"type\":\"Feature\",\"geometry\":{\"type\":\"Point\",\"coordinates\":[1.1,2.1]},\"properties\":{},\"other_member\":\"some_value\"}";

let mut foreign_members = JsonObject::new();
foreign_members.insert(
String::from("other_member"),
Expand Down
43 changes: 23 additions & 20 deletions src/feature_collection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use serde::ser::SerializeMap;
use std::convert::TryFrom;
use std::iter::FromIterator;
use std::str::FromStr;
Expand All @@ -20,7 +21,6 @@ use crate::errors::{Error, Result};
use crate::{util, Bbox, Feature};
use crate::{JsonObject, JsonValue};
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use serde_json::json;

/// Feature Collection Objects
///
Expand All @@ -44,7 +44,7 @@ use serde_json::json;
///
/// assert_eq!(
/// serialized,
/// "{\"features\":[],\"type\":\"FeatureCollection\"}"
/// "{\"type\":\"FeatureCollection\",\"features\":[]}"
/// );
/// ```
///
Expand Down Expand Up @@ -94,24 +94,13 @@ impl<'a> IntoIterator for &'a FeatureCollection {

impl<'a> From<&'a FeatureCollection> for JsonObject {
fn from(fc: &'a FeatureCollection) -> JsonObject {
let mut map = JsonObject::new();
map.insert(String::from("type"), json!("FeatureCollection"));
map.insert(
String::from("features"),
serde_json::to_value(&fc.features).unwrap(),
);

if let Some(ref bbox) = fc.bbox {
map.insert(String::from("bbox"), serde_json::to_value(bbox).unwrap());
match serde_json::to_value(fc).unwrap() {
serde_json::Value::Object(obj) => obj,
value => panic!(
"serializing FeatureCollection should result in an Object, but got something {:?}",
value
),
}

if let Some(ref foreign_members) = fc.foreign_members {
for (key, value) in foreign_members {
map.insert(key.to_owned(), value.to_owned());
}
}

map
}
}

Expand Down Expand Up @@ -168,7 +157,21 @@ impl Serialize for FeatureCollection {
where
S: Serializer,
{
JsonObject::from(self).serialize(serializer)
let mut map = serializer.serialize_map(None)?;
map.serialize_entry("type", "FeatureCollection")?;
map.serialize_entry("features", &self.features)?;

if let Some(ref bbox) = self.bbox {
map.serialize_entry("bbox", bbox)?;
}

if let Some(ref foreign_members) = self.foreign_members {
for (key, value) in foreign_members {
map.serialize_entry(key, value)?;
}
}

map.end()
}
}

Expand Down
6 changes: 5 additions & 1 deletion src/geojson.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,11 @@ impl Serialize for GeoJson {
where
S: Serializer,
{
JsonObject::from(self).serialize(serializer)
match self {
GeoJson::Geometry(ref geometry) => geometry.serialize(serializer),
GeoJson::Feature(ref feature) => feature.serialize(serializer),
GeoJson::FeatureCollection(ref fc) => fc.serialize(serializer),
}
}
}

Expand Down
71 changes: 52 additions & 19 deletions src/geometry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use std::{convert::TryFrom, fmt};
use crate::errors::{Error, Result};
use crate::{util, Bbox, LineStringType, PointType, PolygonType};
use crate::{JsonObject, JsonValue};
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use serde::{ser::SerializeMap, Deserialize, Deserializer, Serialize, Serializer};

/// The underlying value for a `Geometry`.
///
Expand Down Expand Up @@ -123,6 +123,21 @@ impl Value {
pub fn from_json_value(value: JsonValue) -> Result<Self> {
Self::try_from(value)
}

fn serialize_to_map<SM: SerializeMap>(
&self,
map: &mut SM,
) -> std::result::Result<(), SM::Error> {
map.serialize_entry("type", self.type_name())?;
map.serialize_entry(
match self {
Value::GeometryCollection(..) => "geometries",
_ => "coordinates",
},
self,
)?;
Ok(())
}
}

impl TryFrom<JsonObject> for Value {
Expand Down Expand Up @@ -155,16 +170,7 @@ impl fmt::Display for Value {

impl<'a> From<&'a Value> for JsonValue {
fn from(value: &'a Value) -> JsonValue {
match *value {
Value::Point(ref x) => ::serde_json::to_value(x),
Value::MultiPoint(ref x) => ::serde_json::to_value(x),
Value::LineString(ref x) => ::serde_json::to_value(x),
Value::MultiLineString(ref x) => ::serde_json::to_value(x),
Value::Polygon(ref x) => ::serde_json::to_value(x),
Value::MultiPolygon(ref x) => ::serde_json::to_value(x),
Value::GeometryCollection(ref x) => ::serde_json::to_value(x),
}
.unwrap()
::serde_json::to_value(value).unwrap()
}
}

Expand All @@ -173,7 +179,15 @@ impl Serialize for Value {
where
S: Serializer,
{
JsonValue::from(self).serialize(serializer)
match self {
Value::Point(x) => x.serialize(serializer),
Value::MultiPoint(x) => x.serialize(serializer),
Value::LineString(x) => x.serialize(serializer),
Value::MultiLineString(x) => x.serialize(serializer),
Value::Polygon(x) => x.serialize(serializer),
Value::MultiPolygon(x) => x.serialize(serializer),
Value::GeometryCollection(x) => x.serialize(serializer),
}
}
}

Expand Down Expand Up @@ -208,7 +222,7 @@ impl Serialize for Value {
/// let geojson_string = geometry.to_string();
///
/// assert_eq!(
/// "{\"coordinates\":[7.428959,1.513394],\"type\":\"Point\"}",
/// "{\"type\":\"Point\",\"coordinates\":[7.428959,1.513394]}",
/// geojson_string,
/// );
/// ```
Expand Down Expand Up @@ -291,6 +305,23 @@ impl Geometry {
pub fn from_json_value(value: JsonValue) -> Result<Self> {
Self::try_from(value)
}

fn serialize_to_map<SM: SerializeMap>(
&self,
map: &mut SM,
) -> std::result::Result<(), SM::Error> {
self.value.serialize_to_map(map)?;
if let Some(ref bbox) = self.bbox {
map.serialize_entry("bbox", bbox)?;
}

if let Some(ref foreign_members) = self.foreign_members {
for (key, value) in foreign_members {
map.serialize_entry(key, value)?
}
}
Ok(())
}
}

impl TryFrom<JsonObject> for Geometry {
Expand Down Expand Up @@ -333,7 +364,9 @@ impl Serialize for Geometry {
where
S: Serializer,
{
JsonObject::from(self).serialize(serializer)
let mut map = serializer.serialize_map(None)?;
self.serialize_to_map(&mut map)?;
map.end()
}
}

Expand Down Expand Up @@ -374,7 +407,7 @@ mod tests {

#[test]
fn encode_decode_geometry() {
let geometry_json_str = "{\"coordinates\":[1.1,2.1],\"type\":\"Point\"}";
let geometry_json_str = "{\"type\":\"Point\",\"coordinates\":[1.1,2.1]}";
let geometry = Geometry {
value: Value::Point(vec![1.1, 2.1]),
bbox: None,
Expand Down Expand Up @@ -422,8 +455,8 @@ mod tests {
let v = Value::LineString(vec![vec![0.0, 0.1], vec![0.1, 0.2], vec![0.2, 0.3]]);
let geometry = Geometry::new(v);
assert_eq!(
"{\"coordinates\":[[0.0,0.1],[0.1,0.2],[0.2,0.3]],\"type\":\"LineString\"}",
geometry.to_string()
geometry.to_string(),
"{\"type\":\"LineString\",\"coordinates\":[[0.0,0.1],[0.1,0.2],[0.2,0.3]]}"
);
}

Expand All @@ -439,7 +472,7 @@ mod tests {
#[test]
fn encode_decode_geometry_with_foreign_member() {
let geometry_json_str =
"{\"coordinates\":[1.1,2.1],\"other_member\":true,\"type\":\"Point\"}";
"{\"type\":\"Point\",\"coordinates\":[1.1,2.1],\"other_member\":true}";
let mut foreign_members = JsonObject::new();
foreign_members.insert(
String::from("other_member"),
Expand Down Expand Up @@ -482,7 +515,7 @@ mod tests {
foreign_members: None,
};

let geometry_collection_string = "{\"geometries\":[{\"coordinates\":[100.0,0.0],\"type\":\"Point\"},{\"coordinates\":[[101.0,0.0],[102.0,1.0]],\"type\":\"LineString\"}],\"type\":\"GeometryCollection\"}";
let geometry_collection_string = "{\"type\":\"GeometryCollection\",\"geometries\":[{\"type\":\"Point\",\"coordinates\":[100.0,0.0]},{\"type\":\"LineString\",\"coordinates\":[[101.0,0.0],[102.0,1.0]]}]}";
// Test encode
let json_string = encode(&geometry_collection);
assert_eq!(json_string, geometry_collection_string);
Expand Down