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.

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

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

Open
CurryPseudo opened this issue Dec 13, 2021 · 4 comments

Comments

@CurryPseudo
Copy link
Contributor

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 CurryPseudo changed the title #[serde(skip)] causing invalid Struct of Array #[serde(skip)] causing invalid Struct of Array containing different length of field Dec 13, 2021
@CurryPseudo CurryPseudo changed the title #[serde(skip)] causing invalid Struct of Array containing different length of field #[serde(skip)] cause Struct of Array containing different length of field Dec 13, 2021
@CurryPseudo CurryPseudo changed the title #[serde(skip)] cause Struct of Array containing different length of field #[serde(skip)] cause struct of array containing different length of field Dec 13, 2021
@Luthaf
Copy link
Member

Luthaf commented Dec 14, 2021

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.

@CurryPseudo
Copy link
Contributor Author

CurryPseudo commented Dec 15, 2021

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
Copy link
Contributor Author

CurryPseudo commented Dec 15, 2021

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 added a commit to CurryPseudo/soa-derive that referenced this issue Dec 15, 2021
@CurryPseudo
Copy link
Contributor Author

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());
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants