Skip to content

Commit

Permalink
Implement Serialize without going through JsonObject (#233)
Browse files Browse the repository at this point in the history 
* Implement `Serialize` without going through JsonObject

As described in <#152>,
serialization by constructing a JsonObject has to essentially clone all
data, which requires extra allocations.

After this change, we pass data directly to the `Serializer`.

`JsonObject` and `JsonValue` conversion impls were rewritten to delegate
to `Serialize`. This requires some unfortunate `panic`s where we expect
`Serialize` to produce a JSON object, but I think it's better than
duplicating the serialization logic.

* Serialize None properties as null

* Add comments around to_value unwraps and panics
  • Loading branch information
@zyla
zyla authored Sep 20, 2023
1 parent 4cfbf3a commit 085b8e0
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 76 deletions.
1 change: 1 addition & 0 deletions CHANGES.md
View file
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
93 changes: 56 additions & 37 deletions src/feature.rs
View file
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,18 @@ 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());
}
if let Some(ref foreign_members) = feature.foreign_members {
for (key, value) in foreign_members {
map.insert(key.to_owned(), value.to_owned());
// The unwrap() should never panic, because Feature contains only JSON-serializable types
match serde_json::to_value(feature).unwrap() {
serde_json::Value::Object(obj) => obj,
value => {
// Panic should never happen, because `impl Serialize for Feature` always produces an
// Object
panic!(
"serializing Feature should result in an Object, but got something {:?}",
value
)
}
}
map
}
}

Expand Down Expand Up @@ -182,7 +164,22 @@ impl Serialize for Feature {
where
S: Serializer,
{
JsonObject::from(self).serialize(serializer)
let mut map = serializer.serialize_map(None)?;
map.serialize_entry("type", "Feature")?;
map.serialize_entry("geometry", &self.geometry)?;
map.serialize_entry("properties", &self.properties)?;
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 +226,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 +310,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 +339,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 +365,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 +411,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 Expand Up @@ -445,6 +441,29 @@ mod tests {
assert_eq!(decoded_feature, feature);
}

#[test]
fn encode_decode_feature_with_null_properties() {
let feature_json_str = r#"{"type":"Feature","geometry":{"type":"Point","coordinates":[1.1,2.1]},"properties":null}"#;

let feature = crate::Feature {
geometry: Some(Value::Point(vec![1.1, 2.1]).into()),
properties: None,
bbox: None,
id: None,
foreign_members: None,
};
// Test encode
let json_string = encode(&feature);
assert_eq!(json_string, feature_json_str);

// Test decode
let decoded_feature = match decode(feature_json_str.into()) {
GeoJson::Feature(f) => f,
_ => unreachable!(),
};
assert_eq!(decoded_feature, feature);
}

#[test]
fn feature_ergonomic_property_access() {
use serde_json::json;
Expand Down
43 changes: 24 additions & 19 deletions src/feature_collection.rs
View file
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,15 @@ 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());
}

if let Some(ref foreign_members) = fc.foreign_members {
for (key, value) in foreign_members {
map.insert(key.to_owned(), value.to_owned());
// The unwrap() should never panic, because FeatureCollection contains only JSON-serializable types
match serde_json::to_value(fc).unwrap() {
serde_json::Value::Object(obj) => obj,
value => {
// Panic should never happen, because `impl Serialize for FeatureCollection` always produces an
// Object
panic!("serializing FeatureCollection should result in an Object, but got something {:?}", value)
}
}

map
}
}

Expand Down Expand Up @@ -168,7 +159,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
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
Loading

0 comments on commit 085b8e0

Please sign in to comment.