fdeantoni / prost-wkt

Prost Well-Known-Types serialization and deserialization.
Apache License 2.0
76 stars 35 forks source link

Inconsistent behaviour with google.protobuf.Struct #33

Closed tcohen01 closed 1 year ago

tcohen01 commented 1 year ago

Serializing and deserializing google.protobuf.Value of kind Struct will flatten the struct's inner fields field.

However, serializing and deserializing google.protobuf.Struct directly will not flatten the fields field.

This difference can be worked around most of the time, but it makes the usage not very ergonomic when trying to work with a collection of Struct.

This can be easily resolved by adding .field_attribute("google.protobuf.Struct.fields", "#[serde(flatten)]") to the prost build config

(This is for prost-wkt-types)

fdeantoni commented 1 year ago

Thanks for raising this issue @tcohen01! It's definitely important that behaviour is consistent. So you recommend adding the above field_attribute to the wkt-types/build.rs, correct?

tcohen01 commented 1 year ago

Yes, thanks! That should make the struct serde behaviour identical to how it's manually implemented for google.protobuf.Value;

https://github.com/fdeantoni/prost-wkt/blob/0e5a8d7811a1ca0104bed5b48b3e16a3964ffd17/wkt-types/src/pbstruct.rs#L201-L207

https://github.com/fdeantoni/prost-wkt/blob/0e5a8d7811a1ca0104bed5b48b3e16a3964ffd17/wkt-types/src/pbstruct.rs#L294-L304

https://github.com/fdeantoni/prost-wkt/blob/0e5a8d7811a1ca0104bed5b48b3e16a3964ffd17/wkt-types/src/pbstruct.rs#L136-L142

As it already logically flattens the fields field as seen here.

tcohen01 commented 1 year ago

Oh, I actually wonder if the same needs to be done with ListValue and its values field?

.field_attribute("google.protobuf.ListValue.values", "#[serde(flatten)]")

I've not really used this myself, mainly seen a bunch of repeated usage questions regarding Json::Value to google.protobuf.Struct deserialization in Rust and Tokio's discord servers and figured this is common enough to raise it as an issue.

fdeantoni commented 1 year ago

Oh, I actually wonder if the same needs to be done with ListValue and its values field?

.field_attribute("google.protobuf.ListValue.values", "#[serde(flatten)]")

That unfortunately won't work as serde(flatten) only works on structs or maps. In any case, I decided to implement the Serialize and Deserialize traits manually instead, and share it with the Value serde implementation. This way there should be no inconsistencies between Struct/ValueList and Value. I already merged the changes into master branch. Hopefully this addresses the problem correctly, but if not do let me know!

tcohen01 commented 1 year ago

Checked both Struct and ListValue, all seems to be working fine now!

Also still had a little test snippet on this function I had written to help a rust user on discord:

fn serialize_rows_to_protobuf(
    rows: Vec<serde_json::Map<String, serde_json::Value>>,
) -> Result<Vec<prost_wkt_types::Struct>, serde_json::Error> {
    rows.into_iter()
        .map(|row| {
            serde_json::from_value(serde_json::Value::Object(row))
                //.map(|fields| prost_wkt_types::Struct { fields })
        })
        .collect()
}

Before it only worked as intended when uncommenting the inner map, now it works both commented and uncommented.

Thanks for the fixes! Looking forward to the next release :)