Dushistov / couchbase-lite-rust

Lightweight, embedded, syncable NoSQL database engine wrapper for Rust
Apache License 2.0
25 stars 10 forks source link

serde-fleece: Support skipping fields #70

Closed agg23 closed 2 years ago

agg23 commented 2 years ago

serde supports several field attributes to disable or modify deserialization for that field:

#[serde(default)]
#[serde(skip_deserializing)]
#[serde(deserialize_with = "path")] // Probably doesn't need to be supported, and may be more complicated anyway

I can't find any documentation on how to properly support these features in serde, but looking through the deserializer macro (https://github.com/serde-rs/serde/blob/fb2fe409c8f7ad6c95e3096e5e9ede865c8cfb49/serde_derive/src/de.rs#L676-L682) shows that it expects Ok(None) as the result from SeqAccess::next_element() when a field is missing. The current serde-fleece implementation instead returns an error, which is to be expected given the apparent lack of API documentation.

I'm not quite sure how to reconcile the differences in functionality between erroring and returning Ok(None) in those error cases for a generalized deserialization library such as serde-fleece. Not throwing the error works but it seems odd given that the option is there (the return type is Result for a reason).

Dushistov commented 2 years ago

Support skipping fields

Can you describe what is not working?

This works just fine:

    #[derive(Serialize, Deserialize, Debug, PartialEq)]
    struct SkipField {
        i: i32,
        #[serde(default)]
        #[serde(skip_deserializing)]
        s: String,
    }
    assert_eq!(
        SkipField {
            i: 17,
            s: String::new(),
        },
        ser_deser(&SkipField {
            i: 17,
            s: "aaaa".into(),
        })
        .unwrap()
    );

where ser_deser function that serialize to fleece encoding byte array and deserialize from fleece encoding array.

agg23 commented 2 years ago

The problem arises when the field is conditionally deserialized:

#[test]
fn test_optional_skip() {
    #[derive(Serialize, Deserialize, Debug, PartialEq)]
    struct SkipField {
        i: i32,
        #[serde(default)]
        #[serde(skip_serializing)]
        s: String,
    }
    assert_eq!(
        SkipField {
            i: 17,
            s: String::new(),
        },
        ser_deser(&SkipField {
            i: 17,
            s: "aaaa".into(),
        })
        .unwrap()
    );
}

Notice that the only attribute relating to deserialization is #[serde(default)], which means iff the field is not found, the default will be applied. For ease in the test, I disabled serialization of that field as well.

My apologies for not trying #[serde(skip_deserializing)] before creating the issue (on the same token, I didn't test #[serde(deserialize_with = "path")] either). Since that attribute in particular removes the field from the fields array passed to deserialize_struct(), the deserializer implementation in serde-fleece doesn't know about it and doesn't have a problem. However, when the field is made "optional" by default, the serde API doesn't make it clear that it is acceptable for a field to not be found, which suggests you shouldn't ever return errors for missing fields or incorrect lengths.