Don't bail for MaterialColors and Attributes parse failures (#471)

rojo-rbx · Nov 1, 2024 · bc8ab6b · bc8ab6b
1 parent 28b2fae
commit bc8ab6b
Show file tree

Hide file tree

Showing 7 changed files with 83 additions and 35 deletions.
diff --git a/rbx_binary/src/deserializer/error.rs b/rbx_binary/src/deserializer/error.rs
@@ -79,11 +79,4 @@ pub(crate) enum InnerError {
         expected_type_id: u8,
         actual_type_id: u8,
     },
-
-    #[error("Failed to deserialize {class_name}.{prop_name} because {source}")]
-    BadPropertyValue {
-        source: rbx_dom_weak::types::Error,
-        prop_name: String,
-        class_name: String,
-    },
 }
diff --git a/rbx_binary/src/deserializer/state.rs b/rbx_binary/src/deserializer/state.rs
@@ -481,11 +481,19 @@ This may cause unexpected or broken behavior in your final results if you rely o
                                 add_property(instance, &property, value.into());
                             }
                             Err(err) => {
-                                return Err(InnerError::BadPropertyValue {
-                                    source: err,
-                                    class_name: type_info.type_name.to_string(),
-                                    prop_name,
-                                })
+                                log::warn!(
+                                    "Failed to parse Attributes on {} because {:?}; falling back to BinaryString.
+
+rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.",
+                                    type_info.type_name,
+                                    err
+                                );
+
+                                add_property(
+                                    instance,
+                                    &property,
+                                    BinaryString::from(buffer).into(),
+                                );
                             }
                         }
                     }
@@ -497,11 +505,19 @@ This may cause unexpected or broken behavior in your final results if you rely o
                         match MaterialColors::decode(&buffer) {
                             Ok(value) => add_property(instance, &property, value.into()),
                             Err(err) => {
-                                return Err(InnerError::BadPropertyValue {
-                                    source: err,
-                                    class_name: type_info.type_name.to_string(),
-                                    prop_name,
-                                })
+                                log::warn!(
+                                    "Failed to parse MaterialColors on {} because {:?}; falling back to BinaryString.
+
+rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.",
+                                    type_info.type_name,
+                                    err
+                                );
+
+                                add_property(
+                                    instance,
+                                    &property,
+                                    BinaryString::from(buffer).into(),
+                                );
                             }
                         }
                     }

diff --git a/rbx_dom_lua/src/Error.lua b/rbx_dom_lua/src/Error.lua
@@ -5,6 +5,7 @@ Error.Kind = {
 	UnknownProperty = "UnknownProperty",
 	PropertyNotReadable = "PropertyNotReadable",
 	PropertyNotWritable = "PropertyNotWritable",
+	CannotParseBinaryString = "CannotParseBinaryString",
 	Roblox = "Roblox",
 }
 

diff --git a/rbx_dom_lua/src/customProperties.lua b/rbx_dom_lua/src/customProperties.lua
@@ -1,6 +1,8 @@
 local CollectionService = game:GetService("CollectionService")
 local ScriptEditorService = game:GetService("ScriptEditorService")
 
