bevy_reflect: Fix deserialization with readers (bevyengine#6894)

# Objective

Fixes bevyengine#6891

## Solution

Replaces deserializing map keys as `&str` with deserializing them as `String`.

This bug seems to occur when using something like `File` or `BufReader` rather than bytes or a string directly (I only tested `File` and `BufReader` for `rmp-serde` and `serde_json`). This might be an issue with other `Read` impls as well (except `&[u8]` it seems).

We already had passing tests for Message Pack but none that use a `File` or `BufReader`. This PR also adds or modifies tests to check for this in the future.

This change was also based on [feedback](bevyengine#4561 (comment)) I received in a previous PR.

---

## Changelog

- Fix bug where scene deserialization using certain readers could fail (e.g. `BufReader`, `File`, etc.)

crates/bevy_reflect/Cargo.toml

@@ -29,7 +29,7 @@ downcast-rs = "1.2"
 parking_lot = "0.12.1"
 thiserror = "1.0"
 once_cell = "1.11"
-serde = "1"
+serde = { version = "1", features = ["derive"] }
 smallvec = { version = "1.6", features = ["serde", "union", "const_generics"], optional = true }
 glam = { version = "0.22", features = ["serde"], optional = true }
 

crates/bevy_reflect/src/serde/de.rs

@@ -12,6 +12,7 @@ use serde::de::{
 };
 use serde::Deserialize;
 use std::any::TypeId;
+use std::borrow::Cow;
 use std::fmt;
 use std::fmt::{Debug, Display, Formatter};
 use std::slice::Iter;
@@ -210,6 +211,13 @@ impl<'de> Visitor<'de> for U32Visitor {
     }
 }
 
+/// Helper struct for deserializing strings without allocating (when possible).
+///
+/// Based on [this comment](https://github.com/bevyengine/bevy/pull/6894#discussion_r1045069010).
+#[derive(Deserialize)]
+#[serde(transparent)]
+struct BorrowableCowStr<'a>(#[serde(borrow)] Cow<'a, str>);
+
 /// A general purpose deserializer for reflected types.
 ///
 /// This will return a [`Box<dyn Reflect>`] containing the deserialized data.
