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

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

Closed
zeenix opened this issue Jul 15, 2021 · 11 comments
Closed
Labels
api break zvariant_derive Issues/PRs related to zvariant_derive crate

Comments

@zeenix
Copy link
Contributor

zeenix commented Jul 15, 2021

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 :-) )

@zeenix
Copy link
Contributor Author

zeenix commented Jul 15, 2021

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.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2021

@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.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2021

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.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2021

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.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2021

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.

@zeenix
Copy link
Contributor Author

zeenix commented Jul 16, 2021

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

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

@zeenix
Copy link
Contributor Author

zeenix commented Jul 22, 2021

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? 🤔 I mean what do you do with this object?

@zeenix
Copy link
Contributor Author

zeenix commented Jul 22, 2021

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:

@zeenix
Copy link
Contributor Author

zeenix commented Jul 26, 2021

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?

@zeenix
Copy link
Contributor Author

zeenix commented Jul 26, 2021

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 :-)

@zeenix
Copy link
Contributor Author

zeenix commented Sep 15, 2022

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

@zeenix zeenix closed this as completed May 11, 2023
zeenix added a commit to zeenix/zbus that referenced this issue Sep 4, 2024
This is no longer needed.

While this doesn't completely fix dbus2#176, this goes as far as we can IMO.
For D-Bus we need the signature to be able to derialize any data to a
Value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api break zvariant_derive Issues/PRs related to zvariant_derive crate
Projects
None yet
Development

No branches or pull requests

1 participant