SerializeDict and friends make instances not usable for other data formats #186

Closed zeenix closed 1 year ago

In GitLab by @jhartzell42 on Jul 15, 2021, 22:40

I am interested in fixing this myself, but would like to discuss first.

When I use DeserializeDict, and then use the Deserialize instance for e.g. JSON, due to the implementation of DeserializeDict I get JSON full of references to zbus:

{"update_time":{"zvariant::Value::Signature":"s","zvariant::Value::Value":"2021-07-14T20:04:01Z"},"timestamp":{"zvariant::Value::Signature":"s","zvariant::Value::Value":"2021-07-14T20:04:02.324114279Z"},"observation":{"zvariant::Value::Signature":"a{sv}","zvariant::Value::Value":{"date":{"zvariant::Value::Signature":"s","zvariant::Value::Value":"2021-07-14T20:04:01Z"}...

I have a proposal that will address this and simultaneously allow dict-style users of zbus to use all serde macros. The proposal is to have the zbus serializer/deserializer switch whether they work in "dict" mode or "typed" mode based on a new method to be added to the type trait,

fn serialization_mode() -> SerializationMode;
enum SerializationMode {
   Typed,
   Dict,
}

Alternatively, a separate serializer (to Value objects potentially rather than directly to the binary type) could be used.

This would allow the serde instances to all be cross-format and also get support for all the attributes for free.

I think this would be EASIER than adding all the attributes to the *Dict forms of the macros (especially since I'd like to make the change and this would be better for my purposes :-) )

In GitLab by @jhartzell42 on Jul 15, 2021, 22:53

@elmarco I was told by Zeeshan Ali that you are one person to ask about this. I think it can be made easy on the client program if we redefine SerializeDict etc. to implement Serialize. TypeDict would simply stay there and derive the dict version of the Type trait rather than the typed version. I think this would be very source compatible for the client program.

@jhartzell42 I'll look into this more in detail in a week when I'm back from vacation but I wanted to ask if you've looked at Value see/de code in detail and made sure that our special tagging is really needed. Our serializer and deserializer has access to the type signature so I'm not 100% sure the tagging is really needed. I did try my best to avoid (de)serializer needing the signature so this tagging might be an unnecessary remnant of that time.

In GitLab by @jhartzell42 on Jul 16, 2021, 17:47

Are you asking if I'm sure we need the additional method on the Type trait? No, I think you could get the information by reading the signature. In which case, the modified Serializer would just support reading new signatures. But I also think it would make sense to add the method, with a default implementation that just checked the signature, for clarity.

Are you asking if I'm sure we need the additional method on the Type trait?

No, I mean the extra zbus information that gets sent from Serialize impl to the serializer. That's why I wrote:

I did try my best to avoid (de)serializer needing the signature so this tagging might be an unnecessary remnant of that time.

My main thought here is that first we should ensure that there is no way to make things work without changing or adding any API before thinking of more intrusive solutions.

In GitLab by @jhartzell42 on Jul 16, 2021, 23:32

Could you please explain what “extra zbus information” you’re referring to?

I think it can be done without additional APIs, because the Type trait already exists and a type would have a different DBus signature for the Dict vs not Dict situation.

In GitLab by @jhartzell42 on Jul 16, 2021, 23:33

Or do you mean you want to transition away from the Type trait altogether?

Could you please explain what “extra zbus information” you’re referring to?

I mean the "zvariant::Value::Signature" that you are trying to avoid getting with other serializers. What I proposed was to look into the Value ser/de code that adds these "tags" and see if we can change the process to be something that works for other serializers too.

I think it can be done without additional APIs

I guess there was a typo here: "can" -> "can't"? Additional API is fine but changing the fundamental API would require a very compelling reasoning and to only be considered after exhausting other possibilities.

because the Type trait already exists and a type would have a different DBus signature for the Dict vs not Dict situation.

The unwanted "tags" are not related to Type trait itself really.

Alternatively, a separate serializer (to Value objects potentially rather than directly to the binary type) could be used.

Not sure I understood this one. By Value object here you mean zvariant::Value right? If so, what's the use of serializing to it? :thinking: I mean what do you do with this object?

In GitLab by @jhartzell42 on Jul 22, 2021, 22:44

Let me take a look at the code and get back to you; I think I'm confused about how this works :upside_down:

In GitLab by @jhartzell42 on Jul 26, 2021, 21:36

So here's what I was thinking: https://github.com/zeenix/zbus/pull/3 This adds a new pseudo-signature type, which is like (ABC) but actually serializes as a{sv} (and writes the signatures accordingly). This therefore does not break API (no existing users use < or > in signatures) and allows vanilla Serialize/Deserialize instances to be used with SerializeDict-style serializations. Advantages:

No API compatibility break. Problems:
There's no way (I have found yet!) to recover this signature through a variant in deserialization, which means that these can't be used inside DeserializeDict or DeserializeValue. I had the idea of sneaking the signature in here:

https://gitlab.freedesktop.org/dbus/zbus/-/blob/main/zvariant/src/deserialize_value.rs#L41

... but couldn't figure out a way not to leak memory. This means that you can't use this inside a SerializeDict or a variant. But given that it offers a replacement to SerializeDict, that's not perhaps as big a problem. And I'm confident I can figure out that last issue.

And this is enough that you get the general idea of where I'm headed. What are your thoughts?

In GitLab by @jhartzell42 on Jul 26, 2021, 21:42

Here's the gitlab PR: https://gitlab.freedesktop.org/dbus/zbus/-/merge_requests/342 , sorry about that :-)

Don't see this going anywhere, at least any time soon.

dbus2 / zbus-old

SerializeDict and friends make instances not usable for other data formats #186