+local Error = require(script.Parent.Error)
+
 --- A list of `Enum.Material` values that are used for Terrain.MaterialColors
 local TERRAIN_MATERIAL_COLORS = {
 	Enum.Material.Grass,
@@ -51,6 +53,10 @@ return {
 				return true, instance:GetAttributes()
 			end,
 			write = function(instance, _, value)
+				if typeof(value) ~= "table" then
+					return false, Error.new(Error.Kind.CannotParseBinaryString)
+				end
+
 				local existing = instance:GetAttributes()
 				local didAllWritesSucceed = true
 
@@ -160,9 +166,14 @@ return {
 				return true, colors
 			end,
 			write = function(instance: Terrain, _, value: { [Enum.Material]: Color3 })
+				if typeof(value) ~= "table" then
+					return false, Error.new(Error.Kind.CannotParseBinaryString)
+				end
+
 				for material, color in value do
 					instance:SetMaterialColor(material, color)
 				end
+
 				return true
 			end,
 		},

diff --git a/rbx_xml/src/conversion.rs b/rbx_xml/src/conversion.rs
@@ -4,27 +4,35 @@
 use std::borrow::{Borrow, Cow};
 use std::convert::TryInto;
 
-use rbx_dom_weak::types::{
-    Attributes, BrickColor, Color3uint8, MaterialColors, Tags, Variant, VariantType,
+use rbx_dom_weak::{
+    types::{Attributes, BrickColor, Color3uint8, MaterialColors, Tags, Variant, VariantType},
+    Ustr,
 };
 
 pub trait ConvertVariant: Clone + Sized {
-    fn try_convert(self, target_type: VariantType) -> Result<Self, String> {
-        Self::try_convert_cow(Cow::Owned(self), target_type).map(|value| value.into_owned())
+    fn try_convert(self, class_name: Ustr, target_type: VariantType) -> Result<Self, String> {
+        Self::try_convert_cow(class_name, Cow::Owned(self), target_type)
+            .map(|value| value.into_owned())
     }
 
-    fn try_convert_ref(&self, target_type: VariantType) -> Result<Cow<'_, Self>, String> {
-        Self::try_convert_cow(Cow::Borrowed(self), target_type)
+    fn try_convert_ref(
+        &self,
+        class_name: Ustr,
+        target_type: VariantType,
+    ) -> Result<Cow<'_, Self>, String> {
+        Self::try_convert_cow(class_name, Cow::Borrowed(self), target_type)
     }
 
     fn try_convert_cow(
+        class_name: Ustr,
         value: Cow<'_, Self>,
         target_type: VariantType,
     ) -> Result<Cow<'_, Self>, String>;
 }
 
 impl ConvertVariant for Variant {
     fn try_convert_cow(
+        class_name: Ustr,
         value: Cow<'_, Self>,
         target_type: VariantType,
     ) -> Result<Cow<'_, Self>, String> {
@@ -57,18 +65,37 @@ impl ConvertVariant for Variant {
             )),
             (Variant::BinaryString(value), VariantType::Attributes) => {
                 let bytes: &[u8] = value.as_ref();
+                match Attributes::from_reader(bytes) {
+                    Ok(attributes) => Ok(Cow::Owned(attributes.into())),
+                    Err(err) => {
+                        log::warn!(
+                            "Failed to parse Attributes on {} because {:?}; falling back to BinaryString.
 
-                Ok(Cow::Owned(
-                    Attributes::from_reader(bytes)
-                        .map_err(|_| "Unknown or invalid Attributes")?
-                        .into(),
-                ))
+rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.",
+                             class_name,
+                             err
+                        );
+
+                        Ok(Cow::Owned(value.clone().into()))
+                    }
+                }
+            }
+            (Variant::BinaryString(value), VariantType::MaterialColors) => {
+                match MaterialColors::decode(value.as_ref()) {
+                    Ok(material_colors) => Ok(Cow::Owned(material_colors.into())),
+                    Err(err) => {
+                        log::warn!(
+                            "Failed to parse MaterialColors on {} because {:?}; falling back to BinaryString.
+
+rbx-dom may require changes to fully support this property. Please open an issue at https://github.com/rojo-rbx/rbx-dom/issues and show this warning.",
+                            class_name,
+                            err
+                        );
+
+                        Ok(Cow::Owned(value.clone().into()))
+                    }
+                }
             }
-            (Variant::BinaryString(value), VariantType::MaterialColors) => Ok(Cow::Owned(
-                MaterialColors::decode(value.as_ref())
-                    .map_err(|_| "invalid MaterialColors value")?
-                    .into(),
-            )),
             (_, _) => Ok(value),
         }
     }

diff --git a/rbx_xml/src/deserializer.rs b/rbx_xml/src/deserializer.rs
@@ -613,7 +613,7 @@ fn deserialize_properties<R: Read>(
             };
             log::trace!("property's read type: {xml_ty:?}, canonical type: {expected_type:?}");
 
-            let value = match value.try_convert(expected_type) {
+            let value = match value.try_convert(class_name, expected_type) {
                 Ok(value) => value,
 
                 // The property descriptor disagreed, and there was no

diff --git a/rbx_xml/src/serializer.rs b/rbx_xml/src/serializer.rs
@@ -210,7 +210,7 @@ fn serialize_instance<'dom, W: Write>(
 
             let mut serialized_name = serialized_descriptor.name.as_ref();
 
-            let mut converted_value = match value.try_convert_ref(data_type) {
+            let mut converted_value = match value.try_convert_ref(instance.class, data_type) {
                 Ok(value) => value,
                 Err(message) => {
                     return Err(