soa-derive icon indicating copy to clipboard operation
soa-derive copied to clipboard

#[serde(skip)] cause struct of array containing different length of field

Open CurryPseudo opened this issue 3 years ago • 4 comments

Sometimes we derive serde's Serialize/Deserialize for Struct of Array, by #[soa_derive(Serialize, Deserialize)]. By feature from #42, we are able to skip some Struct of Array's fields by #[soa_attr(Vec, serde(skip))], for example:

#[derive(StructOfArray)]
#[soa_derive(Serialize, Deserialize)]
struct Point {
    x: f32,
    y: f32,
    // Skip serialization for PointVec::meta
    #[soa_attr(Vec, serde(skip))]
    meta: bool,
} 

Serialize is ok, but deserialize will get a invalid PointVec because it contains different length of field. For example:

#[test]
fn serde_skip_test() -> Result<(), serde_json::Error> {
    let mut soa = PointVec::new();
    soa.push(Point { x: 1.0, y: 2.0, meta: true });
    soa.push(Point { x: 3.0, y: 4.0, meta: false });


    let json = serde_json::to_string(&soa)?;
    assert_eq!(json, r#"{"x":[1.0,3.0],"y":[2.0,4.0]}"#);
    let soa2: PointVec = serde_json::from_str(&json)?;
    assert_eq!(&soa2, &PointVec {
        x: vec![1.0, 3.0],
        y: vec![2.0, 4.0],
        meta: vec![] // This comes from Vec::default(), having different length with other fields
    });
}

CurryPseudo avatar Dec 13 '21 09:12 CurryPseudo

That's unfortunate, but make sense given how everything is implemented. I could see a couple of options here: document the issue and tell people to use code based on serde::with instead of serde::skip, generate serde::Deserialize from our proc macro, or forbid serde::Deserialize on XXXVec and tell users to create Vec<XXX> with serde and convert it to XXXVec later.

Luthaf avatar Dec 14 '21 10:12 Luthaf

I prefer

generate serde::Deserialize from our proc macro

And I just found we also need to validate the length of each field while deserializing(even without any #[serde(skip)]), if not same we might return an error, by Deserializer::Error::custom.

CurryPseudo avatar Dec 15 '21 03:12 CurryPseudo

~~For #43, I think we just provide a warning when user add attribute like #[soa_attr(Vec, serde(skip))]?~~ Span::warning is not stable yet, we might just mention this issue inside README.md

CurryPseudo avatar Dec 15 '21 04:12 CurryPseudo

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b8e82c712efe0e6b74d3f10eebe06db0 Some ideas about custom deserialize.

// #[derive(StructOfArray)]
// #[soa_derive(PartialEq, Debug, Deserialize)]
struct Foo {
    a: f32,
    // #[soa_attr(serde(skip))]
    b: f32,
    c: f32,
}

// generated by #[derive(StructOfArray)]
#[derive(PartialEq, Debug)]
struct FooVec {
    a: Vec<f32>,
    b: Vec<f32>,
    c: Vec<f32>,
}

// generated by #[soa_derive(Deserialize)]
mod __detail_mod_foo {

    use super::FooVec;
    use serde::de::Error;
    use serde::{Deserialize, Deserializer};
    // same layout as FooVec
    #[derive(Deserialize)]
    struct DeserializedFooVec {
        a: Vec<f32>,
        #[serde(skip)]
        b: Vec<f32>,
        c: Vec<f32>,
    }

    impl TryFrom<DeserializedFooVec> for FooVec {
        type Error = &'static str;

        fn try_from(value: DeserializedFooVec) -> Result<Self, Self::Error> {
            let len = value.a.len();
            if value.c.len() != value.a.len() {
                return Err("c.len() != a.len()");
            }
            Ok(FooVec {
                a: value.a,
                b: std::iter::repeat_with(Default::default).take(len).collect(),
                c: value.c,
            })
        }
    }

    impl<'de> Deserialize<'de> for FooVec {
        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
        where
            D: Deserializer<'de>,
        {
            let deserialized = DeserializedFooVec::deserialize(deserializer)?;
            let r: Result<FooVec, _> = deserialized.try_into();
            r.map_err(|e| <D as Deserializer>::Error::custom(e))
        }
    }
}

#[test]
fn foo() {
    {
        let foo_vec = FooVec {
            a: vec![1.0, 2.0, 3.0],
            b: vec![0.0, 0.0, 0.0],
            c: vec![4.0, 5.0, 6.0],
        };
        let foo_vec_ser = r#"{"a":[1,2,3],"c":[4,5,6]}"#;
        let foo_vec_deser: FooVec = serde_json::from_str(foo_vec_ser).unwrap();
        assert_eq!(foo_vec, foo_vec_deser);
    }
    {
        let foo_vec_ser = r#"{"a":[1,2,3],"c":[4,5]}"#;
        let foo_vec_deser: Result<FooVec, _> = serde_json::from_str(foo_vec_ser);
        assert!(foo_vec_deser.is_err());
    }
}

CurryPseudo avatar Feb 17 '22 03:02 CurryPseudo