@@ -273,8 +281,9 @@ impl<'a, 'de> Visitor<'de> for UntypedReflectDeserializerVisitor<'a> {
         A: MapAccess<'de>,
     {
         let type_name = map
-            .next_key::<String>()?
-            .ok_or_else(|| Error::invalid_length(0, &"at least one entry"))?;
+            .next_key::<BorrowableCowStr>()?
+            .ok_or_else(|| Error::invalid_length(0, &"at least one entry"))?
+            .0;
 
         let registration = self.registry.get_with_name(&type_name).ok_or_else(|| {
             Error::custom(format_args!("No registration found for `{type_name}`"))
@@ -1536,10 +1545,11 @@ mod tests {
             108, 101, 144, 146, 100, 145, 101,
         ];
 
-        let deserializer = UntypedReflectDeserializer::new(&registry);
+        let mut reader = std::io::BufReader::new(input.as_slice());
 
+        let deserializer = UntypedReflectDeserializer::new(&registry);
         let dynamic_output = deserializer
-            .deserialize(&mut rmp_serde::Deserializer::new(input.as_slice()))
+            .deserialize(&mut rmp_serde::Deserializer::new(&mut reader))
             .unwrap();
 
         let output = <MyStruct as FromReflect>::from_reflect(dynamic_output.as_ref()).unwrap();

crates/bevy_scene/Cargo.toml

@@ -34,3 +34,4 @@ thiserror = "1.0"
 [dev-dependencies]
 postcard = { version = "1.0", features = ["alloc"] }
 bincode = "1.3"
+rmp-serde = "1.1"

crates/bevy_scene/src/serde.rs

@@ -9,6 +9,7 @@ use serde::{
     ser::SerializeStruct,
     Deserialize, Deserializer, Serialize, Serializer,
 };
+use std::borrow::Cow;
 use std::fmt::Formatter;
 
 pub const SCENE_STRUCT: &str = "Scene";
@@ -352,14 +353,14 @@ impl<'a, 'de> Visitor<'de> for ComponentVisitor<'a> {
     {
         let mut added = HashSet::new();
         let mut components = Vec::new();
-        while let Some(key) = map.next_key::<&str>()? {
-            if !added.insert(key) {
+        while let Some(BorrowableCowStr(key)) = map.next_key()? {
+            if !added.insert(key.clone()) {
                 return Err(Error::custom(format!("duplicate component: `{key}`")));
             }
 
             let registration = self
                 .registry
-                .get_with_name(key)
+                .get_with_name(&key)
                 .ok_or_else(|| Error::custom(format!("no registration found for `{key}`")))?;
             components.push(
                 map.next_value_seed(TypedReflectDeserializer::new(registration, self.registry))?,
@@ -384,6 +385,13 @@ impl<'a, 'de> Visitor<'de> for ComponentVisitor<'a> {
     }
 }
 
+/// Helper struct for deserializing strings without allocating (when possible).
+///
+/// Based on [this comment](https://github.com/bevyengine/bevy/pull/6894#discussion_r1045069010).
+#[derive(Deserialize)]
+#[serde(transparent)]
+struct BorrowableCowStr<'a>(#[serde(borrow)] Cow<'a, str>);
+
 #[cfg(test)]
 mod tests {
     use crate::serde::{SceneDeserializer, SceneSerializer};
@@ -394,6 +402,8 @@ mod tests {
     use bevy_reflect::{FromReflect, Reflect, ReflectSerialize};
     use bincode::Options;
     use serde::de::DeserializeSeed;
+    use serde::Serialize;
+    use std::io::BufReader;
 
     #[derive(Component, Reflect, Default)]
     #[reflect(Component)]
@@ -567,6 +577,49 @@ mod tests {
         assert_scene_eq(&scene, &deserialized_scene);
     }
 
+    #[test]
+    fn should_roundtrip_messagepack() {
+        let mut world = create_world();
+
+        world.spawn(MyComponent {
+            foo: [1, 2, 3],
+            bar: (1.3, 3.7),
+            baz: MyEnum::Tuple("Hello World!".to_string()),
+        });
+
+        let registry = world.resource::<AppTypeRegistry>();
+
+        let scene = DynamicScene::from_world(&world, registry);
+
+        let scene_serializer = SceneSerializer::new(&scene, &registry.0);
+        let mut buf = Vec::new();
+        let mut ser = rmp_serde::Serializer::new(&mut buf);
+        scene_serializer.serialize(&mut ser).unwrap();
+
+        assert_eq!(
+            vec![
+                145, 129, 0, 145, 129, 217, 37, 98, 101, 118, 121, 95, 115, 99, 101, 110, 101, 58,
+                58, 115, 101, 114, 100, 101, 58, 58, 116, 101, 115, 116, 115, 58, 58, 77, 121, 67,
+                111, 109, 112, 111, 110, 101, 110, 116, 147, 147, 1, 2, 3, 146, 202, 63, 166, 102,
+                102, 202, 64, 108, 204, 205, 129, 165, 84, 117, 112, 108, 101, 172, 72, 101, 108,
+                108, 111, 32, 87, 111, 114, 108, 100, 33
+            ],
+            buf
+        );
+
+        let scene_deserializer = SceneDeserializer {
+            type_registry: &registry.0.read(),
+        };
+        let mut reader = BufReader::new(buf.as_slice());
+
+        let deserialized_scene = scene_deserializer
+            .deserialize(&mut rmp_serde::Deserializer::new(&mut reader))
+            .unwrap();
+
+        assert_eq!(1, deserialized_scene.entities.len());
+        assert_scene_eq(&scene, &deserialized_scene);
+    }
+
     #[test]
     fn should_roundtrip_bincode() {
         let mut world = create_world();